Skip to content
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

Fix ApacheHttpClient's handling of request bodies on DELETE, GET, HEAD & OPTIONS requests #5704

Merged
merged 5 commits into from
Nov 25, 2024

Conversation

Xtansia
Copy link
Contributor

@Xtansia Xtansia commented Nov 11, 2024

Motivation and Context

Though providing request bodies on these HTTP methods is uncommon, it is not disallowed by the spec, and is used in certain Elasticsearch/OpenSearch APIs. Authenticating with Amazon OpenSearch Service can be done via SigV4 and we re-use the SDK's signing logic to provide that in https://github.com/opensearch-project/opensearch-java.
We've received multiple bug reports of users having trouble making certain requests using the ApacheHttpClient (opensearch-project/opensearch-java#712, opensearch-project/opensearch-java#521), where the client itself doesn't complain or error but silently drops the request body resulting in a mismatched signature on the server.
The Netty & CRT Sdk(Async)HttpClient implementations correctly send the request body for all methods, URL Connection does as well with the exception of GET which is hardcoded to swap to POST (and doesn't support PATCH at all).
As such I think it is not harmful to bring the Apache implementation in line with the other client implementations.

Modifications

Generalize the Apache HTTP request factory implementation to attach the request body entity on all HTTP methods.
This matches the original implementations of the separate method classes:

Testing

Added unit tests

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@victorekpo
Copy link

Awesome work! 👏 I just ran into this issue with the scroll API because the DELETE requests have a body. Thanks for looking into this.

@davidh44
Copy link
Contributor

Thanks for the PR, looks good overall!

Builds are failing due to protocol tests which previously expected absent content length header, can we update the tests accordingly?

[ERROR] Failures:
--
[ERROR]   RestJsonContentTypeProtocolTest.runProtocolTest:48 Header 'Content-Length' was expected to be absent

@Xtansia Xtansia force-pushed the fix/apache-always-pass-request-body branch from c50a248 to 222d1f6 Compare November 24, 2024 23:08
Copy link

sonarcloud bot commented Nov 25, 2024

@davidh44 davidh44 added this pull request to the merge queue Nov 25, 2024
Merged via the queue into aws:master with commit b2fcb7e Nov 25, 2024
11 of 19 checks passed
@Xtansia Xtansia deleted the fix/apache-always-pass-request-body branch November 25, 2024 22:13
@davidh44
Copy link
Contributor

@all-contributors please add @Xtansia for code

Copy link
Contributor

@davidh44

I've put up a pull request to add @Xtansia! 🎉

aws-sdk-java-automation pushed a commit that referenced this pull request Dec 3, 2024
…GET, HEAD & OPTIONS requests (#5704)"

This reverts commit b2fcb7e.
aws-sdk-java-automation pushed a commit that referenced this pull request Dec 3, 2024
Revert "Fix ApacheHttpClient's handling of request bodies on DELETE, GET, HEAD & OPTIONS requests (#5704)
Xtansia added a commit to Xtansia/aws-sdk-java-v2 that referenced this pull request Dec 5, 2024
…D & OPTIONS requests (aws#5704)

* Add tests

* Fix ApacheHttpClient's handling of request bodies on DELETE, GET, HEAD & OPTIONS requests

* Fix style

* Handle HttpURLConnection switching GET->POST and not supporting PATCH

* Update protocol test

(cherry picked from commit b2fcb7e)
Xtansia added a commit to Xtansia/aws-sdk-java-v2 that referenced this pull request Dec 6, 2024
…D & OPTIONS requests (aws#5704)

* Add tests

* Fix ApacheHttpClient's handling of request bodies on DELETE, GET, HEAD & OPTIONS requests

* Fix style

* Handle HttpURLConnection switching GET->POST and not supporting PATCH

* Update protocol test

(cherry picked from commit b2fcb7e)
Xtansia added a commit to Xtansia/aws-sdk-java-v2 that referenced this pull request Dec 6, 2024
…D & OPTIONS requests (aws#5704)

* Add tests

* Fix ApacheHttpClient's handling of request bodies on DELETE, GET, HEAD & OPTIONS requests

* Fix style

* Handle HttpURLConnection switching GET->POST and not supporting PATCH

* Update protocol test

(cherry picked from commit b2fcb7e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants