Psr interfaces

[kimpepper]

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.

[dblock]

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?

• Sign DCO, git commit -s --amend for existing commits, force push.
• Add to CHANGELOG.
• Let's add an UPGRADING.md like https://github.com/opensearch-project/opensearch-py/blob/main/UPGRADING.md with details on migrating to this version.
• Should we keep a OpenSearch\ClientBuilder even with different parameters for easier migration?

[dblock]

I noticed you tried to fix DCO?

"Kim Pepper [kim@previousnext.com.au](mailto:kim@previousnext.com.au)", but got "Kim Pepper [kim@pepper.id.au](mailto:kim@pepper.id.au)".

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

[kimpepper]

I noticed you tried to fix DCO?

"Kim Pepper [kim@previousnext.com.au](mailto:kim@previousnext.com.au)", but got "Kim Pepper [kim@pepper.id.au](mailto:kim@pepper.id.au)".

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]

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

[kimpepper]

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]

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.

[dblock]

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

[kimpepper]

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]

@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]

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

[kimpepper]

@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]

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]

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]

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]

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

[kimpepper]

Split out #237 for the endpoint factory changes here.

[dblock]

Thanks @kimpepper! Let's work through those.

[kimpepper]

Thanks for your review @stof . I'm holding off any changes here until the dependent issue #237 lands.

[dblock]

@kimpepper rebase let's see what this looks like?

[kimpepper]

@kimpepper rebase let's see what this looks like?

I think there is a fair bit to do if we are dropping async support in this PR.

[kimpepper]

I did a basic rebase off main but this is still pretty rough. I'm not sure how we plan on handling BC.

Should we split off removing the async client?

[kimpepper]

Thinking about this a bit further, we can keep the existing Transport, deprecate it, and introduce a new PsrTransport.

Still need to figure out how to handle BC for the return types.

[dblock]

This one is a major upgrade that is definitely worth it, so I would introduce this as a breaking change. This is also the right time to change what these APIs return and really try to pack all the breakages in one major upgrade.

[stof]

This PR seems to remove all support for sigv4. Shouldn't it provide a PSR client decorator implementing the sigv4 logic, as all other opensearch clients are shipping support for sigv4 to support the AWS managed opensearch service ?

[dblock]

This PR seems to remove all support for sigv4. Shouldn't it provide a PSR client decorator implementing the sigv4 logic, as all other opensearch clients are shipping support for sigv4 to support the AWS managed opensearch service ?

Yes, we definitely do not want to lose Sigv4 functionality. Breaking change is OK, but we need all the existing features.

[kimpepper]

In the other issues we are trying to keep a BC layer for the 2.x branch. Are we saying this PR would go straight into a 3.x branch without any deprecations in 2.x?

If we try to get this in 2.x and provide BC this PR will:

• Drop async support (aparrently not working anyway according to #219 (comment)
• Deprecate the OpenSearch\ConnectionPool and OpenSearch\Connections namespaces (can be provided by Psr client implementations)
• Deprecate \OpenSearch\Transport
• Create a new \OpenSearch\PsrTransport
• Deprecate ClientBuilder
• Create a new \OpenSearch\PsrClientBuilder
• Deprecate \OpenSeach\Client
• Create a new \OpenSearch\PsrClient
• PsrClient passes PsrTransport $transport param to \OpenSearch\Namespaces\AbstractNamespace::__construct()
• Handle BC and trigger deprecations in \OpenSearch\Namespaces\AbstractNamespace::__construct() for old Transport
• Change \OpenSearch\Namespaces\AbstractNamespace::performRequest() to only return an array and drop it returning FutureArrayInterface or callable (as they aren't used)
• Change \OpenSearch\Handlers\SigV4Handler to work with Psr interfaces (follow up?)

This seems like quite a lot. Do you agree with the approach? Do you think we can split any of this out to sub-tasks or follow-ups?

[dblock]

In the other issues we are trying to keep a BC layer for the 2.x branch. Are we saying this PR would go straight into a 3.x branch without any deprecations in 2.x?

All breaking changes must go to main which will be the next major version of the client (3.0.0).

Drop async support (aparrently not working anyway according to #219 (comment)

If it's really not working and never worked, it's not a feature, and we can take a delete in 2.x before this to trim the code.

Only breaking changes warrant a 3.0. If the sum of these can be done in a way of being deprecated -> added then we can continue doing it incrementally. I think you know best @kimpepper :)

[kimpepper]

Yes, the intention is to provide deprecations in 2.x to allow users to upgrade to the Psr approach before 3.x is released.

[kimpepper]

I've pushed new code to implement the concepts above

I haven't looked at tests yet.

[stof]

@dblock async requests are working in the existing codebase for the method allowing to do a raw request (Client::request) but that's all. None of the generated SDK methods allow configuring the async option when calling performRequest, so the support for async requests in the Transport is only for Client::request. This PR was adding async in all methods though a global switch, making all return types blurry.

[stof]

This technically changes the type of the public $transport property to Transport|null as it does not assign a transport.

[dblock]

@dblock async requests are working in the existing codebase for the method allowing to do a raw request (Client::request) but that's all. None of the generated SDK methods allow configuring the async option when calling performRequest, so the support for async requests in the Transport is only for Client::request. This PR was adding async in all methods though a global switch, making all return types blurry.

Got it. I would remove it and see if anyone complains, but technically it's a breaking change. I will 🙈 if it gets removed.