Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Map does not support polymorphic values #769

Open
dselman opened this issue Nov 29, 2023 · 1 comment · May be fixed by #827
Open

Map does not support polymorphic values #769

dselman opened this issue Nov 29, 2023 · 1 comment · May be fixed by #827
Labels
Difficulty: Medium Type: Bug 🐛 Something isn't working

Comments

@dselman
Copy link
Contributor

dselman commented Nov 29, 2023 •
edited
Loading

Bug Report 🐛

Possibly an enhancement?

Today I tried to use a map with an abstract type as the value:

namespace [email protected]

  abstract concept Animal {}
  concept Dog extends Animal {}
  concept Cat extends Animal {}

  map AnimalsByName {
    o String
    o Animal
  }
  
  concept Zoo {
     o AnimalsByName animalsByName
  }

Code:

const zoo = {
    $class : '[email protected]',
    animalsByName: {
      'fido' : {
        $class : '[email protected]',      
      },
    }
  }

  const factory = new Factory(mm);
  const serializer = new Serializer(factory, mm);
  serializer.fromJSON(zoo)

Expected Behavior

Should be able to deserialise a map that uses abstract types as values.

Current Behavior

Error: Cannot instantiate the abstract type "Animal" in the "[email protected]" namespace.
    at Factory.newResource (/home/runner/AccordProjectConcerto-Polymorphic-Maps/node_modules/@accordproject/concerto-core/lib/factory.js:96:19)
    at Factory.newConcept (/home/runner/AccordProjectConcerto-Polymorphic-Maps/node_modules/@accordproject/concerto-core/lib/factory.js:177:21)
    at JSONPopulator.processMapType (/home/runner/AccordProjectConcerto-Polymorphic-Maps/node_modules/@accordproject/concerto-core/lib/serializer/jsonpopulator.js:220:50)
    at /home/runner/AccordProjectConcerto-Polymorphic-Maps/node_modules/@accordproject/concerto-core/lib/serializer/jsonpopulator.js:195:30
    at Map.forEach (<anonymous>)
    at JSONPopulator.visitMapDeclaration (/home/runner/AccordProjectConcerto-Polymorphic-Maps/node_modules/@accordproject/concerto-core/lib/serializer/jsonpopulator.js:183:17)
    at JSONPopulator.visit (/home/runner/AccordProjectConcerto-Polymorphic-Maps/node_modules/@accordproject/concerto-core/lib/serializer/jsonpopulator.js:125:25)
    at MapDeclaration.accept (/home/runner/AccordProjectConcerto-Polymorphic-Maps/node_modules/@accordproject/concerto-core/lib/introspect/decorated.js:70:24)
    at JSONPopulator.convertItem (/home/runner/AccordProjectConcerto-Polymorphic-Maps/node_modules/@accordproject/concerto-core/lib/serializer/jsonpopulator.js:297:34)

Possible Solution

I think the bug is in processMapType. We are calling newConcept with the name of the map value type, whereas we should be using the $class in value, and we should check that value is assignable to map value.

E.g. see this code in ResourceValidator:

                // is it compatible?
                if(!ModelUtil.isAssignableTo(classDeclaration.getModelFile(), classDeclaration.getFullyQualifiedName(), field)) {
                    ResourceValidator.reportInvalidFieldAssignment(parameters.rootResourceIdentifier, propName, obj, field);
                }

processMapType(mapDeclaration, parameters, value, type) {
        let decl = mapDeclaration.getModelFile()
            .getAllDeclarations()
            .find(decl => decl.name === type);

        // if its a ClassDeclaration, populate the Concept.
        if (decl?.isClassDeclaration()) {
            let subResource = parameters.factory.newConcept(decl.getNamespace(),
                decl.getName(), decl.getIdentifierFieldName() );

            parameters.jsonStack.push(value);
            parameters.resourceStack.push(subResource);
            return decl.accept(this, parameters);
        }
        // otherwise its a scalar value, we only need to return the primitve value of the scalar.
        return value;
    }

Steps to Reproduce

  1. REPL: https://replit.com/@dselman/AccordProjectConcerto-Polymorphic-Maps

Context (Environment)

Concerto v3.14.2

Detailed Description

Possible Implementation
@dselman dselman added Type: Bug 🐛 Something isn't working Difficulty: Medium labels Nov 29, 2023
@dselman dselman mentioned this issue Jan 5, 2024
Merged
5 tasks
@kshitij79 kshitij79 linked a pull request Mar 18, 2024 that will close this issue
Open
5 tasks
@kshitij79
Copy link

@dselman This pull request needs review.

Currently, it lacks test cases, but I will implement them if the changes are in the right direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Difficulty: Medium Type: Bug 🐛 Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(jsonpopulator) Updated processMapType to handle nested abstract c… kshitij79/concerto
2 participants
@dselman @kshitij79