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

Add externalDocs to @Operation to the JavaSpring generator #14177

Merged

Conversation

arey
Copy link
Contributor

@arey arey commented Dec 5, 2022

This Pull Request implements the feature describe in #14141 with a TDD approach.
It adds an externalDocs attribute to the @Operation annotation regarding the operation yaml specification.

PR checklist

@arey
Copy link
Contributor Author

arey commented Dec 5, 2022

The ci/circle1:node1 test has crashed with this error:

Caused by: java.lang.UnsupportedClassVersionError: org/springframework/boot/maven/RepackageMojo has been compiled by a more recent version of the Java Runtime (class file version 61.0), this version of the Java Runtime only recognizes class file versions up to 52.0
    at java.lang.ClassLoader.defineClass1 (Native Method)

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.

Can you also regenerate the samples? Or do none of the samples have exeternal docs? If not, perhaps we should add one?

@@ -4478,6 +4478,7 @@ else if (one.required)
op.requiredParams = requiredParams;
op.optionalParams = optionalParams;
op.externalDocs = operation.getExternalDocs();
op.hasExternalDocs = op.externalDocs != null;
Copy link
Contributor

Choose a reason for hiding this comment

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

You're making changes to DefaultCodegen and CodegenOperation so you'll also likely need a review from the core team as well (even though this is minor)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could maybe initiate the hasExternalDocs attribute at other places. Thus yes the Core Team is welcome :)
@wing328
@jimschubert
@cbornet
@jmini
@etherealjoy
@spacether

@@ -16,6 +16,9 @@ import io.swagger.v3.oas.annotations.media.Schema;
import io.swagger.v3.oas.annotations.responses.ApiResponse;
import io.swagger.v3.oas.annotations.security.SecurityRequirement;
import io.swagger.v3.oas.annotations.tags.Tag;
{{#hasAuthMethods}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be #hasExternalDocs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. I'll fix that.

arey added 2 commits December 5, 2022 16:53
…enerator : fix mustache template with #hasExternalDocs
@arey
Copy link
Contributor Author

arey commented Dec 5, 2022

Can you also regenerate the samples? Or do none of the samples have exeternal docs? If not, perhaps we should add one?

I'll we do that. For unit testing I've added an external docs to the petstore.yaml. So it should be present in the generated samples. May I have only generate the spring-boot-oas3.yaml config?

./bin/generate-samples.sh ./bin/configs/spring-boot-oas3.yaml

@welshm
Copy link
Contributor

welshm commented Dec 5, 2022

Can you also regenerate the samples? Or do none of the samples have exeternal docs? If not, perhaps we should add one?

I'll we do that. For unit testing I've added an external docs to the petstore.yaml. So it should be present in the generated samples. May I have only generate the spring-boot-oas3.yaml config?

./bin/generate-samples.sh ./bin/configs/spring-boot-oas3.yaml

You need to run all of the following:

  ./mvnw clean package 
  ./bin/generate-samples.sh
  ./bin/utils/export_docs_generators.sh

Otherwise tests will fail because the diffs won't match.

arey added 2 commits December 5, 2022 19:09
…enerator: regenerate the spring-boot-oas3.yaml sample
@arey
Copy link
Contributor Author

arey commented Dec 5, 2022

I'll first generate the spring-boot-oas3. This could me fix some indentation and carriage return issues.
I will now regenerate all the samples.

@arey
Copy link
Contributor Author

arey commented Dec 5, 2022

Generate all samples command looks like to failed on the master branch:

$ ./bin/generate-samples.sh ./bin/generate-samples.sh
...
[main] ERROR o.o.c.config.CodegenConfigurator - Unexpected character ('#' (code 35)): expected a valid value (JSON String, Number, Array, Object or token 'null', 'true' or 'false')
 at [Source: (File); line: 1, column: 2]
[main] ERROR o.o.c.config.CodegenConfigurator - Unexpected character ('#' (code 35)): expected a valid value (JSON String, Number, Array, Object or token 'null', 'true' or 'false')
 at [Source: (File); line: 1, column: 2]
Exception in thread "main" java.lang.RuntimeException: Unable to deserialize config file: ./bin/generate-samples.sh
        at org.openapitools.codegen.config.CodegenConfigurator.readDynamicSettings(CodegenConfigurator.java:167)
        at org.openapitools.codegen.config.CodegenConfigurator.fromFile(CodegenConfigurator.java:89)
        at org.openapitools.codegen.cmd.Generate.execute(Generate.java:293)
        at org.openapitools.codegen.cmd.OpenApiGeneratorCommand.run(OpenApiGeneratorCommand.java:32)
        at org.openapitools.codegen.OpenAPIGenerator.main(OpenAPIGenerator.java:66)

@welshm
Copy link
Contributor

welshm commented Dec 5, 2022

Generate all samples command looks like to failed on the master branch:

$ ./bin/generate-samples.sh ./bin/generate-samples.sh
...
[main] ERROR o.o.c.config.CodegenConfigurator - Unexpected character ('#' (code 35)): expected a valid value (JSON String, Number, Array, Object or token 'null', 'true' or 'false')
 at [Source: (File); line: 1, column: 2]
[main] ERROR o.o.c.config.CodegenConfigurator - Unexpected character ('#' (code 35)): expected a valid value (JSON String, Number, Array, Object or token 'null', 'true' or 'false')
 at [Source: (File); line: 1, column: 2]
Exception in thread "main" java.lang.RuntimeException: Unable to deserialize config file: ./bin/generate-samples.sh
        at org.openapitools.codegen.config.CodegenConfigurator.readDynamicSettings(CodegenConfigurator.java:167)
        at org.openapitools.codegen.config.CodegenConfigurator.fromFile(CodegenConfigurator.java:89)
        at org.openapitools.codegen.cmd.Generate.execute(Generate.java:293)
        at org.openapitools.codegen.cmd.OpenApiGeneratorCommand.run(OpenApiGeneratorCommand.java:32)
        at org.openapitools.codegen.OpenAPIGenerator.main(OpenAPIGenerator.java:66)

My guess is there's a serialized config file that needs to be updated because you added hasExternalDocs to the config.

@arey
Copy link
Contributor Author

arey commented Dec 5, 2022

My guess is there's a serialized config file that needs to be updated because you added hasExternalDocs to the config.

Just a human error. I've generated then commited all the samples.
The export_docs_generators.sh does modified any files.

@arey
Copy link
Contributor Author

arey commented Dec 12, 2022

Hi @welshm, do you know if the core team members could have a look on my change to to DefaultCodegen and CodegenOperation ?

@welshm
Copy link
Contributor

welshm commented Dec 12, 2022

Hi @welshm, do you know if the core team members could have a look on my change to to DefaultCodegen and CodegenOperation ?

I'll take another look today and then tag some of them

@@ -170,7 +173,8 @@ public interface {{classname}} {
{{#authMethods}}
@SecurityRequirement(name = "{{name}}"{{#isOAuth}}, scopes={ {{#scopes}}"{{scope}}"{{^-last}}, {{/-last}}{{/scopes}} }{{/isOAuth}}){{^-last}},{{/-last}}
{{/authMethods}}
}{{/hasAuthMethods}}
}{{/hasAuthMethods}}{{#hasExternalDocs}},
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 also work? I'm not an expert on mustache files, but I think you may not need the hasExternalDocs boolean if you check if the parameter externalDocs is set like this.

}{{/hasAuthMethods}}{{#externalDocs}},
  externalDocs = @ExternalDocumentation(description = "{{externalDocs.description}}", url = "{{externalDocs.url}}")
{{/externalDocs}}

Copy link
Contributor

Choose a reason for hiding this comment

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

For example, I see this in the Swift5 templates

     {{#externalDocs}}
     - externalDocs: {{.}}
     {{/externalDocs}}

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 didn't know the mustache syntax but it may work. I will check tomorrow.

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've removed the hasExternalDocs property. It's much simpler and the core classes are no more touched.

@arey
Copy link
Contributor Author

arey commented Dec 16, 2022

Still don't know why the ci/circleci:node1 is failing: https://circleci.com/gh/OpenAPITools/openapi-generator/56336

Caused by: java.lang.UnsupportedClassVersionError: org/springframework/boot/maven/RepackageMojo has been compiled by a more recent version of the Java Runtime (class file version 61.0), this version of the Java Runtime only recognizes class file versions up to 52.0

@welshm does it block my PR?

@welshm
Copy link
Contributor

welshm commented Dec 16, 2022

Still don't know why the ci/circleci:node1 is failing: https://circleci.com/gh/OpenAPITools/openapi-generator/56336

Caused by: java.lang.UnsupportedClassVersionError: org/springframework/boot/maven/RepackageMojo has been compiled by a more recent version of the Java Runtime (class file version 61.0), this version of the Java Runtime only recognizes class file versions up to 52.0

@welshm does it block my PR?

Shouldn't block your PR

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.

@wing328 will need to review and merge, but this looks great to me!

Thanks for adding the external docs!

@wing328
Copy link
Member

wing328 commented Dec 19, 2022

@arey thanks for the PR but some CI tests failed: https://github.com/OpenAPITools/openapi-generator/actions/runs/3680159465/jobs/6327983250

can you please take a look when you've time?

@arey
Copy link
Contributor Author

arey commented Dec 21, 2022

Thanks for the review @wing328
It looks like the ExternalDocumentation java import is not generated. I will investigate.

@arey
Copy link
Contributor Author

arey commented Dec 22, 2022

@wing328 I'll fixed the issue by systematically add the ExternalDocumentation java import (like others io.swagger.v3.oas.annotations.* imports).
I would have liked to make it conditional. But it requires that CodegenOperation shoud be aware of all the CodegenOperation. I don't know if other generators do this kind of import optimization?

@arey arey requested a review from welshm January 2, 2023 07:06
@arey
Copy link
Contributor Author

arey commented Jan 3, 2023

Any news?
I would like to known if it is possible to have this feature in the next 6.3.0 version?

@wing328 wing328 merged commit 917892d into OpenAPITools:master Jan 4, 2023
@wing328 wing328 added this to the 6.3.0 milestone Jan 4, 2023
@wing328 wing328 changed the title #14141 Add externalDocs to @Operation to the JavaSpring generator Add externalDocs to @Operation to the JavaSpring generator Jan 4, 2023
@arey
Copy link
Contributor Author

arey commented Jan 4, 2023

Thanks a lot William for this PR. And thank you for all the time you spent on this great project :)

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.

3 participants