-
Notifications
You must be signed in to change notification settings - Fork 14
Support for const (and/or single-value enums) in polymorphism #11
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
Comments
After playing around with it a bit more, I found out that if there's no parent or if the intended parent is not the first in "allOf" list, then const is respected by generating the following code in init {
require(type == cg_str0) { "type not constant value $cg_str0 - $type" }
} If I understand it correctly, it could be useful for allowing deserialization framework to instantiate this object by supplying the value as the constructor argument. This would fail if there was no such constructor argument, even though there is only one possible valid value. If that's the reasoning, I understand the limitation, although I don't understand why this limitation is not applied in case of polymorphism too. Still, perhaps it would be possible to have an extra constructor without this field? "For humans"? Or at least reorder fields based on whether they have defaults, so that the fields with defaults are at the end of the constructor arguments? |
Hi - thanks for these suggestions. On the first point, using the As you pointed out in your later comment, when you use I don't like your idea of re-ordering the fields - I think most people would expect constructor parameters to be in the same order as in the schema. Using Kotlin named parameters it's easy to include just the parameters you need. On your second point, support for polymorphism - this is tricky because it depends on the serialization / deserialization library being used. I made an early design decision that the generated code should not contain any dependencies on other libraries, and I like to think that the code can be used by any of the JSON libraries. Here I must declare a bias - I have my own serialization / deserialization library named kjson, and while I know that other libraries have a strong presence (you mentioned Jackson in one of your comments), I think that mine has real advantages. It is native Kotlin, rather than a Kotlin facade on Java, it supports Kotlin classes like It includes polymorphic deserialization - see the section on fromJSONPolymorphic. In my day job, I use this functionality to deserialize CloudEvents, using the Of course, none of this is automated. I think it would be difficult to generate classes that could be deserialized automatically in a polymorphic manner, and any inclusion of library-dependent code would violate the principle I mentioned earlier of not adding dependencies on other libraries. But I'm interested to know how others are tackling the problem, and if a more automated means of deserializing members of a class hierarchy can be found, that could be a valuable addition to the library. |
Thanks for a very informative reply! Regarding kjson library, I think we could take a look into using it, but ultimately one of the reasons we're using Jackson is that it's "tried and true", so we have a lot of experience with using it, how to handle which problems, how it integrates with Spring Framework, what performance we can expect from it, etc. So switching is currently somewhat unlikely. How would you recommend defining jsonschema for support of polymorphism? The way I was trying to do it with "type" property and "const" values in children? |
Regarding changing order of properties. I found a somewhat interesting workaround. If I only mark "type" property as required in parent, but don't actually define it, then I can define it as the last property in each child and it works surprisingly well. Perhaps we could be satisfied with this approach for one more reason: if the parent doesn't have any properties at all, then children are generated as native |
Regarding polymorphism using $schema: https://json-schema.org/draft-07/schema
type: object
required:
- type
properties:
type:
type: string
oneOf:
- $ref: "OtherFields.yaml"
properties:
type:
const: SomethingCreatedEvent
default: SomethingCreatedEvent
- $ref: "OtherFields.yaml"
properties:
type:
const: SomethingUpdatedEvent
default: SomethingUpdatedEvent
Unfortunately it generates nested classes |
In regard to the question of the recommended way of defining classes that are to be deserialized polymorphically - yes, I support the idea of defining a a "type" field in the base schema and redefining the same field as Generating code for a |
Sounds good! Specifically for Jackson's subtype annotations we would probably be OK even with just some empty templates that we could then override ourselves. So that, provided the context contains all the info, we would just customize these ourselves.
I couldn't get it to not generate a name. I tried:
Logically, I can't think of a clean way of doing this with Anyway, thanks for your time, I guess we'll try to use what is available for now. Should I close this issue or do you want to keep it for that missing const validation bug? |
Keep it open - I have worked out a fix for that missing validation bug, and I should be able to get a new version uploaded to Maven Central within 24 hours. I'll notify you here. Regarding I think we're both hitting against the mismatch between the JSON Schema specification and the requirements of polymorphism. In any case, this has been a useful discussion, and I hope you're able to make productive use of the library. |
I couldn't make it generate Parent: $schema: https://json-schema.org/draft-07/schema
title: Event
description: Generic parent for all events
oneOf:
- $ref: SomethingChangedEvent.yaml
- $ref: SomethingUpdatedEvent.yaml Child (has parent, doesn't implement interface): $schema: https://json-schema.org/draft-07/schema
title: Something Changed Event
description: Event when Something changed
allOf:
- $ref: "Something.yaml"
properties:
type:
type: string
const: SomethingChangedEvent
default: SomethingChangedEvent Child (no parent, implements interface): $schema: https://json-schema.org/draft-07/schema
title: Something Changed Event
description: Event when Something changed
$ref: "Something.yaml"
properties:
type:
type: string
const: SomethingChangedEvent
default: SomethingChangedEvent |
First of all, the new version of the project with the bug fix - version 0.78 - has been uploaded to Maven Central. It took longer than I expected because I fixed a number of similar issues related to validations not being applied to overriding property definitions at the same time. I'm going to have to investigate further on the cases you have listed above - thanks for doing this investigative work. I'll let you know what I find. |
You expressed an interest in the idea of adding annotations to the generated code – you may be interested to know that annotations are now available in version 0.82 of the code generator. The configuration is described in the Configuration Guide. I hope this helps with your uses of the code generator. |
Thanks for the note. I'll definitely try it out when I get back to the original initiative. Btw, I did end up using your kjson lib already for a small bit of functionality due to JsonPointer support. Although with kotlinx.serialization being released, it's possible that us and others will slowly migrate to that one in the future. |
Thanks for sharing this project!
Would it be possible to support
const
or single-valueenum
as just pre-set value without constructor parameter?Usecase: We're thinking about using this in our project, but we need to figure out how to support polymorphism in at least semi-intuitive way. I'm thinking the support of
const
would make the first step.E.g., this jsonschema:
Along with this referenced
Event.yaml
parent:Currently generate this child:
But this would be more error-proof if
const
would result in this child being generated:Regarding the usecase, one possible next step for us would be to use Jackson2 mixins to also (manually) add polymorphic deserialization support based on this.
I'm not sure how this should work without polymorphism involved, though - perhaps
type
in this case could be justval
field that is not present in constructor parameters? Is this reasonable?The text was updated successfully, but these errors were encountered: