Closed oojacoboo closed 1 year ago
I don't think it's related. This happens way before persisted queries. What is the body and content-type you're sending?
I'd say we need an integration test for the HTTP implementation as a whole. For that, we could do what Apollo Server did and use JS graphql-http
server audits: https://github.com/graphql/graphql-http/blob/main/src/audits/server.ts
It'd be nice to just use those as those should be a reference anyway, and those would be true integration tests.
What's nice about graphql-http
's serverAudits is that they fully test the compliance with graphql-over-http, so it might find other problems as well.
Seems like all GET
requests through WebonyxGraphqlMiddleware
are broken. It always attempts to parse the body, which isn't present for GET
requests. Again, not related to persisted queries, but graphql-http would have covered that.
An improvement for WebonyxGraphqlMiddlewareTest
would be sufficient too.
I've played around with graphq-http
audits and sure enough one of the 13 failed tests was this:
● Integration tests › MAY accept application/x-www-form-urlencoded formatted GET requests
Response status code is not 200: An error occurred: Syntax error in body: ""
@oprypkhantc Yea, the body is empty for a GET
request, naturally. And, the WebonyxGraphqlMiddleware
processes the request very early. Do you know if webonyx/graphql-php
needs/expects a request body? I'm assuming not if persisted queries are supported. The middleware is basically assuming webonyx needs a json decoded request body as the parsedBody on the request object. It might be that all of that logic is unnecessary and can be removed?
Probably unnecessary. I'm pretty sure webonyx
handles parsing the RequestInterface
all on their own and they definitely don't require request bodies for GET requests.
It'd be nice to offload the whole HTTP handling thing to webonyx/graphql-php
, since most of the parsing is already being done on their side. They don't, however, convert ExecutionResult
s into responses. They have an open issue for that, so maybe it's worth it to just contribute to webonyx/graphql-php
instead of writing tests/fixing things here.
If we could offload this to webonyx
then we'd not have to worry about fixing this, or testing it, or writing integration tests for the HTTP layer. For now, just adding a few tests to WebonyxGraphqlMiddlewareTest
should be plenty though, to at least cover the most basic use cases such as this.
So, I was able to run our integration tests for graphql with the request body parsing commented out and all tests are passing. That parsing and body validation check seems entirely unnecessary.
I am, however, getting the following error now:
GraphQL Request must include at least one of those two parameters: "query" or "queryId"
When making a GET
request with a query
key on queryParams
property. That's a webonyx error.
I agree with primarily offloading the request handling to webonyx. I do think it's a good idea that we maintain some minimal integration test coverage for requests though. I don't think we need anything too extensive, but having some coverage there is wise.
When making a
GET
request with aquery
key onqueryParams
property. That's a webonyx error.
The query
parameter must be a top-level query parameter, i.e. /graphql?query=query { __typename }
So, I was able to run our integration tests for graphql with the request body parsing commented out and all tests are passing. That parsing and body validation check seems entirely unnecessary.
It seems like at least 15 of graphql-http tests fail when I comment out that body parsing part, so it seems like it is indeed needed :( Here's one of those:
audit('2C94', 'MUST accept POST requests', async () => {
const res = await fetchFn(await getUrl(opts.url), {
method: 'POST',
headers: { 'content-type': 'application/json' },
body: JSON.stringify({ query: '{ __typename }' }),
});
ressert(res).status.toBe(200);
}),
Response status code is not 200. Response: [400] {"errors":[{"message":"GraphQL Request must include at least one of those two parameters: \"query\" or \"queryId\""}]}
Changing the check to if (empty($request->getParsedBody()) && $request->getMethod() !== 'GET') {
helps.
It seems like at least 15 of graphql-http tests fail when I comment out that body parsing part, so it seems like it is indeed needed :(
Yea, so our boot stack is probably setting the parsedBody already, as we have to parse it for some authorization concerns. I don't know why that should be necessary though for webonyx.
The query parameter must be a top-level query parameter, i.e. /graphql?query=query { __typename }
Not sure what you mean by "top-level" here. You mean appended to the actual request URL, as opposed to being a request param on the request object? And why?
They don't do any body parsing for requests that implement ServerRequestInterface
, and all of the requests that go through WebonyxGraphqlMiddleware
are ServerRequestInterface
, so webonyx
literally never parses request bodies.
The implementation I used (laminas/laminas-httphandlerrunner
+ laminas/laminas-stratigility
) doesn't parse the body either, so I believe that's why the fix is there.
Not sure what you mean by "top-level" here. You mean appended to the actual request URL, as opposed to being a request param on the request object? And why?
You mentioned it being under the queryParams
property, so I assumed your query params look like this: queryParams[query]=query { __typename }
instead of query=query { __typename }
. The latter works for me.
You mentioned it being under the queryParams property, so I assumed your query params look like this: queryParams[query]=query { typename } instead of query=query { typename }. The latter works for me.
Here is an excerpt of the dumped request object:
0 => Laminas\Diactoros\ServerRequest^ {#8591
-attributes: []
-uploadedFiles: []
-serverParams: []
-cookieParams: []
-queryParams: array:4 [
"query" => """
query testAutomaticPersistedQueries {\n
company {\n
id\n
legalEntity {\n
companyName\n
}\n
}\n
}
"""
"variables" => []
"operationName" => "testAutomaticPersistedQueries"
"hash" => "cf5def5f484a49ea52906f038c0176e057a5b305b8a7d87638acfa4581c2c362"
]
-parsedBody: null
-method: "GET"
Ignore the superfulous params. They're just there b/c of our integration test implementation - should be harmless.
Got it. I'm not sure why you're not getting a 200
. Here's the test server I use: https://gist.github.com/oprypkhantc/fe173460cce4ccc0e258749b67989479
If you start that with php -S 0.0.0.0:8085
and do GET http://localhost:8085/graphql?query=query{__typename}
you'll get a 200
with this response:
{
"data": {
"__typename": "Query"
}
}
See #427 as possibly related issue.
So, I don't know if this is a Laminas ServerRequestInterface
implementation bug, or there is something else I'm missing here. Basically, Laminas' ServerRequest
object, which implements the PSR interface and what we use and is mostly used by this lib, has the method withQueryParams
, which is just an immutable method, as you'd expect. That's what gives you the output for the queryParams
in the dump above.
However, when digging into webonyx, the query params are built like so...
\parse_str(\html_entity_decode($request->getUri()->getQuery()), $queryParams);
As you can see, it's getting them from the Uri
, which is initialized at construction and includes the query params properly when appended to the uri string. Either I'm misunderstanding the purpose of queryParams
on the ServerRequestInterface
, there is a flaw in the PSR design, such that query params are spread out across multiple objects, making them unreliable, or there is a Laminas implementation issue where Laminas should be updating the Uri
as well.
It's worth pointing out that Laminas also has Uri::withQuery($query)
which takes the query string. ServerRequestInterface::queryParams
seem like more like a parsedBody type thing for query param. But I don't see anywhere they're "parsed".
The issue is resolved by using Uri::withQuery
. But I still find all this behavior and design awkward.
The additional parsing of the request body logic does appear to be needed based on the StandardServer
. It's looking for the pasedBody. The body, does only need to be parsed for POST
requests though. I'll push a commit for that.
@oprypkhantc I was giving the new persisted queries a go to test the implementation, since I wasn't able to do so prior. In doing so, I'm running into the following issue.
This is part of the
process
method for the middleware, which looks for the request body contents. For reference, theprocess
method is as follows:Was this overlooked? We need an integration test for persisted queries for sure. Am I missing something here?