Concerto Analyser ignores the inheritance chain when comparing properties between two model versions

[ekarademir]

Bug Report 🐛

Given two versions of the same namespace: child1.cto

namespace [email protected]

concept Child {
  o String aProperty optional
  o String bProperty
}

and

child2.cto

namespace [email protected]

import [email protected].{ Parent }

concept Child extends Parent {
  o String bProperty
}

where parent.cto is

namespace [email protected]

concept Parent {
  o String cProperty optional
  o String aProperty optional
}

when child1 is compared to child2, even with parent model provided in the model manager, it still reports a major version change,

import { ModelManager, ModelFile } from "@accordproject/concerto-core";
import {Compare} from '@accordproject/concerto-analysis'
import child1 from './child1.json' with {type: 'json'};
import child2 from './child2.json' with {type: 'json'};
import parent from './parent.json' with {type: 'json'};

const mm1 = new ModelManager({strict: true});
const child1File = new ModelFile(mm1, child1);

const mm2 = new ModelManager({strict: true});
const parentFile = new ModelFile(mm2, parent);
const child2File = new ModelFile(mm2, child2);

const compare = new Compare();
console.log(compare.compare(child1File, child2File));

{
  findings: [
    {
      key: 'optional-property-removed',
      message: 'The optional field "aProperty" was removed from the concept "Child"',
      element: [Field],
      result: 3
    }
  ],
  result: 3
}

(3 is major https://github.com/accordproject/concerto/blob/5e39e2040dcecbf866d200abf21b4d061f8d397d/packages/concerto-analysis/src/compare-config.ts#L22)

but it shouldn't, because aProperty is coming from Parent.

Expected Behavior

Analyser should use the inherited properties, if the inheritance chain is provided.

Current Behavior

Inheritance chain is ignored.

Possible Solution

Here https://github.com/accordproject/concerto/blob/5e39e2040dcecbf866d200abf21b4d061f8d397d/packages/concerto-analysis/src/compare.ts#L133

Instead of own properties, it should also get inherited properties if possible.

[gaumrab]

Hi @ekarademir,

I’ve gone through the issue and reviewed the current comparison behavior. My understanding is that inherited properties, like aProperty, are being flagged as removed because the analyzer uses getOwnProperties() and doesn’t take the inheritance chain into account.

Implementation: In concerto-analysis/src/compare.ts, we can change the line:

this.compareProperties(comparers, a.getOwnProperties(), b.getOwnProperties());

to

const propsA = this.options.includeInherited ? a.getAllProperties() : a.getOwnProperties();
const propsB = this.options.includeInherited ? b.getAllProperties() : b.getOwnProperties();
this.compareProperties(comparers, propsA, propsB);

I’d like to fix this issue with proper testing. May I proceed and raise a PR afterward?

[rnema19]

@ekarademir I'd like to contribute, could you please tell how to produce this issue? Do I have to create a separate scripts file and paste the entire code and then run it?

[rnema19]

@ekarademir is it fine if there's result : 1(minor) Here's the response I'm getting:

{
  findings: [
    {
      key: 'optional-property-added',
      message: 'The optional field "cProperty" was added to the concept "Parent"',
      element: [Field],
      result: 1
    }
  ],
  result: 1
}

[ekarademir]

@gaumrab @rnema19 if you are comfortable with the change please feel free to open up PRs.

@rnema19 that result looks good. If there is a way to distinguish via the key that would be better. Like optional-parent-property-added. But that's optional.

[gaumrab]

ok @ekarademir, I am on it soon will update you here. Thanks for your Response.

[gaumrab]

@ekarademir

includeInherited Option -

What it does: Controls whether to include inherited properties when comparing models.

Two Options: includeInherited: false (Default) Only looks at properties directly written in each model Ignores inherited properties from parent classes Problem: Reports inherited properties as "removed" → Wrong MAJOR change

includeInherited: true (Fixed) Looks at all properties including inherited ones Correctly recognizes inherited properties Result: Accurate comparison → Correct PATCH change

Usage: When to use: false: Simple models without inheritance true: Models with inheritance chains (recommended)

Note: Use includeInherited: true to fix the inheritance bug! 🎯

=== Testing the Fix ===

=== Model Analysis === Child1 properties: [ 'aProperty', 'bProperty' ] Child2 own properties: [ 'bProperty' ] Child2 all properties: [ 'bProperty', 'cProperty', 'aProperty' ]

=== Test 1: Original Behavior (includeInherited: false) ===

Findings: [
  {
    key: 'optional-property-removed',
    message: 'The optional field "aProperty" was removed from the concept "Child"',
    element: Field {
      ast: [Object],
      parent: [ConceptDeclaration],
      decorators: [],
      name: 'aProperty',
      decorator: null,
      type: 'String',
      array: false,
      optional: true,
      validator: null,
      defaultValue: null,
      scalarField: null
    },
    result: 3
  }
]
Result: 3
Expected: MAJOR (3) - BUG CONFIRMED
Actual: MAJOR (3) - BUG CONFIRMED

=== Test 2: Fixed Behavior (includeInherited: true) ===

Findings: [
  {
    key: 'optional-property-added',
    message: 'The optional field "cProperty" was added to the concept "Parent"',
    element: Field {
      ast: [Object],
      parent: [ConceptDeclaration],
      decorators: [],
      name: 'cProperty',
      decorator: null,
      type: 'String',
      array: false,
      optional: true,
      validator: null,
      defaultValue: null,
      scalarField: null
    },
    result: 1
  }
]
Result: 1
Expected: Should NOT be MAJOR because aProperty is inherited
Actual: NOT MAJOR - BUG FIXED!

=== Summary === Original behavior (includeInherited: false): BUG CONFIRMED Fixed behavior (includeInherited: true): BUG FIXED!

[gaumrab]

Hi @ekarademir,

I have raised a PR to fix the inheritance comparison bug (issue #1067).

The implementation adds an includeInherited option to the CompareConfig, allowing users to consider inherited properties when comparing models. This resolves the issue where inherited properties were incorrectly reported as "removed".

Could you please review the implementation and logic? I'd appreciate any feedback or suggestions for improvement.

I understand that one approval is required before this PR can be merged. Your review would be greatly appreciated.

Thank you for your time!

[rnema19]

Hi, I have changed the properties to be used and now we get minor result 1, let me know if anything else is needed. Instead of adding any additional properties, inheritance property used is getProperties() instead of getOwnProperties(), this causes the properties from the Parent class to be inherited and not clash with its own properties.

Thanks!

[farazghani]

is this issue resolved? i