From b032be20d129be51a7b5d3b0a380f0c8ef0c287b Mon Sep 17 00:00:00 2001 From: Joey Baker Date: Sun, 4 Sep 2016 17:25:57 -0700 Subject: [PATCH 01/10] Fix (error) always reply with an error #oops We had a case where reply would never be called. This could case a server hang. --- src/error.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/error.js b/src/error.js index af49e86..a9d0510 100644 --- a/src/error.js +++ b/src/error.js @@ -14,14 +14,15 @@ export default (target, key, descriptor) => { if (code && (code.startsWith('22') || code.startsWith('23'))) { const error = Boom.wrap(e, 406); - // detail tends to be more specific information. So, if we have it, use. - if (detail) { - error.message += `: ${detail}`; - error.reformat(); - } - - reply(error); + // detail tends to be more specific information. So, if we have it, use. + if (detail) { + error.message += `: ${detail}`; + error.reformat(); } + + } + + reply(error); } else if (!e.isBoom) { const { message } = e; let err; From da6b3ce963b6c7d8983593d5940d6535ce0fb3c9 Mon Sep 17 00:00:00 2001 From: Joey Baker Date: Sun, 4 Sep 2016 17:26:51 -0700 Subject: [PATCH 02/10] Feat (error) include `hint` on PG errors Provides useful info! --- src/error.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/error.js b/src/error.js index a9d0510..54fdc7e 100644 --- a/src/error.js +++ b/src/error.js @@ -8,7 +8,8 @@ export default (target, key, descriptor) => { await fn(request, reply); } catch (e) { if (e.original) { - const { code, detail } = e.original; + const { code, detail, hint } = e.original; + let error; // pg error codes https://www.postgresql.org/docs/9.5/static/errcodes-appendix.html if (code && (code.startsWith('22') || code.startsWith('23'))) { @@ -20,6 +21,10 @@ export default (target, key, descriptor) => { error.reformat(); } + // hint might provide useful information about how to fix the problem + if (hint) { + error.message += ` Hint: ${hint}`; + error.reformat(); } reply(error); From bab2e90cbbf77e01093468a76d54f0dc5d31808f Mon Sep 17 00:00:00 2001 From: Joey Baker Date: Sun, 4 Sep 2016 17:27:07 -0700 Subject: [PATCH 03/10] Feat (error) parse PG 42* errors --- src/error.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/error.js b/src/error.js index 54fdc7e..3bb7384 100644 --- a/src/error.js +++ b/src/error.js @@ -13,7 +13,15 @@ export default (target, key, descriptor) => { // pg error codes https://www.postgresql.org/docs/9.5/static/errcodes-appendix.html if (code && (code.startsWith('22') || code.startsWith('23'))) { - const error = Boom.wrap(e, 406); + error = Boom.wrap(e, 406); + } else if (code && (code.startsWith('42'))) { + error = Boom.wrap(e, 422); + // TODO: we could get better at parse postgres error codes + } else { + // use a 502 error code since the issue is upstream with postgres, not + // this server + error = Boom.wrap(e, 502); + } // detail tends to be more specific information. So, if we have it, use. if (detail) { From 506d42f39a76d331b47b3f7379f1b21df9c0eb0b Mon Sep 17 00:00:00 2001 From: Joey Baker Date: Mon, 5 Sep 2016 12:30:15 -0700 Subject: [PATCH 04/10] 2.5.4 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index e2eafdb..cd8773c 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "hapi-sequelize-crud", - "version": "2.5.3", + "version": "2.5.4", "description": "Hapi plugin that automatically generates RESTful API for CRUD", "main": "build/index.js", "config": { From 38ccb3adf69fbb9efb2fa7712843529cf5b534c3 Mon Sep 17 00:00:00 2001 From: Joey Baker Date: Mon, 5 Sep 2016 15:43:48 -0700 Subject: [PATCH 05/10] Chore (deps) update eslint (major) Breaking changes shouldn't affect us --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index cd8773c..9fa69b9 100644 --- a/package.json +++ b/package.json @@ -29,7 +29,7 @@ "babel-plugin-transform-decorators-legacy": "^1.3.4", "babel-plugin-transform-es2015-modules-commonjs": "^6.10.3", "babel-preset-stage-1": "^6.5.0", - "eslint": "2.10.2", + "eslint": "^3.4.0", "eslint-config-pichak": "1.1.0", "ghooks": "1.0.3", "scripty": "^1.6.0" From f2f613b35b4052a05d64628082c08a4ce780fcee Mon Sep 17 00:00:00 2001 From: Joey Baker Date: Sat, 3 Sep 2016 18:48:03 -0700 Subject: [PATCH 06/10] Fix: boom error on invalid `include` Sends a 501 `notImplemented` error when `parseInclude` can't find models to include. --- src/utils.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/utils.js b/src/utils.js index ac752e5..880cf8b 100644 --- a/src/utils.js +++ b/src/utils.js @@ -1,4 +1,5 @@ import { omit, identity } from 'lodash'; +import { notImplemented } from 'boom'; export const parseInclude = request => { const include = Array.isArray(request.query.include) ? request.query.include @@ -8,7 +9,7 @@ export const parseInclude = request => { const noRequestModels = !request.models; if (noGetDb && noRequestModels) { - return new Error('`request.getDb` or `request.models` are not defined.' + return notImplemented('`request.getDb` or `request.models` are not defined.' + 'Be sure to load hapi-sequelize before hapi-sequelize-crud.'); } From de0685c8bb9bf8c6be36e1447796b3d6f299bce9 Mon Sep 17 00:00:00 2001 From: Joey Baker Date: Mon, 5 Sep 2016 17:10:58 -0700 Subject: [PATCH 07/10] Chore: install and configure AVA --- .eslintrc | 8 +++++++- package.json | 11 ++++++++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/.eslintrc b/.eslintrc index cbc6290..c3abca1 100644 --- a/.eslintrc +++ b/.eslintrc @@ -1,3 +1,9 @@ { - "extends": "pichak" + "plugins": [ + "ava" + ], + "extends": [ + "pichak", + "plugin:ava/recommended" + ] } diff --git a/package.json b/package.json index 9fa69b9..160432e 100644 --- a/package.json +++ b/package.json @@ -9,8 +9,9 @@ } }, "scripts": { - "lint": "eslint src test", - "test": "echo \"Error: no test specified\" && exit 1", + "lint": "eslint src", + "test": "ava --require babel-register --source='*.test.js'", + "tdd": "ava --require babel-register --source='*.test.js' --watch", "build": "scripty", "watch": "scripty" }, @@ -23,6 +24,7 @@ "author": "Mahdi Dibaiee (http://dibaiee.ir/)", "license": "MIT", "devDependencies": { + "ava": "^0.16.0", "babel-cli": "^6.10.1", "babel-plugin-add-module-exports": "^0.2.1", "babel-plugin-closure-elimination": "^1.0.6", @@ -31,8 +33,11 @@ "babel-preset-stage-1": "^6.5.0", "eslint": "^3.4.0", "eslint-config-pichak": "1.1.0", + "eslint-plugin-ava": "^3.0.0", "ghooks": "1.0.3", - "scripty": "^1.6.0" + "scripty": "^1.6.0", + "sinon": "^1.17.5", + "sinon-bluebird": "^3.0.2" }, "dependencies": { "boom": "^3.2.2", From 7b757fcc50cc05c8b22de1b157673379597776f7 Mon Sep 17 00:00:00 2001 From: Joey Baker Date: Mon, 5 Sep 2016 17:11:42 -0700 Subject: [PATCH 08/10] Fix (crud) if no `prefix`, things still work --- src/crud.js | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/src/crud.js b/src/crud.js index 87fea03..2476dc1 100644 --- a/src/crud.js +++ b/src/crud.js @@ -1,4 +1,5 @@ import joi from 'joi'; +import path from 'path'; import error from './error'; import _ from 'lodash'; import { parseInclude, parseWhere } from './utils'; @@ -66,10 +67,10 @@ export default (server, model, { prefix, defaultConfig: config, models: permissi } }; -export const list = ({ server, model, prefix, config }) => { +export const list = ({ server, model, prefix = '/', config }) => { server.route({ method: 'GET', - path: `${prefix}/${model._plural}`, + path: path.join(prefix, model._plural), @error async handler(request, reply) { @@ -89,10 +90,10 @@ export const list = ({ server, model, prefix, config }) => { }); }; -export const get = ({ server, model, prefix, config }) => { +export const get = ({ server, model, prefix = '/', config }) => { server.route({ method: 'GET', - path: `${prefix}/${model._singular}/{id?}`, + path: path.join(prefix, model._singular, '{id?}'), @error async handler(request, reply) { @@ -117,12 +118,12 @@ export const get = ({ server, model, prefix, config }) => { }); }; -export const scope = ({ server, model, prefix, config }) => { +export const scope = ({ server, model, prefix = '/', config }) => { const scopes = Object.keys(model.options.scopes); server.route({ method: 'GET', - path: `${prefix}/${model._plural}/{scope}`, + path: path.join(prefix, model._plural, '{scope}'), @error async handler(request, reply) { @@ -143,10 +144,10 @@ export const scope = ({ server, model, prefix, config }) => { }); }; -export const create = ({ server, model, prefix, config }) => { +export const create = ({ server, model, prefix = '/', config }) => { server.route({ method: 'POST', - path: `${prefix}/${model._singular}`, + path: path.join(prefix, model._singular), @error async handler(request, reply) { @@ -159,10 +160,10 @@ export const create = ({ server, model, prefix, config }) => { }); }; -export const destroy = ({ server, model, prefix, config }) => { +export const destroy = ({ server, model, prefix = '/', config }) => { server.route({ method: 'DELETE', - path: `${prefix}/${model._singular}/{id?}`, + path: path.join(prefix, model._singular, '{id?}'), @error async handler(request, reply) { @@ -180,10 +181,10 @@ export const destroy = ({ server, model, prefix, config }) => { }); }; -export const destroyAll = ({ server, model, prefix, config }) => { +export const destroyAll = ({ server, model, prefix = '/', config }) => { server.route({ method: 'DELETE', - path: `${prefix}/${model._plural}`, + path: path.join(prefix, model._plural), @error async handler(request, reply) { @@ -200,12 +201,12 @@ export const destroyAll = ({ server, model, prefix, config }) => { }); }; -export const destroyScope = ({ server, model, prefix, config }) => { +export const destroyScope = ({ server, model, prefix = '/', config }) => { const scopes = Object.keys(model.options.scopes); server.route({ method: 'DELETE', - path: `${prefix}/${model._plural}/{scope}`, + path: path.join(prefix, model._plural, '{scope}'), @error async handler(request, reply) { @@ -228,10 +229,10 @@ export const destroyScope = ({ server, model, prefix, config }) => { }); }; -export const update = ({ server, model, prefix, config }) => { +export const update = ({ server, model, prefix = '/', config }) => { server.route({ method: 'PUT', - path: `${prefix}/${model._singular}/{id}`, + path: path.join(prefix, model._singular, '{id}'), @error async handler(request, reply) { From 7cecd7fb40a27e25d8d1f565c9a55913a75c132d Mon Sep 17 00:00:00 2001 From: Joey Baker Date: Mon, 5 Sep 2016 17:12:13 -0700 Subject: [PATCH 09/10] Test (list) add initial list tests --- src/crud.test.js | 146 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 src/crud.test.js diff --git a/src/crud.test.js b/src/crud.test.js new file mode 100644 index 0000000..26d002d --- /dev/null +++ b/src/crud.test.js @@ -0,0 +1,146 @@ +import test from 'ava'; +import { list } from './crud.js'; +import { stub } from 'sinon'; +import 'sinon-bluebird'; + +const METHODS = { + GET: 'GET', +}; + +test.beforeEach('setup server', (t) => { + t.context.server = { + route: stub(), + }; +}); + +test.beforeEach('setup model', (t) => { + t.context.model = { + findAll: stub(), + _plural: 'models', + _singular: 'model', + }; +}); + +test.beforeEach('setup request stub', (t) => { + t.context.request = { + query: {}, + payload: {}, + models: [t.context.model], + }; +}); + +test.beforeEach('setup reply stub', (t) => { + t.context.reply = stub(); +}); + +test('crud#list without prefix', (t) => { + const { server, model } = t.context; + + list({ server, model }); + const { path } = server.route.args[0][0]; + + t.falsy( + path.includes('undefined'), + 'correctly sets the path without a prefix defined', + ); + + t.is( + path, + `/${model._plural}`, + 'the path sets to the plural model' + ); +}); + +test('crud#list with prefix', (t) => { + const { server, model } = t.context; + const prefix = '/v1'; + + list({ server, model, prefix }); + const { path } = server.route.args[0][0]; + + t.is( + path, + `${prefix}/${model._plural}`, + 'the path sets to the plural model with the prefix' + ); +}); + +test('crud#list method', (t) => { + const { server, model } = t.context; + + list({ server, model }); + const { method } = server.route.args[0][0]; + + t.is( + method, + METHODS.GET, + `sets the method to ${METHODS.GET}` + ); +}); + +test('crud#list config', (t) => { + const { server, model } = t.context; + const userConfig = {}; + + list({ server, model, config: userConfig }); + const { config } = server.route.args[0][0]; + + t.is( + config, + userConfig, + 'sets the user config' + ); +}); + +test('crud#list handler', async (t) => { + const { server, model, request, reply } = t.context; + const allModels = [{ id: 1 }, { id: 2 }]; + + list({ server, model }); + const { handler } = server.route.args[0][0]; + model.findAll.resolves(allModels); + + try { + await handler(request, reply); + } catch (e) { + t.ifError(e, 'does not error while handling'); + } finally { + t.pass('does not error while handling'); + } + + t.truthy( + reply.calledOnce + , 'calls reply only once' + ); + + const response = reply.args[0][0]; + + t.is( + response, + allModels, + 'responds with the list of models' + ); +}); + +test('crud#list handler if parseInclude errors', async (t) => { + const { server, model, request, reply } = t.context; + // we _want_ the error + delete request.models; + + list({ server, model }); + const { handler } = server.route.args[0][0]; + + await handler(request, reply); + + t.truthy( + reply.calledOnce + , 'calls reply only once' + ); + + const response = reply.args[0][0]; + + t.truthy( + response.isBoom, + 'responds with a Boom error' + ); +}); From 03755f94c5224b4cc0653611c3f1b82c95c6a02b Mon Sep 17 00:00:00 2001 From: Joey Baker Date: Mon, 5 Sep 2016 17:21:30 -0700 Subject: [PATCH 10/10] Test (CI) configure circle --- circle.yml | 9 +++++++++ package.json | 5 +++-- 2 files changed, 12 insertions(+), 2 deletions(-) create mode 100644 circle.yml diff --git a/circle.yml b/circle.yml new file mode 100644 index 0000000..7112679 --- /dev/null +++ b/circle.yml @@ -0,0 +1,9 @@ +machine: + node: + version: 6.5.0 + +dependencies: + pre: + - npm prune + post: + - mkdir -p $CIRCLE_TEST_REPORTS/ava diff --git a/package.json b/package.json index 160432e..1e5c0a1 100644 --- a/package.json +++ b/package.json @@ -10,7 +10,7 @@ }, "scripts": { "lint": "eslint src", - "test": "ava --require babel-register --source='*.test.js'", + "test": "ava --require babel-register --source='*.test.js' --tap=${CI-false} | $(if [ -z ${CI:-} ]; then echo 'tail'; else tap-xunit > $CIRCLE_TEST_REPORTS/ava/ava.xml; fi;)", "tdd": "ava --require babel-register --source='*.test.js' --watch", "build": "scripty", "watch": "scripty" @@ -37,7 +37,8 @@ "ghooks": "1.0.3", "scripty": "^1.6.0", "sinon": "^1.17.5", - "sinon-bluebird": "^3.0.2" + "sinon-bluebird": "^3.0.2", + "tap-xunit": "^1.4.0" }, "dependencies": { "boom": "^3.2.2",