Add support for async client Retrofit2 in Java Play Framework 2.6 #7664

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

Jump to bottom

Open

ignaciomolina wants to merge 11 commits into swagger-api:master

from ignaciomolina:feature/play26AsyncClientWithRetrofit2

Open

Add support for async client Retrofit2 in Java Play Framework 2.6 #7664

ignaciomolina wants to merge 11 commits into swagger-api:master from ignaciomolina:feature/play26AsyncClientWithRetrofit2

Conversation

@ignaciomolina

Copy link

Contributor

@ignaciomolina ignaciomolina commented Feb 15, 2018 •

edited

Loading

PR checklist

Read the contribution guidelines.
Ran the shell 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). Windows batch files can be found in .\bin\windows\.
Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

@bbdouglas @JFCote @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger

Description of the PR

Add templates to support Play Framework version 2.6 with async clients using Retrofit2

@ignaciomolina


 Add templates for async client Retrofit2 in Java Play Framework 2.6

cf26cd4

@tzimisce012

Copy link

Contributor

tzimisce012 commented Feb 15, 2018

Good Job there 👍

@ignaciomolina


 Add samples for retrofit2

f9eb376

@JFCote

Copy link

Contributor

JFCote commented Feb 15, 2018

I will have a look at this tomorrow. Thanks

JFCote

JFCote approved these changes

Feb 16, 2018

View reviewed changes

Copy link

Contributor

@JFCote JFCote left a comment

Choose a reason for hiding this comment

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

LGTM

modules/swagger-codegen/src/main/resources/Java/libraries/retrofit2/play26/ApiClient.mustache Outdated

// authentications.put("{{name}}", new OAuth());{{/isOAuth}}{{/authMethods}}

// Prevent the authentications from being modified.

authentications = Collections.unmodifiableMap(authentications);

Copy link

Contributor

Choose a reason for hiding this comment

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

remove empty line

...codegen/src/main/resources/Java/libraries/retrofit2/play26/Play26CallAdapterFactory.mustache Outdated

public Play26CallAdapterFactory() {

}

public Play26CallAdapterFactory(

Copy link

Contributor

Choose a reason for hiding this comment

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

why is there a carriage return here?

@JFCote

Copy link

Contributor

@ignaciomolina For my curiosity only, what is the use of this client and what is the "link" to play framework. I'm the main contributor to the Play Framework server stub codegen and to connect to it, we use java client, c# client, typescript client but I would like to know the advantage of using this one. Thanks :)

@ignaciomolina


 Fix style in java retrofit2 play26 templates

7af5a98

JFCote

JFCote reviewed

Feb 16, 2018

View reviewed changes

...codegen/src/main/resources/Java/libraries/retrofit2/play26/Play26CallAdapterFactory.mustache Outdated

public Play26CallAdapterFactory() {

}

public Play26CallAdapterFactory() { }

Copy link

Contributor

@JFCote JFCote Feb 16, 2018

Choose a reason for hiding this comment

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

it wasn't that constructor, it was the one below with a parameter :)

Copy link

Contributor Author

@ignaciomolina ignaciomolina Feb 16, 2018

Choose a reason for hiding this comment

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

👍

@tzimisce012

Copy link

Contributor

tzimisce012 commented Feb 16, 2018

Well, this client will allow us to use WSClient that is an asynchronous client. A regular java client will be blocking and we will have to set a specific thread pool just for him.

@ignaciomolina


 Fix carrot return in java retrofit2 play26 templates

a522f77

@lukoyanov

Copy link

Contributor

lukoyanov commented Feb 16, 2018 •

edited by wing328

Loading

For my curiosity only, what is the use of this client and what is the "link" to play framework.

@JFCote the only connection of ApiClient.java to play! framework is that internally adds callFactory & callAdapterFactory that uses WSClient to discatch api calls.

lukoyanov

lukoyanov approved these changes

Feb 17, 2018

View reviewed changes

tzimisce012

tzimisce012 approved these changes

Feb 17, 2018

View reviewed changes

cbornet

cbornet reviewed

Feb 19, 2018

View reviewed changes

modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/JavaClientCodegen.java

cliOptions.add(CliOption.newBoolean(USE_RX_JAVA2, "Whether to use the RxJava2 adapter with the retrofit2 library."));

cliOptions.add(CliOption.newBoolean(PARCELABLE_MODEL, "Whether to generate models for Android that implement Parcelable with the okhttp-gson library."));

cliOptions.add(CliOption.newBoolean(USE_PLAY_WS, "Use Play! Async HTTP client (Play WS API)"));

cliOptions.add(CliOption.newString(PLAY_VERSION, "Version of Play! Framework (possible values \"play24\", \"play25\")"));

Copy link

Contributor

@cbornet cbornet Feb 19, 2018

Choose a reason for hiding this comment

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

Do we really have to support each and every version of Play ? This is getting really messy...

Copy link

Contributor

@tzimisce012 tzimisce012 Feb 19, 2018

Choose a reason for hiding this comment

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

Since each change of version of play means that everything changes completly, it is so far necessary to keep all of them...

Copy link

Contributor

@cbornet cbornet Feb 19, 2018

Choose a reason for hiding this comment

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

This is a retrofit generator, not a Play one. Also it seems to me that a lot of templates are just duplicated between 2.5 and 2.6

Copy link

Contributor

@tzimisce012 tzimisce012 Feb 19, 2018

Choose a reason for hiding this comment

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

I am glad that the client generator for play is using retrofit.

There is a folder called common where all duplicated templates between versions 2.5 and 2.6 should be. If there is a duplicated template not located there, this PR should be rejected

Copy link

Contributor

@cbornet cbornet Feb 19, 2018 •

edited

Loading

Choose a reason for hiding this comment

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

I looked and it seems 2.6 is almost entirely duplicate of 2.5 (minor changes but that probably also work for 2.5)
I also think it should be possible to have common templates between 2.4 and 2.5 with minimal work which would greatly simplify the Java generator code.
And 2.5 seems to introduce adapters for CompletionStage (CompletableFuture) which are already part of retrofit-adapters so they could probably be replaced by the built in ones.

Copy link

Contributor

@cbornet cbornet Feb 19, 2018

Choose a reason for hiding this comment

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

Eventually, wouldn't Play developers prefer to get scala.concurrent.Future rather than CompletionStage/CompletableFuture ?

Copy link

Contributor

@cbornet cbornet Feb 19, 2018

Choose a reason for hiding this comment

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

Those minor changes don't work on play 2.5 (100% sure)

OK so they should be applied by mustache tags, not by duplicating the templates.

Copy link

Contributor

@tzimisce012 tzimisce012 Feb 19, 2018

Choose a reason for hiding this comment

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

Meanwhile there are 2 different versions of Play (Scala and Java), we should have an autogenerated client for both versions.

I don't know what could be the reason to program play with java. Moreover, I don't know the reason to program on play at all 🍡

Copy link

Contributor Author

@ignaciomolina ignaciomolina Feb 20, 2018

Choose a reason for hiding this comment

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

ok, I have merged the play version templates

Copy link

Contributor

@cbornet cbornet Feb 20, 2018

Choose a reason for hiding this comment

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

👍 great work !

@ignaciomolina


 Remove duplicated templates for client retrofit2-play

695019e

@ignaciomolina ignaciomolina changed the title ~~(削除) Add templates for async client Retrofit2 in Java Play Framework 2.6 (削除ここまで)~~ (追記) Add support for async client Retrofit2 in Java Play Framework 2.6 (追記ここまで)

Feb 20, 2018

@wing328 wing328 added Client: Java Client: Retrofit labels

Feb 22, 2018

@wing328 wing328 added this to the v2.4.0 milestone

Feb 22, 2018

@wing328 wing328 added the Enhancement: Feature label

Feb 22, 2018

@wing328

Copy link

Contributor

wing328 commented Feb 22, 2018

I ran mvn test in the folder samples/client/petstore/java/retrofit2-play26 but got the following errors:

[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] /private/tmp/swagger-codegen/samples/client/petstore/java/retrofit2-play26/src/main/java/io/swagger/client/PlayCallFactory.java:[141,56] cannot find symbol
 symbol: method setRequestFilter(play.libs.ws.WSRequestFilter)
 location: variable wsRequest of type play.libs.ws.WSRequest
[ERROR] /private/tmp/swagger-codegen/samples/client/petstore/java/retrofit2-play26/src/main/java/io/swagger/client/PlayCallFactory.java:[178,36] cannot find symbol
 symbol: method getSingleHeader(java.lang.String)
 location: variable r of type play.libs.ws.WSResponse

Did you get these errors when running mvn test locally in your machine?

ignaciomolina added 3 commits

March 7, 2018 20:23

@ignaciomolina


 Fix a few bugs in java retrofit2 client for play mustaches

3735f46

@ignaciomolina


 regenerate samples for all java retrofit2 play versions

01fde98

@ignaciomolina


 Add validation library for java retrofit2 clients in sbt and gradle

a5128c4

@ignaciomolina

Copy link

Contributor Author

ignaciomolina commented Mar 7, 2018

Sorry @wing328, you were right, I already fix all the issues and now it compiles fine in Maven, Sbt and Gradle

ignaciomolina added 3 commits

March 7, 2018 21:56

@ignaciomolina


 Merge branch 'master' into feature/play26AsyncClientWithRetrofit2

f66c454

@ignaciomolina


 Fix merge conflict

0f9dd0d

@ignaciomolina


 fix failing test in samples

234d809

@ignaciomolina

Copy link

Contributor Author

ignaciomolina commented Apr 30, 2018

@wing328 Do you think this is good to merge?

@frantuma frantuma added the Language: retrofit label

Jun 27, 2018

@frantuma frantuma force-pushed the master branch from 4ba5164 to 949f0e6 Compare

May 20, 2025 09:13

Labels

Client: Java Client: Retrofit Enhancement: Feature Language: retrofit

7 participants

@ignaciomolina @tzimisce012 @JFCote @lukoyanov @wing328 @cbornet @frantuma

Add support for async client Retrofit2 in Java Play Framework 2.6 #7664

Are you sure you want to change the base?

Add support for async client Retrofit2 in Java Play Framework 2.6 #7664

Uh oh!

Conversation

@ignaciomolina ignaciomolina commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Description of the PR

Uh oh!

tzimisce012 commented Feb 15, 2018

Uh oh!

JFCote commented Feb 15, 2018

Uh oh!

@JFCote JFCote left a comment

Choose a reason for hiding this comment

Uh oh!

@JFCote JFCote Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

@JFCote JFCote Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

JFCote commented Feb 16, 2018

Uh oh!

@JFCote JFCote Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

@ignaciomolina ignaciomolina Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

tzimisce012 commented Feb 16, 2018

Uh oh!

lukoyanov commented Feb 16, 2018 • edited by wing328 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@cbornet cbornet Feb 19, 2018

Choose a reason for hiding this comment

Uh oh!

@tzimisce012 tzimisce012 Feb 19, 2018

Choose a reason for hiding this comment

Uh oh!

@cbornet cbornet Feb 19, 2018

Choose a reason for hiding this comment

Uh oh!

@tzimisce012 tzimisce012 Feb 19, 2018

Choose a reason for hiding this comment

Uh oh!

@cbornet cbornet Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@cbornet cbornet Feb 19, 2018

Choose a reason for hiding this comment

Uh oh!

@cbornet cbornet Feb 19, 2018

Choose a reason for hiding this comment

Uh oh!

@tzimisce012 tzimisce012 Feb 19, 2018

Choose a reason for hiding this comment

Uh oh!

@ignaciomolina ignaciomolina Feb 20, 2018

Choose a reason for hiding this comment

Uh oh!

@cbornet cbornet Feb 20, 2018

Choose a reason for hiding this comment

Uh oh!

wing328 commented Feb 22, 2018

Uh oh!

ignaciomolina commented Mar 7, 2018

Uh oh!

ignaciomolina commented Apr 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

@ignaciomolina ignaciomolina commented Feb 15, 2018 •

edited

Loading

lukoyanov commented Feb 16, 2018 •

edited by wing328

Loading

@cbornet cbornet Feb 19, 2018 •

edited

Loading