From 090f871f180c689ce2d7918b17fcb95789c98b3c Mon Sep 17 00:00:00 2001 From: Zachary Golba Date: Sat, 12 Nov 2016 13:29:11 -0500 Subject: [PATCH] fix: passing value null in post to type number throws type error (#516) * fix: passing value null in post to type number throws type error * test: add regression tests --- .../errors/parameter-not-nullable-error.js | 14 ++ .../route/params/parameter-group/index.js | 40 ++-- .../params/parameter/utils/validate-value.js | 5 + .../route/params/test/parameter-group.test.js | 5 + .../route/params/test/parameter.test.js | 178 ++++++++++++++++-- .../route/params/utils/validate-type.js | 50 ++--- 6 files changed, 231 insertions(+), 61 deletions(-) create mode 100644 src/packages/router/route/params/errors/parameter-not-nullable-error.js diff --git a/src/packages/router/route/params/errors/parameter-not-nullable-error.js b/src/packages/router/route/params/errors/parameter-not-nullable-error.js new file mode 100644 index 00000000..6aa0f06a --- /dev/null +++ b/src/packages/router/route/params/errors/parameter-not-nullable-error.js @@ -0,0 +1,14 @@ +// @flow +import { createServerError } from '../../../../server'; +import type { ParameterLike } from '../index'; + +/** + * @private + */ +class ParameterNotNullableError extends TypeError { + constructor({ path }: ParameterLike) { + super(`Parameter '${path}' is not nullable.`); + } +} + +export default createServerError(ParameterNotNullableError, 400); diff --git a/src/packages/router/route/params/parameter-group/index.js b/src/packages/router/route/params/parameter-group/index.js index f4378159..e31a8579 100644 --- a/src/packages/router/route/params/parameter-group/index.js +++ b/src/packages/router/route/params/parameter-group/index.js @@ -3,7 +3,6 @@ import { FreezeableMap } from '../../../../freezeable'; import { InvalidParameterError } from '../errors'; import isNull from '../../../../../utils/is-null'; import entries from '../../../../../utils/entries'; -import setType from '../../../../../utils/set-type'; import validateType from '../utils/validate-type'; import type { ParameterLike, ParameterLike$opts } from '../index'; @@ -39,34 +38,33 @@ class ParameterGroup extends FreezeableMap { } validate(params: V): V { - return setType(() => { - const validated = {}; + const validated = {}; - if (isNull(params)) { - return params; - } + if (isNull(params)) { + return params; + } - if (validateType(this, params) && hasRequiredParams(this, params)) { - const { sanitize } = this; - let { path } = this; + if (validateType(this, params) && hasRequiredParams(this, params)) { + const { sanitize } = this; + let { path } = this; - if (path.length) { - path = `${path}.`; - } + if (path.length) { + path = `${path}.`; + } - for (const [key, value] of entries(params)) { - const match = this.get(key); + for (const [key, value] of entries(params)) { + const match = this.get(key); - if (match) { - validated[key] = match.validate(value); - } else if (!match && !sanitize) { - throw new InvalidParameterError(`${path}${key}`); - } + if (match) { + Reflect.set(validated, key, match.validate(value)); + } else if (!match && !sanitize) { + throw new InvalidParameterError(`${path}${key}`); } } + } - return validated; - }); + // $FlowIgnore + return validated; } } diff --git a/src/packages/router/route/params/parameter/utils/validate-value.js b/src/packages/router/route/params/parameter/utils/validate-value.js index 914025f7..c3149cd6 100644 --- a/src/packages/router/route/params/parameter/utils/validate-value.js +++ b/src/packages/router/route/params/parameter/utils/validate-value.js @@ -1,4 +1,5 @@ // @flow +import isNull from '../../../../../../utils/is-null'; import { ParameterValueError, ResourceMismatchError } from '../../errors'; import type Parameter from '../index'; @@ -6,6 +7,10 @@ import type Parameter from '../index'; * @private */ function validateOne(param: Parameter, value: V): V { + if (!param.required && isNull(value)) { + return value; + } + if (!param.has(value)) { let expected; diff --git a/src/packages/router/route/params/test/parameter-group.test.js b/src/packages/router/route/params/test/parameter-group.test.js index a7312c2d..05a405f3 100644 --- a/src/packages/router/route/params/test/parameter-group.test.js +++ b/src/packages/router/route/params/test/parameter-group.test.js @@ -44,6 +44,11 @@ describe('module "router/route/params"', () => { }); describe('#validate()', () => { + it('returns null when then value is null', () => { + // $FlowIgnore + expect(subject.validate(null)).to.be.null; + }); + it('fails when required keys are missing', () => { expect(() => subject.validate({})).to.throw(TypeError); expect(() => subject.validate({ id: 1, meta: {} })).to.throw(TypeError); diff --git a/src/packages/router/route/params/test/parameter.test.js b/src/packages/router/route/params/test/parameter.test.js index b6cc1f9b..1b9875dd 100644 --- a/src/packages/router/route/params/test/parameter.test.js +++ b/src/packages/router/route/params/test/parameter.test.js @@ -1,35 +1,177 @@ // @flow import { expect } from 'chai'; -import { it, describe, before } from 'mocha'; +import { it, describe, beforeEach } from 'mocha'; import Parameter from '../parameter'; describe('module "router/route/params"', () => { describe('class Parameter', () => { - let subject: Parameter; + describe('#validate()', () => { + let subject: Parameter; + const primitives = [ + { + type: 'boolean', + valid: true, + falsy: false, + invalid: 'true' + }, + { + type: 'string', + valid: 'test', + falsy: '', + invalid: 1 + }, + { + type: 'number', + valid: 1, + falsy: 0, + invalid: '1' + } + ]; + + describe('- type "array"', () => { + [true, false].forEach(required => { + describe(`- ${required ? 'required' : 'optional'}`, () => { + beforeEach(() => { + subject = new Parameter({ + required, + type: 'array', + path: 'meta.test', + values: [1, 'test', false] + }); + }); + + if (required) { + it('fails when the value is null', () => { + expect(() => subject.validate(null)).to.throw(TypeError); + }); + } else { + it('passes when the value is null', () => { + expect(subject.validate(null)).to.be.null; + }); + } + + it('fails when there is a type mismatch', () => { + expect(() => subject.validate('test')).to.throw(TypeError); + }); + + it('fails when there is a value mismatch', () => { + expect(() => subject.validate([new Date()])).to.throw(TypeError); + }); - before(() => { - subject = new Parameter({ - type: 'array', - path: 'meta.test', - values: [1, 'test', false] + it('returns the value when the type and value match', () => { + expect(subject.validate(['test', false])).to.deep.equal([ + 'test', + false + ]); + }); + }); + }); }); - }); - describe('#validate()', () => { - it('fails when there is a type mismatch', () => { - expect(() => subject.validate('test')).to.throw(TypeError); + describe('- type "buffer"', () => { + [true, false].forEach(required => { + describe(`- ${required ? 'required' : 'optional'}`, () => { + let value; + + beforeEach(() => { + value = new Buffer('test', 'utf8'); + subject = new Parameter({ + required, + type: 'buffer', + path: 'meta.test' + }); + }); + + if (required) { + it('fails when the value is null', () => { + expect(() => subject.validate(null)).to.throw(TypeError); + }); + } else { + it('passes when the value is null', () => { + expect(subject.validate(null)).to.be.null; + }); + } + + it('fails when there is a type mismatch', () => { + expect(() => subject.validate('test')).to.throw(TypeError); + }); + + it('returns the value when the type and value match', () => { + expect(subject.validate(value)).to.equal(value); + }); + }); + }); }); - it('fails when there is a value mismatch', () => { - expect(() => subject.validate([new Date()])).to.throw(TypeError); + describe('- type "object"', () => { + [true, false].forEach(required => { + describe(`- ${required ? 'required' : 'optional'}`, () => { + let value; + + beforeEach(() => { + value = {}; + subject = new Parameter({ + required, + type: 'object', + path: 'meta.test' + }); + }); + + if (required) { + it('fails when the value is null', () => { + expect(() => subject.validate(null)).to.throw(TypeError); + }); + } else { + it('passes when the value is null', () => { + expect(subject.validate(null)).to.be.null; + }); + } + + it('fails when there is a type mismatch', () => { + expect(() => subject.validate('test')).to.throw(TypeError); + }); + + it('returns the value when the type and value match', () => { + expect(subject.validate(value)).to.equal(value); + }); + }); + }); }); - it('returns the value(s) when the type and value(s) match', () => { - expect(subject.validate(['test', false])).to.deep.equal([ - 'test', - false - ]); + primitives.forEach(({ type, valid, falsy, invalid }) => { + describe(`- type "${type}"`, () => { + [true, false].forEach(required => { + describe(`- ${required ? 'required' : 'optional'}`, () => { + beforeEach(() => { + subject = new Parameter({ + type, + required, + path: 'meta.test' + }); + }); + + if (required) { + it('fails when the value is null', () => { + expect(() => subject.validate(null)).to.throw(TypeError); + }); + } else { + it('passes when the value is null', () => { + expect(subject.validate(null)).to.be.null; + }); + } + + it('fails when there is a type mismatch', () => { + expect(() => subject.validate(invalid)).to.throw(TypeError); + }); + + it('returns the value when the type and value match', () => { + expect(subject.validate(valid)).to.equal(valid); + expect(subject.validate(falsy)).to.equal(falsy); + }); + }); + }); + }); }); }); }); diff --git a/src/packages/router/route/params/utils/validate-type.js b/src/packages/router/route/params/utils/validate-type.js index f15178c2..3227ca7c 100644 --- a/src/packages/router/route/params/utils/validate-type.js +++ b/src/packages/router/route/params/utils/validate-type.js @@ -1,5 +1,6 @@ // @flow -import { ParameterTypeError } from '../errors'; +import ParameterTypeError from '../errors/parameter-type-error'; +import ParameterNotNullableError from '../errors/parameter-not-nullable-error'; import isNull from '../../../../../utils/is-null'; import isObject from '../../../../../utils/is-object'; import isBuffer from '../../../../../utils/is-buffer'; @@ -12,33 +13,38 @@ export default function validateType( param: Parameter | ParameterGroup, value: mixed ): true { - const { type } = param; + const { type, required } = param; + const valueIsNull = isNull(value); - if (type) { - const valueType = typeof value; - let isValid; + if (required && valueIsNull) { + throw new ParameterNotNullableError(param); + } else if (valueIsNull || !type) { + return true; + } + + const valueType = typeof value; + let isValid = true; - switch (type) { - case 'array': - isValid = Array.isArray(value); - break; + switch (type) { + case 'array': + isValid = Array.isArray(value); + break; - case 'buffer': - isValid = isBuffer(value); - break; + case 'buffer': + isValid = isBuffer(value); + break; - case 'object': - isValid = isObject(value) || isNull(value); - break; + case 'object': + isValid = isObject(value) || isNull(value); + break; - default: - isValid = type === valueType; - } + default: + isValid = type === valueType; + } - if (!isValid) { - throw new ParameterTypeError(param, valueType); - } + if (!isValid) { + throw new ParameterTypeError(param, valueType); } - return true; + return isValid; }