Add calling next for Adding next() call to prepareOutput middleware […

[bamidei]

…florianholzapfel/express-restify-mongoose#337]

This fixes an issue whereby any middleware registered to occur after would not be called. The specific situation which we have which triggers this is a call to server.on('after', ) where server is a restify server instance. The specified middleware will never be called without this fix.

I notice that you have specific unit tests to ensure that next is not called from prepareOutput. If you would like to not merge this PR instead of changing those tests, could you please explain the reasoning for this? Also, the latest version removed the next parameter from being passed to the postProcess optional parameter which prevents an easy work-around for this not being called in prepareOutput.

[sgilroy]

@bamidei It might be helpful to include more details about the test failures and a link to the tests themselves.

  1) prepareOutput
       calls outputFn with default options and no post* middleware:
     expected spy to not have been called but was called once
    spy() at callback (/home/travis/build/florianholzapfel/express-restify-mongoose/src/middleware/prepareOutput.js:60:9)
  AssertError: expected spy to not have been called but was called once
      spy() at callback (src/middleware/prepareOutput.js:60:9)
      at Object.fail (node_modules/sinon/lib/sinon/assert.js:110:29)
      at failAssertion (node_modules/sinon/lib/sinon/assert.js:69:24)
      at Object.assert.(anonymous function) [as notCalled] (node_modules/sinon/lib/sinon/assert.js:94:21)
      at Context.it (test/unit/middleware/prepareOutput.js:39:18)
  2) prepareOutput
       calls outputFn with default options and no post* middleware (async):
     expected spy to not have been called but was called once
    spy() at callback (/home/travis/build/florianholzapfel/express-restify-mongoose/src/middleware/prepareOutput.js:60:9)
  AssertError: expected spy to not have been called but was called once
      spy() at callback (src/middleware/prepareOutput.js:60:9)
      at Object.fail (node_modules/sinon/lib/sinon/assert.js:110:29)
      at failAssertion (node_modules/sinon/lib/sinon/assert.js:69:24)
      at Object.assert.(anonymous function) [as notCalled] (node_modules/sinon/lib/sinon/assert.js:94:21)
      at Context.it (test/unit/middleware/prepareOutput.js:58:18)
  3) prepareOutput
       calls postProcess with default options and no post* middleware:
     expected spy to not have been called but was called once
    spy() at callback (/home/travis/build/florianholzapfel/express-restify-mongoose/src/middleware/prepareOutput.js:57:11)
  AssertError: expected spy to not have been called but was called once
      spy() at callback (src/middleware/prepareOutput.js:57:11)
      at Object.fail (node_modules/sinon/lib/sinon/assert.js:110:29)
      at failAssertion (node_modules/sinon/lib/sinon/assert.js:69:24)
      at Object.assert.(anonymous function) [as notCalled] (node_modules/sinon/lib/sinon/assert.js:94:21)
      at Context.it (test/unit/middleware/prepareOutput.js:80:18)

I agree that it isn't clear why the tests include sinon.assert.notCalled(next) instead of sinon.assert.calledOnce(next). Perhaps add a commit with this change?

[bamidei]

@sgilroy Thank you, Scott. Yes, those are the tests that I think should change and happy to do it. @Zertz, please let me know if you'd like me to adjust the tests to calledOnce as Scott mentioned or if you believe these tests and corresponding behavior should remain unchanged.

Thanks!

[coveralls]

Coverage decreased (-0.7%) to 97.025% when pulling 976b396b1e60bec8e9e224aff3aa864f9d720a0f on bamidei:call-next-from-prepareOutput into f8b1e7990a2ad3f23edc26d2b65061e5949d7d78 on florianholzapfel:master.

[bamidei]

@sgilroy @Zertz I've done some work on the unit and integration tests and have done a bit more research on the underlying question of whether next() should be called or not.

It is my conclusion that, unfortunately, Express and Restify frameworks have different expectations/requirements around calling next() in this case. For Restify, it is required to call next() and for Express it is not required/recommended and actually causes problems. I've updated the change to reflect this - using the config.restify option which is already present to determine whether or not to call next() from prepareOutput middleware.

A few additional notes:

1. The unit tests were not properly handling the async function callback. It is necessary to wait for the promise to resolve before performing the sinon assertions. I've done this in a way that works without adding additional frameworks to the project. However, to be more robust, I recommend adding the Chai assertion library which has better handling for testing code with promises which can make the tests a bit easier to read and maintain.

2. Some external links which provide insights into my conclusion that Restify should have next() called.

• A closed PR which someone had made to 'fix' Restify so that next() was not required in order to ensure 'after' middleware was invoked. This PR was rejected by the maintainer as not correct.
• Restify documentation concerning next() usage. Note that, in their documentation, they state: 'Calling next() will move to the next function in the chain. Unlike other REST frameworks, calling res.send() does not trigger next() automatically. In many applications, work can continue to happen after res.send(), so flushing the response is not synonymous with completion of a request.'
• A stack overflow post which highlights the exact issue which we are witnessing with the after middleware not being called. See the last post in the chain which I'll quote here: 'Another issue (currently) is that the Server after event is not emitted if all handlers don't call next. For example, if you are using auditLogger to log requests using the after event, you won't get logs for any requests that reach a handler that doesn't call next. I have opened a PR to fix the issue so that the after event is emitted either way, but calling next is the expected norm for apps using restify.'
  -- (The PR mentioned there chained to the one above which was closed as incorrect.)