graphile / crystal

🔮 Graphile's Crystal Monorepo; home to Grafast, PostGraphile, pg-introspection, pg-sql2 and much more!
https://graphile.org/
Other
12.63k stars 572 forks source link

Option --body-size-limit not respected for non-JSON payloads #604

Closed johnchristopherjones closed 6 years ago

johnchristopherjones commented 7 years ago

This appears to be a regression on #372:

mutation UpdateArticleById($input: UpdateArticleByIdInput!) {
  updateArticleById(input:$input) {
    article {
      id updatedAt
    }
  }
}

{input: {articlePatch: {content: "[102400 bytes]"}, id: "8a93eafc-9189-45be-98de-cc618fca3427"}}

results in:

{
  "errors": [
    {
      "message": "request entity too large",
      "stack": "Error\n    at readStream (/usr/local/share/.config/yarn/global/node_modules/raw-body/index.js:196:17)\n    at getRawBody (/usr/local/share/.config/yarn/global/node_modules/raw-body/index.js:106:12)\n    at read (/usr/local/share/.config/yarn/global/node_modules/body-parser/lib/read.js:76:3)\n    at jsonParser (/usr/local/share/.config/yarn/global/node_modules/body-parser/lib/types/json.js:127:5)\n    at /usr/local/share/.config/yarn/global/node_modules/postgraphql/build/postgraphql/http/createPostGraphQLHttpRequestHandler.js:236:29\n    at /usr/local/share/.config/yarn/global/node_modules/postgraphql/build/postgraphql/http/createPostGraphQLHttpRequestHandler.js:235:146"
    }
  ]
}
benjie commented 7 years ago

What version of PostGraphQL are you using?

johnchristopherjones commented 7 years ago

Postgraphile 4.0.0-alpha2.20 and Postgraphql 3.2.0 both appear to exhibit this behavior.

The error isn't being emitted by Postgraphql/Postgraphile itself except in the response when --show-error-stack is used.

benjie commented 7 years ago

Does using --body-size-limit instead of --bodySizeLimit fix the issue?

https://www.graphile.org/postgraphile/usage-cli/

johnchristopherjones commented 7 years ago

Ah, no, I was actually using --body-size-limit and misreported that. The option --bodySizeLimit correctly returns the error that it's an unknown option. I was paging through the source to try to figure it out so the camelCase got stuck in my head.

Edit: updated title

johnchristopherjones commented 7 years ago

Uhm. Okay I'm embarrassed. I've tracked this down to a different typo in my command line after swatting at this for far too long. Thank you for the the quick response, but this was user error.

benjie commented 7 years ago

Are you able to add a breakpoint here:

https://github.com/postgraphql/postgraphql/blob/c19626a62802f600c19010bbccaf2951f61a256c/src/postgraphql/http/createPostGraphQLHttpRequestHandler.js#L101-L102

And inspect the value of options.bodySizeLimit there?

(Or just edit the file in node_modules and console.dir it)

If it's valid then maybe it's an issue with your input syntax; see:

https://github.com/expressjs/body-parser#limit

benjie commented 7 years ago

Ah; okay! 👍

johnchristopherjones commented 7 years ago

Okay, this wasn't me just chasing my tale entirely.

I wasn't having any problem in GraphiQL but my client app was still giving me the error.

I was using form data to submit the query and variables, like so:

curl -X POST -H 'ContentType: application/json' --data "query=$UriEncodedQuery&variables=$UriEncodedVariables"

I have some software that was doing things this way surreptitiously and backed my way into what it was doing.

Surprisingly, this worked without any other error, but it apparently caused variables to reach a JSON parser without the limit option. Submitting the entire body as JSON like the header claims resolves the problem.

I guess it'd be more correct to either emit an error when receiving form data or somehow pass the JSON parser options along when receiving form data.

benjie commented 7 years ago

A PR adding the limit option to the urlencoded and/or text body parsers (if valid) would be welcome 👍

https://github.com/postgraphql/postgraphql/blob/c19626a62802f600c19010bbccaf2951f61a256c/src/postgraphql/http/createPostGraphQLHttpRequestHandler.js#L100-L107

johnchristopherjones commented 7 years ago

I’ll look into it this week.

On Oct 23, 2017, at 11:22 AM, Benjie Gillam notifications@github.com wrote:

A PR adding the limit option to the urlencoded and/or text body parsers (if valid) would be welcome 👍

https://github.com/postgraphql/postgraphql/blob/c19626a62802f600c19010bbccaf2951f61a256c/src/postgraphql/http/createPostGraphQLHttpRequestHandler.js#L100-L107 https://github.com/postgraphql/postgraphql/blob/c19626a62802f600c19010bbccaf2951f61a256c/src/postgraphql/http/createPostGraphQLHttpRequestHandler.js#L100-L107 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/postgraphql/postgraphql/issues/604#issuecomment-338695284, or mute the thread https://github.com/notifications/unsubscribe-auth/AA02LIvFHj6-qIgRiAIalGL6h_BmFtnpks5svK85gaJpZM4QC3Hz.