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

[Spring] The java8 parameter is ignored, always behaves as true #5614

Open
ebautistabar opened this issue May 11, 2017 · 11 comments
Open

[Spring] The java8 parameter is ignored, always behaves as true #5614

ebautistabar opened this issue May 11, 2017 · 11 comments

Comments

@ebautistabar
Copy link

Description

The java8 parameter in the spring generator is ignored, always behaves as true. It doesn't matter that it's specified explicitly or not. The generated code always contains Java 8 default interfaces.

Swagger-codegen version

Latest from master.

Command line used for generation

java -jar swagger-codegen-cli.jar generate -i swagger.json -l spring -o build -D hideGenerationTimestamp=true --invoker-package custom.package --model-package custom.package.model --api-package custom.package.api --additional-properties dateLibrary=java8-localdatetime,basePackage=custom,configPackage=custom.swagger,useBeanValidation=true,java8=false

Steps to reproduce

Execute the given command with any valid JSON file.

Suggest a Fix

I guess the issue is in the SpringCodegen.java, but haven't checked thoroughly.

@wing328
Copy link
Contributor

wing328 commented May 12, 2017

@ebautistabar thanks for reporting the issue. Seems like java8 is only used in the pom template:

swagger-codegen|master⚡ ⇒ grep -R java8 modules/swagger-codegen/src/main/resources/JavaSpring/
modules/swagger-codegen/src/main/resources/JavaSpring//libraries/spring-boot/pom.mustache:        <java.version>{{#java8}}1.8{{/java8}}{{^java8}}1.7{{/java8}}</java.version>
modules/swagger-codegen/src/main/resources/JavaSpring//libraries/spring-boot/pom.mustache:        {{#java8}}
modules/swagger-codegen/src/main/resources/JavaSpring//libraries/spring-boot/pom.mustache:        {{/java8}}
modules/swagger-codegen/src/main/resources/JavaSpring//libraries/spring-boot/pom.mustache:        {{^java8}}
modules/swagger-codegen/src/main/resources/JavaSpring//libraries/spring-boot/pom.mustache:        {{/java8}}
modules/swagger-codegen/src/main/resources/JavaSpring//libraries/spring-cloud/pom.mustache:        <java.version>{{#java8}}1.8{{/java8}}{{^java8}}1.7{{/java8}}</java.version>
modules/swagger-codegen/src/main/resources/JavaSpring//libraries/spring-cloud/pom.mustache:        {{#java8}}
modules/swagger-codegen/src/main/resources/JavaSpring//libraries/spring-cloud/pom.mustache:        {{/java8}}
modules/swagger-codegen/src/main/resources/JavaSpring//libraries/spring-cloud/pom.mustache:        {{^java8}}
modules/swagger-codegen/src/main/resources/JavaSpring//libraries/spring-cloud/pom.mustache:        {{/java8}}
modules/swagger-codegen/src/main/resources/JavaSpring//libraries/spring-mvc/pom.mustache:        {{#java8}}
modules/swagger-codegen/src/main/resources/JavaSpring//libraries/spring-mvc/pom.mustache:        {{/java8}}
modules/swagger-codegen/src/main/resources/JavaSpring//libraries/spring-mvc/pom.mustache:        {{^java8}}
modules/swagger-codegen/src/main/resources/JavaSpring//libraries/spring-mvc/pom.mustache:        {{/java8}}
modules/swagger-codegen/src/main/resources/JavaSpring//libraries/spring-mvc/pom.mustache:        <java.version>{{#java8}}1.8{{/java8}}{{^java8}}1.7{{/java8}}</java.version>
modules/swagger-codegen/src/main/resources/JavaSpring//swaggerDocumentationConfig.mustache:                {{#java8}}
modules/swagger-codegen/src/main/resources/JavaSpring//swaggerDocumentationConfig.mustache:                {{/java8}}
modules/swagger-codegen/src/main/resources/JavaSpring//swaggerDocumentationConfig.mustache:                {{^java8}}
modules/swagger-codegen/src/main/resources/JavaSpring//swaggerDocumentationConfig.mustache:                {{/java8}}

May I know if you've time to contribute the fix to hide the Java8 default interface using {{^java8}} ... {{/java8}}?

cc @cbornet @jfiala

@ebautistabar
Copy link
Author

Sure, I'll try to fix it @wing328

ebautistabar added a commit to ebautistabar/swagger-codegen that referenced this issue May 13, 2017
The java8 parameter was used both signal use of the default
interface and to signal general use of Java8, e.g. when the
Java8 date libraries were used.

Now we use a different parameter to signal use of the default
interface: defaultInterface. Java8 is considered in use when
either the default interface or the Java8 date libraries are
used.
@ebautistabar
Copy link
Author

ebautistabar commented May 13, 2017

@wing328 The java8 property is being used for 2 different but related things:

  • to signal that Java 8 is being used, e.g. when the Java 8 libraries are selected; this type of use is present in all the Java generators through the AbstractJavaCodegen class
  • to signal that default interfaces must be used; this type of use is specific of the Spring generator through SpringCodegen

Whenever a user selects Java 8 date libraries, the generator considers that Java 8 must be used and sets java8=true internally. By the time we read the java8 property in SpringCodegen it has already been modified, so we can't know the original value which the user selected.

The fact that java8 doesn't appear in the api templates is due to the generator setting a different property jdk8 internally, which is later used by the templates to render the default interface. If java8 is true, then jdk8 is true, so effectively they are the same.

A possible solution is changing the name of the parameter in the Spring generator to something other than java8. If the parameter truly represents if we want to use default interfaces or not, then it could be called defaultInterface. With this solution, the user cannot select Java 8 explicitly. Java 8 would only be selected indirectly by choosing to use default interfaces or the Java 8 date libraries. This is the option implemented in the commit above. But then I realized there's another possibility.

If we want to have the option of, e.g. setting Java 8 in the POM file but using Joda or legacy date libraries and without default interfaces, there is another option: keeping the existing java8 parameter and adding another defaultInterface parameter just for the interface feature. java8 would be false by default and would be enabled automatically if the Java 8 date libraries or the default interface were chosen. But this would let us do it the other way around: choosing Java 8 without those features.

Some questions:

  • which solution do you prefer?
  • the fix would involve changing the parameters of the swagger-codegen-cli tool (either adding one, or changing a name). Should the PR be done on master or 2.3.0?

@wing328
Copy link
Contributor

wing328 commented May 14, 2017

the fix would involve changing the parameters of the swagger-codegen-cli tool (either adding one, or changing a name). Should the PR be done on master or 2.3.0?

It should be done against 2.3.0 since it's a breaking change.

keeping the existing java8 parameter and adding another defaultInterface parameter just for the interface feature

To me, this is more preferable as developers who have been using the java8 mustache tag in their customized templates can continue to do so.

cc @cbornet @jfiala

@cbornet
Copy link
Contributor

cbornet commented May 14, 2017

See #4854 . It may have already been done in 2.3.0.
@ebautistabar Can you check ?

@ebautistabar
Copy link
Author

ebautistabar commented May 15, 2017

@cbornet I've looked at that PR and I'm not sure it solves the issue completely. It doesn't set the java8 property when using a Java 8 date library, which solves the bug with the default interface being included. That part is good. However, the mustache templates use the java8 property to determine if Java 8 should be used, e.g. in the POM file. The result is that we select a Java 8 date library but end up with Java 7 in the POM. Also another tag is mentioned in the PR: jsr130. But that tag is only used in a small number of templates, I guess with another purpose, so it doesn't solve our problem.

I would suggest someone with more experience than me with this code to look thoroughly at it to ensure the current state and the correct way to proceed.

@wing328 wing328 modified the milestones: v2.3.0, v2.2.3 Jul 13, 2017
@wing328
Copy link
Contributor

wing328 commented Jul 13, 2017

@ebautistabar I did a review and the Java8 option is set correctly: https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/SpringCodegen.java#L142-L144

Then I added --additional-properties java8=true to ./bin/springboot-petstore-server.sh and there were changes related to auto-generated code.

I would suggest you to clone the swagger-codegen repo again and give it another try with the 2.3.0 branch.

@ebautistabar
Copy link
Author

@wing328 I performed the following test today. I lay out here the steps for reproducibility and in case I'm doing something wrong.

Note that in all the commands I set both the dateLibrary and java8 properties.

Now that we have the three versions (with default java8, with explicit java8=true and with explicit java8=false), compare each pair of folders with a diff tool.

The result is that they are all the same.

The expectation is that using different parameters will result in different output.

@wing328 wing328 modified the milestones: v2.3.0, v3.0.0 Dec 17, 2017
@wing328 wing328 modified the milestones: v3.0.0, Future Feb 10, 2018
@steventong
Copy link

Hi, I want to use java8-localdatetime too, but my feign client contains default interface, is this bug fixed on 3.0.0-RC?

Thanks!

@mike-taylor1
Copy link

Same issue using swagger-codegen v2.3.2 after converting dateLibrary to java8-localdatetime for my API. Looks like this was discussed on issue #3408 and again on #4854. The proposal on the latter looks like it got rejected and the issue is still open with the current target being v3.0.0. Can anyone confirm?

Thanks.

@InternetPseudonym
Copy link

--additional-properties dateLibrary=java8-localdatetime

hey, thanks - thats excactly the (weird) commandline option i was looking for!

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

No branches or pull requests

6 participants