-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
[Java] Add support for Spring 5 WebClient #435
Conversation
The new sample output Please run:
|
If you like you can credit yourself in the root |
For the CI, can you please add the new generated sample to <module>samples/client/petstore/java/webclient</module> Here: openapi-generator/CI/pom.xml.circleci Lines 843 to 856 in 001f5ae
(the list is not really sorted... You can add your project somewhere near the other java client tests) |
Thank you for the update. I have fetch your branch on my computer to perform some tests I have tried to run:
An I got a lot of compile errors, about I also have compile error like this one:
|
@jmini thanks |
@jmini fixed some bugs. 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? |
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, Lines 283 to 287 in 960412a
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -151,6 +153,10 @@ public String getHelp() { | |||
|
|||
@Override | |||
public void processOpts() { | |||
if (WEBCLIENT.equals(getLibrary()) && "threetenbp".equals(dateLibrary)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -280,6 +286,8 @@ public void processOpts() { | |||
} else if (RESTTEMPLATE.equals(getLibrary())) { | |||
additionalProperties.put("jackson", "true"); | |||
supportingFiles.add(new SupportingFile("auth/Authentication.mustache", authFolder, "Authentication.java")); | |||
} else if (WEBCLIENT.equals(getLibrary())) { | |||
additionalProperties.put("jackson", "true"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here I would set other values like java8Mode
, dateLibrary
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I have tested the sample locally:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks good 👍 we can update the README later to specify JDK8 as the minimal version.
@0v1se: Thank you a lot for this contribution. |
PR checklist
./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\
.master
,3.1.x
,4.0.x
. Default:master
.Description of the PR
Basic support for Spring 5 WebClient with project reactor's Mono/Flux