Body parser default ContentLength limit

[sberryman]

While it would be nice if all JSON payloads were under 100KB, that won't be the case 100% of the time.

Express 4 uses the body-parser package to decode JSON and Raw payloads which is nice but the default is a bit small for my use case.

The template loads the body-parser package here: https://github.com/openfaas-incubator/node8-express-template/blob/master/template/node8-express/index.js#L12

app.use(bodyParser.json());

The parser accepts an options object, one of those options is limit which is a string parsed by the bytes package or a number of bytes. Body parser parses the limit option using the following code:

var limit = typeof opts.limit !== 'number'
    ? bytes.parse(opts.limit || '100kb')
    : opts.limit

If you decide to only implement the bytes parser (easy option) it could be a very simple change:

app.use(bodyParser.json({ limit: process.env.BODYPARSER_JSON_LIMIT }));

[alexellis]

Thank you for this feedback. It may also affect the node10-express repo, so if we make changes we should dual-maintain them. I'll ping a couple of people for some more input.

[padiazg]

I think it's a valid proposal. I think we should use a process.env.BODYPARSER_JSON_OPTIONS instead, to be more generic. We don't know what other option from the parsers someone would need for his project. Besides verify, almost all options can be used with this templates.

[sberryman]

@padiazg so you propose process.env.BODYPARSER_JSON_OPTIONS as a JSON string which is parsed and passed to body parser?

à la

app.use(bodyParser.json(JSON.parse(process.env.BODYPARSER_JSON_OPTIONS || '{}'));

I've never used JSON env variables in a project I've worked on, I don't recall a reason why I've avoided it though.

Edit: Personally, I feel like the developer should be responsible for parsing the body. That is the way it works with streaming and serializing mode. I haven't looked at the other templates to see if you are auto parsing body for golang, python, etc. Some consistency would be nice though.

[alexellis]

I've never used JSON env variables in a project I've worked on, I don't recall a reason why I've avoided it though.

Same, it may look a little odd in the template.

Are there other variables we need to tune? If we tuned this where would you expect to see it @sberryman?

These are the options I can think of:

• A one-off in the Dockerfile and allow people to fork and maintain their own values in their copy of the template
• Dockerfile (then update via BUILD_ARG flag on OpenFaaS CLI)
• Runtime arg (anyone can update this at runtime by adding it to their stack.yml)

[sberryman]

I personally don't need to modify any other variable but I would assume someone in the future would need to modify one or more.

It may be too late to avoid a breaking change and remove parsing from the template. If that is the case, I would expect one or more environment variables allowing the developer to change defaults.

The options depend on what you consider a new version of a function. If you believe everything in production should be immutable then rule out runtime argument.

Let's use a hypothetical scenario of a function which resizes an image. The image bytes and resize options are passed to a function encoded as JSON.

Now you write the function, deploy it to production and have been using it for a while. Another developer in the organization sees this useful function and decides they want to use it to resize very large images to produce thumbnails. The new developer passes the new (VERY LARGE) image and gets an error that the payload is too large.

Do you expect the author of the resize function to adjust the code or dockerfile and re-deploy a new version or adjust a runtime argument in their yaml or external configuration software? I can see it going both ways personally. I would cast a vote for runtime argument though.

[alexellis]

@padiazg if we set this as a run-time environmental variable, via faas-cli deploy --env key=value does that make sense for you too? Would you be able to create a PR for us to test this out?

Alex

[padiazg]

I have done this already, as we talk, but forgot to do the PR. I'll do it today. Sorry for the long delay.

[padiazg]

Finally I could finish testing the proposals, sorry, too much pending work.

The proposals are at: https://github.com/padiazg/node8-express-template https://github.com/padiazg/node10-express-template

Gist for testing them here: https://gist.github.com/padiazg/6a5e9159e346d041bd91d6990a894a8a

They are ready to become PRs if tests are successful.