Skip to content

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

Open
ignaciomolina wants to merge 11 commits intoswagger-api:masterfrom
ignaciomolina:feature/play26AsyncClientWithRetrofit2
Open

Add support for async client Retrofit2 in Java Play Framework 2.6#7664
ignaciomolina wants to merge 11 commits intoswagger-api:masterfrom
ignaciomolina:feature/play26AsyncClientWithRetrofit2

Conversation

@ignaciomolina
Copy link
Copy Markdown
Contributor

@ignaciomolina ignaciomolina commented Feb 15, 2018

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

@tzimisce012
Copy link
Copy Markdown
Contributor

Good Job there 👍

@JFCote
Copy link
Copy Markdown
Contributor

JFCote commented Feb 15, 2018

I will have a look at this tomorrow. Thanks

Copy link
Copy Markdown
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

// authentications.put("{{name}}", new OAuth());{{/isOAuth}}{{/authMethods}}
// Prevent the authentications from being modified.
authentications = Collections.unmodifiableMap(authentications);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove empty line

public Play26CallAdapterFactory() {
}

public Play26CallAdapterFactory(
Copy link
Copy Markdown
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
Copy Markdown
Contributor

JFCote commented Feb 16, 2018

@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 :)


public Play26CallAdapterFactory() {
}
public Play26CallAdapterFactory() { }
Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍

@tzimisce012
Copy link
Copy Markdown
Contributor

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.

@lukoyanov
Copy link
Copy Markdown
Contributor

lukoyanov commented Feb 16, 2018

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.

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
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.

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 great work !

@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 this to the v2.4.0 milestone Feb 22, 2018
@wing328
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Contributor Author

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

@ignaciomolina
Copy link
Copy Markdown
Contributor Author

@wing328 Do you think this is good to merge?

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.

7 participants