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 new generator: Java Play Framework Server Generator #4943

Merged
merged 7 commits into from
Mar 10, 2017
Merged

Add new generator: Java Play Framework Server Generator #4943

merged 7 commits into from
Mar 10, 2017

Conversation

JFCote
Copy link
Contributor

@JFCote JFCote commented Mar 6, 2017

First commit of the Java Play Framework server generator. It is highly based on Spring so there might me a couple of things that don't make sense (like options or parameters) for the Play Framework.

First time ever contributing to a GitHub project. Bare with me! If anything is wrong, let me know!

There is an existing issue asking for this framework here: #3057

I only support Java and currently have no time/interest in supporting Scala.

This was developed at the company I work for: Stingray. www.stingray.com

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

This is a working version of the Java Play Framework Server swagger-codegen. It will support most of the swagger specs except having arrays in some places because of a bug in the swagger-play (no support for this in the ImplicitParam). I have commented on this in the code itself and created the bugs in the swagger-play project but no one seems to be taking care of it ;)

Anyway, since it's my first time contributing on GitHub, I may have done n00b things, so just let me know and I'll provide fixes a soon as possible.

This has been run and tested using the Maven command "clean package" and everything runs, so it should be ok.

Thanks!

…y based on Spring so there might me a couple of things that don't make sense (like options or parameters) for the Play Framework.

//TODO: Validate this?
// play framework uses the jackson lib, why have a parameter for this?
additionalProperties.put("jackson", "true");
Copy link
Contributor

Choose a reason for hiding this comment

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

Putting something in the additionalProperties will allow you to use it in the mustache templates. For this case, one can do the following:

{{#jackson}}
// do something for jackson here
{{/jackson}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will keep it there so it use jackson by default. Will remove the TODO


@Override
public String getName() {
return "javaPlayFramework";
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest lower case with "-", e.g. java-play-framework
which is the convention used by other generators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in my next commit! Thanks

@@ -0,0 +1,8 @@
This software is licensed under the Apache 2 license, quoted below.
Copy link
Contributor

Choose a reason for hiding this comment

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

We might later need to empty this file as there's a discussion about not setting any license to the auto-generated code and let the users to decide which license (e.g. MIT, GNU, Apache2, etc) to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no problem. I was just mimicking the output of a brand new Play Framework project.

@wing328
Copy link
Contributor

wing328 commented Mar 6, 2017

@JFCote thanks for the contribution 👏 👏 👏

I left you some comments.

I would also suggest you to add a shell script (example) or batch file (example) to generate Petstore sample for this new generator. (or I can do it later for you after this PR is merged)

@wing328
Copy link
Contributor

wing328 commented Mar 6, 2017

Btw, for your upcoming PRs (not this PR), I would suggest creating a new branch as per git best-practice.

@JFCote
Copy link
Contributor Author

JFCote commented Mar 6, 2017

Just to be sure:
@wing328 : When you say to create a new branch, do you want me to create a branch in my forked repo and then submit this branch as a PR for my next PR?

@wing328
Copy link
Contributor

wing328 commented Mar 6, 2017

@JFCote yup, currently you're making changes in your master.

I usually create a new branch (e.g. git checkout -b 'fix_issue_9999') for the change.

@JFCote
Copy link
Contributor Author

JFCote commented Mar 6, 2017

@wing328 : Ok no problem I will do that on my next commits.

Copy link
Contributor

@ePaul ePaul left a comment

Choose a reason for hiding this comment

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

Hi, thanks for your pull request. I noted one issue which I just fixed everywhere from where you copied, so it would be nice if you fixed this before merging.

Please use {{baseName}} instead of {{paramName}} in annotations which refer to the on-the-wire parameter name.
{{paramName}} is the sanitized version of the parameter name, {{baseName}} how it was mentioned in the API definition. (See #4898 for a lot of cleanup regarding this.)

I noted all points where I found this.

(Though I don't know anything about how Play works, so it could be that in some of those places actually the sanitized name is better. If so, please tell me.)

@@ -0,0 +1 @@
{{#isFormParam}}{{#notFile}}@ApiParam(value = "{{{description}}}"{{#required}}, required=true{{/required}} {{#allowableValues}}, allowableValues="{{#enumVars}}{{{name}}}{{^-last}}, {{/-last}}{{#-last}}{{/-last}}{{/enumVars}}"{{/allowableValues}}{{#defaultValue}}, defaultValue="{{{defaultValue}}}"{{/defaultValue}}) @RequestPart(value="{{paramName}}"{{#required}}, required=true{{/required}}{{^required}}, required=false{{/required}}) {{{dataType}}} {{paramName}}{{/notFile}}{{#isFile}}@ApiParam(value = "file detail") @RequestPart("file") MultipartFile {{baseName}}{{/isFile}}{{/isFormParam}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use {{baseName}} instead of {{paramName}} in annotations which refer to the on-the-wire parameter name.
{{paramName}} is the sanitized version of the parameter name, {{baseName}} how it was mentioned in the API definition. (See #4898 for a lot of cleanup regarding this.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file has been deleted because it is not used (copy pasted from swing)

@ApiResponse(code = {{{code}}}, message = "{{{message}}}"{{#returnType}}, response = {{{returnType}}}.class{{/returnType}}){{#hasMore}}, {{/hasMore}}{{/responses}} })
{{#hasParams}}
@ApiImplicitParams({
{{#allParams}}{{^isPathParam}}@ApiImplicitParam(name = "{{paramName}}", value = "{{{description}}}"{{#required}}, required = true{{/required}}{{#defaultValue}}, defaultValue = "{{{defaultValue}}}"{{/defaultValue}}, dataType = "{{{dataTypeForImplicitParam}}}", paramType = "{{>paramType}}"){{#hasMore}},
Copy link
Contributor

Choose a reason for hiding this comment

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

Here possibly too? (I'm not quite sure what this annotation does.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here this annotation declares all params that are not "path" because in play framewowrk, every parameters that are not "path" are not parameters, they are extracted from the request() object. For this reason, I need to use an implicitParams. You are right, this should be a baseName (everything in the annotation should be baseName I think!)

//TODO: Maybe implement this in the future if we can support collection in the body params: see bug in swagger-play: https://github.com/swagger-api/swagger-play/issues/130
//TODO: Tt seems it is not detected that it's a list based on the collectionFormat field?
//WIP when both bugs will be fixed
//List<Pair> {{paramName}}Pair = SwaggerUtils.parameterToPairs("{{collectionFormat}}", "{{paramName}}", request().getQueryString("{{paramName}}"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too (at least the second one, I'm not sure about the first one).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

//}
{{/collectionFormat}}
{{^collectionFormat}}
String value{{paramName}} = request().getQueryString("{{paramName}}");
Copy link
Contributor

Choose a reason for hiding this comment

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

one more.

{{/queryParams}}
{{#formParams}}
{{^notFile}}
Http.MultipartFormData.FilePart {{paramName}} = request().body().asMultipartFormData().getFile("{{paramName}}");
Copy link
Contributor

Choose a reason for hiding this comment

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

One more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

//TODO: Maybe implement this in the future if we can support collection in the body params: see bug in swagger-play: https://github.com/swagger-api/swagger-play/issues/130
//TODO: Tt seems it is not detected that it's a list based on the collectionFormat field?
//WIP when both bugs will be fixed
//List<Pair> {{paramName}}Pair = SwaggerUtils.parameterToPairs("{{collectionFormat}}", "{{paramName}}", ((String[]) request().body().asMultipartFormData().asFormUrlEncoded().get("{{paramName}}"))[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

One (or two?) more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

//}
{{/collectionFormat}}
{{^collectionFormat}}
String value{{paramName}} = ((String[]) request().body().asMultipartFormData().asFormUrlEncoded().get("{{paramName}}"))[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

//TODO: Maybe implement this in the future if we can support collection in the body params: see bug in swagger-play: https://github.com/swagger-api/swagger-play/issues/130
//TODO: Tt seems it is not detected that it's a list based on the collectionFormat field?
//WIP when both bugs will be fixed
//List<Pair> {{paramName}}Pair = SwaggerUtils.parameterToPairs("{{collectionFormat}}", "{{paramName}}", request().getHeader("{{baseName}}"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too, possibly? (I'm not quite sure what the point of the first part in the pair is.) The second one is correctly {{baseName}}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was already ok!

Copy link
Contributor

Choose a reason for hiding this comment

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

My question was about the "{{paramName}}" in the SwaggerUtils.parameterToPairs() call.

I have no idea what this pair list is good for, but as this line is commented out, I guess it will not break anything.

@@ -0,0 +1 @@
{{#isQueryParam}}{{#useBeanValidation}}{{>beanValidationQueryParams}}{{/useBeanValidation}}@ApiParam(value = "{{{description}}}"{{#required}}, required = true{{/required}}{{#allowableValues}}, allowableValues = "{{#enumVars}}{{{name}}}{{^-last}}, {{/-last}}{{#-last}}{{/-last}}{{/enumVars}}"{{/allowableValues}}{{#defaultValue}}, defaultValue = "{{{defaultValue}}}"{{/defaultValue}}) @RequestParam(value = "{{paramName}}"{{#required}}, required = true{{/required}}{{^required}}, required = false{{/required}}{{#defaultValue}}, defaultValue="{{{defaultValue}}}"{{/defaultValue}}) {{{dataType}}} {{paramName}}{{/isQueryParam}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Another occurrence which should be "{{baseName}}".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this review. I will look at this tomorrow and send a fix. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file was deleted because not used

@JFCote
Copy link
Contributor Author

JFCote commented Mar 6, 2017

@wing328 Any idea why the CI didn't pass this time?

@ePaul
Copy link
Contributor

ePaul commented Mar 6, 2017

The CI currently sometimes randomly breaks in some Javascript based tests → see #4498.
Just try it again (it will run again when you change the top commit, e.g. by rebasing or adding more commits).

@ePaul
Copy link
Contributor

ePaul commented Mar 7, 2017

I don't know much about Play, so I can't comment on the general stuff, but the {{baseName}}/{{paramName}} stuff looks good now.

@wing328
Copy link
Contributor

wing328 commented Mar 8, 2017

@JFCote thanks for the PR. I did a test with Petstore but got the following errors when running sbt test:

[error] /Users/williamcheng/Code/jan2017/swagger-codegen/samples/server/petstore/java-play-framework/app/controllers/UserApiController.java:64: <identifier> expected
[error]             body = mapper.treeToValue(nodebody, List<User>.class);
[error] /Users/williamcheng/Code/jan2017/swagger-codegen/samples/server/petstore/java-play-framework/app/controllers/UserApiController.java:84: <identifier> expected
[error]             body = mapper.treeToValue(nodebody, List<User>.class);
[error] (compile:compileIncremental) javac returned nonzero exit code

Did you get similar error when running sbt test locally?

@JFCote
Copy link
Contributor Author

JFCote commented Mar 8, 2017

@wing328 : Yes I did. I'm not sure how to get around this one. Since I was not able to support array/list directly in the body/form/header/query params (because of this bug: swagger-api/swagger-play#130), I did not put more effort in fixing this.

As you can see, the @ApiImplicitParam does not support collection very well, unless I'm doing something wrong. So even if we fix the line body = mapper.treeToValue(nodebody, List<User>.class) by some manipulations, the generated swagger documentation will not show the correct thing. You can see my TODO in the mustache file itself.

So for the moment, maybe a warning somewhere during the generation telling that array are not supported directly in the body/form/header/query param would be good? Or if you know a way to fix this, feel free to fix it :) I had the "warning" in my todo list for a while but I'm not sure where to put this.

People can easily get around this bug by creating an object that contains a list inside which most people do by using standard enveloppe with meta and data.

Let me know what you think

@wing328
Copy link
Contributor

wing328 commented Mar 9, 2017

So even if we fix the line body = mapper.treeToValue(nodebody, List.class) by some manipulations, the generated swagger documentation will not show the correct thing

@JFCote I would suggest we fix the code first to make it compile and then the swagger documentation later as it's more important the code compiles without issue right after code generation.

@JFCote
Copy link
Contributor Author

JFCote commented Mar 9, 2017

@wing328 : I will look into it shortly. I may have question for you in the process. I'll let you know.

@JFCote
Copy link
Contributor Author

JFCote commented Mar 9, 2017

@wing328 : I have added the requested change so now it compiles perfectly. There are still problem with the documentation, but at least it compiles ok :)
Let me know if there is anything else.
I'm looking forward for the big merge :)

@wing328
Copy link
Contributor

wing328 commented Mar 9, 2017

@JFCote thanks. Let me check ...

@wing328
Copy link
Contributor

wing328 commented Mar 9, 2017

@JFCote I got a different error this time:

[error] /private/var/tmp/swagger-codegen/samples/server/petstore/java-play-framework/app/controllers/PetApiControllerImp.java:47: cannot find symbol
[error]   symbol:   class MultipartFile
[error]   location: class controllers.PetApiControllerImp
[error]     void uploadFile( Long petId, String additionalMetadata, MultipartFile file) {
[error] /private/var/tmp/swagger-codegen/samples/server/petstore/java-play-framework/app/controllers/PetApiController.java:242: package Http.MultipartFormData does not exist
[error]         Http.MultipartFormData.FilePart file = request().body().asMultipartFormData().getFile("file");
[info] /private/var/tmp/swagger-codegen/samples/server/petstore/java-play-framework/app/swagger/SwaggerUtils.java: /private/var/tmp/swagger-codegen/samples/server/petstore/java-play-framework/app/swagger/SwaggerUtils.java uses unchecked or unsafe operations.
[info] /private/var/tmp/swagger-codegen/samples/server/petstore/java-play-framework/app/swagger/SwaggerUtils.java: Recompile with -Xlint:unchecked for details.

Seems like due to missing import.

I wonder if you can take a look.

@JFCote
Copy link
Contributor Author

JFCote commented Mar 9, 2017

I have found the problem. Will push a change shortly.
Sorry I didn't find this earlier. It's because I was testing the generation with another script since I'm in windows. Now, I will test it using git-bash with the .sh.
Stay tuned, it should be there shortly :)

@JFCote
Copy link
Contributor Author

JFCote commented Mar 9, 2017

Here it is. I have even started the Play Server without problem and was able to see the documentation in Chrome. :)

Copy link
Contributor

@clasnake clasnake left a comment

Choose a reason for hiding this comment

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

Shall we generate the petstore samples as well?

@wing328
Copy link
Contributor

wing328 commented Mar 10, 2017

@JFCote it works for me!

java-play-framework|JFCote-master⚡ ⇒ sbt test
[info] Loading project definition from /private/var/tmp/swagger-codegen/samples/server/petstore/java-play-framework/project
[info] Set current project to swagger-java-playframework (in build file:/private/var/tmp/swagger-codegen/samples/server/petstore/java-play-framework/)
[info] Compiling 4 Scala sources and 14 Java sources to /private/var/tmp/swagger-codegen/samples/server/petstore/java-play-framework/target/scala-2.11/classes...
[info] /private/var/tmp/swagger-codegen/samples/server/petstore/java-play-framework/app/swagger/SwaggerUtils.java: /private/var/tmp/swagger-codegen/samples/server/petstore/java-play-framework/app/swagger/SwaggerUtils.java uses unchecked or unsafe operations.
[info] /private/var/tmp/swagger-codegen/samples/server/petstore/java-play-framework/app/swagger/SwaggerUtils.java: Recompile with -Xlint:unchecked for details.
[success] Total time: 41 s, completed Mar 10, 2017 10:08:18 PM

@wing328 wing328 merged commit 20c8f9a into swagger-api:master Mar 10, 2017
@wing328
Copy link
Contributor

wing328 commented Mar 10, 2017

@clasnake yup, there are some other enhancements I can think of as well. I'll create a task for tracking.

Thanks for reviewing the PR.

@wing328
Copy link
Contributor

wing328 commented Mar 10, 2017

Tweet: https://twitter.com/wing328/status/840205008444440576

Please help retweet to promote the new generator 🙇

@wing328
Copy link
Contributor

wing328 commented Mar 10, 2017

Opened #5003 for tracking enhancements.

@wing328 wing328 changed the title Java Play Framework Server Generator Add new generator: Java Play Framework Server Generator Mar 12, 2017
spr3nk3ls pushed a commit to spr3nk3ls/swagger-codegen that referenced this pull request Mar 28, 2017
* First commit of the Java Play Framework server generator. It is highly based on Spring so there might me a couple of things that don't make sense (like options or parameters) for the Play Framework.

* Fix suggestions in the PR discussion + add .bat and .sh file as requested.

* Updated Readme.md file

* Remove unused mustache file + fix baseName vs paramName in all the mustache files.

* Fix the compilation error when we have a body which is a list or map. Doesn't fix the problem with the annotation itself.

* Fix the problem with the Http.MultipartFormData.FilePart
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.

4 participants