Implement PSR-7 RequestInterface support

[simPod]

ServerRequestInterface is not needed, parent RequestInterface is sufficient
Replaced custom PSR-7 test stubs with nyholm/psr-7 implementation
Fixed reading body contents

• ServerRequestInterface extends RequestInterface so for this case I think it's not a BC break.
• Rephrasing exception message might be considered a BC break I guess.

[vladar]

It looks like now we do some of the work of the ServerRequestInterface (like parsing body and query parameters).

Also as far as I remember this can have a side effect of parsing request body twice (once in the getParsedBody and the second time when we parse it ourselves).

Just out of curiosity, is there any particular reason for this change?

[simPod]

It looks like now we do some of the work of the ServerRequestInterface (like parsing body and query parameters).

This is no needed if PSR-7 is implemented properly. The side effect is probably that StreamInterface::getContents() moves stream pointer to the end so it's always not a proper way to read the stream. It should be casted to string instead.

Coincidentally, I also gave a talk about it:
https://slides.com/simonpodlipsky/php-http#/21
https://youtu.be/J2Psk2KAOr8?t=1290 even though it's in 🇨🇿 only 😕

Also as you can see there were some bugs hidden in variables parsing in tests https://github.com/webonyx/graphql-php/pull/598/files#diff-b4fa16ac4c701a6ab7884483b307e95aR213

Just out of curiosity, is there any particular reason for this change?

I wanted to call gql API in monolithic app (the app also provides gql server) but did not want to issue http request. Instead I compose PSR request and pass it directly to the gql server service.

I could not pass RequestInterface to ServerRequestInterface though as it's contravariant. But there's no need for ServerRequestInterface.

[vladar]

My main concern is parsing request body twice for some apps: first time in getParsedBody (say user does this in his own code) and then we do json_decode in our StandardServer.

I would expect ServerRequestInterface implementation to cache this so that it would be only parsed once if calling getParsedBody multiple times?

[simPod]

IMO as long as it's under user's control it's not a problem. User is not required to parse it and we do it only once.

Also we have Server::executeRequest() so user can eventually optimise through it. On our side we get rid of ServerRequestInterface which is heavier (cookies, files) than RequestInterface so in terms of performance this should actually win even though it's micro optimisation.

[vladar]

Looks like we've got a conflict on this. Mind checking?

[simPod]

Resolved

[mwijngaard]

@vladar Could you please merge this? Or at least indicate why this will not be merged?

As I indicated in #651 :

According to the PSR-7 spec, the behavior of ServerRequestInterface::parsedBody is only really specified for multipart/form-data and application/x-www-form-urlencoded requests (in which case it should contain the contents of $_POST). With other request types, the parsedBody MAY return an unserialized version of the body, but since it's not really practical to build parsers for all known formats into a http library it should be perfectly acceptable to return nothing. In PHP, for other request types the $_POST superglobal is empty and the official GraphQL documentation (https://graphql.org/learn/serving-over-http/) uses POST requests with json contents in body, no forms.

The current implementation is not compatible with PSR-7 libraries that do not automatically deserialize the json body, which is probably most if not all. To prevent this problem in the future, it would also seem wise to me to use a lightweight 3rd party psr-7 library in tests, instead of stubs. So merging this PR seems like a good idea to me.

Thanks for your efforts!

[coveralls]

Coverage decreased (-0.009%) to 86.121% when pulling 10e62cf on simPod:fix-psr-impl into 7a7add0 on webonyx:master.

[mwijngaard]

Come on guys, this is taking too long...

[vladar]

Thanks, @simPod 🎉 🚢

[mwijngaard]

Thank you!

[PowerKiKi]

This PR is breaking ecodev/graphql-upload, because it removed the support for getParsedBody(). I still think it's a mistake to try to parse all and any kind of content-type, as I explained a while ago in #218.

More concretely ecodev/graphql-upload expects to receive multipart/form-data, do some light magic and inject instances of UploadedFileInterface into the GraphQL variables array and forward the request to webonyx/graphql-php for further processing. The end-user will receive an instance of UploadedFileInterface in his resolver.

Because we work with objects, we cannot re-serialize to JSON, and then expect webonyx/graphql-php to re-parse the JSON and get the instances back. IMHO there really is no other way than to support, in priority, whatever may exist in getParsedBody(). If nothing exists, then maybe let webonyx/graphql-php parse it itself. But really why would you duplicate parsing logic that already exists elsewhere (in a more robust way) ?

And then what happen when somebody use application/my-app-v2+json ? or application/totally-custom-content-type-that-only-me-know-how-to-parse ?

Trying to parse the body in webonyx/graphql-php, and especially not supporting external parsing, voids the interoperability that PSR-7 is supposed to bring.

@vladar would you accept a PR to restore the support of getParsedBody() ?

[simPod]

@PowerKiKi I'm not sure I understand the issue you have. Can you provide some example why PSR-7 interface is not possible to use and why there's need for getParsedBody()?

[PowerKiKi]

@mwijngaard, you said:

since it's not really practical to build parsers for all known formats into a http library it should be perfectly acceptable to return nothing

But then by advocating to drop getParsedBody(), you basically say that webonyx/graphql-php must "build parsers for all known formats". I agree with your first argument, but then we cannot expect webonyx/graphql-php to do something that we think is not reasonnable.

@simPod, see the actual code of ecodev/graphql-upload:

https://github.com/Ecodev/graphql-upload/blob/971a680049129299df969fdfbf48dee0a1defad2/src/UploadMiddleware.php#L39-L69

The critical part being:

return $request
    ->withHeader('content-type', 'application/json')
    ->withParsedBody($resultWithUploadedFiles);

Here $resultWithUploadedFiles contains the GraphQL query and the variables sent by client. But now it also contains some variables that are instances of UploadedFileInterface.

But after this PR whatever clever things we did with our uploaded files are totally ignored by webonyx/graphql-php.

The only way for webonyx/graphql-php to see the new variables would be to store them in the raw body, but we cannot serialize UploadedFileInterface instances, so we cannot do this:

return $request
    ->withHeader('content-type', 'application/json')
    ->withBody(json_encode($resultWithUploadedFiles));

For more context, see also the GraphQL multipart request specification (and their example).

[simPod]

withBody() does not accept string but StreamInterface. Can't you use stream ->withBody($uploadedFile->getStream())?

[PowerKiKi]

No, I cannot, because I don't have just one file. I have a structure similar to this:

function someFunction(RequestInterface $request)
{
    $parsedBody = [
        'query' => 'some valid graphql query',
        'variables' => [
            'foo' => 'bar',
            'baz' => 123,
            'file1' => new UploadedFileInterface(), // pseudo-code here
            'file2' => new UploadedFileInterface(),
        ],
    ];

    $serializedBody = json_encode($parsedBody); // WAT !? I break my files here...

    $newRequest = $request->withBody(new \Laminas\Diactoros\CallbackStream(fn () => $serializedBody));
}

Come to think of it, I totally skipped over the fact that ServerRequestInterface was dropped entirely. That does not make any sense to me. Straight from the spec:

ServerRequestInterface specifies a method for retrieving a tree of upload files in a normalized structure, with each leaf an instance of UploadedFileInterface.

Without ServerRequestInterface, we cannot upload files (in a PSR kind-of-way). \GraphQL\Server\StandardServer is pretty much an HTTP server, why on earth would it receive anything different than the interface that was tailored-made for this purpose ?

If you have a special use-case where the request does not come straight from your nginx/apache, then it would be as easy as:

$request = new Laminas\Diactoros\ServerRequest();
$request = $request->withMethod('POST')->withHeader('content-type', 'application/json')->withParsedBody([
    'query' => $argv[1] ?? '',
    'variables' => json_decode($argv[2] ?? '{}', true),
]);

$result = $server->execute($request);

The more I think about it, the more this PR should be reverted entirely 🤔

[PowerKiKi]

@mwijngaard, you also said:

parsedBody MAY return an unserialized version of the body

This is incorrect and the spec very clearly states the opposite (emphasize mine):

Otherwise, this method may return any results of deserializing
the request body content; as parsing returns structured content, the
potential types MUST be arrays or objects only. A null value indicates
the absence of body content.

That means that we can rely on getParsedBody(), and we should rely on it, as first possible input, for maximum interoperability.

I'll refrain from posting for a while now. @vladar could you please take the time necessary (when you see fit) to review this recent discussion and consider whether this PR should be reverted, or improved, or nothing at all ?

[simPod]

I'm not arguing about anything you or I said, just trying to understand your use case. Thanks for the example code, would you be able to create a test case here in order not to break your use case in the future and also so I can work with it and improve the implementation?

[vladar]

Oh yeah, I totally forgot about uploaded files use-case. Which is a very important one. And this was indeed a breaking change. Also, we broke custom content types with it as @PowerKiKi correctly pointed out.

At the moment reverting this seems the right thing to do. But maybe @simPod has a better idea.

To recap, we must support:

1. Custom content-types, i.e. x-my-custom-type
2. Uploaded files

Ideally, we should also support RequestInterface instances but that is contravariant as @simPod correctly mentioned. Maybe we can just additionally check if the request is instanceof ServerRequestInterface and use the old logic based on getParsedBody but if it's not - then keep the current one?

BTW @simPod, what is the primary use-case for RequestInterface implementations? I am trying to understand when you would use it vs ServerRequestInterface?

Also @PowerKiKi I encourage you to open a new issue to surface the problem in case if someone else searches for it.

[simPod]

Maybe we can just additionally check if the request is instanceof ServerRequestInterface and use the old logic based on getParsedBody but if it's not - then keep the current one?

Exactly what I was thinking. Will look into it though might not cover every case as we're missing tests here.

Definitely would not revert as there are several other changes included, rather improve.

My use case is forwarding already constructed Requests into Server. ServerRequest is unnecessarily too narrow.

[mwijngaard]

Ah ok, I didn't realise supporting uploaded query files was also an intended use case, since it's not part of the official documentation (https://graphql.org/learn/serving-over-http/), but sure, I can see how that would be useful.

My point about it not being practical to implement support for parsing all types of formats was about a PSR-7 implementation. By relying only on ServerRequestInterface you basically expect any generic PSR-7 implementation to support all formats of all its consumers (graphql-php is then only one of its consumers). This is not realistic. It might sort of work for application/json body, but application/graphql is already more exotic. The only way then to work with graphql-php would be to implement a custom implementation of PSR7 (which, I gather, is what you're doing @PowerKiKi ?), or use multipart/form-data requests (since those are specified).

Graphql-php as a consumer of PSR7 requests, supports only a specific set of formats, so it seems perfectly acceptable to build parsing for those in the StandardServer component.

I agree with bringing back support for ServerRequestInterface for the uploaded files or pre-parsed body case, but replacing the new support for plain RequestInterface by reverting is definitely wrong.

[spawnia]

It might sort of work for application/json body, but application/graphql is already more exotic.

Random info from the bleeding edge of the GraphQL-over-HTTP working group, we plan to propose the content type application/graphql+json as a new standard: graphql/graphql-over-http#83

[PowerKiKi]

@mwijngaard,

By relying only on ServerRequestInterface you basically expect any generic PSR-7 implementation to support all formats of all its consumers

The only way then to work with graphql-php would be to implement a custom implementation of PSR7

There is no need to have custom PSR7 implementations. Body parsing can live outside of PSR7. This is exactly what is done in BodyParamsMiddleware. Having your request pass through multiple middlewares, and potentially be mutated, fits very well our use-case. There is a lot of flexibility in middlewares configuration to do exactly what you want (and nothing more), with the lib of your choice. So you can support specific content-type for specific routes.

replacing the new support for plain RequestInterface by reverting is definitely wrong

Could you explain, preferably in code, what would be impossible if the typing was reverted to the semantically correctServerRequestInterface ?

Specifying that type does not necessarily implies that we must only use getParsedBody(). I think your use-case of doing some body parsing is still possible even with a ServerRequestInterface. As a matter of fact your PR #651 did not change the type. So what make you think that the type must not be reverted if you did not change it yourself ?

@spawnia,

If/when application/graphl+json gets standardized, it could be implemented in our parsePsrRequest(), even if it accepts ServerRequestInterface. In fact, this new content-type would be the best argument to implement some parsing ourselves, as it is a very specific format that might not be found in other, more generalist, libraries.

[mwijngaard]

@PowerKiKi sorry to take so long to reply, but a couple things I would still like to share:

1. I think you attach too much significance to ServerRequestInterface. It basically just exists for performance reasons and to work with/around some quirks of the php runtime (file uploads and pre-parsed request params). Without these quirks, RequestInterface would be perfectly suitably to model an http request on the server. If you're not running php in its default single request mode (eg when using reactphp), then the ServerRequestInterface doesn't add anything, since php does not do any of the parsing for you. Body parsing is not and cannot be part of the http spec, since that would make it very hard to extend. My original pr didn't change the interface because i was just looking for a minimal change.
2. regarding the body parsing in graphql php, I guess it all depends on where you want to put your responsibilities. Do you consider the psr7 part of graphql php to be an implementation for handling http requests, or for handling pre-parsed http requests. The first felt more natural to me, especially considering the fact that the official documentation mentions recommends content types. But considering the way php is mostly used, both, I guess, are valid, so the new mechanism in Read getParsedBody() instead of getBody() when Request is ServerRequest #715 seems like the best solution. Thanks for you contribution to that :-)

[PowerKiKi]

I think we agree on body parsing. Both approaches can be considered valid.

I still disagree on ServerRequestInterface though. You say:

Without these quirks, RequestInterface would be perfectly suitably

But PHP does have those quirks, and thus we need ServerRequestInterface to properly model an incoming, server-side HTTP request. You cannot wish for PHP runtime to be different and ignore the reality. I am pretty sure the PSR guys did not create that interface just for fun.

If you're not running php in its default single request mode

I am convinced that the vast majority of real-world usage in production still use single request mode. And even though you do run something like reactphp, then ServerRequestInterface wouldn't hurt anything. Why not use something that is specifically tailored made for your use-case, that is semantically extremely clear, as opposed to something more generic with "hidden" support for more specific things via brittle instanceof assertion ?

[mwijngaard]

It's not so much that I wish for php to be different (although for certain things that is certainly the case :-)), but it's good to be aware of why things are the way they are, and to realize that the psr7 concept of the 'server http request' is special case of all incoming http requests on the server, and not the other way around. ServerRequestInterface is an optimization and should be treated as such, which is why the instanceof is perfect for this case.

[PowerKiKi]

If it is only an optimization, how do you explain that PSR-15 for middleware is typed with ServerRequestInterface, and not RequestInterface ?

I mean the PSR-7 itself could not be more clear:

ServerRequestInterface is a

Representation of an incoming, server-side HTTP request.

And RequestInterface is a

Representation of an outgoing, client-side request.

So I guess you could argue that graphql-php can accept an outgoing request, but IMHO that really is incorrect.

Or you could argue that the PSR does not have good interfaces, and you might be right (I really don't know). But assuming we accepted to work with PSR, I feel we should stick to what they define, and not re-interpret them. That would ruin the entire idea behind PSR.

the 'server http request' is special case of all incoming http requests on the server

If you watch only the class inheritance, then yes, it is a special case. But if you read the added semantics clearly explained in the comments, then no. An incoming request cannot be a special case of an outgoing request. They are semantically the exact opposite.

[mwijngaard]

Fair enough. I've had a reread of both the psr specs and I would agree with you that ServerRequestInterface is intended for this case. In fact, reactphp is even mentioned specifically in psr7. The question then remains how we should support parsing of the body when it has not yet been parsed, since the spec does not really offer an api for that as far as I can tell. The getParsedBody method does provide a method of indicating that the body has not yet been parsed. Do you have any idea how that should work?

[PowerKiKi]

How to to parse the body when it has not yet been parsed yet is a bit opinion based.

If I had to choose, I would only support parsing of application/graphl+json if/when it is standardized.

And everything else, including the very common application/json (!), must be parsed before calling graphql-php.

This way concerns are clearly separated. You can pick whichever parsing library fits your use-case/preferences, and graphql-php is not overwhelmed by all kind parsing issues/quirks that might arise in the future.

[mwijngaard]

That's not really what I meant. How would you indicate to graphql php that the body has not yet been parsed, and should therefore be parsed by the library,regardless of format.

There are clear guidelines to what content types a graphql server implementation should support in the official docs. Just treating application/graphql-json as a special case seems arbitrary, so parsing all the other recommended content types should be supported as well.

[PowerKiKi]

Sorry for the misunderstanding.

There are (widely used and reasonable) de-facto standards, but there isn't any official specification regarding supported content-type (yet), as explained here.

You can indicate to graphql php that the body has not been parsed by returning null in getParsedBody(). As the PSR7 spec says:

 * Otherwise, this method may return any results of deserializing
 * the request body content; as parsing returns structured content, the
 * potential types MUST be arrays or objects only. A null value indicates
 * the absence of body content.

So our implementation could become:

function parsePsrRequest(ServerRequestInterface $request)
{
    $bodyParams = $request->getParsedBody();

    if ($bodyParams === null && $request->getHeaderLine('content-type') === 'application/graphql+json') {
        $bodyParams = parseBodyAccordingToFutureSpecification($request->getBody());

        if ($bodyParams === null) {
            throw new InvariantViolation('Expected to receive a JSON array in body for "application/graphql+json" PSR-7 request');
        }

        if (!is_array($bodyParams)) {
            throw new RequestError(
                'GraphQL Server expects JSON object or array, but got ' .
                Utils::printSafeJson($bodyParams)
            );
        }
    } elseif ($bodyParams === null) {
        throw new InvariantViolation('Expected to receive a parsed body for PSR-7 request but got null');
    }

    parse_str(html_entity_decode($request->getUri()->getQuery()), $queryParams);

    return $this->parseRequestParams(
        $request->getMethod(),
        $bodyParams,
        $queryParams
    );
}

It is shorter and IMHO easier to follow than the current version. And yes we do lose out-of-the-box support for application/json, application/graphql and that obscure case of parse_str((string) $request->getBody(), $bodyParams);. But that's my whole point: support the official specification when it is out, and everything else is up to end-user (via pre-parsed body).

[mwijngaard]

I guess you could interpret the spec that way, but that is not literally what is says. 'the absence of body content' is not the same as 'the absence of parsed body content'. Nevertheless it seems useful for our case to do it this way. I would still argue that we support existing common practices from the 'serving over http' doc, so parsing application/json, application/graphql and application/graphql+json should be supported. I'll see if I can find the time to make a pr that doesn't break bc next week.

[PowerKiKi]

Yes, there is room for interpretation. But hopefully that's reasonable enough for our use-case.