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

feat(core): infer $class when obvious #577

Closed
wants to merge 2 commits into from

Conversation

mttrbrts
Copy link
Member

@mttrbrts mttrbrts commented Dec 16, 2022

Closes #542

This change allows us to validate and deserialize JSON objects where the root type is known, but where no $class properties for nested objects are provided. This will work for basic cases where there is only one possible concrete subtype.

For example, an object such as this will validate provided that we tell the validator what the root type is.

{
    "$class" : "test.Vehicle",
    "color": "red",
    "wheels" : [{
        "brand" : "Michelin"
    }],
    "owner": {
        "email": "[email protected]"
    }
}

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
  • Extend the documentation, if necessary
  • Merging to main from fork:branchname

// class/subclass we can choose that instead
const subclasses = classDeclaration.getAssignableClassDeclarations()
.filter(clazz => !clazz.isAbstract());
if (subclasses.length === 1){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is risky imo because it opens up a scenario whereby deserialisation can break because you've simply added a new concept which extends an abstract type. This will be confusing to document and to understand.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be much safer (and more work!) if this was explicit in the model, via a final declaration. Properties, or arrays, of final types would never need $class when serialised or deserialised.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could map final to sealed in C# and to final in Java.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will also need to check the same logic applies to relationships.

--> IAddress address vs. --> Address address

@mttrbrts
Copy link
Member Author

Closing in light of @dselman's comments.
#542 (comment)

@mttrbrts mttrbrts closed this Dec 21, 2022
@DS-AdamMilazzo
Copy link

I agree with Dan's comment about deserialization. The approach I recommend is to simply treat an object as an instance of the declared field type if it's not annotated. This should be even simpler to implement than this PR (because you don't have to care about how many concrete assignable types there may be), more general, and would not break in the case Dan describes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Concerto should allow $class to be omitted or abbreviated when it's obvious from context
3 participants