New Dynamic Model pattern to support type-checked additional properties #23
Conversation
Why: The SDK code generator is being updated to improve the pattern used for dynamic models (i.e. models that support "additionalProperties"). What: The changes in this PR can be summarized as follows: 1) An enhanced DynamicModel class which is now parameterized with the additionalProperties value type and includes a map for them 2) A new DynamicModelTypeAdapterFactory class which is registered with Gson to perform serialization and deserialization of ALL generated DynamicModel subclasses. 3) GsonSerializationHelper is now deprecated as it's no longer needed. 4) Tests related to the above functional changes.
padamstx
force-pushed
the
dynmodel-renovation
branch
from
e0747dd to
4116361
Compare
|
Note that this PR will need to be managed together with a corresponding PR in the openapi-sdkgen project (which will be delivered "real soon now" :) ). The codegen PR will change the way we generate dynamic models so they look like the ModelAPXXX classes that are included in this PR. |
|
Note: this PR will need to be approved and merged into the master branch, then a new tagged release will need to be created before the corresponding openapi-sdkgen changes can be submitted in a PR. This is because the code generator has a build-time dependency on the java core (we have unit tests that verify that certain generated models will compile cleanly) and the codegen PR will need to include a change to its pom.xml that points to the new release of the java core package that includes these DynamicModel changes. |
|
|
||
| /** | ||
| * Returns the TypeToken which describes the type of additional properties stored in the map. | ||
| * @return The TypeToken which descriptes the map value type |
| /** | ||
| * An adapter for serializing/deserializing instances of type T, where T represents a generated dynamic model class | ||
| * which is a subclass of DynamicModel. Subclasses of DynamicModel will have zero or more explicitly-defined fields | ||
| * plus an inherited map to store addtional (arbitrary) properties. |
There was a problem hiding this comment.
I think "inherited" is not accurate -- the map is a proper member variable if I understand the code.
Typo: "addtional"
There was a problem hiding this comment.
Well, technically the generated model class does inherit the map... from DynamicModel :)
But I changed the wording slightly.
| * the name of the property to set | ||
| * @param value | ||
| * the value of the property to be set | ||
| * @return the previous value of the property, or null if the property was not previously set |
There was a problem hiding this comment.
The convention for all other setters in model classes is to return void. Is there good reason to break with that convention here?
There was a problem hiding this comment.
Yeah, the reason I followed the "Map.put" model is so that I could easily tell if we have a duplicate property while deserializing a JSON string (by checking to see if the result of the set operation is non-null). Technically, I don't need to do it this way... i could first check to see if a value already exists for a given key, but the Map "put" method (which is really just wrapped by DynamicModel.setProperty) does that all in one step so thought it would be better to just follow that same pattern with the setProperty method.
| @@ -0,0 +1,85 @@ | |||
| /* | |||
| * Copyright (C) 2011 Google Inc. | |||
There was a problem hiding this comment.
Do we want an IBM Copyright on top of Googles?
There was a problem hiding this comment.
I'll also add the Google copyright prolog to the DynamicModelTypeAdapterFactory class since some of that code was modeled after a couple of the Gson internal type adapters.
|
I pushed another commit to address the review comments |
|
cc: @germanattanasio |
|
cc: @ricellis |
Why:
The SDK code generator is being updated to improve the pattern used for
dynamic models (i.e. models that support "additionalProperties").
What:
The changes in this PR can be summarized as follows:
additionalProperties value type and includes a map for them
to perform serialization and deserialization of ALL generated DynamicModel
subclasses.