-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
fix(kotlin-spring): add parentCtorCallNeeded model template property #17008
base: master
Are you sure you want to change the base?
Conversation
The new parentCtorCallNeeded property can be used by the template to determine if there should be a constructor invocation on the parent type. Right now its value is computed in a way that could probably use some improvement because it is language specifically checking if the base type is a kotlin map or not. This might seem overly brittle, but was the only way I could differentiate between the two different edge cases that are present in the repro spec. This was the previous commit that changed the default behavior from one edge case to the other (both are wrong because we can only know for sure at generation time whether a constructor call will be necessary or not). OpenAPITools@0bb08a7 Below is a yaml OpenAPI spec document that can be used to test/verify the currently known edge cases (which were also mentioned in the GH issue tracker's comments) ```yaml openapi: 3.0.3 info: version: "1.0.0" title: "" paths: /documents/v1: get: responses: 200: description: lorem content: application/json: schema: type: array items: $ref: "#/components/schemas/ParentSchema" components: schemas: JarFiles: type: array items: $ref: "#/components/schemas/JarFile" JarFile: type: object required: - filename - contentBase64 - hasDbMigrations additionalProperties: true properties: filename: type: string nullable: false minLength: 1 maxLength: 255 hasDbMigrations: description: Indicates whether the cordapp jar in question contains any embedded migrations that Cactus can/should execute between copying the jar into the cordapp directory and starting the node back up. type: boolean nullable: false contentBase64: type: string format: base64 nullable: false minLength: 1 maxLength: 1073741824 ParentSchema: type: object properties: id: type: string type: $ref: "#/components/schemas/DiscriminatingType" required: - id - type discriminator: propertyName: type mapping: subtypeA: "#/components/schemas/SubtypeA" subtypeB: "#/components/schemas/SubtypeB" subtypeC: "#/components/schemas/SubtypeC" SubtypeA: type: object allOf: - $ref: '#/components/schemas/ParentSchema' - type: object properties: subtypeAproperty: type: integer SubtypeB: type: object allOf: - $ref: '#/components/schemas/ParentSchema' - type: object properties: subtypeBproperty: type: string SubtypeC: type: object additionalProperties: true allOf: - $ref: '#/components/schemas/ParentSchema' - type: object properties: subtypeAproperty: type: integer DiscriminatingType: type: string enum: - subtypeA - subtypeB - subtypeC ``` Fixes OpenAPITools#8366 Signed-off-by: Peter Somogyvari <[email protected]>
…ler errors 1. Started overriding a specific template (dataClass.mustache) in the main-server sub-package of the corda connector because of issues that are further explained here [1] and here [2]. 2. Also had to update generator configuration to specifically exclude spring-doc generation because it seems to be broken within the template as well: it does not provide updated dependencies for the grandle and maven manifests and so the `io.swagger.core.v3:swagger-annotations` package was missing and failing the build in a second way. 3. The example value for the return array of `ListFlowsV1Response` in the openapi.json spec file of the corda connector was containing dollar signs ($) which ended up being appended to the kotlin code's annotations as documentation, but the dollar signs have a special meaning in kotlin and lead to syntax errors. Updating the examples to not have dollar signs in the openapi.json specification document resulted in fixing this issue. 4. Also updated the artifact version in the openapi generator configuration file. This is just a temporary fix, what we really need is scripts bumping this up as part of the automated release process. [1] OpenAPITools/openapi-generator#8366 (comment) [2] OpenAPITools/openapi-generator#17008 Fixes hyperledger-cacti#2662 Signed-off-by: Peter Somogyvari <[email protected]>
…ler errors 1. Started overriding a specific template (dataClass.mustache) in the main-server sub-package of the corda connector because of issues that are further explained here [1] and here [2]. 2. Also had to update generator configuration to specifically exclude spring-doc generation because it seems to be broken within the template as well: it does not provide updated dependencies for the grandle and maven manifests and so the `io.swagger.core.v3:swagger-annotations` package was missing and failing the build in a second way. 3. The example value for the return array of `ListFlowsV1Response` in the openapi.json spec file of the corda connector was containing dollar signs ($) which ended up being appended to the kotlin code's annotations as documentation, but the dollar signs have a special meaning in kotlin and lead to syntax errors. Updating the examples to not have dollar signs in the openapi.json specification document resulted in fixing this issue. 4. Also updated the artifact version in the openapi generator configuration file. This is just a temporary fix, what we really need is scripts bumping this up as part of the automated release process. [1] OpenAPITools/openapi-generator#8366 (comment) [2] OpenAPITools/openapi-generator#17008 Fixes hyperledger-cacti#2662 Signed-off-by: Peter Somogyvari <[email protected]>
…ler errors 1. Started overriding a specific template (dataClass.mustache) in the main-server sub-package of the corda connector because of issues that are further explained here [1] and here [2]. 2. Also had to update generator configuration to specifically exclude spring-doc generation because it seems to be broken within the template as well: it does not provide updated dependencies for the grandle and maven manifests and so the `io.swagger.core.v3:swagger-annotations` package was missing and failing the build in a second way. 3. The example value for the return array of `ListFlowsV1Response` in the openapi.json spec file of the corda connector was containing dollar signs ($) which ended up being appended to the kotlin code's annotations as documentation, but the dollar signs have a special meaning in kotlin and lead to syntax errors. Updating the examples to not have dollar signs in the openapi.json specification document resulted in fixing this issue. 4. Also updated the artifact version in the openapi generator configuration file. This is just a temporary fix, what we really need is scripts bumping this up as part of the automated release process. [1] OpenAPITools/openapi-generator#8366 (comment) [2] OpenAPITools/openapi-generator#17008 Fixes hyperledger-cacti#2662 Signed-off-by: Peter Somogyvari <[email protected]> (cherry picked from commit 99ec2ff)
…ler errors 1. Started overriding a specific template (dataClass.mustache) in the main-server sub-package of the corda connector because of issues that are further explained here [1] and here [2]. 2. Also had to update generator configuration to specifically exclude spring-doc generation because it seems to be broken within the template as well: it does not provide updated dependencies for the grandle and maven manifests and so the `io.swagger.core.v3:swagger-annotations` package was missing and failing the build in a second way. 3. The example value for the return array of `ListFlowsV1Response` in the openapi.json spec file of the corda connector was containing dollar signs ($) which ended up being appended to the kotlin code's annotations as documentation, but the dollar signs have a special meaning in kotlin and lead to syntax errors. Updating the examples to not have dollar signs in the openapi.json specification document resulted in fixing this issue. 4. Also updated the artifact version in the openapi generator configuration file. This is just a temporary fix, what we really need is scripts bumping this up as part of the automated release process. [1] OpenAPITools/openapi-generator#8366 (comment) [2] OpenAPITools/openapi-generator#17008 Fixes hyperledger-cacti#2662 Signed-off-by: Peter Somogyvari <[email protected]>
…ler errors 1. Started overriding a specific template (dataClass.mustache) in the main-server sub-package of the corda connector because of issues that are further explained here [1] and here [2]. 2. Also had to update generator configuration to specifically exclude spring-doc generation because it seems to be broken within the template as well: it does not provide updated dependencies for the grandle and maven manifests and so the `io.swagger.core.v3:swagger-annotations` package was missing and failing the build in a second way. 3. The example value for the return array of `ListFlowsV1Response` in the openapi.json spec file of the corda connector was containing dollar signs ($) which ended up being appended to the kotlin code's annotations as documentation, but the dollar signs have a special meaning in kotlin and lead to syntax errors. Updating the examples to not have dollar signs in the openapi.json specification document resulted in fixing this issue. 4. Also updated the artifact version in the openapi generator configuration file. This is just a temporary fix, what we really need is scripts bumping this up as part of the automated release process. [1] OpenAPITools/openapi-generator#8366 (comment) [2] OpenAPITools/openapi-generator#17008 Fixes #2662 Signed-off-by: Peter Somogyvari <[email protected]>
…ler errors 1. Started overriding a specific template (dataClass.mustache) in the main-server sub-package of the corda connector because of issues that are further explained here [1] and here [2]. 2. Also had to update generator configuration to specifically exclude spring-doc generation because it seems to be broken within the template as well: it does not provide updated dependencies for the grandle and maven manifests and so the `io.swagger.core.v3:swagger-annotations` package was missing and failing the build in a second way. 3. The example value for the return array of `ListFlowsV1Response` in the openapi.json spec file of the corda connector was containing dollar signs ($) which ended up being appended to the kotlin code's annotations as documentation, but the dollar signs have a special meaning in kotlin and lead to syntax errors. Updating the examples to not have dollar signs in the openapi.json specification document resulted in fixing this issue. 4. Also updated the artifact version in the openapi generator configuration file. This is just a temporary fix, what we really need is scripts bumping this up as part of the automated release process. [1] OpenAPITools/openapi-generator#8366 (comment) [2] OpenAPITools/openapi-generator#17008 Fixes hyperledger-cacti#2662 Signed-off-by: Peter Somogyvari <[email protected]>
Any update on this PR? I would love to have this fix released. |
@benjamineckstein I haven't heard anything yet unfortunately. |
} | ||
return false; | ||
} | ||
|
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.
these logics are only for kotlin. what about moving these to postProcessModels in abstract kotlin generators (or just kotlin spring generator) and store the value in x-is-parent-ctor-call-needed instead?
e.g.
Line 873 in f323a3e
for (ModelMap mo : objects.getModels()) { |
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.
@wing328 Thank you for the review!
these logics are only for kotlin. what about moving these to postProcessModels in abstract kotlin generators (or just kotlin spring generator) and store the value in x-is-parent-ctor-call-needed instead
Sure, but please bear with me. I've lost the dev env in the meantime so will have to set everything up from scratch for a full test again.
Quick question: Would I also have to move property out of modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenModel.java
onto some kotlin specific model (if that exists) or is that OK where it is right now?
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.
Line 876 in f323a3e
cm.vendorExtensions.put("x-has-data-class-body", true); |
The new parentCtorCallNeeded property can be used by the template to
determine if there should be a constructor invocation on the parent type.
Right now its value is computed in a way that could probably use some
improvement because it is language specifically checking if the base type
is a kotlin map or not.
This might seem overly brittle, but was the only way I could differentiate
between the two different edge cases that are present in the repro spec.
This was the previous commit that changed the default behavior from one
edge case to the other (both are wrong because we can only know for sure
at generation time whether a constructor call will be necessary or not).
0bb08a7
Below is a yaml OpenAPI spec document that can be used to test/verify
the currently known edge cases (which were also mentioned in the GH
issue tracker's comments)
Fixes #8366
Signed-off-by: Peter Somogyvari [email protected]
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.1.0 minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)@jimschubert (2017/09) ❤️, @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04) @andrewemery (2019/10) @4brunu (2019/11) @yutaka0m (2020/03) @stefankoppier (2022/06)