carbon-io / carbond

MIT License
2 stars 5 forks source link

Error: request entity too large #335

Open SandyChapman opened 6 years ago

SandyChapman commented 6 years ago

I'm writing a little carbon.io application that accepts files. The problem I'm hitting is that the files are several MB in size and carbon is throwing an error when receiving them.

[2018-05-01T18:54:26.406Z] mymac.local:carbon-io.carbond.Service:ERROR: Error: request entity too large
    at makeError (/Users/schapman/Workspace/myproj-carbon/node_modules/raw-body/index.js:198:15)
    at IncomingMessage.onData (/Users/schapman/Workspace/myproj-carbon/node_modules/raw-body/index.js:121:17)
    at IncomingMessage.emit (events.js:180:13)
    at addChunk (_stream_readable.js:274:12)
    at readableAddChunk (_stream_readable.js:261:11)
    at IncomingMessage.Readable.push (_stream_readable.js:218:10)
    at HTTPParser.parserOnBody (_http_common.js:135:22)

It looks likes this needs to be addressed somewhere where there's no hook available. I took a look at carbond.ParameterParser, but it doesn't seem to allow overriding the built-in body parsing.

It looks like this needs to be overridden when the raw-body parser is added to the middleware with the limit property in options, like so:

var getRawBody = require('raw-body')
app.use(function (req, res, next) {
      getRawBody(
        stream = req,
        options = {
          length: req.headers['content-length'],
          limit: '100mb',
        },
        callback = function (err, reslt) {
          if (err) {
            return next(err)
          }
          next();
          console.log(req)
        })
    })

Is there any way to override this? I'm not a proficient node programmer by any means but am willing to look at adding a capabilities to carbon given some direction on how to approach this.

Thanks!

willshulman commented 6 years ago

Hi Sandy-

Did you try adding this middleware function to the middleware property of your Service?

https://docs.carbon.io/en/v0.7/packages/carbond/docs/guide/services.html#service-middleware

This is how you define middleware in Carbon.io, not via app.use.

Give that a try and let us know if that solves the issue.

-will

SandyChapman commented 6 years ago

@willshulman : It looks like this is happening before any of the additional service middlewares get called in the bodyParser middleware.

By changing lines 1552 & 1553 in Service.js from this:

    app.use(bodyParser.json()) // XXX We will probably not want this back now that we do EJSON parameter love
    app.use(bodyParser.urlencoded({ extended: true }))

to this:

    app.use(bodyParser.json({ limit: '50mb' }));
    app.use(bodyParser.urlencoded({ extended: true, limit: '50mb' }))

I'm able to workaround this error, but hard-coding yet another limit seems like a poor approach. Maybe someone wants a higher limit? Maybe someone wants a lower limit? One-size fits all doesn't work in this situation.

My recommendation would be to remove the hardcoded body-parser middlewares in Service.js.

We could optionally add a _initializeDefaultBodyParser method to create a default body-parser after self._initializeMiddleware() is called which will automatically apply body-parse if req.body is undefined. This potentially has it's own drawbacks as then the user cannot add middleware after the body is parsed which they may want to use.

An alternative is to add a new property that allows disabling the addition of the default bodyParser which defaults to false. I.e. I could define a service like so such that I can define my own custom body parser:

var carbon = require('carbon-io')
var o = carbon.atom.o(module).main
var __ = carbon.fibers.__(module)
var bodyParser = require('body-parser');

__(function () {
    module.exports = o({
        _type: carbon.carbond.Service,
        disableBodyParse: true,
        middleware: [
            function (req, res, next) {
                console.log('Pre parse: ' + req.body); // req.body is undefined
                next();
            },
            bodyParser.json({ limit: '50mb' }),
            bodyParser.urlencoded({ extended: true, limit: '50mb' }),
            function (req, res, next) {
                console.log('Post parse: ' + req.body); // req.body is defined
                next();
            },
        ],
        port: 8888,
        endpoints: {
            logs: o({
                _type: carbon.carbond.Endpoint,
                post: function (req, res) {
                    console.log(req.body);
                    res.statusCode = 204;
                    res.send('');
                }
            })
        }
    })
})

One last thing to note is that the body-parser documentation recommends adding the parser per route, not per service. I'm not sure that fits carbon's design philosophy though, but it does maximize the flexibility of the solution.

Thoughts on this @willshulman ?

willshulman commented 6 years ago

Hi Sandy-

Thanks for your detailed reply! I think what you propose, or something similar, is a good idea. We will discuss internally and follow up.

-will

SandyChapman commented 6 years ago

@willshulman : Any update on this? I'm happy to contribute if there's a suggested direction to go in.

willshulman commented 6 years ago

Hi Sandy-

Thanks. We would love for you to help with this and make a pull request. We would like to take a slight variant on one of your suggestions:

(1) Add bodyParsers property to Service which is an array of middleware functions. Default value defined in the constructor should be an array containing what we now have as our body parser middleware (these https://github.com/carbon-io/carbond/blob/master/lib/Service.js#L1553-L1554).

(2) Change _initializeHttpServer to use this.bodyParsers instead of the hard-coded body parsers.

To customize the bodyParsers for a Service you can either:

(1) Configure bodyParsers with a custom set of body parsers (2) Configure bodyParsers to be [] and add body parsers in middleware

Does that make sense?