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

Handle HTTP exceptions in HttpTransport #254

Merged

Conversation

kimpepper
Copy link
Collaborator

@kimpepper kimpepper commented Jan 15, 2025

Description

As discussed in #251 (comment) we agreed that we should keep the existing behaviour of throwing exceptions.

This PR:

  • Reverts the opt-in flag for throwing exceptions
  • Creates an interface for HTTP Exceptions that can return a status code and headers for client debugging
  • Creates a hierarchy of Exceptions for HTTP Status Codes that implement the interface
  • Creates an HTTP Exception Factory for creating the exceptions based on status code and errors in the response body
  • Creates an Error Message Extractor which copies the logic from the deprecated Connection class for extracting a useful error message from the response body
  • Adds tests

Issues Resolved

#250

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Looks good!

I think PHPStan is right to complain on its warnings in src/OpenSearch/Client.php.

@kimpepper kimpepper force-pushed the remove-throw-exceptions-flag branch from f99404f to 12c5ea2 Compare January 15, 2025 23:10
@kimpepper
Copy link
Collaborator Author

Test fails are due to #255

@kimpepper kimpepper force-pushed the remove-throw-exceptions-flag branch from 12c5ea2 to b0867d9 Compare January 17, 2025 00:25
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 63.63636% with 44 lines in your changes missing coverage. Please review.

Project coverage is 23.45%. Comparing base (887df5e) to head (66e7996).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/OpenSearch/Connections/Connection.php 22.22% 7 Missing ⚠️
...ch/Common/Exceptions/NoDocumentsToGetException.php 0.00% 4 Missing ⚠️
...ch/Common/Exceptions/NoShardAvailableException.php 0.00% 4 Missing ⚠️
...arch/Common/Exceptions/RoutingMissingException.php 0.00% 4 Missing ⚠️
...mon/Exceptions/ScriptLangNotSupportedException.php 0.00% 4 Missing ⚠️
...rch/Common/Exceptions/Unauthorized401Exception.php 0.00% 4 Missing ⚠️
src/OpenSearch/Exception/ConflictHttpException.php 0.00% 2 Missing ⚠️
...rc/OpenSearch/Exception/ForbiddenHttpException.php 0.00% 2 Missing ⚠️
src/OpenSearch/Exception/HttpException.php 66.66% 2 Missing ⚠️
...penSearch/Exception/NotAcceptableHttpException.php 0.00% 2 Missing ⚠️
... and 5 more
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #254      +/-   ##
============================================
- Coverage     24.02%   23.45%   -0.58%     
- Complexity     3398     3557     +159     
============================================
  Files           485      524      +39     
  Lines         12984    13595     +611     
============================================
+ Hits           3120     3189      +69     
- Misses         9864    10406     +542     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kimpepper kimpepper force-pushed the remove-throw-exceptions-flag branch from 75ce0d6 to fff8698 Compare January 17, 2025 02:32
Signed-off-by: Kim Pepper <[email protected]>
@kimpepper kimpepper force-pushed the remove-throw-exceptions-flag branch from fff8698 to 66e7996 Compare January 17, 2025 02:33
@kimpepper
Copy link
Collaborator Author

Not sure about the code coverage report fails. We are adding tests here.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

👏

@dblock dblock merged commit d30188f into opensearch-project:main Jan 17, 2025
46 of 47 checks passed
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.

2 participants