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

ebautistabar commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

Sure, I'll try to fix it @wing328

ebautistabar commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

ebautistabar commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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

Download https://oss.sonatype.org/content/repositories/snapshots/io/swagger/swagger-codegen-cli/2.3.0-SNAPSHOT/swagger-codegen-cli-2.3.0-20171030.060918-211.jar
Download http://petstore.swagger.io/v2/swagger.json
Run java -jar swagger-codegen-cli-2.3.0-20171030.060918-211.jar generate -i swagger.json -l spring -o implicit -D hideGenerationTimestamp=true --additional-properties dateLibrary=java8-localdatetime
Run java -jar swagger-codegen-cli-2.3.0-20171030.060918-211.jar generate -i swagger.json -l spring -o false -D hideGenerationTimestamp=true --additional-properties dateLibrary=java8-localdatetime,java8=false
Run java -jar swagger-codegen-cli-2.3.0-20171030.060918-211.jar generate -i swagger.json -l spring -o true -D hideGenerationTimestamp=true --additional-properties dateLibrary=java8-localdatetime,java8=true

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.

steventong commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

--additional-properties dateLibrary=java8-localdatetime

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

swagger-api / swagger-codegen