-
-
Notifications
You must be signed in to change notification settings - Fork 111
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(serializer): add inferClass option #861
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Dan Selman <[email protected]>
Signed-off-by: Dan Selman <[email protected]>
Yes, I say so
We need to be backwards compatible. So I say, no for v3, but this sounds reasonable for v4 |
This change has the side effect of making some types implicitly final, if they weren't in scope of the model manager when the serialisation occurred. For example, it's possible that a namespace is unambiguous to one client, and ambiguous to another. |
Yes, this one is worrying. I don't see a good solution to this, in as far as adding a new model file to the model manager could have a side-effect on serialisation of types, other than adding an explicit One solution could be to store the unambiguous "final types" on the root node when we call
|
Signed-off-by: Dan Selman <[email protected]>
Signed-off-by: Dan Selman <[email protected]>
In the real world, I see this optimization as hugely beneficial for fixed scope scenarios, such as the following:
In these cases, it's a fair assumption that the namespace domain is stable (e.g. it only contains the metamodel definitions), so the ambiguous cases should not arise. Use of an explicit Serialisation of userland models is more dangerous unless the system can also persist the namespace domain and make an assumption like you suggest. Making everything implicitly |
Adhoc testing shows about 1/3 file size reduction for a 29Mb model definition, and 73Mb Decorator Command Set file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll likely need similar changes to the metamodel utils and decorator manager to allow loading and printing of modelfile definitions and dcss with inferred classes.
Signed-off-by: Dan Selman <[email protected]>
… ds-serialization-infer-class
Simplified inference logic to assume the type of a field, if $class is missing. |
Signed-off-by: Dan Selman <[email protected]>
Before we can load AST created with
Which does not use |
Nicely done. The size reduction is still around 1/3 for metamodel and DCS, which is good news! Are you going to patch up the modelfile definition (and similar) in this PR too? |
Yes. I will add a test that does the full roundtrip with inferClass true/false on a meta model... |
Signed-off-by: Dan Selman <[email protected]>
Signed-off-by: Dan Selman <[email protected]>
Signed-off-by: Dan Selman <[email protected]>
… ds-serialization-infer-class
Signed-off-by: Dan Selman <[email protected]>
Signed-off-by: Dan Selman <[email protected]>
This PR is stale because it has been open 15 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
Closes #482 #542
Adds the option
inferClass
to theSerializer
(false by default). When this option istrue
$class
will only by included in the JSON created forResource
objects, in the following circumstances:In addition, when a
$class
attribute is included it will be shortened (the namespace is removed) when the namespace of theResource
is the same as the namespace of the type of the property (see examples).Changes
JSONGenerator
updated to remove $class from JSON when it can be inferredJSONPopulator
updated to infer $class FQN for objects when it is short or missingfromAst
methods updated to support either short or fully-qualified names for the meta modelFlags
inferClass
when model manager is in strict mode?Examples
Using these two model files:
And:
Example 1
This example will deserialize with
inferClass=true
.person
property has the$class
valueOwner
because it is not of the type of the field (Person
) but is of type[email protected]
which is in the same namespace as the field type, so is shortened to justOwner
.animals
array has a $class ofDog
. A $class is required because the type of the property isAnimal
and there are two types that specialiseAnimal
:Cat
andDog
, so a discriminator is required.Dog
not[email protected]
because the typeDog
and the typeAnimal
are defined in the same namespace. During deserialisation the serialised assumes that $class short names are defined in the same namespace as the type of their property in the model.person.address
as the type of the field is[email protected]
is the same as the resource.Example 2
This example will deserialize with
inferClass=true
.[email protected]
because the typeCat
and the typeAnimal
are not defined in the same namespace.Example 3
This example will deserialize with
inferClass=true
.owner
and the first element in theanimals
array.Example 4
Here is a meta model instance with
inferClass=true
:Related Issues
Author Checklist
--signoff
option of git commit.main
fromfork:branchname