Map does not support polymorphic values

[dselman]

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

[kshitij79]

@dselman This pull request needs review.

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

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 10 days.

[Mamatha1718]

Hi @dselman @DianaLease . I would like tackle this issue. Thank you for the detailed explanation. You're correct that abstract types like Animal cannot be instantiated directly, as they are intended to be used as base types for concrete subclasses like Dog or Cat. I understand the issue and would like to tackle it by ensuring that the system properly handles polymorphic maps. Specifically, we should enhance the deserialization process to check the $class property of the map values and instantiate the correct concrete type instead of attempting to instantiate the abstract type directly.
Thank You.

[DianaLease]

@Mamatha1718 I have assigned the issue to you, thank you!

[Mamatha1718]

Hi @DianaLease , Thank you for assigning the issue to me. I've made a PR, but I’m facing an issue. In the time of npm test
One of the screenshots I mentioned shows the following error: fromJSON <> toJSON not specified $class .

Could you please help me understand why this error is occurring? I believe it might be related to how the class types are not properly defined or linked during serialization/deserialization. Could you clarify how to resolve this?
Thank You.

[sanketshevkar]

@DianaLease shouldn't we merge the PR by Kshitij, if its not conflicting with any other changes made after that PR?
#827

@mttrbrts has already approved the PR and you and Dan seemed fine with the changes as well.