Repository navigation
fix(concerto-core): validate metamodel AST bounds and required structures - #1603
Siddharth-Dangi wants to merge 1 commit into
Conversation
e257b58 to
d8bbd53
Compare
|
Thanks for the PR, @Siddharth-Dangi. Validating the metamodel AST (bounds and required structures) before it reaches the introspection classes means malformed input fails with a clear model error instead of a runtime crash, and the tests cover the new checks. The Concerto maintainers have been asked to review. We aim to give a first review within a few working days, though I can't promise a date. At 690 changed lines, reviewers may ask whether it can be split into smaller pieces, which usually speeds things up. Drafted by an Accord Project agent, reviewed by a maintainer. |
|
Thanks @mira-accord-project! Glad to hear the fail-fast validation strategy resonates. Most of the diff (~480 lines) consists of regression tests across the validator classes. If reviewers prefer, I'm very happy to split it into two focused PRs (e.g. 1: Numeric/String/Collection validator bounds; 2: ModelFile & Property AST checks). Looking forward to the review! |
d8bbd53 to
3548563
Compare
…ures
Metamodel AST instances created directly (e.g. deserialized from JSON or
inbound from external tools) could bypass structural validation and propagate
invalid state down into printers, generators, and serializers:
- NumberValidator accepted ASTs with undefined or empty bounds ({ lower: undefined, upper: undefined }) because hasOwnProperty check bypassed null guards, resulting in an unconfigured validator and returning undefined bounds. Additionally, null or undefined ASTs threw uncaught TypeErrors.
- StringValidator accepted regex validators missing a pattern, with pattern: null, or with an empty pattern, which created RegExp(/null/) or /(?:)/ instead of reporting a model error.
- StringValidator length bounds did not use NullUtil.isNull consistently, causing empty length bounds to be treated differently depending on whether properties were null or undefined; furthermore non-integer bounds were not rejected.
- CollectionSizeValidator accepted null/undefined validators without a clean model error and did not reject non-integer bounds.
- ObjectProperty with null or missing type was silently accepted during validate(), leading to downstream crashes (e.g. Printer.toCTO throwing TypeError: Cannot read properties of null (reading 'name')).
- Null or undefined elements in ModelFile declarations or imports threw raw TypeErrors rather than IllegalModelException; non-array imports and declarations now also throw IllegalModelException.
- Null or undefined properties in ClassDeclaration properties threw raw TypeErrors rather than IllegalModelException.
- ObjectMapKeyType, ObjectMapValueType, and RelationshipMapValueType threw raw TypeErrors when type was null or missing instead of IllegalModelException.
- BaseModelManager.fromAst threw raw TypeErrors when ast or ast.models was null or contained null elements.
- BaseModelManager.validateAst leaked the metamodel namespace into modelFiles if deserialization validation threw an exception before cleanup.
Normalize bounds using NullUtil.isNull, ensure length and size bounds are positive integers, fail fast with IllegalModelException when required structures are null, empty, or non-arrays, ensure regex pattern is a non-empty string, and clean up temporary metamodel files in a finally block.
Signed-off-by: Siddharth-Dangi <mr.siddharthdangi@gmail.com>
3548563 to
6747535
Compare
Summary
Metamodel AST instances created directly (e.g. deserialized from raw JSON models, external integration pipelines, or programmatic AST generation) could bypass structural validation and propagate invalid state down into printers, generators, and serializers:
NullUtil.isNulland reject{ lower: undefined, upper: undefined }(which previously passed becausehasOwnPropertychecks evaluated to true) as well as non-numeric bounds. Handlenull/undefinedvalidator AST objects gracefully.minLength/maxLength.minSizeandmaxSize.ObjectPropertyto specify a validtypeandtype.nameduringvalidate(), and enforce thatsizeValidatoronly applies to array or map properties without triggering internal TypeErrors on unresolved types.IllegalModelExceptionrather than propagating rawTypeErrorcrashes.typepresence for object and relationship map keys/values, and align withMetaModelNamespace.fromAst(), validate thatastandast.modelsexist and elements are non-null.validateAst(), clean up temporary metamodel model files within afinallyblock to prevent namespace pollution on validation failure.Maintainer Approval & Feedback Requested (@jamieshorten @mttrbrts)
Before merging, I would appreciate maintainer input and approval on the following design decisions:
IllegalModelException: Does the team agree that inbound AST structures (such asdeclarations: [null]or non-array collections) should throwIllegalModelExceptionimmediately upon model loading, or are there any existing downstream tools that rely on lenient AST handling?Property.validate()requiringObjectPropertyto have a definedtype(mirroringRelationshipProperty), while keeping constructor parsing lenient for backward compatibility. Does this align with your architectural expectations?StringRegexValidatorpattern must be a non-empty string (as emptyregex=//is not permitted by Concerto's CTO PEG grammar). Please confirm if this restriction matches your intended model semantics.Testing
astmodelmanager.js,collectionsizevalidator.js,stringvalidator.js,numbervalidator.js,property.js, andmapdeclaration.js.concerto-coreand all 8 monorepo workspaces pass with 0 regressions.concerto-core: Statements: 99.05%, Branches: 96.01%, Functions: 99.49%, Lines: 99.11%.licchk), and JSDoc (doc) pass cleanly.Signed-off-by: Siddharth-Dangi mr.siddharthdangi@gmail.com