[Java][Spring] Allows for generation of spring controller code using the delegate pattern #4439

JLLeitschuh · 2016-12-20T23:41:31Z

PR checklist

Read the contribution guildelines.
Ran the shell/batch script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates)
Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

Adds the ability to generate Spring Server code using the decorator pattern.

Because annotations on interface parameters aren't inherited in java and aren't accessible using reflection spring requires you to have annotations like @RequestBody on the concrete class.

This means that as a developer you can not just override the methods in your controller, you must also copy the annotations from the Api interface's methods.

This fixes the problem by allowing you to generate interfaces that can be implemented and don't require you to copy the annotations from the parent.

Note: It seems like someone didn't generate the samples for one of their changes so there is a bit more noise in those files than this PR should create for consumers.

JLLeitschuh · 2016-12-22T02:53:29Z

Some checkstyle things that clearly need to be cleaned up

wing328 · 2017-01-10T14:52:23Z

modules/swagger-codegen/src/main/resources/JavaSpring/apiDecorator.mustache

+
+import io.swagger.annotations.*;
+{{#jdk8}}
+    import org.springframework.http.HttpStatus;


Minor suggestion: what about removing the leading spaces?

wing328 · 2017-01-10T14:53:14Z

(the Appveyor issue can be ignored)

JLLeitschuh · 2017-01-10T18:15:47Z

Fixed. Other than that is this good?

wing328 · 2017-01-12T16:45:58Z

@JLLeitschuh I did some tests using the following commands:

java -jar modules/swagger-codegen-cli/target/swagger-codegen-cli.jar generate \
  -i modules/swagger-codegen/src/test/resources/2_0/petstore.yaml \
  -l spring  -DdecoratorPattern=false \
  -o /var/tmp/spring/decoratorPattern_false/

java -jar modules/swagger-codegen-cli/target/swagger-codegen-cli.jar generate \
  -i modules/swagger-codegen/src/test/resources/2_0/petstore.yaml \
  -l spring  -DdecoratorPattern=true \
  -o /var/tmp/spring/decoratorPattern_true/

Both failed to compile with different error messages (while the generated code without decoratorPattern works fine without issue)

Can you give it a try to see if you can repeat the issue?

JLLeitschuh · 2017-01-12T19:35:32Z

You're right, I had some logic that was wrong.
Good catch.
Those should work now.

cbornet · 2017-01-12T21:25:12Z

I think this is not a decorator pattern. It looks more like a delegation.
Also, shouldn't there be an implementation of the delegation interface generated by default ? For instance, doesn't it crash if you don't provide a PetApiDelegator bean/@service ?

JLLeitschuh · 2017-01-13T18:38:46Z

I suppose that you are right, this may be more like delegation.
I was expecting the users to implement the interfaces. (That's what I plan to do). Also, one reason for using interfaces is because now users are free to use one class to implement many controllers or can use one class per controller.
I don't really want to also generate concrete services.

cbornet · 2017-01-13T19:05:47Z

Well, I think the generated server should at least start without crashing and provide basic "200 OK" endpoints. After that you can add the implementations to swagger-ignore so that they are not overridden by further regen.

JLLeitschuh · 2017-01-13T20:11:35Z

So you are saying add an additional flag flag with something like -DdecoratorPattern=true -DdecoratorPatternImplementations=false to get what functionality I want and -DdecoratorPattern=true -DdecoratorPatternImplementations=true to get what you want?

I mean, I can understand the desire but I don't really want to implement this as I have no use for it.
You are more than welcome to open a PR against this branch to add that feature if you want and I'll merge it into this one.

JLLeitschuh · 2017-01-16T21:16:31Z

@wing328 Are you looking for anything else on this PR?

wing328 · 2017-01-17T02:32:42Z

@JLLeitschuh I'll do more tests later today. Will keep you posted.

cbornet · 2017-01-17T06:23:08Z

@JLLeitschuh can you rename the class/option xxxDecorator to xxxService ?
As for the option, I wouldn't have too many possibilities and always generate the concrete class. Don't you need at least one concrete class yourself ?

wing328 · 2017-01-17T16:34:22Z

@JLLeitschuh Performed more tests and didn't find any issues.

Please kindly reply to @cbornet 's suggestions: #4439 (comment)

JLLeitschuh · 2017-01-17T16:40:37Z

Yea, but I want to be free to implement it as I want, perhaps I want to create a class that implements two of the controller interfaces? Maybe 4? I don't want another concrete implementation on the classpath that I have to fight spring to get it to not autowire.
I'm using the generated code as a source set for a larger project.

JLLeitschuh · 2017-01-17T17:12:29Z

I can do the renaming of xxxDecorator to xxxService

JLLeitschuh · 2017-01-17T17:25:26Z

Wouldn't xxxDelegate make more sense @cbornet?

…ttern

JLLeitschuh · 2017-01-17T19:12:05Z

I just changed everything to use the word delegate instead.
I also added a sample with the delegate pattern combined with java 8

cbornet · 2017-01-18T09:17:47Z

I feel service would be better in the Spring world since you would probably use a component annotated by @service

wing328 · 2017-01-18T09:34:03Z

@JLLeitschuh thanks for creating the Petstore sample test with decorator pattern for Java8 but I got compilation errors when running mvn test under samples/server/petstore/springboot-delegate-j8. Here is part of the error message:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.1:compile (default-compile) on project swagger-spring: Compilation failure: Compilation failure:
[ERROR] /private/var/tmp/pr/JLLeitschuh-feat/swagger-codegen/samples/server/petstore/springboot-delegate-j8/src/main/java/io/swagger/api/PetApiController.java:[17,137] cannot find symbol
[ERROR] symbol:   class Pet
[ERROR] location: class io.swagger.api.PetApiController
[ERROR] /private/var/tmp/pr/JLLeitschuh-feat/swagger-codegen/samples/server/petstore/springboot-delegate-j8/src/main/java/io/swagger/api/PetApiController.java:[17,12] cannot find symbol
[ERROR] symbol:   class ResponseEntity
[ERROR] location: class io.swagger.api.PetApiController
[ERROR] /private/var/tmp/pr/JLLeitschuh-feat/swagger-codegen/samples/server/petstore/springboot-delegate-j8/src/main/java/io/swagger/api/PetApiController.java:[22,12] cannot find symbol
[ERROR] symbol:   class ResponseEntity
[ERROR] location: class io.swagger.api.PetApiController
[ERROR] /private/var/tmp/pr/JLLeitschuh-feat/swagger-codegen/samples/server/petstore/springboot-delegate-j8/src/main/java/io/swagger/api/PetApiController.java:[28,240] cannot find symbol
[ERROR] symbol:   class List

Did you get similar error message when running mvn test locally in your machine?

JLLeitschuh · 2017-01-18T16:27:40Z

samples/server/petstore/spring-mvc-j8-async/src/main/java/io/swagger/api/FakeApiController.java


+
+
+    public Callable<ResponseEntity<Client>> testClientModel(@ApiParam(value = "client model" ,required=true ) @RequestBody Client body) {


This should not be being generated. I need to fix this.

JLLeitschuh · 2017-01-18T16:56:39Z

@wing328
You were right. My logic was still wrong.
I didn't understand that even in mustache when a value is false #[value] still evaluates to true because the value exists.
I fixed that issue.
That made the diff against the other samples smaller.

JLLeitschuh · 2017-01-19T00:17:37Z

I just published my changes internally and I'm using the delegatePattern=true and java8=true and everything seems to be compiling nicely.

wing328 · 2017-01-19T09:58:33Z

@JLLeitschuh I did more tests and no longer found issues. Thanks for your contribution.

JLLeitschuh · 2017-01-19T16:06:15Z

🎉 🎆 Thank you @wing328 Awesome work on this project. 🎆 🎉
Any ETA on the next release?
I'll continue to use my internally published version for now but I'd love to not have to rely on it for too long.

wing328 · 2017-01-19T16:16:47Z

ETA for 2.2.2 release is Jan/Feb 2017

cbornet · 2017-01-19T18:25:11Z

No 2.3 ?

wing328 · 2017-01-20T03:10:37Z

@cbornet yup, I also want to release 2.3.0 right after 2.2.2.

JLLeitschuh · 2017-01-20T06:37:36Z

I think I may have accidentally forgotten to change the rm lines of the scrips when I copied them.
I'll open another PR later tomorrow if I have time

JLLeitschuh · 2017-01-20T06:38:01Z

bin/spring-delegate-j8.sh

+ags="$@ generate -t modules/swagger-codegen/src/main/resources/JavaSpring -i modules/swagger-codegen/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml -l spring -o samples/server/petstore/springboot-delegate-j8 -DdelegatePattern=true,hideGenerationTimestamp=true,java8=true"
+
+echo "Removing files and folders under samples/server/petstore/springboot-delegate-j8/src/main"
+rm -rf samples/server/petstore/springboot/src/main


This is wrong. Should be the same as the line above.

JLLeitschuh · 2017-01-20T06:38:09Z

bin/spring-delegate.sh

+ags="$@ generate -t modules/swagger-codegen/src/main/resources/JavaSpring -i modules/swagger-codegen/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml -l spring -o samples/server/petstore/springboot-delegate -DdelegatePattern=true,hideGenerationTimestamp=true"
+
+echo "Removing files and folders under samples/server/petstore/springboot-delegate/src/main"
+rm -rf samples/server/petstore/springboot/src/main


This is wrong. Should be the same as the line above.

… scripts (#4630) Related #4439

…ttern (swagger-api#4439) * Allows for generation of spring conroller code using the decorator pattern * Change Decorator to Delegate in spring codegen

… scripts (swagger-api#4630) Related swagger-api#4439

JLLeitschuh closed this Dec 22, 2016

JLLeitschuh reopened this Dec 22, 2016

JLLeitschuh mentioned this pull request Jan 3, 2017

[JAVA] Support Javax injection for creation of API delegate instead of static factory pattern #3930

Open

wing328 reviewed Jan 10, 2017

View reviewed changes

wing328 added the Server: Spring label Jan 11, 2017

wing328 added this to the v2.2.2 milestone Jan 11, 2017

wing328 added the Enhancement: Feature label Jan 11, 2017

Allows for generation of spring conroller code using the decorator pa…

455aaa4

…ttern

JLLeitschuh changed the title ~~Allows for generation of spring controller code using the decorator pattern~~ Allows for generation of spring controller code using the delegate pattern Jan 17, 2017

JLLeitschuh commented Jan 18, 2017

View reviewed changes

Change Decorator to Delegate in spring codegen

5ca81eb

wing328 merged commit 36c3fa0 into swagger-api:master Jan 19, 2017

JLLeitschuh deleted the feat/JLL/spring-decorator-pattern branch January 19, 2017 15:58

JLLeitschuh commented Jan 20, 2017

View reviewed changes

JLLeitschuh mentioned this pull request Jan 23, 2017

[Java][Spring] Fix the rm to remove the correct file in two spring generation bash scripts #4630

Merged

3 tasks

wing328 pushed a commit that referenced this pull request Jan 25, 2017

Fix the rm to remove the correct file in two spring generation bash…

efd0732

… scripts (#4630) Related #4439

wing328 changed the title ~~Allows for generation of spring controller code using the delegate pattern~~ [Java][Spring] Allows for generation of spring controller code using the delegate pattern Feb 21, 2017

wing328 added the Server: Java label Feb 21, 2017

davidgri pushed a commit to davidgri/swagger-codegen that referenced this pull request May 11, 2017

Fix the rm to remove the correct file in two spring generation bash…

edc131a

… scripts (swagger-api#4630) Related swagger-api#4439




		public Callable<ResponseEntity<Client>> testClientModel(@ApiParam(value = "client model" ,required=true ) @RequestBody Client body) {

[Java][Spring] Allows for generation of spring controller code using the delegate pattern #4439

[Java][Spring] Allows for generation of spring controller code using the delegate pattern #4439

Uh oh!

Conversation

JLLeitschuh commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Description of the PR

Uh oh!

JLLeitschuh commented Dec 22, 2016

Uh oh!

wing328 Jan 10, 2017

Choose a reason for hiding this comment

Uh oh!

JLLeitschuh Jan 10, 2017

Choose a reason for hiding this comment

Uh oh!

wing328 commented Jan 10, 2017

Uh oh!

JLLeitschuh commented Jan 10, 2017

Uh oh!

wing328 commented Jan 12, 2017

Uh oh!

JLLeitschuh commented Jan 12, 2017

Uh oh!

cbornet commented Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JLLeitschuh commented Jan 13, 2017

Uh oh!

cbornet commented Jan 13, 2017

Uh oh!

JLLeitschuh commented Jan 13, 2017

Uh oh!

JLLeitschuh commented Jan 16, 2017

Uh oh!

wing328 commented Jan 17, 2017

Uh oh!

cbornet commented Jan 17, 2017

Uh oh!

wing328 commented Jan 17, 2017

Uh oh!

JLLeitschuh commented Jan 17, 2017

Uh oh!

JLLeitschuh commented Jan 17, 2017

Uh oh!

JLLeitschuh commented Jan 17, 2017

Uh oh!

JLLeitschuh commented Jan 17, 2017

Uh oh!

cbornet commented Jan 18, 2017

Uh oh!

wing328 commented Jan 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JLLeitschuh Jan 18, 2017

Choose a reason for hiding this comment

Uh oh!

JLLeitschuh commented Jan 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JLLeitschuh commented Jan 19, 2017

Uh oh!

wing328 commented Jan 19, 2017

Uh oh!

JLLeitschuh commented Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wing328 commented Jan 19, 2017

Uh oh!

cbornet commented Jan 19, 2017

Uh oh!

wing328 commented Jan 20, 2017

Uh oh!

JLLeitschuh commented Jan 20, 2017

Uh oh!

JLLeitschuh Jan 20, 2017

Choose a reason for hiding this comment

Uh oh!

JLLeitschuh Jan 20, 2017

JLLeitschuh commented Dec 20, 2016 •

edited

Loading

cbornet commented Jan 12, 2017 •

edited

Loading

wing328 commented Jan 18, 2017 •

edited

Loading

JLLeitschuh commented Jan 18, 2017 •

edited

Loading

JLLeitschuh commented Jan 19, 2017 •

edited

Loading