graphqlite icon indicating copy to clipboard operation
graphqlite copied to clipboard

GraphQLMiddleware - error when processing queries with an empty request body

Open ironexdev opened this issue 3 years ago • 5 comments

When I send query {{domain}}/graphql?query={whatever} using GET http method to GraphQLite server I get the following error

image

InvalidArgumentException: Syntax error in body: "" in file /home/dockeruser/app/vendor/thecodingmachine/graphqlite/src/Http/WebonyxGraphqlMiddleware.php on line 87
Stack trace:
  1. InvalidArgumentException->() /home/dockeruser/app/vendor/thecodingmachine/graphqlite/src/Http/WebonyxGraphqlMiddleware.php:87
  2. TheCodingMachine\GraphQLite\Http\WebonyxGraphqlMiddleware->process() /home/dockeruser/app/src/Core/MiddlewareStack.php:54
  3. App\Core\MiddlewareStack->handle() /home/dockeruser/app/vendor/tuupola/cors-middleware/src/CorsMiddleware.php:124
  4. Tuupola\Middleware\CorsMiddleware->process() /home/dockeruser/app/src/Core/MiddlewareStack.php:54
  5. App\Core\MiddlewareStack->handle() /home/dockeruser/app/src/Core/Kernel.php:69
  6. App\Core\Kernel->processRequest() /home/dockeruser/app/src/Core/Kernel.php:42
  7. App\Core\Kernel->__construct() /home/dockeruser/app/vendor/php-di/php-di/src/Definition/Resolver/ObjectCreator.php:143
  8. DI\Definition\Resolver\ObjectCreator->createInstance() /home/dockeruser/app/vendor/php-di/php-di/src/Definition/Resolver/ObjectCreator.php:71
  9. DI\Definition\Resolver\ObjectCreator->resolve() /home/dockeruser/app/vendor/php-di/php-di/src/Definition/Resolver/ResolverDispatcher.php:71
 10. DI\Definition\Resolver\ResolverDispatcher->resolve() /home/dockeruser/app/vendor/php-di/php-di/src/Container.php:390
 11. DI\Container->resolveDefinition() /home/dockeruser/app/vendor/php-di/php-di/src/Container.php:199
 12. DI\Container->make() /home/dockeruser/app/src/bootstrap.php:42
 13. require() /home/dockeruser/app/public/index.php:3

However, when I add a body to the request, it works

image

It seems to work as it should after I added condition $request->getBody()->getContents() to check if request body is not empty before it is decoded from json

https://github.com/thecodingmachine/graphqlite/blob/8a081f63112768a82eb9a6533ebacdd27090bb36/src/Http/WebonyxGraphqlMiddleware.php#L81

image

ironexdev avatar Dec 29 '21 20:12 ironexdev

I'm not exactly certain what issue you're running into here, but there are a couple of things that should be pointed out:

  1. GraphQL requests should be over HTTP POST and not GET.
  2. When getting the contents from the body, the body is cleared out from the request object IIRC. I find this behavior strange, personally. However, I'm sure there is a valid reason for that design decision. Regardless, that's something to be aware of. I'd assume from your example that there isn't any body contents being parsed and therefore no resulting error.

Please try dumping the full contents of the body and inspect what's being passed before the exception is being thrown.

oojacoboo avatar Dec 29 '21 20:12 oojacoboo

@oojacoboo

  1. According to https://graphql.org/learn/serving-over-http/#get-request it is possible to send GraphQL requests using GET method.

  2. Problem is that when I send request using GET method with an empty body, InvalidArgumentException (line 86) is thrown. Variable $content contains an empty string, variable $data contains null and json_last_error() returns 4, which translates to syntax error.

https://github.com/thecodingmachine/graphqlite/blob/8a081f63112768a82eb9a6533ebacdd27090bb36/src/Http/WebonyxGraphqlMiddleware.php#L81-L90

It is same as if you did this

json_decode("");
var_dump(json_last_error()); // returns 4
var_dump(json_last_error_msg()); // returns Syntax error

ironexdev avatar Dec 29 '21 22:12 ironexdev

@ironexdev Sorry, I overlooked that you were including the actual query in the request string. I wasn't aware that was supported as part of the spec. In general, GraphQL APIs are sent over POST requests with a body. We don't use the lib with GET requests, and I'm assuming this is the same for many others. So, it is possible this isn't being properly supported ATM.

When you added that check for the body content, does the query get executed properly with actual query string parameters (types and fields)? AFAIK, the body content is required to process the request. I get that there probably isn't an error. But does it work with actual requests to return back the desired response?

oojacoboo avatar Dec 29 '21 22:12 oojacoboo

@oojacoboo

When you added that check for the body content, does the query get executed properly with actual query string parameters (types and fields)?

Yes, for example this query works as expected

{{domain}}/graphql?query={authenticatedUser{authenticated,securelyAuthenticated,user{email}}}

I think it's because query parameters are parsed when the request gets from WebonyxGraphqlMiddleware to GraphQL\Server\Helper->parsePsrRequest which does the following in order to parse parameters from the query parse_str(html_entity_decode($request->getUri()->getQuery()), $queryParams). And it also ignores request body if request method is GET.

Flow

  1. WebonyxGraphqlMiddleware->process
  • https://github.com/thecodingmachine/graphqlite/blob/8a081f63112768a82eb9a6533ebacdd27090bb36/src/Http/WebonyxGraphqlMiddleware.php#L74-L100
  1. GraphQL\Server\StandardServer->executePsrRequest
  • https://github.com/webonyx/graphql-php/blob/c13e5df784015f773e46b0afa518320f2ed3071b/src/Server/StandardServer.php#L165-L170
  1. GraphQL\Server\Helper->parsePsrRequest
  • https://github.com/webonyx/graphql-php/blob/c13e5df784015f773e46b0afa518320f2ed3071b/src/Server/Helper.php#L505-L540

ironexdev avatar Dec 30 '21 11:12 ironexdev

@ironexdev Can you submit a PR for this with tests?

We probably just want to add a request method check, if the query string is being properly parsed already.

if ($request->getMethod() === 'POST' && empty($request->getParsedBody())) {

I looked briefly for some HTTP GET style API requests in the tests, but didn't see anything. I think a couple tests in tests/Integration/EndToEndTest.php should be good.

oojacoboo avatar Dec 31 '21 18:12 oojacoboo

@ironexdev did this get resolved?

oojacoboo avatar Sep 20 '23 10:09 oojacoboo

Fixed by #621

oojacoboo avatar Sep 20 '23 13:09 oojacoboo