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

[HttpClient][Contracts] add HttpClient\ApiClientInterface et al. #30414

Closed
wants to merge 3 commits into
base: master
from

Conversation

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 1, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

This PR adds a new ApiClientInterface on top of #30413.
It's goal it to provide great DX when using (json) APIs.

It also provides an implementation of the interface in the HttpClient component:

image

@nicolas-grekas nicolas-grekas added this to the next milestone Mar 1, 2019

@nicolas-grekas nicolas-grekas changed the title Contracts http json [HttpClient][Contracts] add HttpClient\ApiClientInterface et al. Mar 1, 2019

* @throws TransportExceptionInterface When an unsupported option is passed or
* if any error happens at the transport layer
*/
public function request(string $method, string $url, iterable $options = []): ResponseInterface;

This comment has been minimized.

@dbu

dbu Mar 1, 2019

Contributor

in my opinion, the interface is a very leaky abstraction. rather than having all these things configured on a per-request basis, they should be configured on the client instance. having the options here tells people to leak all kind of details into their code interacting with the http client, instead of having that managed via DI on the http client itself.

if, say, there is one host where we do not want ssl verification because of an outdated certificate, i'd rather have a meta client that choses the correctly configured client based on a request detail like the hostname than littering this decision over all my code that interacts with the http client.

this is the reasoning we took for the PSR-18 request method, and if we would do the same here it would be possible to write an adapter from the symfony http client to use any psr-18 client. (the adapter in this PR is for using the symfony client as PSR-18 client, which is the other direction)

This comment has been minimized.

@stof

stof Mar 1, 2019

Member

well, headers and body also appear as options in that signature for instance

This comment has been minimized.

@dbu

dbu Mar 1, 2019

Contributor

yes, i am not arguing against headers and body, but against all the nitty-gritty http protocol options.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 4, 2019

Author Member

hi @dbu, I have a different opinion here: all concrete HTTP clients have to deal with these settings. The defined options do abstract these. In PSR-18, you decided to not expose anything about the transport layer. This effectively forbids from controlling these settings in any interoperable ways. Your SSL example is a really good one: there is no way with PSR-18 itself to implement the logic you describe. I made a different choice and decided to expose the layer via the abstraction provided by the options. This will allow implementing interoperable decorators that do e.g. conditional SSL routing, and much more. IMHO, users need power tools, we should not restrict what they can do. If they use an HTTP client, it's legit that they can decide to control every aspect of it. Trying to save them from themselves could lead to the opposite effect: they will hack around the limitations and bind themselves to one implementation. That's exactly what happened to the Blackfire Player: its implementation is now bound to Guzzle because it needs DSN pre-resolution for security reasons; escaping the interop promise.

This comment has been minimized.

@dbu

dbu Mar 5, 2019

Contributor

i agree that being overly limiting is a problem. however, this is not good flexibility but prompting bad patterns. having an $options parameter means that the configuration of how you want your client to work is carried throughout your application or hardcoded in all places that send requests.
using correctly configured clients is a much better way to be sure all requests the application does are done with the correct settings. my example with ssl was to illustrate that even if you have a complicated situation, you can still inject one single client to your code and handle things in the client. the common use case however, will be that you send requests to one single server and want the same settings for all requests.

if i understand correctly, the list of options is not closed to what is mentioned in the interface, but could also be additional things. clients will not implement all of them, and some of the options are likely not well defined. if everything in the end uses lib_curl, you will have that as a de-facto standard, but clients that do not build on curl will likely not respect that.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 5, 2019

Author Member

both ways are and should be supported: default options allow injecting preconfigured clients - of course, that's out of scope as no contracts can cover DI configuration.
Enabling decorators to precisely control HTTP-transport settings in an interoperable way is a goal of the proposed contracts. The list of options is in the contracts precisely because I want to make it a requirement to support them all - or throw. All options are supported by both native and curl implementations. There is zero binding to curl here. Note that not supporting an option is also possible, but then a client MUST throw. That applies to custom options of course so that if you rely on one of them, you'll bind yourself to one specific implementation, but at least you'll know when swapping the implementation breaks your need (very alike using methods that are only on implementations and not on some interfaces.)

This comment has been minimized.

@dbu

dbu Mar 5, 2019

Contributor

The list of options is in the contracts precisely because I want to make it a requirement to support them all - or throw.

i either missed that part or its not yet explicitly written out. when going with $options, i think its a very good point, because it makes things a lot more reliable.
do i get it correctly that the list is open, so additional options might be supported, but clients MUST throw an exception if they don't know any option that they have been passed?

i am still not convinced that this is the right approach. but with that clarification, the whole thing looks a lot better to me 👍

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 5, 2019

Author Member

My communication style is never explicit enough, thanks for helping me brain-dump :) I'm sure there is room for clarifications by PRs after merging. You're right about added and unsupported options, that's what I meant by the description next to @throws.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:contracts-http-json branch 2 times, most recently from 66d6dad to 8463b51 Mar 4, 2019

@nicolas-grekas nicolas-grekas requested a review from sroze as a code owner Mar 4, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:contracts-http-json branch 3 times, most recently from b49be7c to 930e338 Mar 4, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:contracts-http-json branch 2 times, most recently from 3031ad2 to d94085f Mar 5, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:contracts-http-json branch 3 times, most recently from d88aaaa to 03450a2 Mar 6, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 7, 2019

PR rebased, ready for reviews, votes, merge :)

$responses = new \SplObjectStorage();
foreach ($apiResponses as $r) {
$responses[$r->response] = $r;

This comment has been minimized.

@ostrolucky

ostrolucky Mar 7, 2019

Contributor

This whole method assumes all instances are ApiResponse, not ApiResponseInterface, why? Consumer of this helper expects instances of ApiResponseInterface.

Please also document expected type of this parameter in docblock. I know it's internal, but that's not proper justification for not documenting it in code, lot of people have to read internals of Symfony to debug issues.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 7, 2019

Author Member

Sure, documented, and now check with proper exception. Only ApiResponse are accepted because that's the design of the component: the responses are not standalone object, they're bound by "friendship" with the client that emitted them. That's how clients know how to stream of complete them.

This comment has been minimized.

@ostrolucky

ostrolucky Mar 7, 2019

Contributor

That seems like break of contract of ApiClientInterface. ApiClientInterface::complete says you can use ApiResponseInterface, but ApiClient::complete rejects everything that is not ApiResponse.

This comment has been minimized.

@HeahDude

HeahDude Mar 7, 2019

Member

The contract is on ApiClientInterface::complete not ApiResponseInterface::complete

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 7, 2019

Author Member

It's very alike PSR 6 cache items: they are not interoperable, like responses are not interoperable from one client to another.

Here is how PSR 6 tells it:

Calling Libraries SHOULD NOT assume that an Item created by one Implementing Library is compatible with a Pool from another Implementing Library.

Same for responses.

This comment has been minimized.

@ostrolucky

ostrolucky Mar 7, 2019

Contributor

I wonder why not design fitting contract? We are in design phase. Contract should declare everything implementations need. If even main implementation works around the contract, that's not a good signal.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 7, 2019

Author Member

But a contract is not only about type hints. If the contracts says that responses cannot be passed from one client implementation to another, that's the contracts, and a perfectly valid one. Exactly as PSR 6.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 8, 2019

Author Member

Rereading, I may not have answered your question @ostrolucky.

Contract should declare everything implementations need

That's actually what happens here. Tell me if I'm wrong, but I understand from your comment that you think we should look for completely standalone response classes: we should design each interface as a standalone contract (eg responses) that could be reused independently with any other interface in the domain (eg clients). What I'm trying to explain above is that a class is not the only possible unit to define the boundaries of reusable code. Here, like in PSR6, the client is the only "legal" factory for response objects. For this reason, we do not need to define any contracts to make them talk to each other: that communication is purely internal details.

Another way to look at the problem is that one: do we need to work on a set of interfaces that would allow any response objects to work with any clients? Let's be more concrete: do we need to define interfaces that would allow a response created by a client based on curl to be streamed by a client based on fopen?

Maybe it's possible, by adding more abstractions in between. But which problem would this solve in practice? Nobody will ever need to do that.

So, in conclusion, the client+response form a tuple and are bound together by some internal details that the outside world should not care about. That's actually the design decision that makes the contracts work so seamlessly and so consistently.

This comment has been minimized.

@ostrolucky

ostrolucky Mar 8, 2019

Contributor

contracts says that responses cannot be passed from one client implementation to another

  1. Can you point me to a part of the contract in http-client which says this? I don't see it.
  2. Even if contract says that, how does this non-interoperability benefit user?

That's actually the design decision that makes the contracts work so seamlessly and so consistently.

My impression is it makes whole thing work less seamlessly and less consistently. Even you had to workaround your own contract with SplObjectStorage, because http client implementation rejects any request it didn't itself create. Anybody creating their own implementation will have to jump through same hoops. And it has code smells like methods in response accepting other responses and task scheduling bult in, instead of in client or separate class. This is less likely to happen with contracts defining boundaries properly, like having ResponseInterface::isFinished

Exactly as PSR 6.

PSR-6 doesn't forbid anybody to make pools work with items from other pools. It's unfortunate that symfony/cache chose this option. symfony/http-client goes even more overboard and doesn't even reuse same Response (Item) class across Clients (ItemPools). Imagine how much more work you would have if every cache adapter came with its own item class. This is happening here, just nobody realizes because there isn't many implementations (yet).

that communication is purely internal details

This is leaky internal detail and will surface when trying to implement own Response class. And contract doesn't warn even in text form. Not even clients do that. Every client pretends it accepts interface responses, while deeper layer that it uses rejects everything else than concrete, final class.

do we need to define interfaces that would allow a response created by a client based on curl to be streamed by a client based on fopen?

Http-client goes way more far than that. It's not that consumer is not able to use curl response in fopen client, but they are not able to use ANY other response. They cannot even use wrapped curl response in curl client, or curl response subclass. If consumer wants to create new Response, they must create new client as well. That includes cases when consumer would wish to add just one utility method. Combined with the fact everything has to be decorator because nothing is inheritable, this looks too verbose. And when user begins to have a need to have own response classes, they can no longer safely use interface, because anything else than their own implementation might blow up when passed to ApiClientInterface::complete or HttpClientInterface::stream, better to typehint to their own client instead across whole application to avoid can of worms. Even then, user must be careful each time passing these to external libraries. Maybe I am overreacting because this applies only to those mentioned bulk methods (and their use will be rare), but this will be PITA when somebody will need to use those in this context.

But which problem would this solve in practice? Nobody will ever need to do that.

I might wish to create: XmlResponse, CsvResponse, StripeResponse, StringeableResponse, MockResponse, UserCreateResponse, TransactionAwareResponse, TraceableResponse etc. I might wish to use unique response class for every API endpoint. Use case this solves is preserve ability to safely pass these to ApiClientInterface::complete or HttpClientInterface::stream without unwinding & rewinding these response wrappers (which cannot even happen without instanceof checks, because they can be passed to different layers)

This comment has been minimized.

@nicolas-grekas
Show resolved Hide resolved src/Symfony/Component/HttpClient/Response/ApiResponse.php

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:contracts-http-json branch 2 times, most recently from d5bf2a0 to 359dc36 Mar 7, 2019

@dbu
Copy link
Contributor

dbu left a comment

so essentially, these contracts are for automatically converting the response body to array, right?

is that worth all the interfaces? what if we instead had a general http client interface with methods and tell people to use json_decode on the response body?

(this is meant as an actual question - i am fine with an answer that says "yes, there is a significant benefit" - in that case i'd love some additional phpdoc to explain what the benefit is, why people should use this)

$this->client = $client;
if ($defaultOptions) {
[, $this->defaultOptions] = self::prepareRequest(null, null, $defaultOptions, $this->defaultOptions);

This comment has been minimized.

@dbu

dbu Mar 8, 2019

Contributor

would it be possible to refactor the HttpClientTrait to have separate methods for the url and the options? i see that one section of the trait prepareRequest uses null === $url as a flag to do auth or not. i think this could be refactored too, to be more explicit and to have an options resolution that is not preparing the request.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 8, 2019

Author Member

That's not completely possible as there is some coupling between options and url - the most obvious example is the base_uri options. I'd be happy to review a PR if you want to work on one.

* Provides flexible methods for interacting with HTTP APIs.
*
* Unless otherwise specified, implementations MUST send a JSON-compatible
* "accept" header - typically "application/json".

This comment has been minimized.

@dbu

dbu Mar 8, 2019

Contributor

Unless specified otherwise in the 'headers' $option, implementations MUST ...

This comment has been minimized.

@dbu

dbu Mar 8, 2019

Contributor

actually, why? if the client must be for json, why is there an interface? are there more meaningful implementations than json_decode?

what if i want to implement this for an xml api? (not soap, there array would make little sense as a response type, but some generic xml thing)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 8, 2019

Author Member

Except toArray(), which doesn't fit other content types, the interface fits xml yes.
The idea here is to tell ppl that if they don't explicitly an Accept header, then they can expect application/json to be sent for them. Then, if one client is configured to send xml, one should ensure that such a client would be injected only where that default is expected.
Would that make sense? How would you formulate that?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 9, 2019

Author Member

Reformulated. Better?

This comment has been minimized.

@dbu

dbu Mar 11, 2019

Contributor

yes!

*
* @throws TransportExceptionInterface When an unsupported option is passed
*/
public function get(string $url, iterable $options = []): ApiResponseInterface;

This comment has been minimized.

@dbu

dbu Mar 8, 2019

Contributor

these http method named methods would make sense with any client, not only for an API client. why is this tied into api client and not a decorator for the plain client?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 8, 2019

Author Member

What I mean by "api" is that the response bodies are small enough to be buffered in memory. E.g. a .iso of a video file wouldn't fit this definition - one should use HttpClientInterface then.
A decorator wouldn't provide the required abstraction level: I'm looking for something ppl can type-hint for without binding them to one implementation.

*
* @author Nicolas Grekas <p@tchwork.com>
*/
interface ApiClientInterface

This comment has been minimized.

@dbu

dbu Mar 8, 2019

Contributor

imho it would be useful to also have the generic request method on this interface, for APIs that accept special http methods.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 8, 2019

Author Member

We already have such a thing: it's HttpClientInterface

This comment has been minimized.

@dbu

dbu Mar 11, 2019

Contributor

but you do not extend the HttpClientInterface here. there is a very small cost to also proxy that base method (assuming that the api client will implement such a method anyways)

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 8, 2019

so essentially, these contracts are for automatically converting the response body to array, right?

I'm actually wondering if we shouldn't move toArray() to the base ResponseInterface... so, not really :) see #30499

These contracts are for talking with endpoints that serve reasonably small responses - json/xml/text payloads.
Provided responses are reasonably small, the complete() method becomes a useful tool: no need to deal with chunks anymore, you'll only need to care about completed and buffered responses.

The goal is also to provide those get/post/put/etc methods that cover 90% of API endpoint needs.
So: this aims at providing a useful abstraction to access APIs with reduced boilerplate.

If the contracts are too restrictive for your API, no issue: use HttpClientInterface instead.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:contracts-http-json branch from 359dc36 to 545c141 Mar 8, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:contracts-http-json branch 3 times, most recently from d9ac67e to 80dcf41 Mar 9, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 9, 2019

PR rebased on top of #30499 - waiting for it to be merged.
ApiResponseInterface is gone now.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:contracts-http-json branch 3 times, most recently from 096b42d to a4fd5d9 Mar 9, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 10, 2019

Rebased, ready.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:contracts-http-json branch from a4fd5d9 to d164226 Mar 10, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 10, 2019

Thinking twice here, I'm fine if we don't merge. With #30499 in, talking with JSON endpoint might already be easy enough.

The benefit of this new interface would be that:

  • it provides helper methods for standard HTTP verbs;
  • it provides the complete() method, that prevents dealing with chunks directly, by yielding only completed responses.

The downside is that it requires ppl to make a choice between this interface and HttpClientInterface - and we know making a choice is hard.

If nobody comments to rescue this PR, I'm going to close it in a few days :)

@dbu

This comment has been minimized.

Copy link
Contributor

dbu commented Mar 11, 2019

it provides helper methods for standard HTTP verbs;

you could provide a HttpMethodsClient (or whatever you want to call it) that decorates a HttpClientInterface with methods for the HTTP verbs.

and apart from that, i share your concern that having 2 clients can be confusing for people. i am not sure of the added value. maybe adding something to the response to make it convenient for non-streaming situations would solve the issue with less additional code and interfaces?

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Mar 11, 2019

I also think that this PR does not bring enough value and makes things more complex for our users as they need to choose which abstraction to use. Let's close.

@fabpot fabpot closed this Mar 11, 2019

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:contracts-http-json branch Mar 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.