-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Use enum type for discriminator #13846
Changes from 1 commit
9f35097
74093a7
d53f07f
0cf0de7
2519d25
e07aca2
c8b4a9f
0e08a4a
d1964ab
e0f1b1c
7857121
77cc895
268da29
135f677
0b0a69f
bb7bbd9
82e05e3
ac76b0c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3385,8 +3385,16 @@ protected CodegenDiscriminator createDiscriminator(String schemaName, Schema sch | |
discriminator.setPropertyBaseName(sourceDiscriminator.getPropertyName()); | ||
discriminator.setPropertyGetter(toGetter(discriminator.getPropertyName())); | ||
|
||
// FIXME: for now, we assume that the discriminator property is String | ||
discriminator.setPropertyType(typeMapping.get("string")); | ||
// FIXME: there are other ways to define the type of the discriminator property (inline | ||
// for example). Handling those scenarios is too complicated for me, I'm leaving it for | ||
// the future.. | ||
String propertyType = | ||
Optional.ofNullable(schema.getProperties()) | ||
.map(p -> (Schema) p.get(discPropName)) | ||
.map(Schema::get$ref) | ||
.map(ModelUtils::getSimpleRef) | ||
.orElseGet(() -> typeMapping.get("string")); | ||
discriminator.setPropertyType(propertyType); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will impact more than just the JavaSpring generation - can you regenerate the samples and tests to see what impact this has on other clients? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @welshm I've run the commands in the PR instructions above. No files changed to commit to Git. I'm not certain that it worked properly, but the scripts seemed to run without errors. |
||
|
||
// check to see if the discriminator property is an enum string | ||
if (schema.getProperties() != null && | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -170,6 +170,37 @@ components: | |
type: integer | ||
oneOf: | ||
- $ref: '#/components/schemas/FruitType' | ||
FruitTypeEnum: | ||
type: string | ||
enum: [APPLE, BANANA] | ||
FruitOneOfEnumMappingDisc: | ||
type: object | ||
properties: | ||
fruitType: | ||
$ref: "#/components/schemas/FruitTypeEnum" | ||
required: fruitType | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The required property must be an array. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I see the problem now. I've pushed a fix. |
||
oneOf: | ||
- $ref: '#/components/schemas/AppleOneOfEnumMappingDisc' | ||
- $ref: '#/components/schemas/BananaOneOfEnumMappingDisc' | ||
discriminator: | ||
propertyName: fruitType | ||
mapping: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The mapping to the enum value might cause problems with the auto generated JsonSubType. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually generated a sample Project:
@welshm Isn't there another issue / pr dealing with duplicated JsonSubTypes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bernie-schelberg-mywave Could you please add a enum based discriminator to Ione of the generated sample projects (for example There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm running openapi-generator version 6.2.0 and the above is the current behaviour when you add a mapping to the discriminator. Both the derived classes and the mapping values are added as JsonSubTypes. The documentations seems to indicate that this is the expected behaviour.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cachescrubber Yes, there's another project for duplicated JsonSubTypes: #13815 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done (added enum based discriminator to samples) |
||
APPLE: '#/components/schemas/AppleOneOfEnumMappingDisc' | ||
BANANA: '#/components/schemas/BananaOneOfEnumMappingDisc' | ||
AppleOneOfEnumMappingDisc: | ||
type: object | ||
required: | ||
- seeds | ||
properties: | ||
seeds: | ||
type: integer | ||
BananaOneOfEnumMappingDisc: | ||
type: object | ||
required: | ||
- length | ||
properties: | ||
length: | ||
type: integer | ||
FruitGrandparentDisc: | ||
oneOf: | ||
- $ref: '#/components/schemas/AppleGrandparentDisc' | ||
|
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.
nit: I know this isn't a variable you named, but I think full naming is better to disambiguate.
discriminatorPropertyName
might be more clear what the variable clearly representsThere 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.
Variable has been renamed, please review again