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

Allow Spring generated code to use new OAS 3 annotations #9775

Merged
merged 33 commits into from
Nov 2, 2021

Conversation

welshm
Copy link
Contributor

@welshm welshm commented Jun 15, 2021

  • Add in oas3 option for Spring codegen to use newer annotations

  • Add useSpringController option for Spring codegen

  • Use useSpringfox to fix some unwanted imports for Spring codegen

  • Use jdk8 to add OffsetDateTime import for models in Spring codegen

  • Add JsonValue to enumOuterClass.mustache to allow enums to be
    generated properly

  • To test, use oas3: true useSpringController: true and useSpringfox: false in a config file to generate a Spring target.

To close #9774

PR checklist

  • Read the contribution guidelines.
  • 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.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
  • File the PR against the correct branch: master, 5.1.x, 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.

welshm and others added 3 commits June 15, 2021 14:23
- Add in `oas3` option for Spring codegen to use newer annotations
- Add `useSpringController` option for Spring codegen
- Use `useSpringfox` to fix some unwanted imports for Spring codegen
- Use `jdk8` to add OffsetDateTime import for models in Spring codegen
- Add `JsonValue` to `enumOuterClass.mustache` to allow enums to be
  generated properly
Bring `welshm` fork up to master
@welshm
Copy link
Contributor Author

welshm commented Jun 15, 2021

I think @diyfr and @nmuesch might be good reviewers for this?

if (!this.apiFirst && !this.reactive) {
additionalProperties.put("useSpringfox", true);
if (this.useSpringfox) {
if (!this.apiFirst && !this.reactive) {

Choose a reason for hiding this comment

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

minor, can simplify to: this.useSpringFox && !this.apiFirst && !this.reactive

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'll add comments - I assume that springFox cannot be used when apiFirst and reactive is enabled but we don't use those options so I wanted it to be clear that this check is separate from the useSpringFox option check

@welshm
Copy link
Contributor Author

welshm commented Jun 21, 2021

@daonomic or @lwlee2608 Would you be an appropriate reviewer for this?

@welshm
Copy link
Contributor Author

welshm commented Jun 29, 2021

@wing328 - wondering if you can help me find an appropriate reviewer or how I might go about getting this added?

@welshm
Copy link
Contributor Author

welshm commented Jul 12, 2021

@4brunu Would you be able to help me get a review started on this? I have not had responses from anyone else so far

@4brunu
Copy link
Contributor

4brunu commented Jul 12, 2021

You need to copy the java technical committee members for review.

@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) @nmuesch (2021/01)

@welshm
Copy link
Contributor Author

welshm commented Jul 15, 2021

I'm happy to join or participate in a technical committee if that might help...

@welshm
Copy link
Contributor Author

welshm commented Jul 26, 2021

@4brunu do you happen to know what the expected response time is for opening a PR and receiving a review or for applying for the technical commmittee?

Part of wanting to contribute back is so that my organization doesn't diverge too much from the project here, but it's now been over a month with no response to opening the PR other than from yourself...

@wing328
Copy link
Member

wing328 commented Jul 26, 2021

Let me try to take a look tomorrow or later this week.

@welshm
Copy link
Contributor Author

welshm commented Jul 30, 2021

Let me try to take a look tomorrow or later this week.

Just following up here - I will be unavailalbe Aug 3 - 6 if there is feedback on this PR.

I am very much interested in joining the technical committee for Spring and Java if you're taking applicants (I did send an email) in order to help keep the generator supported for those languages.

@welshm
Copy link
Contributor Author

welshm commented Aug 6, 2021

@wing328 any chance to get this reviewed?

@welshm
Copy link
Contributor Author

welshm commented Aug 23, 2021

@wing328 Updated with latest master and regenerated tests/samples

@MichaelKunze
Copy link

Would be cool if this PR gets considered! Need this too.

@welshm
Copy link
Contributor Author

welshm commented Sep 1, 2021

@wing328 @4brunu is there anything more I can do to get this reviewed or looked at by someone on the team that can approve this?

This PR has been open for 2+ months, and I've kept it current with the main branch several times, but if no one will review it then there is no value in me maintaining this branch/PR.

I am more than happy to get more involved in the project, but my emails asking to join the team have also gone unanswered.

@InfoSec812
Copy link
Contributor

Functionally, I think this is quite good. I would like to see future improvements to readability as time permits. I think it would be nice to break out the OAS3/Swagger annotations into separate sub-templates which get included conditionally so that there isn't quite so much noise.

I'd be more than happy to tackle that - do you have an example or time for a quick discussion on what you imagine the file structure would look like (e.g. a separate file for Swagger2 vs. OAS3 each for boot/mvc/cloud? I could see a lot of repetition occurring there as some options (Java8, async, etc.) apply to both.

Definitely learned a lot with this PR and will definitely look to be more thorough in testing further PRs. Some of the mustache's are not features I am currently using so getting familiar with it all.

@welshm HERE is an example where the generatedAnnotation is coming from an included template. I don't think it's necessary for this PR, but it would be nice later to make things more readable/maintainable.

@welshm
Copy link
Contributor Author

welshm commented Oct 29, 2021

Functionally, I think this is quite good. I would like to see future improvements to readability as time permits. I think it would be nice to break out the OAS3/Swagger annotations into separate sub-templates which get included conditionally so that there isn't quite so much noise.

I'd be more than happy to tackle that - do you have an example or time for a quick discussion on what you imagine the file structure would look like (e.g. a separate file for Swagger2 vs. OAS3 each for boot/mvc/cloud? I could see a lot of repetition occurring there as some options (Java8, async, etc.) apply to both.
Definitely learned a lot with this PR and will definitely look to be more thorough in testing further PRs. Some of the mustache's are not features I am currently using so getting familiar with it all.

@welshm HERE is an example where the generatedAnnotation is coming from an included template. I don't think it's necessary for this PR, but it would be nice later to make things more readable/maintainable.

I'd love to keep making improvements for Spring support (Java and eventually Kotlin) so I'll look to make some of those changes in a future PR

@wing328
Copy link
Member

wing328 commented Nov 2, 2021

Did a few more tests and the results are good so let's merge this one and file separate PRs for enhancements if needed.

@wing328 wing328 merged commit b117d29 into OpenAPITools:master Nov 2, 2021
@atkawa7
Copy link

atkawa7 commented Nov 2, 2021

@wing328 would you be able to do a minor release to test this out

@welshm welshm deleted the allow_oas3_annotations branch November 3, 2021 01:22
@wing328
Copy link
Member

wing328 commented Nov 3, 2021

@atkawa7 can you please use the SNAPSHOT version (links in the readme) before v5.3.1 release (which is scheduled a month later)?

@atkawa7
Copy link

atkawa7 commented Nov 3, 2021

Thanks @wing328

@mhinders
Copy link

Really happy about this change. I would however be inclined to use springdocs for viewing the API definition. As it is currently the Spring controllers are generated with @org.springframework.stereotype.Controller and not @org.springframework.web.bind.annotation.RestController which means that manual configuration is required as explained here https://springdoc.org/#my-rest-controller-using-controller-annotation-is-ignored. Could the templates be changed to use RestController?

@welshm
Copy link
Contributor Author

welshm commented Dec 17, 2021

Really happy about this change. I would however be inclined to use springdocs for viewing the API definition. As it is currently the Spring controllers are generated with @org.springframework.stereotype.Controller and not @org.springframework.web.bind.annotation.RestController which means that manual configuration is required as explained here https://springdoc.org/#my-rest-controller-using-controller-annotation-is-ignored. Could the templates be changed to use RestController?

It can be updated to do that - just for clarity though, I had added this as an option for the interfaces but had left the generated API controller implementations (apiController.mustache) unchanged - would you be expecting both to be annotated with @RestController? My opinion is probably not, as the interface has all the annotations you need.

@wing328 wing328 added this to the 5.3.1 milestone Dec 18, 2021
@BlackSunshine-manage
Copy link

Hello! how i can use this plugin?? this is a big work and i waited to long time this. You're really cool!

@Jey2k
Copy link

Jey2k commented Dec 23, 2021

Hi,
Quite a noob in this part, but I'm currently trying to migrate one of our application and I would like to use this generator. So first thanks for your work.
The last problem detected is with "oas3" activated, and a default value on a query param it gives some code not compatible with Swagger v3 annotations:
eg:
,@Parameter(name = "size", description = "Size of the results page (maximum size is 1'000)", defaultValue = "1000") @RequestParam(value = "size", required = false, defaultValue = "1000") Integer size

I suppose it should be included into @Schema, isn't it ?
eg:
,@Parameter(name = "size", description = "Size of the results page (maximum size is 1'000)", schema = @Schema( defaultValue = "1000" )) @RequestParam(value = "size", required = false, defaultValue = "1000") Integer size

@marinus-suniram
Copy link
Contributor

I have the same problem with the default value

@welshm
Copy link
Contributor Author

welshm commented Dec 29, 2021

Please see this change #11181 from @cachescrubber which is addressing the issues of default params and Schema (along with other issues)

@kdebski85
Copy link

I also have the issue with defaultValue for Parameter. I created #11203

@mhinders
Copy link

mhinders commented Jan 3, 2022

Really happy about this change. I would however be inclined to use springdocs for viewing the API definition. As it is currently the Spring controllers are generated with @org.springframework.stereotype.Controller and not @org.springframework.web.bind.annotation.RestController which means that manual configuration is required as explained here https://springdoc.org/#my-rest-controller-using-controller-annotation-is-ignored. Could the templates be changed to use RestController?

It can be updated to do that - just for clarity though, I had added this as an option for the interfaces but had left the generated API controller implementations (apiController.mustache) unchanged - would you be expecting both to be annotated with @RestController? My opinion is probably not, as the interface has all the annotations you need.

I missed that the annotation is present on the interface. That is sufficient for me.

@shivaprasadgurram
Copy link

springdoc-openapi-ui not compatible with openapi-generator-maven-plugin. I have a dependency of spring-openapi-ui with version 1.6.9. I can see generated classes are using V3 annotations but when I try to load swagger-ui.html it is not working. Not sure what is going wrong. Can any one help me?

Need: I should be able to open swagger-ui page from browser.

@Moonergfp
Copy link

springdoc-openapi-ui not compatible with openapi-generator-maven-plugin. I have a dependency of spring-openapi-ui with version 1.6.9. I can see generated classes are using V3 annotations but when I try to load swagger-ui.html it is not working. Not sure what is going wrong. Can any one help me?

Need: I should be able to open swagger-ui page from browser.

what would be shown when you open swagger-ui page?

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.

[REQ][Java][Spring] Allow Spring generated code to use new OAS 3 annotations