Skip to content
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

[Java Spring OAS3] Fix numerous OAS3 related Bugs #11181

Merged

Conversation

cachescrubber
Copy link
Contributor

@cachescrubber cachescrubber commented Dec 23, 2021

#9775 introduced in 5.3.1 causes numerous bugs when generating code.

This PR fixes the following Bugs

#11168
#11165
#11164
#11163
#11007
#11203
#11218

This PR implements the following Requests / Improvements

#10409

This PR resolves other, older PRs

#10879

PR checklist

  • [ x] Read the contribution guidelines.
  • [ x] Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • [x ] Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    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*.
    For Windows users, please run the script in Git BASH.
  • [x ] File the PR against the correct branch: master (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

Sorry, something went wrong.

@cachescrubber
Copy link
Contributor Author

Cc: @wing328 , @welshm. Please have a look at this.

Copy link
Contributor

@welshm welshm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing a lot of the issues I introduced! I think the paramDoc mustache is a great simplification overall and makes this much easier to understand

import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.Parameter;
import io.swagger.v3.oas.annotations.media.Content;
import io.swagger.v3.oas.annotations.media.Schema;
import io.swagger.v3.oas.annotations.parameters.RequestBody;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's value in having both annotations present - but open to removing if no one else feels the same way

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would address issue when we would actually need io.swagger.v3.oas.annotations.parameters.RequestBody. So far, the import is unused, hence the removal.

Copy link
Contributor

@welshm welshm Dec 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used - by deleting this import the "@RequestBody" annotation is changing to org.springframework.web.bind.annotation.RequestBody from the import org.springframework.web.bind.annotation.*

Copy link
Contributor Author

@cachescrubber cachescrubber Dec 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this is how it should be - org.springframework.web.bind.annotation.RequestBody ist the annotation required technically by Spring Mvc or OpenFeign to handle a request. This caused #11165.

io.swagger.v3.oas.annotations.parameters.RequestBody is never used, as far as I can see.

If we need them both, they should be fully qualified to disambiguate. For example

  ResponseEntity<ResourceOrder> createResourceOrder(
      @Parameter(name = "resourceOrder", description = "The ResourceOrder to be created", required = true)
      @io.swagger.v3.oas.annotations.parameters.RequestBody
      @org.springframework.web.bind.annotation.RequestBody
      @Valid ResourceOrderCreate resourceOrder);

Question is : Do we actually need to support @io.swagger.v3.oas.annotations.parameters.RequestBody? What is it good for? Do Tools like SpringFox or SpringDoc support the annotation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it is supported by SpringDoc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on hvaing both annotations?

{{#withXml}}
<!-- XML processing: Jackson -->
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: Indentation looks off

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

{{/hateoas}}
{{#useBeanValidation}}
{{/hateoas}}
{{#useBeanValidation}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe a general question - I assume by having this match the indentation of the pom file this is causing white space to be generated, which is likely why it was fully left aligned.

Are there any community guidelines on this?

@@ -0,0 +1 @@
{{#allowableValues}}allowableValues = "{{#values}}{{{.}}}{{^-last}}, {{/-last}}{{#-last}}{{/-last}}{{/values}}"{{/allowableValues}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this handle enums and containers properly? I don't see a change to "values.mustache" but maybe that's already correct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no values.mustache. Which file do you refer to?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was making an assumption about the reference to #values - does this work properly with enum values and containers then?

@cachescrubber
Copy link
Contributor Author

Re #11168. @io.swagger.v3.oas.annotations.Hidden is not applicable to Parameters. The correct replacement for
@ApiIgnore would be @Parameter(hidden = true) IMO.

@welshm
Copy link
Contributor

welshm commented Dec 23, 2021

Re #11168. @io.swagger.v3.oas.annotations.Hidden is not applicable to Parameters. The correct replacement for

@ApiIgnore would be @Parameter(hidden = true) IMO.

That seems reasonable to me

@cachescrubber
Copy link
Contributor Author

Short update before Xmas. I duplicated all test configs under bin/configs/spring*.yaml and added the oas3 flag. Executing the scripts revealed even more problems, some of which I have addressed with the last commits. Basically WIP working through the list of test script configurations.

@cachescrubber
Copy link
Contributor Author

cachescrubber commented Dec 24, 2021

@welshm

Your OAS3 implementation basically disables springfox. At the time beeing, springfox moved on and support for openapi3 is
available with the 3.0.0 release. Should we revert the following?

        // Springfox cannot be used with oas3 or apiFirst or reactive. So, write the property back after determining
        // whether it should be enabled or not.
        boolean useSpringFox = false;
        if (!apiFirst && !reactive && !oas3) {
            useSpringFox = true;
            additionalProperties.put("useSpringfox", true);
        }
        writePropertyBack("useSpringfox", useSpringFox);

@welshm
Copy link
Contributor

welshm commented Dec 24, 2021

@welshm

Your OAS3 implementation basically disables springfox. At the time beeing, springfox moved on and support for openapi3 is

available with the 3.0.0 release. Should we revert the following?


        // Springfox cannot be used with oas3 or apiFirst or reactive. So, write the property back after determining

        // whether it should be enabled or not.

        boolean useSpringFox = false;

        if (!apiFirst && !reactive && !oas3) {

            useSpringFox = true;

            additionalProperties.put("useSpringfox", true);

        }

        writePropertyBack("useSpringfox", useSpringFox);



Last I was reading about SpringFox, general consensus seemed to be for OAS 3 SpringDoc was greatly preferred.

Re-enabling SpringFox support seems fine but I imagine could be done as a separate PR as I could see that likely needing a whole lot more changes

@cachescrubber
Copy link
Contributor Author

cachescrubber commented Dec 24, 2021

@welshm Agreed regarding Springfox. The last commit is +1y old and the current 3.0.0 starter is incompatible to spring-boot 2.6.x. Springfox is currently unmaintained.

oas=true and 5.3.1 + this PR will also resolve #10409.

Copy link
Contributor

@welshm welshm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like Bitrise failed to download a maven depedency.

CircleCI failed on OpenAPIDocumentationConfig

[ERROR] COMPILATION ERROR : 
[ERROR] /home/circleci/OpenAPITools/openapi-generator/samples/server/petstore/spring-mvc-default-value/src/main/java/org/openapitools/configuration/OpenAPIDocumentationConfig.java:[14,48] cannot find symbol
  symbol:   class RelativePathProvider
  location: package springfox.documentation.spring.web.paths
[ERROR] /home/circleci/OpenAPITools/openapi-generator/samples/server/petstore/spring-mvc-default-value/src/main/java/org/openapitools/configuration/OpenAPIDocumentationConfig.java:[49,53] cannot find symbol
  symbol:   class RelativePathProvider
  location: class org.openapitools.configuration.OpenAPIDocumentationConfig

Looks like that also needs to be updated for full Springfox removal

@Override
public CodegenModel fromModel(String name, Schema model) {
CodegenModel codegenModel = super.fromModel(name, model);
if (oas3) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel it would be more correct to avoid adding the imports in the first place, but I'm happy to look into that separately

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this is how it is - the swagger2 imports are hard coded in AbstractJavaCodegen and can only be removed reliably in a post processor. They are not controlled by a mustache template. To change this, all Java based generators would have to be modified.

@cachescrubber
Copy link
Contributor Author

cachescrubber commented Dec 25, 2021

Looks like that also needs to be updated for full Springfox removal

Ok, got it. I have to revert to the latest 2.9.2 release of springfox. Since springfox is on removal, there is no value in upgrading all templates to 3.0.0 (which removes several deprected apis, including RelativePathProvider). A commit fixing this is coming soon.

@cachescrubber
Copy link
Contributor Author

cachescrubber commented Dec 30, 2021

I'm happy to follow up with a separate change for the annotation for @RequestBody to have both.

@welshm Ok with me. Anyway, I would suggest to check the SpringDoc documentation if there is a use case where this annotation is actually used.

@cachescrubber
Copy link
Contributor Author

@wing328, @welshm

I think the OAS3 story is now complete. I won't commit anything else from my side other then as a response to your feedback.

Please advise me how to proceed.

PS: Happy new year to you.

@welshm
Copy link
Contributor

welshm commented Dec 30, 2021

@wing328, @welshm

I think the OAS3 story is now complete. I won't commit anything else from my side other then as a response to your feedback.

Please advise me how to proceed.

PS: Happy new year to you.

Happy New year!

Need to wait for @wing328 to review and merge it at this point I believe

@@ -153,13 +156,18 @@ public interface {{classname}} {
produces = { {{#produces}}"{{{mediaType}}}"{{^-last}}, {{/-last}}{{/produces}} }{{/hasProduces}}{{#hasConsumes}},
consumes = { {{#consumes}}"{{{mediaType}}}"{{^-last}}, {{/-last}}{{/consumes}} }{{/hasConsumes}}{{/singleContentTypes}}
)
{{#jdk8-default-interface}}default {{/jdk8-default-interface}}{{#responseWrapper}}{{.}}<{{/responseWrapper}}ResponseEntity<{{>returnTypes}}>{{#responseWrapper}}>{{/responseWrapper}} {{#delegate-method}}_{{/delegate-method}}{{operationId}}({{#allParams}}{{>queryParams}}{{>pathParams}}{{>headerParams}}{{>bodyParams}}{{>formParams}}{{>cookieParams}}{{^-last}},{{/-last}}{{#-last}}{{#reactive}}, {{/reactive}}{{/-last}}{{/allParams}}{{#reactive}}{{#oas3}}@Hidden{{/oas3}}{{^oas3}}@springfox.documentation.annotations.ApiIgnore{{/oas3}} final ServerWebExchange exchange{{/reactive}}{{#vendorExtensions.x-spring-paginated}}, {{#oas3}}@Hidden{{/oas3}}{{^oas3}}@springfox.documentation.annotations.ApiIgnore{{/oas3}} final org.springframework.data.domain.Pageable pageable{{/vendorExtensions.x-spring-paginated}}){{^jdk8-default-interface}};{{/jdk8-default-interface}}{{#jdk8-default-interface}}{{#unhandledException}} throws Exception{{/unhandledException}} {
{{#jdk8-default-interface}}default {{/jdk8-default-interface}}{{#responseWrapper}}{{.}}<{{/responseWrapper}}ResponseEntity<{{>returnTypes}}>{{#responseWrapper}}>{{/responseWrapper}} {{#delegate-method}}_{{/delegate-method}}{{operationId}}(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm looks like @Hidden may still be relevant from SpringDoc


Additionally, to @Hidden annotation from swagger-annotations, its possible to restrict the generated OpenAPI description using package or path configuration.

As well as the guide on migrating from SpringFox

@ApiIgnore → @Parameter(hidden = true) or @Operation(hidden = true) or @Hidden

Although for method annotation rather than parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. We should add @Hidden in a separat PR when it's needed.

@welshm
Copy link
Contributor

welshm commented Dec 30, 2021

May also fix

#11203

@wing328
Copy link
Member

wing328 commented Jan 2, 2022

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@wing328 wing328 added this to the 5.4.0 milestone Jan 2, 2022
@cachescrubber
Copy link
Contributor Author

Ups, I used $COMPANY email address ....

@wing328 I could squash commit the whole branch in a new one and submit another PR. Would that be acceptable?

@wing328
Copy link
Member

wing328 commented Jan 2, 2022

@cachescrubber you can simply add your company email address as a secondary email address in your Github account. Let me know if you need help on that.

@cachescrubber
Copy link
Contributor Author

@wing328 Ok, I used the secondary email approach. Commits are now linked to my account.

@wing328
Copy link
Member

wing328 commented Jan 2, 2022

Tested the new samples locally and all compiles without issues.

@cemo
Copy link

cemo commented Jan 18, 2022

@wing328 when will the release be cut? Is it possible to cut a patch release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants