Psr interfaces #233

kimpepper · 2024-10-22T04:32:58Z

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 · 2024-10-22T23:51:41Z

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 · 2024-10-23T04:48:02Z

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 · 2024-10-23T05:51:20Z

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

kimpepper · 2024-10-23T09:52:49Z

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 · 2024-10-23T12:10:55Z

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 · 2024-10-24T14:48:36Z

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

kimpepper · 2024-10-25T04:18:27Z

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 · 2024-10-25T12:42:48Z

@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 · 2024-10-25T16:21:58Z

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

kimpepper · 2024-11-01T05:35:55Z

@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 · 2024-11-01T06:08:36Z

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 · 2024-11-01T15:26:18Z

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 · 2024-11-01T15:29:17Z

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 :)

Signed-off-by: Kim Pepper <[email protected]>

kimpepper · 2024-11-02T02:43:19Z

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

kimpepper · 2024-11-02T04:15:58Z

Split out #237 for the endpoint factory changes here.

dblock · 2024-11-04T14:48:13Z

Thanks @kimpepper! Let's work through those.

stof · 2024-11-06T10:51:43Z

src/OpenSearch/EndpointFactory.php

+     */
+    private function createEndpoint(string $class): EndpointInterface
+    {
+        $fullPath = '\\OpenSearch\\Endpoints\\' . $class;


This looks wrong to me if $class is already the class-string

stof · 2024-11-06T11:34:00Z

src/OpenSearch/Client.php

-     * @var MlNamespace
-     */
-    protected $ml;
+    protected MonitoringNamespace $monitoring;


Turning protected properties into typed properties is technically a BC break for child classes if they redefine the properties.

Btw, if this is going to be released as a major version of the package, maybe private visibility should be used instead (removing the properties themselves from the API covered by semver as child classes won't have access to them anymore). After all, the child classes can read them using the public getter.

stof · 2024-11-06T11:38:15Z

src/OpenSearch/Namespaces/AbstractNamespace.php

+     *
+     * @throws \Psr\Http\Client\ClientExceptionInterface|\Exception
+     */
+    protected function performRequest(EndpointInterface $endpoint): Promise|ResponseInterface


This return type check actually changes the return type of all public methods on namespaces, which have not been updated

stof · 2024-11-06T11:39:14Z

src/OpenSearch/Client.php

@@ -1714,16 +1613,15 @@ public function createPointInTime(array $params = [])
         * $params['body']                   = (array) The document (Required)
         *
         * @param array $params Associative array of parameters
-         * @return array
+         * @return \Http\Promise\Promise|\Psr\Http\Message\ResponseInterface


returning a ResponseInterface instead of parsing its body makes it impossible to document the structure of the response (defeating half of the goals of #217).
It also forces the caller to know how each endpoint is encoding its response (is it a JSON response or a plain text response ?), while the SDK could know that (this should be part of the API description)

stof · 2024-11-06T11:42:10Z

src/OpenSearch/Client.php


-        return $this->transport->resultOrFuture($promise, $options);
+        if ($this->isAsync()) {


instead of an instance-level flag (which separates the configuration from the usage, forcing all caller code to actually support both return types as they don't know how the client was configured), maybe this should be a argument of that method, allowing to use a conditional return type to be more precise (or even separate methods)

stof · 2024-11-06T11:44:01Z

src/OpenSearch/Client.php

        );
-
-        return $this->transport->resultOrFuture($promise, $endpoint->getOptions());
+        if ($this->isAsync()) {


this isAsync flag changing the return type makes it impossible to properly define the return type of methods of the SDK, as they will change return type depending on the configuration of the client, which would be a pain for DX.

Also, as discussed in #219 (comment), it looks like the existing version of opensearch-php does not actually support using the async mode for those methods (unless I missed a way to enable it), so maybe this switch should be removed entirely here, to allow keeping a well-typed SDK.

kimpepper · 2024-11-06T21:51:50Z

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

kimpepper requested review from VachaShah, dblock, harshavamsi, shyim and saimedhi as code owners October 22, 2024 04:32

kimpepper mentioned this pull request Oct 22, 2024

[PROPOSAL] Request feedback on replacing ezimuel/ringphp #199

Open

dblock requested changes Oct 22, 2024

View reviewed changes

kimpepper force-pushed the psr-interfaces branch from 82721bd to 2f4661d Compare October 22, 2024 22:56

kimpepper force-pushed the psr-interfaces branch from 16b664e to f14e9a2 Compare October 23, 2024 04:43

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

Switch to PSR interfaces

ec11701

Signed-off-by: Kim Pepper <[email protected]>

kimpepper force-pushed the psr-interfaces branch from df01728 to ec11701 Compare November 2, 2024 02:23

kimpepper mentioned this pull request Nov 2, 2024

Return static from fluid setters #236

Merged

kimpepper mentioned this pull request Nov 2, 2024

Replace callable with Endpoint factory #237

Open

stof reviewed Nov 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Psr interfaces #233

Psr interfaces #233

kimpepper commented Oct 22, 2024 •

edited

Loading

dblock left a comment

dblock commented Oct 22, 2024

kimpepper commented Oct 23, 2024

shyim commented Oct 23, 2024

kimpepper commented Oct 23, 2024

dblock commented Oct 23, 2024

dblock commented Oct 24, 2024

kimpepper commented Oct 25, 2024

dblock commented Oct 25, 2024

dblock commented Oct 25, 2024

kimpepper commented Nov 1, 2024 •

edited

Loading

kimpepper commented Nov 1, 2024

dblock commented Nov 1, 2024

dblock commented Nov 1, 2024 •

edited

Loading

kimpepper commented Nov 2, 2024

kimpepper commented Nov 2, 2024

dblock commented Nov 4, 2024

stof Nov 6, 2024

stof Nov 6, 2024

stof Nov 6, 2024

stof Nov 6, 2024

stof Nov 6, 2024

stof Nov 6, 2024

kimpepper commented Nov 6, 2024


		return $this->transport->resultOrFuture($promise, $options);
		if ($this->isAsync()) {

Psr interfaces #233

Are you sure you want to change the base?

Psr interfaces #233

Conversation

kimpepper commented Oct 22, 2024 • edited Loading

Description

Issues Resolved

dblock left a comment

Choose a reason for hiding this comment

dblock commented Oct 22, 2024

kimpepper commented Oct 23, 2024

shyim commented Oct 23, 2024

kimpepper commented Oct 23, 2024

dblock commented Oct 23, 2024

dblock commented Oct 24, 2024

kimpepper commented Oct 25, 2024

dblock commented Oct 25, 2024

dblock commented Oct 25, 2024

kimpepper commented Nov 1, 2024 • edited Loading

kimpepper commented Nov 1, 2024

dblock commented Nov 1, 2024

dblock commented Nov 1, 2024 • edited Loading

kimpepper commented Nov 2, 2024

kimpepper commented Nov 2, 2024

dblock commented Nov 4, 2024

stof Nov 6, 2024

Choose a reason for hiding this comment

stof Nov 6, 2024

Choose a reason for hiding this comment

stof Nov 6, 2024

Choose a reason for hiding this comment

stof Nov 6, 2024

Choose a reason for hiding this comment

stof Nov 6, 2024

Choose a reason for hiding this comment

stof Nov 6, 2024

Choose a reason for hiding this comment

kimpepper commented Nov 6, 2024

kimpepper commented Oct 22, 2024 •

edited

Loading

kimpepper commented Nov 1, 2024 •

edited

Loading

dblock commented Nov 1, 2024 •

edited

Loading