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

Changing the limit for JSON bodyParser (#372) #379

Merged
merged 6 commits into from
Mar 5, 2017

Conversation

guyemerson
Copy link
Contributor

@guyemerson guyemerson commented Mar 2, 2017

Add sizeLimit as an option, which controls the maximum size of a JSON body that can can be accepted by bodyParser.json. This option is available both at the command line and as middleware.

@guyemerson
Copy link
Contributor Author

The only check that is still failing is to do with the version of graphql, but that is not affected by this pull request.

Copy link
Collaborator

@calebmer calebmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, I noticed you had made a commit when responding to another notification before I saw you opened a PR 😊

Could we change the name of the config option? sizeLimit is a little generic.

Also, it may be worth discussing if there is a reasonable default we could use that would remove the need for a size limit all together.

docs/cli.md Outdated
@@ -38,6 +38,7 @@ The usage of the `postgraphql` binary is as follows. To pull up this documentati
-a, --classic-ids use classic global id field name. required to support Relay 1
-j, --dynamic-json enable dynamic JSON in GraphQL inputs and outputs. uses stringified JSON by default
-M, --disable-default-mutations disable default mutations, mutation will only be possible through Postgres functions
-l, --size-limit set the maximum size of JSON bodies that can be parsed (default 100kB)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we change the name to --body-size-limit? Could we also add a note that you can define it using a pretty size like 200kB.

docs/library.md Outdated
@@ -64,6 +64,7 @@ Arguments include:
- `enableCors`: Enables some generous CORS settings for the GraphQL endpoint. There are some costs associated when enabling this, if at all possible try to put your API behind a reverse proxy.
- `exportJsonSchemaPath`: Enables saving the detected schema, in JSON format, to the given location. The directories must exist already, if the file exists it will be overwritten.
- `exportGqlSchemaPath`: Enables saving the detected schema, in GraphQL schema format, to the given location. The directories must exist already, if the file exists it will be overwritten.
- `sizeLimit`: Set the maximum size of JSON bodies that can be parsed (default 100kB).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we change this to bodySizeLimit and add a similar note about pretty bytes?

@calebmer
Copy link
Collaborator

calebmer commented Mar 5, 2017

I tried restarting the build. I’ll investigate if it keeps failing 😣

@benjie benjie dismissed calebmer’s stale review March 5, 2017 20:14

Changes have been made 👍

@benjie benjie merged commit 6cf9ac7 into graphile:master Mar 5, 2017
@benjie
Copy link
Member

benjie commented Mar 5, 2017

Thanks! 👍

benjie pushed a commit that referenced this pull request Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants