[csharp][java] Fix enum discriminator default value #19614
Conversation
david-marconis
force-pushed
the
feature/fix-enum-discriminator
branch
from
4bd0ec3 to
9c0a126
Compare
|
This has been sitting for a month, so I'll ping the technical committee for csharp and java: @mandrean (2017/08) @shibayan (2020/02) @Blackclaws (2021/03) @lucamazzanti (2021/05) @iBicha (2023/07) @bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04) @lwlee2608 (2019/10) @martin-mfg (2023/08) |
|
Hi @david-marconis, thanks for the PR! Using public Dog() {
this.petType = this.getClass().getSimpleName();
}The new constructor is now public Dog() {
}Instead, shouldn't we do either this: public Dog() {
this.petType = PetTypeEnum.DOG;
}... or this? public Dog() {
this.petType=PetTypeEnum.fromValue("Dog");
} |
That is a fair point and this is actually solved in the c# codegen, but not Java. I'll have a stab at it when I get the time. |
|
@martin-mfg I tried my hand at fixing it and it wasn't as straight forward as I wanted. I utilized the same solution as C# in order to solve the case for Java as well (setting I also discovered that there is still one bug where it will not set the isEnum properly on the GrandparentAnimal:
type: object
required:
- pet_type
properties:
pet_type:
type: string
discriminator:
propertyName: pet_type
ParentPet:
type: object
allOf:
- $ref: '#/components/schemas/GrandparentAnimal'
ChildCat:
allOf:
- $ref: '#/components/schemas/ParentPet'
- type: object
properties:
name:
type: string
pet_type:
x-enum-as-string: true
type: string
enum:
- ChildCat
default: ChildCat
required:
- pet_typeThis won't work, because the property Let me know if this is a good solution or if we should try to solve it in a different way. Perhaps with a new property instead of utilizing defaultValue of a readWriteVar. |
There was a problem hiding this comment.
I think this case can be ignored for now though as it is an unusual case.
Yes, I agree.
Let me know if this is a good solution or if we should try to solve it in a different way. Perhaps with a new property instead of utilizing defaultValue of a readWriteVar.
Generally I don't see a problem with the current approach. But please take care of the following "Nonnull" issue, as well as the inline comment below:
Using your added example input spec, here's how this PR's changes affect the generated code: https://gist.github.com/martin-mfg/d4e28b386eab99e53374eef866cc1a50 . Besides the desired enum initialization changes, it's also removing some [PS: The "Nonnull" change is caused by unrelated commits on the master branch.]@javax.annotation.Nonnull annotations. I guess this change was not intended and should be avoided/reverted.
| protected static String getEnumValueForProperty( | ||
| String modelName, CodegenDiscriminator discriminator, CodegenProperty var) { | ||
| if (!discriminator.getIsEnum() && !var.isEnum) { | ||
| return null; |
There was a problem hiding this comment.
In this case, I guess we shouldn't override the defaultValue at all. But currently we're overriding it with null. In line 3125. Or am I wrong?
There was a problem hiding this comment.
Thanks for pointing this out. I changed it to use the existing defaultValue instead of null.
If I set defaultValue to the enum discriminator in DefaultCodegen then there is only this change to dart-dio when I generate the samples:
diff --git a/samples/openapi3/client/petstore/dart-dio/petstore_client_lib_fake/doc/ChildWithNullable.md b/samples/openapi3/client/petstore/dart-dio/petstore_client_lib_fake/
doc/ChildWithNullable.md
index 770494fcf4c..d3e5b182076 100644
--- a/samples/openapi3/client/petstore/dart-dio/petstore_client_lib_fake/doc/ChildWithNullable.md
+++ b/samples/openapi3/client/petstore/dart-dio/petstore_client_lib_fake/doc/ChildWithNullable.md
@@ -8,7 +8,7 @@ import 'package:openapi/api.dart';
## Properties
Name | Type | Description | Notes
------------ | ------------- | ------------- | -------------
-**type** | **String** | | [optional]
+**type** | **String** | | [optional] [default to ChildWithNullableTypeEnum.childWithNullable]
**nullableProperty** | **String** | | [optional]
**otherProperty** | **String** | | [optional]I am not sure if this change is fine or if I should just keep it to Java and C# for now, but this could also be a problem that needs to be solved for other languages.
|
And just for the record I want to mention two more things. They don't need to be fixed here though.
|
I'm not quite sure how to fix this. It has to do with this method: public static boolean shouldGenerateFreeFormObjectModel(String name, CodegenConfig config) {
// there are 3 free form use cases
// 1. free form with no validation that is not allOf included in any composed schemas
// 2. free form with validation
// 3. free form that is allOf included in any composed schemas
// this use case arises when using interface schemas
// generators may choose to make models for use case 2 + 3
Schema refSchema = new Schema();
refSchema.set$ref("#/components/schemas/" + name);
Schema unaliasedSchema = config.unaliasSchema(refSchema);
return unaliasedSchema.get$ref() != null;
}But it is unclear to me how it should be modified and what further implications that would cause.
I investigated this issue and found a few bugs in the
This eliminates the warnings seen in the logs and probably fixes these issues: After I fixed these two bugs, I saw that I could actually set the |
There was a problem hiding this comment.
Thanks for the additional effort regarding the warnings!
This eliminates the warnings seen in the logs
I actually still get the following warnings:
[main] WARN org.openapitools.codegen.utils.ModelUtils - Failed to get the schema name: null
[main] WARN org.openapitools.codegen.DefaultCodegen - 'Lizard' defines discriminator 'petType', but the referenced OneOf schema 'Chameleon' is missing petType
Is the behaviour different for you?
Since the PR already looks good to me, I'll approve it now.
You are of course very welcome to have another look at the above warnings and improve the situation there if you want. Possibly in a new PR if this one here gets merged.
Whack one down and another resurfaces 😅. I have fixed it again; now there should be no more warnings. There was another bug in the Also the "Failed to get schema" was an error on my part, I left in a useless line of code. |
|
@martin-mfg, @wing328: Any chance of getting this merged soon? |
|
hey, any update on this PR? |
No; it's waiting to be merged by the project maintainer when he has time. |
|
@wing328 Can you please have a look at this. It has been approved for almost two months now. If there is anything else I need to add or do, please let me know. |
|
@david-marconis Hi David, thanks for the PR and sorry for the delay in reviewing as we've got many many PRs from the awesome community. I'll review this week and let you know if I've any questions or simply merge if I don't spot anything. |
|
I tested with but got errors when running Does it build without issues for your locally using the test included in this PR? |
modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
Show resolved
Hide resolved
modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
Show resolved
Hide resolved
|
cc @OpenAPITools/generator-core-team |
No. it fails on my end as well. I discussed this issue in this comment and I am unsure on how to fix it. This issue is not really related to the discriminator being an enum, as it fails if I remove the enum block from the CarType, and this issue is also present in the repo before this PR. So I think it could be filed as an issue and be solved in another PR instead of this one. |
|
thanks for the explanation. will try to get this merged this week |
When you define a schema with an enum discriminator and you use allOf to include the parent schema in the child schema the current generated code fails to compile due to the generator thinking the discriminator is of type string instead of enum.
This is mentioned in some issues I could find which this PR should fix:
#12412 #16073 #16672 #19194 #19261
These PRs attempt to solve it, but they have some flaws.
#10959 #16394
This was tested with
javaandcsharpgenerators which both have issues. Other generators might also have issues with this if they implement custom logic of handling default values of enums like thecsharpgenerator currently does.The issue in the code lied in the fact that when checking whether or not the discriminator is an enum it only checks the properties of the child schema, however the property might be defined in the referenced parent schema.
This can be verified by running the added tests against the current master and observe them fail.
The code was modified to return the referenced schema in addition to the discriminator when searching for it recursively and when checking for the property check both in the child schema and the schema that defines the discriminator.I am unsure if it is possible to have a situation where the property is neither in the child schema or the schema with the discriminator, but in an intermediary in the ref chain. This would be uncommon and I am unsure if it is possible with the current code, but I don't understand it well enough to tell whether this is possible or if it is a valid spec. Perhaps it would be better to traverse up the allOf reference chain, but I an not familiar enough with how objects can be defined with combinations of allOf, anyOf and oneOf. At least this implementation avoids another recursive traversal of the schema tree. Let me know if this should be changed or if the new implementation is good enough.I changed the code to check all parents and all descendants for the discriminator property. This should cover most if not all possible use cases. Here is the schema I used with a mix of anyOf, allOf and oneOf mappings:
openapi-generator/modules/openapi-generator/src/test/resources/3_0/enum_discriminator_inheritance.yaml
Lines 1 to 128 in 452e95b
PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master(upcoming 7.6.0 minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)