[Java] Add support for Spring 5 WebClient by 0v1se · Pull Request #435 · OpenAPITools/openapi-generator

0v1se · 2018-07-02T15:00:27Z

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: master, 3.1.x, 4.0.x. Default: master.
[-] Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Basic support for Spring 5 WebClient with project reactor's Mono/Flux

jmini · 2018-07-02T16:20:10Z

Shippable — Run 1154 status is FAILED.

The new sample output samples/client/petstore/java/webclient is not up-to-date with the generator.

Please run:

mvn clean verify on the root
run bin/java-petstore-webclient.sh
commit the changes to this PR

jmini · 2018-07-02T16:30:19Z

If you like you can credit yourself in the root README.md under template creator:
https://github.com/OpenAPITools/openapi-generator#template-creator

jmini · 2018-07-02T16:46:54Z

For the CI, can you please add the new generated sample to /CI/pom.xml.circleci:

 <module>samples/client/petstore/java/webclient</module>

Here:

openapi-generator/CI/pom.xml.circleci

Lines 843 to 856 in 001f5ae

    
           <module>samples/client/petstore/java/feign</module> 
        
           <module>samples/client/petstore/java/jersey1</module> 
        
           <module>samples/client/petstore/java/jersey2</module> 
        
           <module>samples/client/petstore/java/okhttp-gson</module> 
        
           <module>samples/client/petstore/java/retrofit</module> 
        
           <module>samples/client/petstore/java/retrofit2</module> 
        
           <module>samples/client/petstore/java/retrofit2rx</module> 
        
           <module>samples/client/petstore/jaxrs-cxf-client</module> 
        
           <module>samples/client/petstore/java/resttemplate</module> 
        
           <module>samples/client/petstore/java/resttemplate-withXml</module> 
        
           <module>samples/client/petstore/java/vertx</module> 
        
           <module>samples/client/petstore/java/resteasy</module> 
        
           <module>samples/client/petstore/java/google-api-client</module> 
        
           <module>samples/client/petstore/java/rest-assured</module>

(the list is not really sorted... You can add your project somewhere near the other java client tests)

jmini · 2018-07-03T06:45:53Z

Thank you for the update.

I have fetch your branch on my computer to perform some tests

I have tried to run:

mvn verify -f samples/client/petstore/java/webclient

An I got a lot of compile errors, about org.threeten.bp missing. Maybe you should generate the sample with java8 date lib.

I also have compile error like this one:

[ERROR] /Users/jbr/Git/openapi-generator/samples/client/petstore/java/webclient/src/main/java/org/openapitools/client/api/PetApi.java:[144,72] cannot find symbol
[ERROR]   symbol:   variable CollectionFormat
[ERROR]   location: class org.openapitools.client.ApiClient

0v1se · 2018-07-03T07:01:19Z

@jmini thanks
will check this and update

0v1se · 2018-07-03T10:49:19Z

@jmini fixed some bugs.
Sample project compiles ok now.

I disabled threetenbp this way (no need for threetenbp cause java8 is required):

        if (WEBCLIENT.equals(getLibrary()) && "threetenbp".equals(dateLibrary)) {
            dateLibrary = "java8";
        }

Is it ok to disable threeten this way? Or is there better method?

jmini · 2018-07-03T15:19:47Z

Is it ok to disable threeten this way? Or is there better method?

Good question. For the moment we do not have really a way to define that some options are only available with certain lib or other config.

I had a quick look at the code, vertx requires java8 by doing it setJava8Mode(true) and additionalProperties.put("java8", "true") in the big if-elseif block. Maybe you can do it in a similar manner:

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/JavaClientCodegen.java

Lines 283 to 287 in 960412a

    
           } else if (VERTX.equals(getLibrary())) { 
        
               typeMapping.put("file", "AsyncFile"); 
        
               importMapping.put("AsyncFile", "io.vertx.core.file.AsyncFile"); 
        
               setJava8Mode(true); 
        
               additionalProperties.put("java8", "true");

jmini

This looks good to me in general.

Because you have raised it I have started to think about where to do the fixes for unsupported/wrong config.

I did not check this in detail.

jmini · 2018-07-03T15:22:29Z

...es/openapi-generator/src/main/java/org/openapitools/codegen/languages/JavaClientCodegen.java


    @Override
    public void processOpts() {
+        if (WEBCLIENT.equals(getLibrary()) && "threetenbp".equals(dateLibrary)) {


I guess that this check is too early. I am not sure, but I think that if the value is set to something on the command line, the value will be set with super.processOpts() and will override your check.

If possible I would override the wrong/unsupported values in the "else if" block line 290.

jmini · 2018-07-03T15:22:57Z

...es/openapi-generator/src/main/java/org/openapitools/codegen/languages/JavaClientCodegen.java

            additionalProperties.put("jackson", "true");
            supportingFiles.add(new SupportingFile("auth/Authentication.mustache", authFolder, "Authentication.java"));
+        } else if (WEBCLIENT.equals(getLibrary())) {
+            additionalProperties.put("jackson", "true");


here I would set other values like java8Mode, dateLibrary...

At first I added this check at line 290, but it was not working, because dateLibrary is used in AbstractJavaCodegen.processOpts(), so setting it on line 290 doesn't work.

As you stated, setting it on line 156 doesn't always work either (because it's set inside AbstractJavaCodegen.processOpts if parameter about date library is present).

I think, the only solution is to divide processOpts(). Currently it does some things:

gets params and sets fiels (e.g dateLibrary)

does some actions according to fields
For example, in the end of processOpts:

if ("threetenbp".equals(dateLibrary)) { additionalProperties.put("threetenbp", "true"); additionalProperties.put("jsr310", "true"); typeMapping.put("date", "LocalDate"); typeMapping.put("DateTime", "OffsetDateTime"); importMapping.put("LocalDate", "org.threeten.bp.LocalDate"); importMapping.put("OffsetDateTime", "org.threeten.bp.OffsetDateTime"); } else if ("joda".equals(dateLibrary)) {

Possibly, this method (processOpts) should do only one thing (should be divided)
and the method for checking/updating fields should be added.

Actually, it's not a big deal now. I can add back threetenbp support (but it's useless cause java8 support only), then there is no reason to do all these things now.

What do you think?

Thank you a lot for the analysis and the explanations.

I think that we can do the refactoring you have proposed (splitting processOpts()) in a separated PR.

jmini · 2018-07-04T04:51:38Z

I have tested the sample locally:

$ mvn verify -f samples/client/petstore/java/webclient/
[INFO] Scanning for projects...
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] Building petstore-webclient 1.0.0
[INFO] ------------------------------------------------------------------------
[INFO] 
...
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 3.694 s
[INFO] Finished at: 2018-07-03T16:01:27+02:00
[INFO] Final Memory: 21M/256M
[INFO] ------------------------------------------------------------------------

wing328

Overall it looks good 👍 we can update the README later to specify JDK8 as the minimal version.

jmini · 2018-07-04T07:41:25Z

@0v1se: Thank you a lot for this contribution.

jmini · 2018-07-04T07:49:48Z

Twitt: https://twitter.com/oas_generator/status/1014415775766073344

0v1se added 4 commits July 2, 2018 17:10

basic support for Spring's WebClient

b4a6143

added generated samples for Spring WebClient

4d87ba0

minor fixes

8cf7adb

removed temporary files

a25cf38

0v1se mentioned this pull request Jul 2, 2018

Add support for Spring 5 WebClient and projectreactor #434

Closed

jmini added the Client: Java label Jul 2, 2018

jmini changed the title ~~Add support for Spring 5 WebClient~~ [Java] Add support for Spring 5 WebClient Jul 2, 2018

0v1se added 3 commits July 3, 2018 08:23

update samples for java-webclient

d6f6f66

template creator

1af504d

CI

355b515

0v1se mentioned this pull request Jul 3, 2018

[JAVA] support WebClient based (Spring WebFlux framework) client generation swagger-api/swagger-codegen#7216

Open

fixes

315b312

jmini reviewed Jul 3, 2018

View reviewed changes

jmini approved these changes Jul 4, 2018

View reviewed changes

wing328 approved these changes Jul 4, 2018

View reviewed changes

jmini added this to the 3.1.0 milestone Jul 4, 2018

jmini merged commit b90c53d into OpenAPITools:master Jul 4, 2018

wing328 added the Enhancement: New generator label Jul 4, 2018

This was referenced Jul 4, 2018

[java-client] WebClient requires java8 #460

Merged

Shippable: fix bin/java-petstore-webclient.sh #463

Merged

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019

[Java-client] Add Spring 5 WebClient as new library (OpenAPITools#435)

c795787

Uh oh!

Conversation

0v1se commented Jul 2, 2018

PR checklist

Description of the PR

Uh oh!

jmini commented Jul 2, 2018

Uh oh!

jmini commented Jul 2, 2018

Uh oh!

jmini commented Jul 2, 2018

Uh oh!

jmini commented Jul 3, 2018

Uh oh!

0v1se commented Jul 3, 2018

Uh oh!

0v1se commented Jul 3, 2018

Uh oh!

jmini commented Jul 3, 2018

Uh oh!

jmini left a comment

Choose a reason for hiding this comment

Uh oh!

jmini Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

jmini Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

0v1se Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

jmini Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

jmini commented Jul 4, 2018

Uh oh!

wing328 left a comment

Choose a reason for hiding this comment

Uh oh!

jmini commented Jul 4, 2018

Uh oh!

jmini commented Jul 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants