fix(core): detect ambiguous imports / declarations

Signed-off-by: Matt Roberts <code@rbrts.uk>
accordproject · Feb 5, 2024 · b19d943 · b19d943
1 parent 3b62a2c
commit b19d943
Show file tree

Hide file tree

Showing 13 changed files with 79 additions and 57 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/packages/concerto-core/src/introspect/declaration.js b/packages/concerto-core/src/introspect/declaration.js
@@ -69,6 +69,24 @@ class Declaration extends Decorated {
         this.fqn = ModelUtil.getFullyQualifiedName(this.modelFile.getNamespace(), this.name);
     }
 
+    /**
+     * Semantic validation of the structure of this decorated. Subclasses should
+     * override this method to impose additional semantic constraints on the
+     * contents/relations of fields.
+     *
+     * @param {...*} args the validation arguments
+     * @throws {IllegalModelException}
+     * @protected
+     */
+    validate(...args) {
+        super.validate(...args);
+
+        // #648 - check for clashes against imported types
+        if (this.getModelFile().isImportedType(this.getName())){
+            throw new IllegalModelException(`Type '${this.getName()}' clashes with an imported type with the same name.`, this.modelFile, this.ast.location);
+        }
+    }
+
     /**
      * Returns the ModelFile that defines this class.
      *

diff --git a/packages/concerto-core/test/composer/composermodel.js b/packages/concerto-core/test/composer/composermodel.js
@@ -33,18 +33,6 @@ const COMPOSER_MODEL =
  */
 namespace system@1.0.0
 
-event Event {
-}
-
-participant Participant {
-}
-
-asset Asset {
-}
-
-transaction Transaction {
-}
-
 /**
  * Abstract system participant that all participants extend.
  * Has no properties, and is soley used as a basis to model other assets.

diff --git a/packages/concerto-core/test/data/parser/assetdeclaration.systypename.cto b/packages/concerto-core/test/data/parser/assetdeclaration.systypename.cto
diff --git a/packages/concerto-core/test/data/parser/classdeclaration.dupeassetname.cto b/packages/concerto-core/test/data/parser/classdeclaration.dupeassetname.cto
@@ -14,12 +14,12 @@
 
 namespace com.testing@1.0.0
 
-asset Asset identified by id {
+asset MyAsset identified by id {
   o String id
   o String newValue
 }
 
-asset Asset identified by id {
+asset MyAsset identified by id {
   o String id
   o String newValue
 }
diff --git a/packages/concerto-core/test/data/parser/classdeclaration.dupeconceptname.cto b/packages/concerto-core/test/data/parser/classdeclaration.dupeconceptname.cto
@@ -14,12 +14,12 @@
 
 namespace com.testing@1.0.0
 
-concept Concept{
+concept MyConcept {
   o String id
   o String newValue
 }
 
-concept Concept{
+concept MyConcept {
   o String id
   o String newValue
 }
diff --git a/packages/concerto-core/test/introspect/assetdeclaration.js b/packages/concerto-core/test/introspect/assetdeclaration.js
@@ -112,16 +112,6 @@ describe('AssetDeclaration', () => {
                 asset.validate();
             }).should.throw(/more than one field named/);
         });
-
-        it('should throw an an IllegalModelException if its not a System Type and is called Asset', () => {
-            let asset = loadLastAssetDeclaration('test/data/parser/assetdeclaration.systypename.cto');
-            try {
-                asset.validate();
-            } catch (err) {
-                err.should.be.an.instanceOf(IllegalModelException);
-                err.message.should.match(/Asset is a reserved type name./);
-            }
-        });
     });
 
     describe('#getProperty', () => {

diff --git a/packages/concerto-core/test/introspect/classdeclaration.js b/packages/concerto-core/test/introspect/classdeclaration.js
@@ -357,7 +357,7 @@ describe('ClassDeclaration', () => {
     describe('#_resolveSuperType', () => {
 
         it('should return Asset if no super type', () => {
-            let classDecl = modelManager.getType('system@1.0.0.Asset');
+            let classDecl = modelManager.getType('concerto@1.0.0.Asset');
             classDecl._resolveSuperType().should.not.be.null;
         });
 
@@ -394,7 +394,7 @@ describe('ClassDeclaration', () => {
     describe('#getSuperTypeDeclaration', () => {
 
         it('should return Concept if no super type', () => {
-            let classDecl = modelManager.getType('system@1.0.0.Asset');
+            let classDecl = modelManager.getType('concerto@1.0.0.Asset');
             classDecl.getSuperTypeDeclaration().should.not.be.null;
         });
     });

diff --git a/packages/concerto-core/test/introspect/introspector.js b/packages/concerto-core/test/introspect/introspector.js
@@ -58,7 +58,7 @@ describe('Introspector', () => {
             let classDecl = introspector.getClassDeclarations();
             const scalarDecl = classDecl.filter(declaration =>  declaration.isScalarDeclaration?.());
             const mapDecl = classDecl.filter(declaration =>  declaration.isMapDeclaration?.());
-            classDecl.length.should.equal(44);
+            classDecl.length.should.equal(40);
             scalarDecl.length.should.equal(0);
             mapDecl.length.should.equal(0);
         });

diff --git a/packages/concerto-core/test/introspect/modelfile.js b/packages/concerto-core/test/introspect/modelfile.js
@@ -282,6 +282,46 @@ describe('ModelFile', () => {
             }).should.throw('Importing types from different versions ("1.0.0", "2.0.0") of the same namespace "org.freddos@2.0.0" is not permitted.');
         });
 
+        it('should throw when declaring an ambiguous type', () => {
+            const myModelManager = new ModelManager();
+
+            const model1 = `namespace A@1.0.0
+
+            concept B {
+                o String name
+            }`;
+
+            const model2 = `namespace B@1.0.0
+
+            import A@1.0.0.{B}
+
+            concept B {
+            }`;
+
+
+            let modelFile1 = ParserUtil.newModelFile(myModelManager, model1);
+            myModelManager.addModelFile(modelFile1);
+
+            let modelFile2 = ParserUtil.newModelFile(myModelManager, model2);
+
+            (() => {
+                myModelManager.addModelFile(modelFile2);
+            }).should.throw('Type \'B\' clashes with an imported type with the same name.');
+        });
+
+        it('should recognise a user-space type with the same name as a Prototype', () => {
+            const model1 = `
+            namespace A@1.0.0
+            concept Transaction {}
+            `;
+
+            let modelFile1 = ParserUtil.newModelFile(modelManager, model1);
+
+            (() => {
+                modelManager.addModelFile(modelFile1);
+            }).should.throw('Type \'Transaction\' clashes with an imported type with the same name.');
+        });
+
     });
 
     describe('#getDefinitions', () => {

diff --git a/packages/concerto-core/test/introspect/participantdeclaration.js b/packages/concerto-core/test/introspect/participantdeclaration.js
@@ -31,7 +31,7 @@ describe('ParticipantDeclaration', () => {
     describe('#getParticipantDeclarations', () => {
         it('should get participants', () => {
             let participants = modelManager.getParticipantDeclarations();
-            participants.should.have.lengthOf(3);
+            participants.should.have.lengthOf(2);
             participants[0].declarationKind().should.equal('ParticipantDeclaration');
         });
     });

diff --git a/packages/concerto-core/test/model/concept-identified.js b/packages/concerto-core/test/model/concept-identified.js
@@ -42,7 +42,7 @@ describe('Concept Identifiers', function () {
             --> Product product
         }
         transaction Request {}
-        event Event {}
+        event MyEvent {}
         `, 'test.cto');
         classDecl = modelManager.getType('org.accordproject@1.0.0.Order');
     });
@@ -98,7 +98,7 @@ describe('Concept Identifiers', function () {
             const txn = factory.newResource('org.accordproject@1.0.0', 'Request');
             dayjs(txn.getTimestamp()).isValid().should.be.true;
 
-            const event = factory.newResource('org.accordproject@1.0.0', 'Event');
+            const event = factory.newResource('org.accordproject@1.0.0', 'MyEvent');
             dayjs(event.getTimestamp()).isValid().should.be.true;
         });
 

diff --git a/packages/concerto-core/test/modelmanager.js b/packages/concerto-core/test/modelmanager.js
@@ -882,11 +882,11 @@ concept Bar {
     });
 
     describe('#getDeclarations', () => {
-        const numberModelBaseAssets = 13;
+        const numberModelBaseAssets = 12;
         const numberModelBaseEnums = 2;
-        const numberModelBaseParticipants = 5;
-        const numberModelBaseEvents = 1;
-        const numberModelBaseTransactions = 21;
+        const numberModelBaseParticipants = 4;
+        const numberModelBaseEvents = 0;
+        const numberModelBaseTransactions = 20;
         const numberModelBaseConcepts = 2;
 
         describe('#getAssetDeclarations', () => {
@@ -1122,7 +1122,7 @@ concept Bar {
             modelManager.addModelFiles([composerModel, modelBase, farm2fork, concertoModel]);
 
             modelManager.getModelFiles().length.should.equal(5);
-            modelManager.getModelFiles().map(mf => mf.getAllDeclarations()).flat().length.should.equal(69);
+            modelManager.getModelFiles().map(mf => mf.getAllDeclarations()).flat().length.should.equal(65);
 
             const filtered = modelManager.filter(declaration => declaration.getFullyQualifiedName() === 'org.accordproject.test@1.0.0.Product');