feat(map): Resolve namespace resolution for Map Types #9

jonathan-casey · 2023-09-26T14:50:26Z

Description

Handle MapDeclaration cases for the resolve command.

Author Checklist

Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
Vital features and changes captured in unit and/or integration tests
Commits messages follow AP format

Signed-off-by: Jonathan Casey <[email protected]>

mttrbrts · 2023-09-26T19:31:41Z

lib/metamodelutil.js

+        const nameK = metaModel.key.$class === `${MetaModelNamespace}.ObjectMapKeyType` ? metaModel.key.type.name : ModelUtil.getShortName(metaModel.key.$class);
+        metaModel.key.$class === `${MetaModelNamespace}.ObjectMapKeyType` ? metaModel.key.type.namespace = resolveName(nameK, table) : metaModel.key.namespace = resolveName(nameK, table);
+        const nameV = metaModel.value.$class === `${MetaModelNamespace}.ObjectMapValueType` ? metaModel.value.type.name : ModelUtil.getShortName(metaModel.value.$class);
+        metaModel.key.$class === `${MetaModelNamespace}.ObjectMapKeyType` ? metaModel.value.type.namespace = resolveName(nameV, table) : metaModel.value.namespace = resolveName(nameV, table);


Nit, please refactor this to have a branch for ObjectMapKeyType and an else case. This will make this easier to read and DRY the repeated tenary operators.

This should have a new case for MapDeclaration which calls resolveTypeNames on both key and value (recursion).

There should then be cases for ObjectMapKeyType and ObjectMapValueType that follows the established pattern:

const name = metaModel.type.name; metaModel.type.namespace = resolveName(name, table); (metaModel.decorators || []).forEach((decorator) => { resolveTypeNames(decorator, table); });

A good test-case to add would be:

namespace A scalar MyScalar extends String namespace B map EventMap { o MyScalar o Event // requires implicit resolution using `concerto` namespace }

Looks like we may also have a bug here with ScalarDeclaration missing? Can you test and create an issue for that please?

dselman · 2023-09-26T21:17:09Z

lib/metamodelutil.js

@@ -64,6 +65,14 @@ function createNameTable(priorModels, metaModel) {
        'Participant': concertoNs,
        'Transaction ': concertoNs,
        'Event': concertoNs,
+        'StringMapKeyType': concertoNs,


Not sure I understand this. StringMapKeyType is not defined in the [email protected] namespace - that is a type from the metamodel. What are we trying to do here?

The intention is that any type used within a map context should have its namespace resolved.

I guess my question is - why differentiate between types on the metamodel vs those on the root model when resolving namespace - why should namespace apply to only root types, but not in the case of primitives? The metamodel is versioned, after all.

If I think about this from the point of view that a primitive type will always be a primitive type it doesn't really make sense to version it to a namespace, and in that case there is no need to generate namespace values for primitives in the context of a resolve command output.

Feels like I may not be seeing the forest for the trees.., it would be useful if you can share a scenario under which the resolve function is necessary, from a client perspective.

What the createNameTable code is doing (before you changes) is saying that anywhere we see the unqualified type Event being used (et al) it should be resolved as [email protected].

The concerto namespace is special in that it is managed by the ModelManager implicitly. I.e. any model file can use Event without having to import it. Here is the code that adds the concerto namespace to the model manager on construction.
https://github.com/accordproject/concerto/blob/main/packages/concerto-core/lib/basemodelmanager.js#L126

Your code change is saying that StringMapKeyType is also defined in the concerto namespace, which is not correct.

I don't think you need to update the logic in createNameTable - it uses the imported types to build the name resolution table (resolving from a short type name to a namespace). Given that maps must import their types to use them as keys or values this should just work...

dselman

The "resolution" process takes an AST with imports and unqualified type names and uses the imports to set the namespace property on all the TypeIdentifier nodes. In effect it is an optimisation that makes it easier for consumers of the AST: they no longer need to do the (somewhat complex) import resolution logic.

dselman · 2023-10-02T14:33:26Z

lib/metamodelutil.js

+        const nameK = metaModel.key.$class === `${MetaModelNamespace}.ObjectMapKeyType` ? metaModel.key.type.name : ModelUtil.getShortName(metaModel.key.$class);
+        metaModel.key.$class === `${MetaModelNamespace}.ObjectMapKeyType` ? metaModel.key.type.namespace = resolveName(nameK, table) : metaModel.key.namespace = resolveName(nameK, table);
+        const nameV = metaModel.value.$class === `${MetaModelNamespace}.ObjectMapValueType` ? metaModel.value.type.name : ModelUtil.getShortName(metaModel.value.$class);
+        metaModel.key.$class === `${MetaModelNamespace}.ObjectMapKeyType` ? metaModel.value.type.namespace = resolveName(nameV, table) : metaModel.value.namespace = resolveName(nameV, table);


This should have a new case for MapDeclaration which calls resolveTypeNames on both key and value (recursion).

There should then be cases for ObjectMapKeyType and ObjectMapValueType that follows the established pattern:

const name = metaModel.type.name; metaModel.type.namespace = resolveName(name, table); (metaModel.decorators || []).forEach((decorator) => { resolveTypeNames(decorator, table); });

A good test-case to add would be:

namespace A scalar MyScalar extends String namespace B map EventMap { o MyScalar o Event // requires implicit resolution using `concerto` namespace }

dselman · 2023-10-02T14:34:37Z

lib/metamodelutil.js

+        const nameK = metaModel.key.$class === `${MetaModelNamespace}.ObjectMapKeyType` ? metaModel.key.type.name : ModelUtil.getShortName(metaModel.key.$class);
+        metaModel.key.$class === `${MetaModelNamespace}.ObjectMapKeyType` ? metaModel.key.type.namespace = resolveName(nameK, table) : metaModel.key.namespace = resolveName(nameK, table);
+        const nameV = metaModel.value.$class === `${MetaModelNamespace}.ObjectMapValueType` ? metaModel.value.type.name : ModelUtil.getShortName(metaModel.value.$class);
+        metaModel.key.$class === `${MetaModelNamespace}.ObjectMapKeyType` ? metaModel.value.type.namespace = resolveName(nameV, table) : metaModel.value.namespace = resolveName(nameV, table);


Looks like we may also have a bug here with ScalarDeclaration missing? Can you test and create an issue for that please?

Signed-off-by: Jonathan Casey <[email protected]>

coveralls · 2023-10-20T10:40:44Z

Pull Request Test Coverage Report for Build 6588566313

9 of 11 (81.82%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.4%) to 93.671%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/metamodelutil.js	9	11	81.82%

Totals
Change from base Build 5830242667:	-0.4%
Covered Lines:	92
Relevant Lines:	95

💛 - Coveralls

Signed-off-by: Jonathan Casey <[email protected]>

package.json

test/cto/model.cto

test/metamodelutil.js

package.json

lib/metamodelutil.js

Signed-off-by: Jonathan Casey <[email protected]>

dselman

LGTM!

jonathan-casey requested a review from a team September 26, 2023 14:50

jonathan-casey changed the title ~~feat(map): resolution~~ feat(map): Resolve namespace resolution for Map Types Sep 26, 2023

jonathan-casey added 3 commits September 26, 2023 15:53

feat(map): resolves namespace for Map Type

d12bcaa

Signed-off-by: Jonathan Casey <[email protected]>

chore(dep): add concerto-core dep

af9dcc4

Signed-off-by: Jonathan Casey <[email protected]>

feat(map): extend test coverage

595838c

Signed-off-by: Jonathan Casey <[email protected]>

jonathan-casey force-pushed the jonathan/patch_resolution branch from bacbf59 to 595838c Compare September 26, 2023 14:53

chore(dep): add node types

f877b87

Signed-off-by: Jonathan Casey <[email protected]>

mttrbrts approved these changes Sep 26, 2023

View reviewed changes

dselman requested changes Sep 26, 2023

View reviewed changes

dselman requested changes Oct 2, 2023

View reviewed changes

feat(map): define ns recursively

754e9bd

Signed-off-by: Jonathan Casey <[email protected]>

jonathan-casey added 3 commits October 20, 2023 11:58

feat(*): add scalar support

c1afe87

Signed-off-by: Jonathan Casey <[email protected]>

feat(map): use latest concerto-core

f099dd6

Signed-off-by: Jonathan Casey <[email protected]>

feat(map): test coverage for resolving imports on maps

1c1b392

Signed-off-by: Jonathan Casey <[email protected]>

mttrbrts requested changes Oct 20, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

test/cto/model.cto Outdated Show resolved Hide resolved

test/metamodelutil.js Outdated Show resolved Hide resolved

dselman requested changes Oct 20, 2023

View reviewed changes

package.json Show resolved Hide resolved

dselman reviewed Oct 20, 2023

View reviewed changes

lib/metamodelutil.js Show resolved Hide resolved

dselman reviewed Oct 20, 2023

View reviewed changes

lib/metamodelutil.js Show resolved Hide resolved

jonathan-casey added 3 commits October 20, 2023 14:37

feat(map): test refactor

7b847fb

Signed-off-by: Jonathan Casey <[email protected]>

chore(*): no circular dependency

e145900

Signed-off-by: Jonathan Casey <[email protected]>

feat(map): resolve decorators on MapDeclarations and Scalars

0faf117

Signed-off-by: Jonathan Casey <[email protected]>

dselman approved these changes Oct 20, 2023

View reviewed changes

mttrbrts approved these changes Oct 20, 2023

View reviewed changes

jonathan-casey merged commit 5ead0b9 into main Oct 20, 2023
9 checks passed

jonathan-casey deleted the jonathan/patch_resolution branch October 20, 2023 13:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(map): Resolve namespace resolution for Map Types #9

feat(map): Resolve namespace resolution for Map Types #9

jonathan-casey commented Sep 26, 2023 •

edited

Loading

mttrbrts Sep 26, 2023

dselman Oct 2, 2023 •

edited

Loading

dselman Oct 2, 2023 •

edited

Loading

dselman Sep 26, 2023

jonathan-casey Sep 27, 2023 •

edited

Loading

dselman Oct 2, 2023 •

edited

Loading

dselman left a comment •

edited

Loading

dselman Oct 2, 2023 •

edited

Loading

dselman Oct 2, 2023 •

edited

Loading

coveralls commented Oct 20, 2023 •

edited

Loading

dselman left a comment

feat(map): Resolve namespace resolution for Map Types #9

feat(map): Resolve namespace resolution for Map Types #9

Conversation

jonathan-casey commented Sep 26, 2023 • edited Loading

Description

Author Checklist

mttrbrts Sep 26, 2023

Choose a reason for hiding this comment

dselman Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

dselman Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

dselman Sep 26, 2023

Choose a reason for hiding this comment

jonathan-casey Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

dselman Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

dselman left a comment • edited Loading

Choose a reason for hiding this comment

dselman Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

dselman Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

coveralls commented Oct 20, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6588566313

💛 - Coveralls

dselman left a comment

Choose a reason for hiding this comment

jonathan-casey commented Sep 26, 2023 •

edited

Loading

dselman Oct 2, 2023 •

edited

Loading

dselman Oct 2, 2023 •

edited

Loading

jonathan-casey Sep 27, 2023 •

edited

Loading

dselman Oct 2, 2023 •

edited

Loading

dselman left a comment •

edited

Loading

dselman Oct 2, 2023 •

edited

Loading

dselman Oct 2, 2023 •

edited

Loading

coveralls commented Oct 20, 2023 •

edited

Loading