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.

Sign up for GitHub

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

Add env vars to use raw body and set max json size #195

Merged
merged 1 commit into from
Feb 3, 2020
Merged

Add env vars to use raw body and set max json size #195

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions template/node12/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,15 @@ const app = express()
const handler = require('./function/handler');
const bodyParser = require('body-parser')

// app.use(bodyParser.urlencoded({ extended: false }));
app.use(bodyParser.json());
app.use(bodyParser.raw());
app.use(bodyParser.text({ type : "text/*" }));
if (process.env.RAW_BODY === 'true') {
Copy link

Choose a reason for hiding this comment

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

Hey @burtonr

I'd suggest to keep support of json and text parsers even when RAW_BODY is true.

To keep completely old behavior I suggest inverting the variable name something like NORAW_BODY and enable raw parser by default.

jsonLimit can be also set only if MAX_JSON_SIZE is specified

Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to keep support of json and text parsers even when RAW_BODY is true.

I don't see the point in that change, that's the behaviour we have today?

Copy link

Choose a reason for hiding this comment

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

If I understand this correctly RAW_BODY should just enable/disable raw parser, is not it?

If it is set to true the app will have no JSON and TEXT parsers

Copy link
Member

Choose a reason for hiding this comment

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

The use-case was that users need to disable JSON parsing and only access a RAW body. The docs PR I hope helps with this, but I'd like @burtonr to explain the use-case better there. https://docs.openfaas.com/cli/templates/#nodejs-12-node12-of-watchdog-template

Copy link

Choose a reason for hiding this comment

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

RAW_BODY confuses me since it was TLDR. maybe ONLY_RAW name is better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, but it's important to understand that the bodyParser.raw() only enables the raw body for the content type that you pass in. So, this change makes it so that when a user sets the RAW_BODY variable to true, the raw body will be available for all content types.
The idea is that a user would define a very specific function that requires the raw body, where other functions will not have this enabled and have the JSON body available as normal.

app.use(bodyParser.raw({ type: '*/*' }))
} else {
var jsonLimit = process.env.MAX_JSON_SIZE || '100kb' //body-parser default
app.use(bodyParser.json({ limit: jsonLimit}));
app.use(bodyParser.raw()); // "Content-Type: application/octet-stream"
app.use(bodyParser.text({ type : "text/*" }));
}

app.disable('x-powered-by');

class FunctionEvent {
Expand Down