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

Psr interfaces #233

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

kimpepper
Copy link
Contributor

@kimpepper kimpepper commented Oct 22, 2024

Fixes #199

Description

This PR removes the dependency on a specific HTTP client implementation and introduces PSR-7, PSR-17 and PSR-18 interfaces.

Issues Resolved

It greatly simplifies the volume and complexity of the library codebase and removes the need to manage:

  • client configuration: it can be now done directly on the implementing HTTP client, e.g. SSL, Basic Auth, Retries
  • connection pools/host round-robin: this can be done with HTTP client middleware

Users of this library can choose from a number of synchronous and asynchronous HTTP clients including:

  • Guzzle 7
  • Symfony HTTP client

It greatly simplifies the volume and complexity of the library codebase. Specifically, it:

  • removes a number of transient dependencies including ezimuel/ringphp which is not longer getting updates

Breaking changes

  • We no longer json_decode the response body into an array.
  • We leave that to the client to decide how they want to do that, e.g. deserialize into an object
  • Clients can make use of \Psr\Http\Message\StreamInterface to decode the response in a memory efficient way

Tasks:

  • Replace Guzzle specific code with PSR interfaces
  • Remove unused code including Connection, ConnectionPool and CllientBuilder
  • Ensure test coverage

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.

I love this change, thank you! Let's iterate till we get this to green. I think it's a lot simpler than #228, what do you think @shyim?

@dblock
Copy link
Member

dblock commented Oct 22, 2024

I noticed you tried to fix DCO?

"Kim Pepper [[email protected]](mailto:[email protected])", but got "Kim Pepper [[email protected]](mailto:[email protected])".

Something is not adding up between your settings and the commit signature.

@kimpepper
Copy link
Contributor Author

I noticed you tried to fix DCO?

"Kim Pepper [[email protected]](mailto:[email protected])", but got "Kim Pepper [[email protected]](mailto:[email protected])".

Something is not adding up between your settings and the commit signature.

I think I fixed that now? Let me know if it's still not rigt.

I've updated the ClientBuilder and refactored the endpoints callable into it's own factory class. I think this makes it easier to understand, and also allows clients to swap out an implementation or use a dependency injection container if they want.

I updated the client and namespace templates also. I regenerated afterwards and it looks like there are a lot of new API methods added. Should I use something other than https://github.com/opensearch-project/opensearch-api-specification/releases/download/main-latest/opensearch-openapi.yaml ?

@shyim
Copy link
Collaborator

shyim commented Oct 23, 2024

I think setHandler are not working anymore right? So aws sig v4 needs to be adjusted?

@kimpepper
Copy link
Contributor Author

I think setHandler are not working anymore right? So aws sig v4 needs to be adjusted?

Yes, I think we'd need to rewrite the guzzle middleware/handler without the ring dependency to make it easier for people who use AWS OpenSearch service to use signed requests.

@dblock
Copy link
Member

dblock commented Oct 23, 2024

Something is not adding up between your settings and the commit signature.

I think I fixed that now? Let me know if it's still not rigt.

Yep looks good.

I've updated the ClientBuilder and refactored the endpoints callable into it's own factory class. I think this makes it easier to understand, and also allows clients to swap out an implementation or use a dependency injection container if they want.

👍

I updated the client and namespace templates also. I regenerated afterwards and it looks like there are a lot of new API methods added. Should I use something other than https://github.com/opensearch-project/opensearch-api-specification/releases/download/main-latest/opensearch-openapi.yaml ?

Let's work on merging #223 separately from this not to mix in the PSR changes. It needs some attention to tests. If nobody picks it up here I can do it soon.

@kimpepper kimpepper force-pushed the psr-interfaces branch 2 times, most recently from be04c45 to df01728 Compare October 24, 2024 07:55
@dblock
Copy link
Member

dblock commented Oct 24, 2024

Fixed CI in #234. Will merge and make a release with #223 next.

@kimpepper
Copy link
Contributor Author

Thinking about this more, returning HTTP Response might not be the best DX.

We should introduce a Response object that contains the body of the HTTP Response and any meta-data. We can add a toArray method to return the deserialized JSON. We can have 2 sub-classes ErrorResponse and OKResponse. The error response can have getErrorCode() and getErrorMessage() methods.

The Promise would also return this too.

Thoughts?

@dblock
Copy link
Member

dblock commented Oct 25, 2024

@kimpepper In other clients we return a strongly typed object that derives from a base class that also gives access to the underlying raw HTTP response from the network library "as is" along with some helper methods like status code or raw body. Note that OpenSearch returns more than JSON, too.

@dblock
Copy link
Member

dblock commented Oct 25, 2024

I fixed what was broken in #223 and merged, rebase.

@kimpepper
Copy link
Contributor Author

kimpepper commented Nov 1, 2024

@kimpepper In other clients we return a strongly typed object that derives from a base class that also gives access to the underlying raw HTTP response from the network library "as is" along with some helper methods like status code or raw body. Note that OpenSearch returns more than JSON, too.

I think the strongly typed object would require a lot of work on the generator side and might be out of scope for this issue?

Not sure whether it's worth a simple wrapper around the PSR Response and HTTP Promise that gets returned?

@kimpepper
Copy link
Contributor Author

From my understanding the response content types can be one of:

  • plain text
  • json
  • nd-json
  • xml

but I couldn't find the docs to back that up.

@dblock
Copy link
Member

dblock commented Nov 1, 2024

I think the strongly typed object would require a lot of work on the generator side and might be out of scope for this issue?

100%

Not sure whether it's worth a simple wrapper around the PSR Response and HTTP Promise that gets returned?

I would if we think it makes it easier to extend it later in a common way across multiple transports (it feels like it would).

@dblock
Copy link
Member

dblock commented Nov 1, 2024

From my understanding the response content types can be one of:

  • plain text
  • json
  • nd-json
  • xml

but I couldn't find the docs to back that up.

We cover the range in https://github.com/opensearch-project/opensearch-api-specification, see https://github.com/opensearch-project/opensearch-api-specification/blob/07e329e8d01fd0576de6a0a3c35412fd5a9163db/tools/src/tester/MimeTypes.ts#L10.

OpenSearch returns plain text, JSON, nd-json, CBOR, SMILE, and YAML, most from cat APIs.

Did you see XML somewhere? That may be a miss in API specs :)

@kimpepper
Copy link
Contributor Author

I created a separate PR #236 to deal with the bulk change to static return types.

@kimpepper
Copy link
Contributor Author

Split out #237 for the endpoint factory changes here.

@dblock
Copy link
Member

dblock commented Nov 4, 2024

Thanks @kimpepper! Let's work through those.

@kimpepper
Copy link
Contributor Author

Rebased on main and made a minor doc change.

@dblock
Copy link
Member

dblock commented Dec 16, 2024

@kimpepper
Copy link
Contributor Author

Yep, I think that's a great idea.

@kimpepper
Copy link
Contributor Author

Added UPGRADING.md

@kimpepper kimpepper force-pushed the psr-interfaces branch 2 times, most recently from a67354b to ebc598c Compare December 16, 2024 23:29
Signed-off-by: Kim Pepper <[email protected]>
src/OpenSearch/EndpointInterface.php Outdated Show resolved Hide resolved
src/OpenSearch/EndpointInterface.php Show resolved Hide resolved
src/OpenSearch/Namespaces/BooleanRequestWrapper.php Outdated Show resolved Hide resolved
src/OpenSearch/RequestFactoryInterface.php Show resolved Hide resolved
src/OpenSearch/TransportInterface.php Show resolved Hide resolved
tests/Endpoints/AbstractEndpointTest.php Show resolved Hide resolved
util/template/client-class Outdated Show resolved Hide resolved
UPGRADING.md Outdated Show resolved Hide resolved
src/OpenSearch/Aws/SigningClientDecorator.php Outdated Show resolved Hide resolved
kimpepper and others added 2 commits December 18, 2024 10:37
Co-authored-by: Christophe Coevoet <[email protected]>
Signed-off-by: Kim Pepper <[email protected]>
Signed-off-by: Kim Pepper <[email protected]>
@dblock
Copy link
Member

dblock commented Dec 18, 2024

The UPGRADING seems to imply that we do have some backwards incompatible changes in returns and configuration? I that case I do think this should bump us to 3.0, right? The deprecations would say 4.0.

@kimpepper
Copy link
Contributor Author

I added a lot of BC and deprecation triggers so this would go in 2.4.x and the BC layer removed in 3.0.x

@dblock
Copy link
Member

dblock commented Dec 18, 2024

Cool, cool. @stof and @shyim let's get this merged? Help review?

@kimpepper
Copy link
Contributor Author

The only thing not mentioned is dropping async which wasn't working anyway.

@dblock
Copy link
Member

dblock commented Dec 19, 2024

The only thing not mentioned is dropping async which wasn't working anyway.

Add a line. Better safe than sorry.

@kimpepper
Copy link
Contributor Author

Add a line. Better safe than sorry.

Added more details to the CHANGELOG.md

@kimpepper
Copy link
Contributor Author

I've been doing some manual testing of the new API with the drupal/search_api_opensearch module to see how easy it is to use and so far so good.

@kimpepper
Copy link
Contributor Author

I needed to create my own factories for Guzzle and Symfony as we allow Drupal config to manage a few of the settings such as base URI and basic auth.

@kimpepper
Copy link
Contributor Author

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.

[PROPOSAL] Request feedback on replacing ezimuel/ringphp
4 participants