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.

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

Support for References in Map values #799

Open
mttrbrts opened this issue Feb 5, 2024 · 13 comments
Open

Support for References in Map values #799

mttrbrts opened this issue Feb 5, 2024 · 13 comments

Comments

@mttrbrts
Copy link
Member

mttrbrts commented Feb 5, 2024

Feature Request 🛍️

Extend Map Declarations to allow reference types for values.
e.g.

map AddressBook {
  o GUID
  --> Person
}

Use Case

Today, Concerto supports two types of properties in declarations

  1. Child properties, using o notation, https://concerto.accordproject.org/docs/design/specification/model-properties
  2. Relationship properties, using --> notation, https://concerto.accordproject.org/docs/design/specification/model-relationships

Map Declarations are a new feature (which haven't been fully released, draft documentation is here). However, Map declarations only allow map value types to use child objects, e.g.

map AddressBook {
  o GUID
  o Person
}

However, for many scenarios, it would be more semantically correct to allow foreign key relationships to objects rather than forcing containment in the map. For example, an AddressBook doesn't conceptually contain people, it should be a pointer to them instead.

The design for maps does not include relationship types for Map keys.

Possible Solution

Implementing this change requires touching multiple places in the code:

  1. Updating the metamodel to allow Relationship types for Map Values, https://github.com/accordproject/concerto-metamodel
  2. Extending the parser to allow the new syntax, and printing of a model back to CML (concerto-cto)
  3. Extending the introspection API to validate models when they are added to a ModelManager (concerto-core)
  4. Entity validation against a model definition, e.g. are map values in an instance valid relationship references.
  5. Extending all of the other modules to include support for relationships in Map Declarations (E.g. code generation in concerto-codegen)

Context

Detailed Description

@kailash360
Copy link

@mttrbrts

Can you please elaborate this issue?

@mttrbrts
Copy link
Member Author

mttrbrts commented Mar 6, 2024

@kailash360 I've extended the issue description above with a high-level description. This is quite a big piece of work that touches many parts of the stack. I recommend decomposing it into phases.

@apoorvsxna
Copy link
Contributor

Hi @mttrbrts . I've been looking into this issue and would like to take it up.

@apoorvsxna
Copy link
Contributor

apoorvsxna commented Mar 21, 2024

Hi there. I have a doubt. I was looking through the parser.pegjs file and came across this grammar definition for - ObjectMapValueTypeDeclaration

Screenshot (88)

This includes handling of the relationship property syntax.

Also, ObjectMapValueTypeDeclaration is already included in MapValueType

Screenshot (89)

I think the parser should be able to parse relationship properties in the current state. To verify this, I tried testing the parser on the online version of peggy. I tested it using this model.

namespace test

concept Person identified by email {
  o String email
  o DateTime dob
}

map m {
  o String
  --> Person
}

which contains the new syntax.

The input was parsed successfully.

Screenshot (90)

Am I correct in assuming that the parser does not require any changes to support the new syntax?

@apoorvsxna
Copy link
Contributor

Also, when compiling the model with concerto-cli, the following error is received.
Screenshot (91)
which I think is not a syntax error, but rather a "validation(?)" error.

@mttrbrts
Copy link
Member Author

Also, when compiling the model with concerto-cli, the following error is received. which I think is not a syntax error, but rather a "validation(?)" error.

You're right @apoorvsxna. The concerto compile command is for transforming concerto models to other formats. For parsing, try the concerto parse command.

It looks like you're right, the parsing logic appears to already be there. The next step if to extend the introspection API to validate the AST JSON that parsing produces.

@apoorvsxna
Copy link
Contributor

I've made some changes in the metamodel and validation and am now able to compile models containing map values that have the relationship property syntax.
However, there is no change in the resulting compiled model. Does the issue require me to add logic for code generation using the equivalent reference types in each language?

@sanketshevkar
Copy link
Member

Hi @apoorvsxna
Can you please confirm by compiling do you mean parsing?
If not which target language are you compiling it to?

If you are just parsing the cto model to get JSON AST, (if you man compiling = parsing). I don't believe that's going to change.

@apoorvsxna
Copy link
Contributor

apoorvsxna commented Mar 28, 2024

@sanketshevkar I am referring to compiling using concerto compile. I tried compiling two example models- one containing the relationship property syntax and one without it.

I compiled these to Java, and the result was identical, apart from the package name of course.

Model (containing the new relationship map value syntax) -

namespace org.updated

concept Person identified by email {
  o String email
  o DateTime dob
}

map m {
  o String
  --> Person    # relationship map-value
}

Generated code (same for both models- with/without relationship map value)-

// this code is generated and should not be modified
package org.updated;

import concerto.Concept;
import concerto.Asset;
import concerto.Transaction;
import concerto.Participant;
import concerto.Event;
import com.fasterxml.jackson.annotation.*;

@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS, property = "$class")
@JsonIgnoreProperties({"id"})
@JsonIdentityInfo(generator = ObjectIdGenerators.PropertyGenerator.class, property = "email")
public class Person extends Concept {
   
   // the accessor for the identifying field
   public String getID() {
      return this.getEmail();
   }

   private String email;
   private java.util.Date dob;
   public String getEmail() {
      return this.email;
   }
   public java.util.Date getDob() {
      return this.dob;
   }
   public void setEmail(String email) {
      this.email = email;
   }
   public void setDob(java.util.Date dob) {
      this.dob = dob;
   }
}

In order to represent Person as a relationship property, should this be generated a bit differently?

@sanketshevkar
Copy link
Member

The design for maps does not include relationship types for Map keys.

I think this issue requires to add maps to support relationship types as keys. That means we need concerto to parse cto file, generate the JSON AST and validate the metamodel in the AST. This all would reside in Concerto repo.

The compile process is basically code-generation/code-conversion process. This resides in Concerto-Codegen repo.
Once the parsing+validation support is build, then we need to add support for this to codegen repo.

Copy link
Contributor

github-actions bot commented Dec 9, 2024

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.

@github-actions github-actions bot added the Stale label Dec 9, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 21, 2024
@sanketshevkar
Copy link
Member

@mttrbrts @dselman do we see a requirement for this? Should we reopen this?

@sanketshevkar sanketshevkar reopened this Jan 2, 2025
@mttrbrts
Copy link
Member Author

mttrbrts commented Jan 2, 2025

@sanketshevkar

@mttrbrts @dselman do we see a requirement for this? Should we reopen this?

Not urgently. I'd rather focus on getting the existing Map feature fully released. Do we have a ticket for that?

@github-actions github-actions bot removed the Stale label Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants