[Siddharth-Dangi]

Summary

An identifying field in Concerto must uniquely identify a resource instance with a scalar value. At model load time, ClassDeclaration.validate() verified that identifying fields were strings (or scalar strings) and not optional, but did not check whether the property was declared as an array (idField.isArray()).

Consequently, models declaring array identifying fields (e.g. asset Person identified by names { o String[] names } or with scalar string arrays, directly or through inheritance) loaded without errors into ModelManager, but failed at runtime during resource instantiation or deserialization:

• Factory.newResource('test@1.0.0', 'Person', 'alice') sets 'alice' onto an array field, failing downstream with Model violation: The field "names" has a value of ""alice"". Expected type of value: "String[]".
• Passing an array to Factory.newResource(..., ['alice']) throws Error: Invalid or missing identifier for Type "Person".
• Serializer.fromJSON() throws Error: Invalid or missing identifier for Type "Person".

Per maintainer feedback on simplifying and expediting review, this pull request isolates solely the array identifying field check, eliminating any overlap with other open PRs.

Changes

• packages/concerto-core/src/introspect/classdeclaration.ts:
  • Added if (idField.isArray()) throw new IllegalModelException('Identifying fields cannot be arrays.', this.modelFile, this.ast.location); within ClassDeclaration.validate().
• packages/concerto-core/test/data/parser/assetdeclaration.arrayid.cto:
  • Added parser test CTO model declaring an asset with an array identifier.
• packages/concerto-core/test/introspect/assetdeclaration.js:
  • Added unit test verifying loadAssetDeclaration throws IllegalModelException on array identifying field.
• packages/concerto-core/test/introspect/classdeclaration.js:
  • Added comprehensive test coverage:
    • String array identifier (asset Person identified by names { o String[] names })
    • Scalar string array identifier (concept Person identified by ssnArray { o SSN[] ssnArray })
    • Inherited string array identifier (abstract asset Base { o String[] ids } asset Child identified by ids extends Base)
    • Participant identified by array (participant Member identified by memberIds { o String[] memberIds })
    • Multi-level 3rd-level subclass identified by array defined in root class

Testing

• packages/concerto-core: 1,386 passing unit tests, 0 failures.
• test:esm: All 10 ESM smoke checks pass.
• npm run licchk: 100% compliant.
• npm run lint: Clean, 0 errors.

Signed-off-by: Siddharth-Dangi mr.siddharthdangi@gmail.com

[mira-accord-project]

Thank you for the pull request, and for the clear write-up of how an array identifying field loads and then fails at runtime. That part looks like a real gap, and I could not find an existing issue or pull request for it. Concerto maintainers have been asked to review; I can't say when, but this will be in their queue.

One thing that will help review: the change also covers several other areas, namely default values on array, scalar and relationship properties, enum declaration rules, IdentifiedBy AST checks and map value types. Some of that overlaps open pull requests (#1606 and #1608 on default values, #1603 on AST validation). It would be easier to review, and to merge, if the array identifying field check went in on its own, with the rest split out or rebased once those pull requests settle. A maintainer may well have a different view, so please wait for their steer before reworking anything.

Also, please check that your commit is signed off (DCO) as the repository requires.

Drafted by an Accord Project agent, reviewed by a maintainer.

[Siddharth-Dangi]

Thanks @mira-accord-project! Glad to hear the array identifying field gap is confirmed. (Also confirmed that the DCO check has passed green).

I completely agree with the suggestion: to make this as fast and clean as possible to review and merge, I have a trimmed branch ready that isolates solely the array identifying field check and its inheritance tests with zero overlap with #1606 or #1608.

Happy to push that focused version right away, or wait for the maintainers' steer as you suggested. Thank you!

[Siddharth-Dangi]

Hi @mira-accord-project & maintainers — I have updated this PR to isolate strictly the array identifying field check and its test suite:

• Stripped all extraneous AST/declaration checks that overlap with open PRs (fix(concerto-core): reject default and size modifiers on scalar-typed properties (#1605) #1606, fix(concerto-core): reject invalid default values on array, complex object, and enum properties (#1607) #1608, fix(concerto-core): validate metamodel AST bounds and required structures #1603).
• Branch now consists of a single signed-off commit (7c9b7d3) touching just 4 files (+99 lines).
• Includes comprehensive regression test coverage across direct array identifiers, scalar string array identifiers, inheritance, and multi-level hierarchies.
• All 1,386 unit tests, ESM smoke checks, and DCO/Snyk security checks are passing green.

Ready for maintainer review. Thank you!

[mira-accord-project]

Thank you for this, Siddharth, and for narrowing it down to the array identifier check. The change is small and sits in the right place in ClassDeclaration.validate(). It runs after the String and optional checks, so scalar string arrays and identifiers inherited from a super type are both caught. The error wording matches the neighbouring "cannot be optional" message. The new tests cover direct, scalar, inherited, participant and three-level cases, and each one reaches the new throw. I found nothing to change in the code. What remains is process rather than code: the CI workflows are still waiting for a first-time-contributor approval, so there is no test run at this commit yet, and a maintainer review is still needed. Please be aware that models with an array identifier that previously loaded will now be rejected at load time. They already failed when instantiating or deserialising, so this looks like the intended outcome, but maintainers may want to note it in the release notes.

No problems found.

---

_{Automated review by Mira, an Accord Project agent, at 89970bf. Findings are labelled blocking, should-fix or nit; a maintainer makes the final call.}