New error handling method loses error descriptions #723

Closed
raydog opened this Issue Jan 9, 2013 · 27 comments

Projects

None yet

4 participants

@raydog
raydog commented Jan 9, 2013

For example, in middleware/json.js, line 71:

next(400, "invalid json, empty body')

This passes the error along to bodyParser.js, line 54, which does:

if (err) return next(err);

The problem is that we lose the detailed error message, and the error that we see is merely the HTTP status description doubled. (IE: Bad Request: Bad Request) This makes debugging a bit more difficult.

Each of those next(err) calls should also pass along the detailed message, or we could switch back to the utils.error() approach that connect used before. IMHO, having the error isolated to a single parameter makes more sense than having the errors leak out into the second parameter, which typically has non-error data.

@tj
Member
tj commented Jan 9, 2013

next(err, msg) should be effectively the same as the old util, if not we have a bug

@dougwilson
Contributor

It looks like it is a bug in bodyParser since it tried to compose middleware itself, which bypasses the special next(err, msg) handling.

@dougwilson
Contributor

@visionmedia This may be the reason you didn't like that signature in the first place 😉 since middleware can't really be handled just like a plain function anymore, as the callers need to know how to handle the new signature.

@tj
Member
tj commented Jan 9, 2013

ah yes, that's super lame haha, reverting! this time I'll make sure to reference the issue hahaha

@tj tj pushed a commit that referenced this issue Jan 9, 2013
TJ Holowaychuk Revert "add support for `next(status[, msg])`"
see #723

This reverts commit 87ffafd.
6db901f
@tj tj closed this Jan 9, 2013
@defunctzombie
Contributor

I think the next(status, msg) case can work. Maybe body parser just needs to be fixed. I will take a look when I am in front of a computer. The idea is that the error object should be created for you and nothing lost. @visionmedia if I fix this issue will you reconsider?

@dougwilson
Contributor

@shtylman you also need to fix every use of the limit middleware though out all the other middleware, as it's use suffers from the same problem (the caller does if (err) return next(err) dropping the possible message string). Another issue with your other patch was the removal of utils.error which also broke all third-party code that was using that utility function to make the errors.

@tj
Member
tj commented Jan 9, 2013

it's not just bodyParser, we do this sort of composition lots in our apps with custom middleware as well

@tj
Member
tj commented Jan 9, 2013

although the utils are private so they shouldn't have been used anyway

@defunctzombie
Contributor

I will poke at it and see what is going on.

Yes. This is the downfall of utils!
On Jan 9, 2013 5:25 PM, "TJ Holowaychuk" notifications@github.com wrote:

although the utils are private so they shouldn't have been used anyway


Reply to this email directly or view it on GitHubhttps://github.com/senchalabs/connect/issues/723#issuecomment-12070167.

@dougwilson
Contributor

@visionmedia Ah, I didn't know that utils were private, especially since I didn't notice anywhere saying they were and they are exposed at require('connect').utils.

@tj
Member
tj commented Jan 9, 2013

they were exposed originally for Express but that was kinda weird to begin with since they should have been in npm as much as possible

@dougwilson
Contributor

Perhaps this calls for an HTTP error factory module, then? 😄

@defunctzombie
Contributor

there is already an httperrors modules which makes it easy to instantiate
error objects of specific types. The reason I wanted the next(status, msg)
was that it seemed like a very common thing to do when it comes to
middleware erroring out with some error code. This way connect can create
the proper error object with a statusCode field (or status or whatever) and
the rest of the middleware gets that accordingly.

On Wed, Jan 9, 2013 at 5:36 PM, Douglas Christopher Wilson <
notifications@github.com> wrote:

Perhaps this calls for an HTTP error factory module, then? [image:
😄]


Reply to this email directly or view it on GitHubhttps://github.com/senchalabs/connect/issues/723#issuecomment-12070667.

@defunctzombie
Contributor

Wait, unless I am missing something. Nothing was broken. There are tests to check for a proper body. @raydog can you present some code that is failing and describe how you expect it to work?

@defunctzombie
Contributor

to me the existence of util.error clearly shows what we really want to be happening. That is why I proposed next(status, msg) as a special case. We should not go cray with next, but given that it is a good way to indicate a specific error code (which is a very common usecase for a web app) I see no reason why this should not exist.

@dougwilson
Contributor

You can see the behavior reversion if you add this to test/bodyParser.js (which is probably should be):

  describe('with application/json', function(){
    it('should next(err) on empty JSON', function(done){
      var app = connect();

      app.use(connect.bodyParser());

      app.use(function(req, res){
        res.end('whoop');
      });

      app.use(function(err, req, res, next){
        err.message.should.equal('invalid json, empty body');
        res.statusCode = 500;
        res.end();
      });

      app.request()
      .post('/')
      .set('Content-Type', 'application/json')
      .write('')
      .end(function(res){
        res.statusCode.should.equal(500);
        done();
      });
    });

    it('should next(err) on invalid strict JSON', function(done){
      var app = connect();

      app.use(connect.bodyParser({'strict': true}));

      app.use(function(req, res){
        res.end('whoop');
      });

      app.use(function(err, req, res, next){
        err.message.should.equal('invalid json');
        res.statusCode = 500;
        res.end();
      });

      app.request()
      .post('/')
      .set('Content-Type', 'application/json')
      .write('"string"')
      .end(function(res){
        res.statusCode.should.equal(500);
        done();
      });
    });
  });
@defunctzombie
Contributor

So unless I have lost my mind (which is entirely possible), I backed out
Tj's reversal of my patch, added the tests and ran npm test.
Everything passed.

On Wed, Jan 9, 2013 at 6:09 PM, Douglas Christopher Wilson <
notifications@github.com> wrote:

You can see the behavior reversion if you add this to test/bodyParser.js(which is probably should be):

it('should next(err) on empty JSON', function(done){
  var app = connect();

  app.use(connect.bodyParser());

  app.use(function(req, res){
    res.end('whoop');
  });

  app.use(function(err, req, res, next){
    err.message.should.equal('invalid json, empty body');
    res.statusCode = 500;
    res.end();
  });

  app.request()
  .post('/')
  .set('Content-Type', 'application/json')
  .write(' ')
  .end(function(res){
    res.statusCode.should.equal(500);
    done();
  });
});

it('should next(err) on invalid strict JSON', function(done){
  var app = connect();

  app.use(connect.bodyParser({'strict': true}));

  app.use(function(req, res){
    res.end('whoop');
  });

  app.use(function(err, req, res, next){
    err.message.should.equal('invalid json');
    res.statusCode = 500;
    res.end();
  });

  app.request()
  .post('/')
  .set('Content-Type', 'application/json')
  .write('"string"')
  .end(function(res){
    res.statusCode.should.equal(500);
    done();
  });
})


Reply to this email directly or view it on GitHubhttps://github.com/senchalabs/connect/issues/723#issuecomment-12072046.

@dougwilson
Contributor

I'm not sure what is wrong with that command and the tests (I haven't written tests for connect before). You need to run it with mocha manually, like ./node_modules/.bin/mocha test/bodyParser.js; the assertions on the message seem to be eaten.

@defunctzombie
Contributor

Hm, it does seem that make (which runs tests and running it manually produce different behavior. This is a bit unsettling. @visionmedia thoughts?

@dougwilson
Contributor

@shtylman it's because the way the other tests a copied are testing the status message in such a way that doesn't work. I'm going to change the tests I made you they actually fail in a moment.

@tj
Member
tj commented Jan 9, 2013

i dont use npm test ever, npm scripts make zero sense to me, npm isn't a makefile

@defunctzombie
Contributor

@visionmedia make test had the same issue when I copied the provided
tests. But apparently the tests have issues.

On Wed, Jan 9, 2013 at 6:28 PM, TJ Holowaychuk notifications@github.comwrote:

i dont use npm test ever, npm scripts make zero sense to me, npm isn't a
makefile


Reply to this email directly or view it on GitHubhttps://github.com/senchalabs/connect/issues/723#issuecomment-12072830.

@dougwilson
Contributor

Here are the adjusted tests that will fail properly:

  describe('with application/json', function(){
    it('should next(err) on empty JSON', function(done){
      var app = connect();

      app.use(connect.bodyParser());

      app.use(function(req, res){
        res.end('whoop');
      });

      app.use(function(err, req, res, next){
        err.message.should.equal('invalid json, empty body');
        res.statusCode = 500;
        res.end();
      });

      app.request()
      .post('/')
      .set('Content-Type', 'application/json')
      .write('')
      .end(function(res){
        res.statusCode.should.equal(500);
        res.body.should.be.empty;
        done();
      });
    });

    it('should next(err) on invalid strict JSON', function(done){
      var app = connect();

      app.use(connect.bodyParser({'strict': true}));

      app.use(function(req, res){
        res.end('whoop');
      });

      app.use(function(err, req, res, next){
        err.message.should.equal('invalid json');
        res.statusCode = 500;
        res.end();
      });

      app.request()
      .post('/')
      .set('Content-Type', 'application/json')
      .write('"string"')
      .end(function(res){
        res.statusCode.should.equal(500);
        res.body.should.be.empty;
        done();
      });
    });
  });
@defunctzombie
Contributor

Anyhow, I took a look at what is going on. The issue is that a function is
passed to pretend to be the "next" when body parser invokes _json. The
problem is that you are now replacing the middleware stack with your own
"next" implementation which is no longer conforming with what the connect
next supports. This is going to happen anytime you subvert a a function
with your own and don't do it the proper way (capturing 'arguments' and
passing that along).

On Wed, Jan 9, 2013 at 6:30 PM, Roman Shtylman shtylman@gmail.com wrote:

@visionmedia make test had the same issue when I copied the provided
tests. But apparently the tests have issues.

On Wed, Jan 9, 2013 at 6:28 PM, TJ Holowaychuk notifications@github.comwrote:

i dont use npm test ever, npm scripts make zero sense to me, npm isn't a
makefile


Reply to this email directly or view it on GitHubhttps://github.com/senchalabs/connect/issues/723#issuecomment-12072830.

@dougwilson
Contributor

@shtylman that is right. It's exactly what the initial issue text said.

@defunctzombie
Contributor

Changing the next(err) call in bodyParser to next.apply(null, arguments)
makes your tests pass. I personally think this is a bug in body parser :/

On Wed, Jan 9, 2013 at 6:37 PM, Douglas Christopher Wilson <
notifications@github.com> wrote:

@shtylman https://github.com/shtylman that is right. It's exactly what
the initial issue text said.


Reply to this email directly or view it on GitHubhttps://github.com/senchalabs/connect/issues/723#issuecomment-12073142.

@defunctzombie
Contributor

@dougwilson I read that as needing to pass msg along explicitly while it should happen through arguments. My reading comprehension is not what it used to be :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment