outmoded / discuss

The "mailing list"
99 stars 9 forks source link

server.inject error handling with promises surfaces problems with any method besides GET #155

Closed nackjicholson closed 7 years ago

nackjicholson commented 9 years ago

My hapijs 9+ server seems to work just fine when running normally. But during testing I ran into some strange behavior when using promises and server.inject

I have two very similar routes a GET and DELETE. The tests for get work properly, the tests for delete also pass, but bluebird reports an Unhandled rejection.

Abbreviated version

Two identical routes, differ in method only. One is GET one is DELETE.

I verified the same happens for any method other than GET

Tests pass, but an Unhandled Rejection notice appears for the DELETE route.

code:

  // Handler for DELETE /{id}
  function handler(request, reply) {
      elasticsearch
        .delete({
          index: 'calculations-001',
          type: 'calculation',
          id: request.params.id
        })
        .then(reply)
        .catch((err) => {
          reply(Boom.wrap(err, err.status));
        });
    }

test:

    it('should reply with 404 if document already deleted', (done) => {
      const request = { method: 'DELETE', url: '/42' };
      let err = new Error('Not found');
      err.status = 404;

      elasticsearch.delete.returns(Bluebird.reject(err));

      server.inject(request, (res) => {
        const actual = res.result;
        const expected = Boom.wrap(err, err.status).output.payload;
        assert.equal(actual, expected, 'responds with boom not found response');
        done();
      });
    });

mocha output:

    Destroy DELETE /{id}
Unhandled rejection Error: Not found
      ✓ should reply with 404 if document already deleted

FULL CONTENT

I have my api routes encapsulated by a plugin.

lib/api/index.js

import requireDir from 'require-dir';
import {Client as ElasticSearchClient} from 'elasticsearch';
const calculations = requireDir('./calculations');

/**
 * Calculation Service API Plugin
 *
 * Handles the bootstrapping of anything necessary for this service.
 *
 * @param server
 * @param {object} options
 * @param next
 */
function register(server, { elasticsearchHost }, next) {
  const elasticsearch = new ElasticSearchClient({
    host: elasticsearchHost,
    sniffOnStart: true
  });

  server.route([
    calculations.destroy(elasticsearch),
    calculations.show(elasticsearch),
  ]);

  next();
}

register.attributes = {
  name: 'api',
  version: '1.0.0'
};

export default register;

lib/api/calculations/show.js

import Boom from 'boom';
import Joi from 'joi';

function show(elasticsearch) {
  return {
    method: 'GET',
    path: '/{id}',
    handler(request, reply) {
      elasticsearch
        .get({
          index: 'calculations-001',
          type: 'calculation',
          id: request.params.id
        })
        .then(reply)
        .catch((err) => {
          reply(Boom.wrap(err, err.status));
        });
    },
    config: {
      description: 'Show resource route for calculation data.',
      tags: ['api'],
      validate: {
        params: {
          id: Joi.string()
        }
      }
    }
  };
}

export default show;

lib/api/calculations/destroy.js

import Boom from 'boom';
import Joi from 'joi';

function destroy(elasticsearch) {
  return {
    method: 'DELETE',
    path: '/{id}',
    handler(request, reply) {
      elasticsearch
        .delete({
          index: 'calculations-001',
          type: 'calculation',
          id: request.params.id
        })
        .then(reply)
        .catch((err) => {
          reply(Boom.wrap(err, err.status));
        });
    },
    config: {
      description: 'Destroy resource route for calculation data.',
      tags: ['api'],
      validate: {
        params: {
          id: Joi.string()
        }
      }
    }
  };
}

export default destroy;

And the plugin test test/lib/api/index.js

import assert from 'assert';
import Boom from 'boom';
import proxyquire from 'proxyquire';
import Bluebird from 'bluebird';
import sinon from 'sinon';
import {Server} from 'hapi';

describe('lib/api', () => {
  let server;
  let elasticsearch;

  beforeEach(() => {
    elasticsearch = {
      get: sinon.stub(),
      delete: sinon.stub()
    };

    server = new Server();
    server.connection();

    const api = proxyquire('../../../lib/api', {
      elasticsearch: { Client: sinon.stub().returns(elasticsearch)}
    });

    server.register(api, err => err);
  });

  describe('Show GET /{id}', () => {
    it('should return calculation document', (done) => {
      const calcObj = { foo: 'bar' };

      elasticsearch.get.returns(Bluebird.resolve(calcObj));

      server.inject('/42', (res) => {
        const actual = res.result;
        assert.equal(actual, calcObj, 'calc returned when found');
        done();
      });
    });

    it('should reply with 404 when calculation document not found', (done) => {
      let err = new Error('Not found');
      err.status = 404;

      elasticsearch.get.returns(Bluebird.reject(err));

      server.inject('/42', (res) => {
        const actual = res.result;
        const expected = Boom.wrap(err, err.status).output.payload;
        assert.equal(actual, expected, 'responds with boom not found response');
        done();
      });
    });
  });

  describe('Destroy DELETE /{id}', () => {
    it('should delete a calculation document', (done) => {
      const request = { method: 'DELETE', url: '/42' };
      const deleteRes = { foo: 'bar' };

      elasticsearch.delete.returns(Bluebird.resolve(deleteRes));

      server.inject(request, (res) => {
        const actual = res.result;
        assert.equal(actual, deleteRes, 'successful delete response');
        done();
      });
    });

    it('should reply with 404 if document already deleted', (done) => {
      const request = { method: 'DELETE', url: '/42' };
      let err = new Error('Not found');
      err.status = 404;

      elasticsearch.delete.returns(Bluebird.reject(err));

      server.inject(request, (res) => {
        const actual = res.result;
        const expected = Boom.wrap(err, err.status).output.payload;
        assert.equal(actual, expected, 'responds with boom not found response');
        done();
      });
    });
  });
});

final test output

  lib/api
    Show GET /{id}
      ✓ should return calculation document
      ✓ should reply with 404 when calculation document not found
    Destroy DELETE /{id}
      ✓ should delete a calculation document
Unhandled rejection Error: Not found
    at Context.<anonymous> (/Users/nackjicholson/repos/calculations-storage-service/es6/test/lib/api/index.js:73:17)
    at callFnAsync (/Users/nackjicholson/repos/calculations-storage-service/node_modules/mocha/lib/runnable.js:309:8)
    at Test.Runnable.run (/Users/nackjicholson/repos/calculations-storage-service/node_modules/mocha/lib/runnable.js:264:7)
    at Runner.runTest (/Users/nackjicholson/repos/calculations-storage-service/node_modules/mocha/lib/runner.js:419:10)
    at /Users/nackjicholson/repos/calculations-storage-service/node_modules/mocha/lib/runner.js:526:12
    at next (/Users/nackjicholson/repos/calculations-storage-service/node_modules/mocha/lib/runner.js:340:14)
    at /Users/nackjicholson/repos/calculations-storage-service/node_modules/mocha/lib/runner.js:350:7
    at next (/Users/nackjicholson/repos/calculations-storage-service/node_modules/mocha/lib/runner.js:283:14)
    at Immediate._onImmediate (/Users/nackjicholson/repos/calculations-storage-service/node_modules/mocha/lib/runner.js:318:5)
    at processImmediate [as _immediateCallback] (timers.js:371:17)
      ✓ should reply with 404 if document already deleted

  4 passing (334ms)
Marsup commented 9 years ago

Since you're using mocha and it's known to fail on node's domains, I'd suggest you try to use something like https://www.npmjs.com/package/inject-then in your tests see if it helps.

nackjicholson commented 7 years ago

used inject-then