From c59943a71754e00d6cf83dc4d42eda6427416841 Mon Sep 17 00:00:00 2001 From: Joey Baker Date: Wed, 7 Sep 2016 18:09:48 -0700 Subject: [PATCH 1/2] Fix (validation) use joi's concat It turns out defaultsDeep doesn't ever correctly combine Joi objects. So, the only option is to use Joi's concat method to combine Joi schemas. This complicates `getConfigForMethod`, but simplifies actual route creation. I ran into this because I'm setting up [lout](https://github.com/hapijs/lout) on a server, and it requires properly formatted Joi schemas. This leads me to believe there was something already wrong and Lout just exposed the problem. --- src/crud.js | 45 +--- src/get-config-for-method.js | 99 ++++++-- src/get-config-for-method.test.js | 407 ++++++++++++++++++++++++++++-- 3 files changed, 473 insertions(+), 78 deletions(-) diff --git a/src/crud.js b/src/crud.js index 64e44b7..2d527b2 100644 --- a/src/crud.js +++ b/src/crud.js @@ -14,6 +14,7 @@ const createAll = ({ config, attributeValidation, associationValidation, + scopes, }) => { Object.keys(methods).forEach((method) => { methods[method]({ @@ -25,6 +26,7 @@ const createAll = ({ attributeValidation, associationValidation, config, + scopes, }), }); }); @@ -64,6 +66,8 @@ export default (server, model, { prefix, defaultConfig: config, models: permissi include: joi.array().items(joi.string().valid(...modelAssociations)), }; + const scopes = Object.keys(model.options.scopes); + // if we don't have any permissions set, just create all the methods if (!permissions) { createAll({ @@ -73,6 +77,7 @@ export default (server, model, { prefix, defaultConfig: config, models: permissi config, attributeValidation, associationValidation, + scopes, }); // if permissions are set, but we can't parse them, throw an error } else if (!Array.isArray(permissions)) { @@ -87,6 +92,7 @@ export default (server, model, { prefix, defaultConfig: config, models: permissi config, attributeValidation, associationValidation, + scopes, }); // if we've gotten here, we have complex permissions and need to set them } else { @@ -108,6 +114,7 @@ export default (server, model, { prefix, defaultConfig: config, models: permissi method, attributeValidation, associationValidation, + scopes, config: permissionConfig, }), }); @@ -119,6 +126,7 @@ export default (server, model, { prefix, defaultConfig: config, models: permissi prefix, attributeValidation, associationValidation, + scopes, config: permissionConfig, }); } @@ -170,19 +178,11 @@ export const get = ({ server, model, prefix = '/', config }) => { reply(instance); }, - config: _.defaultsDeep(config, { - validate: { - params: { - id: joi.any(), - }, - }, - }), + config, }); }; export const scope = ({ server, model, prefix = '/', config }) => { - const scopes = Object.keys(model.options.scopes); - server.route({ method: 'GET', path: path.join(prefix, model._plural, '{scope}'), @@ -198,13 +198,7 @@ export const scope = ({ server, model, prefix = '/', config }) => { reply(list); }, - config: _.defaultsDeep(config, { - validate: { - params: { - scope: joi.string().valid(...scopes), - }, - }, - }), + config, }); }; @@ -266,8 +260,6 @@ export const destroyAll = ({ server, model, prefix = '/', config }) => { }; export const destroyScope = ({ server, model, prefix = '/', config }) => { - const scopes = Object.keys(model.options.scopes); - server.route({ method: 'DELETE', path: path.join(prefix, model._plural, '{scope}'), @@ -285,13 +277,7 @@ export const destroyScope = ({ server, model, prefix = '/', config }) => { reply(list); }, - config: _.defaultsDeep(config, { - validate: { - params: { - scope: joi.string().valid(...scopes), - }, - }, - }), + config, }); }; @@ -312,14 +298,7 @@ export const update = ({ server, model, prefix = '/', config }) => { reply(instance); }, - config: _.defaultsDeep(config, { - validate: { - payload: joi.object().required(), - params: { - id: joi.any(), - }, - }, - }), + config, }); }; diff --git a/src/get-config-for-method.js b/src/get-config-for-method.js index de8c828..c9a318a 100644 --- a/src/get-config-for-method.js +++ b/src/get-config-for-method.js @@ -1,6 +1,15 @@ -import { defaultsDeep } from 'lodash'; +import { set, get } from 'lodash'; import joi from 'joi'; +// if the custom validation is a joi object we need to concat +// else, assume it's an plain object and we can just add it in with .keys +const concatToJoiObject = (joi, candidate) => { + if (!candidate) return joi; + else if (candidate.isJoi) return joi.concat(candidate); + else return joi.keys(candidate); +}; + + export const sequelizeOperators = { $and: joi.any(), $or: joi.any(), @@ -47,41 +56,81 @@ export const payloadMethods = [ 'update', ]; -export default ({ method, attributeValidation, associationValidation, config = {} }) => { +export const scopeParamsMethods = [ + 'destroyScope', + 'scope', +]; + +export const idParamsMethods = [ + 'get', + 'update', +]; + +export default ({ + method, attributeValidation, associationValidation, scopes = [], config = {}, +}) => { const hasWhere = whereMethods.includes(method); const hasInclude = includeMethods.includes(method); const hasPayload = payloadMethods.includes(method); - const methodConfig = { ...config }; + const hasScopeParams = scopeParamsMethods.includes(method); + const hasIdParams = idParamsMethods.includes(method); + // clone the config so we don't modify it on multiple passes. + let methodConfig = { ...config, validate: { ...config.validate } }; if (hasWhere) { - defaultsDeep(methodConfig, { - validate: { - query: { - ...attributeValidation, - ...sequelizeOperators, - }, - }, - }); + const query = concatToJoiObject(joi.object() + .keys({ + ...attributeValidation, + ...sequelizeOperators, + }), + get(methodConfig, 'validate.query') + ); + + methodConfig = set(methodConfig, 'validate.query', query); } if (hasInclude) { - defaultsDeep(methodConfig, { - validate: { - query: { - ...associationValidation, - }, - }, - }); + const query = concatToJoiObject(joi.object() + .keys({ + ...associationValidation, + }), + get(methodConfig, 'validate.query') + ); + + methodConfig = set(methodConfig, 'validate.query', query); } if (hasPayload) { - defaultsDeep(methodConfig, { - validate: { - payload: { - ...attributeValidation, - }, - }, - }); + const payload = concatToJoiObject(joi.object() + .keys({ + ...attributeValidation, + }), + get(methodConfig, 'validate.payload') + ); + + methodConfig = set(methodConfig, 'validate.payload', payload); + } + + if (hasScopeParams) { + const params = concatToJoiObject(joi.object() + .keys({ + scope: joi.string().valid(...scopes), + }), + get(methodConfig, 'validate.params') + ); + + methodConfig = set(methodConfig, 'validate.params', params); + } + + if (hasIdParams) { + const params = concatToJoiObject(joi.object() + .keys({ + id: joi.any(), + }), + get(methodConfig, 'validate.params') + ); + + methodConfig = set(methodConfig, 'validate.params', params); } return methodConfig; diff --git a/src/get-config-for-method.test.js b/src/get-config-for-method.test.js index 8a9512f..284ddeb 100644 --- a/src/get-config-for-method.test.js +++ b/src/get-config-for-method.test.js @@ -5,16 +5,22 @@ import whereMethods, includeMethods, payloadMethods, + scopeParamsMethods, + idParamsMethods, sequelizeOperators, } from './get-config-for-method.js'; test.beforeEach((t) => { + t.context.models = ['MyModel']; + + t.context.scopes = ['aScope']; + t.context.attributeValidation = { myKey: joi.any(), }; t.context.associationValidation = { - include: ['MyModel'], + include: joi.array().items(joi.string().valid(t.context.models)), }; t.context.config = { @@ -22,11 +28,10 @@ test.beforeEach((t) => { }; }); -test('get-config-for-method validate.query seqeulizeOperators', (t) => { +test('validate.query seqeulizeOperators', (t) => { whereMethods.forEach((method) => { const configForMethod = getConfigForMethod({ method }); const { query } = configForMethod.validate; - const configForMethodValidateQueryKeys = Object.keys(query); t.truthy( query, @@ -34,15 +39,20 @@ test('get-config-for-method validate.query seqeulizeOperators', (t) => { ); Object.keys(sequelizeOperators).forEach((operator) => { - t.truthy( - configForMethodValidateQueryKeys.includes(operator), - `applies sequelize operator "${operator}" in validate.where for ${method}` + t.ifError( + query.validate({ [operator]: true }).error + , `applies sequelize operator "${operator}" in validate.where for ${method}` ); }); + + t.truthy( + query.validate({ notAThing: true }).error + , 'errors on a non-valid key' + ); }); }); -test('get-config-for-method validate.query attributeValidation', (t) => { +test('validate.query attributeValidation', (t) => { const { attributeValidation } = t.context; whereMethods.forEach((method) => { @@ -50,16 +60,96 @@ test('get-config-for-method validate.query attributeValidation', (t) => { const { query } = configForMethod.validate; Object.keys(attributeValidation).forEach((key) => { - t.truthy( - query[key] + t.ifError( + query.validate({ [key]: true }).error , `applies attributeValidation (${key}) to validate.query` ); }); + + t.truthy( + query.validate({ notAThing: true }).error + , 'errors on a non-valid key' + ); }); }); -test('get-config-for-method validate.query associationValidation', (t) => { - const { attributeValidation, associationValidation } = t.context; +test('query attributeValidation w/ config as plain object', (t) => { + const { attributeValidation } = t.context; + const config = { + validate: { + query: { + aKey: joi.boolean(), + }, + }, + }; + + whereMethods.forEach((method) => { + const configForMethod = getConfigForMethod({ + method, + attributeValidation, + config, + }); + const { query } = configForMethod.validate; + + const keys = [ + ...Object.keys(attributeValidation), + ...Object.keys(config.validate.query), + ]; + + keys.forEach((key) => { + t.ifError( + query.validate({ [key]: true }).error + , `applies ${key} to validate.query` + ); + }); + + t.truthy( + query.validate({ notAThing: true }).error + , 'errors on a non-valid key' + ); + }); +}); + +test('query attributeValidation w/ config as joi object', (t) => { + const { attributeValidation } = t.context; + const queryKeys = { + aKey: joi.boolean(), + }; + const config = { + validate: { + query: joi.object().keys(queryKeys), + }, + }; + + whereMethods.forEach((method) => { + const configForMethod = getConfigForMethod({ + method, + attributeValidation, + config, + }); + const { query } = configForMethod.validate; + + const keys = [ + ...Object.keys(attributeValidation), + ...Object.keys(queryKeys), + ]; + + keys.forEach((key) => { + t.ifError( + query.validate({ [key]: true }).error + , `applies ${key} to validate.query` + ); + }); + + t.truthy( + query.validate({ notAThing: true }).error + , 'errors on a non-valid key' + ); + }); +}); + +test('validate.query associationValidation', (t) => { + const { attributeValidation, associationValidation, models } = t.context; includeMethods.forEach((method) => { const configForMethod = getConfigForMethod({ @@ -70,22 +160,106 @@ test('get-config-for-method validate.query associationValidation', (t) => { const { query } = configForMethod.validate; Object.keys(attributeValidation).forEach((key) => { - t.truthy( - query[key] + t.ifError( + query.validate({ [key]: true }).error , `applies attributeValidation (${key}) to validate.query when include should be applied` ); }); Object.keys(associationValidation).forEach((key) => { - t.truthy( - query[key] + t.ifError( + query.validate({ [key]: models }).error , `applies associationValidation (${key}) to validate.query when include should be applied` ); }); + + t.truthy( + query.validate({ notAThing: true }).error + , 'errors on a non-valid key' + ); }); }); -test('get-config-for-method validate.payload associationValidation', (t) => { +test('query associationValidation w/ config as plain object', (t) => { + const { associationValidation, models } = t.context; + const config = { + validate: { + query: { + aKey: joi.boolean(), + }, + }, + }; + + includeMethods.forEach((method) => { + const configForMethod = getConfigForMethod({ + method, + associationValidation, + config, + }); + const { query } = configForMethod.validate; + + Object.keys(associationValidation).forEach((key) => { + t.ifError( + query.validate({ [key]: models }).error + , `applies ${key} to validate.query` + ); + }); + + Object.keys(config.validate.query).forEach((key) => { + t.ifError( + query.validate({ [key]: true }).error + , `applies ${key} to validate.query` + ); + }); + + t.truthy( + query.validate({ notAThing: true }).error + , 'errors on a non-valid key' + ); + }); +}); + +test('query associationValidation w/ config as joi object', (t) => { + const { associationValidation, models } = t.context; + const queryKeys = { + aKey: joi.boolean(), + }; + const config = { + validate: { + query: joi.object().keys(queryKeys), + }, + }; + + includeMethods.forEach((method) => { + const configForMethod = getConfigForMethod({ + method, + associationValidation, + config, + }); + const { query } = configForMethod.validate; + + Object.keys(associationValidation).forEach((key) => { + t.ifError( + query.validate({ [key]: models }).error + , `applies ${key} to validate.query` + ); + }); + + Object.keys(queryKeys).forEach((key) => { + t.ifError( + query.validate({ [key]: true }).error + , `applies ${key} to validate.query` + ); + }); + + t.truthy( + query.validate({ notAThing: true }).error + , 'errors on a non-valid key' + ); + }); +}); + +test('validate.payload associationValidation', (t) => { const { attributeValidation } = t.context; payloadMethods.forEach((method) => { @@ -93,20 +267,213 @@ test('get-config-for-method validate.payload associationValidation', (t) => { const { payload } = configForMethod.validate; Object.keys(attributeValidation).forEach((key) => { - t.truthy( - payload[key] + t.ifError( + payload.validate({ [key]: true }).error , `applies attributeValidation (${key}) to validate.payload` ); }); + + t.truthy( + payload.validate({ notAThing: true }).error + , 'errors on a non-valid key' + ); }); }); -test('get-config-for-method does not modify initial config on multiple passes', (t) => { +test('payload attributeValidation w/ config as plain object', (t) => { + const { attributeValidation } = t.context; + const config = { + validate: { + payload: { + aKey: joi.boolean(), + }, + }, + }; + + payloadMethods.forEach((method) => { + const configForMethod = getConfigForMethod({ + method, + attributeValidation, + config, + }); + const { payload } = configForMethod.validate; + + const keys = [ + ...Object.keys(attributeValidation), + ...Object.keys(config.validate.payload), + ]; + + keys.forEach((key) => { + t.ifError( + payload.validate({ [key]: true }).error + , `applies ${key} to validate.payload` + ); + }); + + t.truthy( + payload.validate({ notAThing: true }).error + , 'errors on a non-valid key' + ); + }); +}); + +test('payload attributeValidation w/ config as joi object', (t) => { + const { attributeValidation } = t.context; + const payloadKeys = { + aKey: joi.boolean(), + }; + const config = { + validate: { + payload: joi.object().keys(payloadKeys), + }, + }; + + payloadMethods.forEach((method) => { + const configForMethod = getConfigForMethod({ + method, + attributeValidation, + config, + }); + const { payload } = configForMethod.validate; + + const keys = [ + ...Object.keys(attributeValidation), + ...Object.keys(payloadKeys), + ]; + + keys.forEach((key) => { + t.ifError( + payload.validate({ [key]: true }).error + , `applies ${key} to validate.payload` + ); + }); + + t.truthy( + payload.validate({ notAThing: true }).error + , 'errors on a non-valid key' + ); + }); +}); + +test('validate.params scopeParamsMethods', (t) => { + const { scopes } = t.context; + + scopeParamsMethods.forEach((method) => { + const configForMethod = getConfigForMethod({ method, scopes }); + const { params } = configForMethod.validate; + + scopes.forEach((key) => { + t.ifError( + params.validate({ scope: key }).error + , `applies "scope: ${key}" to validate.params` + ); + }); + + t.truthy( + params.validate({ scope: 'notAthing' }).error + , 'errors on a non-valid key' + ); + }); +}); + +test('params scopeParamsMethods w/ config as plain object', (t) => { + const { scopes } = t.context; + const config = { + validate: { + params: { + aKey: joi.boolean(), + }, + }, + }; + + scopeParamsMethods.forEach((method) => { + const configForMethod = getConfigForMethod({ + method, + scopes, + config, + }); + const { params } = configForMethod.validate; + + scopes.forEach((key) => { + t.ifError( + params.validate({ scope: key }).error + , `applies "scope: ${key}" to validate.params` + ); + }); + + Object.keys(config.validate.params).forEach((key) => { + t.ifError( + params.validate({ [key]: true }).error + , `applies ${key} to validate.params` + ); + }); + + t.truthy( + params.validate({ notAThing: true }).error + , 'errors on a non-valid key' + ); + }); +}); + +test('params scopeParamsMethods w/ config as joi object', (t) => { + const { scopes } = t.context; + const paramsKeys = { + aKey: joi.boolean(), + }; + const config = { + validate: { + params: joi.object().keys(paramsKeys), + }, + }; + + scopeParamsMethods.forEach((method) => { + const configForMethod = getConfigForMethod({ + method, + scopes, + config, + }); + const { params } = configForMethod.validate; + + scopes.forEach((key) => { + t.ifError( + params.validate({ scope: key }).error + , `applies "scope: ${key}" to validate.params` + ); + }); + + Object.keys(paramsKeys).forEach((key) => { + t.ifError( + params.validate({ [key]: true }).error + , `applies ${key} to validate.params` + ); + }); + + t.truthy( + params.validate({ notAThing: true }).error + , 'errors on a non-valid key' + ); + }); +}); + + +test('validate.payload idParamsMethods', (t) => { + idParamsMethods.forEach((method) => { + const configForMethod = getConfigForMethod({ method }); + const { params } = configForMethod.validate; + + t.ifError( + params.validate({ id: 'aThing' }).error + , 'applies id to validate.params' + ); + }); +}); + +test('does not modify initial config on multiple passes', (t) => { const { config } = t.context; const originalConfig = { ...config }; whereMethods.forEach((method) => { - getConfigForMethod({ method, config }); + getConfigForMethod({ method, ...t.context }); }); t.deepEqual( From 8ee56612520c0a6a143ff05309c1ab8ea3e54f0a Mon Sep 17 00:00:00 2001 From: Joey Baker Date: Thu, 8 Sep 2016 13:21:09 -0700 Subject: [PATCH 2/2] Test: only run src test files #oops --- package.json | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index be32ea5..d0f18f6 100644 --- a/package.json +++ b/package.json @@ -10,8 +10,8 @@ }, "scripts": { "lint": "eslint src", - "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", + "test": "ava --require babel-register --source='src/**/*.js' --source='!build/**/*' --tap=${CI-false} src/**/*.test.js | $(if [ -z ${CI:-} ]; then echo 'tail'; else tap-xunit > $CIRCLE_TEST_REPORTS/ava/ava.xml; fi;)", + "tdd": "ava --require babel-register --source='src/**/*.js' --source='!build/**/*' --watch src/**/*.test.js", "build": "scripty", "watch": "scripty" }, @@ -31,6 +31,7 @@ "babel-plugin-transform-decorators-legacy": "^1.3.4", "babel-plugin-transform-es2015-modules-commonjs": "^6.14.0", "babel-preset-stage-1": "^6.13.0", + "babel-register": "^6.14.0", "eslint": "^3.4.0", "eslint-config-pichak": "^1.1.2", "eslint-plugin-ava": "^3.0.0",