Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[java][client][resteasy][jersey2][google-api-client][okhttp-json] several fixes to make sent requests more accurate #3703

Merged

Conversation

jmini
Copy link
Member

@jmini jmini commented Aug 20, 2019

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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.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

Fixes #3276
Fixes #3719
Fixes #3720

@auto-labeler
Copy link

auto-labeler bot commented Aug 20, 2019

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

jmini added a commit to jmini/openapi-experiments that referenced this pull request Aug 20, 2019
@jmini
Copy link
Member Author

jmini commented Aug 20, 2019

I found one issue with Jersey2 and sending a null entity:

java.lang.IllegalStateException: Entity must not be null for http method PUT.
    at org.glassfish.jersey.client.JerseyInvocation.validateHttpMethodAndEntity(JerseyInvocation.java:163)
    at org.glassfish.jersey.client.JerseyInvocation.<init>(JerseyInvocation.java:112)
    at org.glassfish.jersey.client.JerseyInvocation.<init>(JerseyInvocation.java:108)
    at org.glassfish.jersey.client.JerseyInvocation.<init>(JerseyInvocation.java:99)
    at org.glassfish.jersey.client.JerseyInvocation$Builder.method(JerseyInvocation.java:445)
    at org.glassfish.jersey.client.JerseyInvocation$Builder.put(JerseyInvocation.java:334)

I think the 2 solutions mentioned on StackOverflow might be interesting:

  • Disable those checks with SUPPRESS_HTTP_COMPLIANCE_VALIDATION.
  • Send an empty text entity.

Any opinion (can be solved in a follow-up PR)

@jmini
Copy link
Member Author

jmini commented Aug 20, 2019

WIP:

For the jersey2 client, for null value, I will stop calling jackson with the serialize method.

Entity<?> entity = serialize(body, formParams, contentType);

Instead when body is null Entity entity must be set to Entity.json("").

@jmini jmini added the WIP Work in Progress label Aug 20, 2019
@jmini jmini force-pushed the issue3276-no-new-object-for-empty-request-body branch from ceffe3f to 346c04a Compare August 21, 2019 09:44
@jmini jmini force-pushed the issue3276-no-new-object-for-empty-request-body branch from 346c04a to 2b53faa Compare August 21, 2019 10:08
@jmini jmini force-pushed the issue3276-no-new-object-for-empty-request-body branch from 2b53faa to 2bb9395 Compare August 21, 2019 11:47
@jmini jmini changed the title [java][client][resteasy][jersey2] Do not create new Object for empty request body [java][client][resteasy][jersey2][google-api-client][okhttp-json] several fixes to make sent requests more accurate Aug 21, 2019
@jmini jmini removed the WIP Work in Progress label Aug 21, 2019
@bkabrda
Copy link
Contributor

bkabrda commented Aug 21, 2019

The jersey2 part seems to be fine, it manages to solve #3719

I don't really have too many experience with the rest of the libraries, so it's hard for me to comment them, but 👍 from me on the jersey2 part and the general aim of this PR.

@jmini
Copy link
Member Author

jmini commented Aug 22, 2019

Approved by @kevinoid in #3276 (comment)

Discussed with @wing328 on Slack.

# for free to join this conversation on GitHub. Already have an account? # to comment