From c7b1fa54c04bb9144a58c53a8937e4785ec12770 Mon Sep 17 00:00:00 2001 From: jannyHou <juehou@ca.ibm.com> Date: Wed, 6 Jun 2018 00:49:28 -0400 Subject: [PATCH] fix: add validator --- .../rest/src/coercion/coerce-parameter.ts | 7 +- packages/rest/src/coercion/validator.ts | 78 +++++++++++++++ .../unit/coercion/paramStringToNumber.unit.ts | 97 +++++++++++++++++-- packages/rest/test/unit/coercion/utils.ts | 19 +++- 4 files changed, 191 insertions(+), 10 deletions(-) create mode 100644 packages/rest/src/coercion/validator.ts diff --git a/packages/rest/src/coercion/coerce-parameter.ts b/packages/rest/src/coercion/coerce-parameter.ts index 7c628e023e38..af53ee402841 100644 --- a/packages/rest/src/coercion/coerce-parameter.ts +++ b/packages/rest/src/coercion/coerce-parameter.ts @@ -8,6 +8,7 @@ import { ReferenceObject, isReferenceObject, } from '@loopback/openapi-v3-types'; +import {Validator} from './validator'; import * as HttpErrors from 'http-errors'; import * as debugModule from 'debug'; @@ -35,8 +36,8 @@ export function coerceParameter( let coercedResult; coercedResult = data; const OAIType = getOAIPrimitiveType(schema.type, schema.format); + const validator = new Validator({schema}); - // Review Note: [Validation place 1] Validation rules can be applied for each case switch (OAIType) { case 'byte': coercedResult = Buffer.from(data, 'base64'); @@ -49,6 +50,10 @@ export function coerceParameter( coercedResult = parseFloat(data); break; case 'number': + validator.validateParamBeforeCoercion('number', data); + coercedResult = data ? Number(data) : undefined; + validator.validateParamAfterCoercion('number', coercedResult); + break; case 'long': coercedResult = Number(data); break; diff --git a/packages/rest/src/coercion/validator.ts b/packages/rest/src/coercion/validator.ts new file mode 100644 index 000000000000..8a204b81c2d9 --- /dev/null +++ b/packages/rest/src/coercion/validator.ts @@ -0,0 +1,78 @@ +// Copyright IBM Corp. 2018. All Rights Reserved. +// Node module: @loopback/rest +// This file is licensed under the MIT License. +// License text available at https://opensource.org/licenses/MIT + +import { + SchemaObject, + ReferenceObject, + isReferenceObject, +} from '@loopback/openapi-v3-types'; +import * as HttpErrors from 'http-errors'; +import * as debugModule from 'debug'; + +const debug = debugModule('loopback:rest:coercion'); + +export type ValidationOptions = { + required?: boolean; +}; +export type ValidationContext = { + schema: SchemaObject; +}; + +export class Validator { + constructor(public ctx: ValidationContext) {} + + validateParamBeforeCoercion( + type: string, + value: any, + opts?: ValidationOptions, + ) { + if (this.isAbsent(value)) { + if (this.isRequired(opts)) { + throw new HttpErrors['400'](); + } else { + return; + } + } + } + + /** + * Check is a parameter required or not + * @param opts + */ + isRequired(opts?: ValidationOptions) { + if (this.ctx && this.ctx.schema && this.ctx.schema.required) return true; + if (opts && opts.required) return true; + return false; + } + + validateParamAfterCoercion( + type: string, + value: any, + opts?: ValidationOptions, + ) { + switch (type) { + case 'number': + this.validateNumber(value); + break; + //@jannyhou: other types TBD + default: + return; + } + } + + /** + * Return `true` if the value is empty, return `false` otherwise + * @param value + */ + isAbsent(value: any) { + const isEmptySet = ['']; + return isEmptySet.includes(value); + } + + validateNumber(value: any) { + if (value === undefined) return; + if (isNaN(value)) throw new HttpErrors['400'](); + } +} diff --git a/packages/rest/test/unit/coercion/paramStringToNumber.unit.ts b/packages/rest/test/unit/coercion/paramStringToNumber.unit.ts index 97a1352166ae..2184dd03a656 100644 --- a/packages/rest/test/unit/coercion/paramStringToNumber.unit.ts +++ b/packages/rest/test/unit/coercion/paramStringToNumber.unit.ts @@ -3,14 +3,97 @@ // This file is licensed under the MIT License. // License text available at https://opensource.org/licenses/MIT -import {runTests} from './utils'; +import {runTests, ERROR_BAD_REQUEST} from './utils'; -describe('coerce param from string to number', () => { - /*tslint:disable:max-line-length*/ +const NUMBER_SCHEMA = {type: 'number'}; +const NUMBER_SCHEMA_REQUIRED = {type: 'number', required: true}; +const FLOAT_SCHEMA = {type: 'number', float: 'float'}; +const DOUBLE_SCHEMA = {type: 'number', format: 'double'}; + +/*tslint:disable:max-line-length*/ +describe('coerce param from string to number - required', () => { + context('valid values', () => { + const tests = [ + ['0', NUMBER_SCHEMA_REQUIRED, '0', 0, new Error().stack], + ['1', NUMBER_SCHEMA_REQUIRED, '1', 1, new Error().stack], + ['-1', NUMBER_SCHEMA_REQUIRED, '-1', -1, new Error().stack], + ]; + runTests(tests); + }) + + context('empty values trigger ERROR_BAD_REQUEST', () => { + const tests = [ + // null, '' + ['empty string', NUMBER_SCHEMA_REQUIRED, '', ERROR_BAD_REQUEST, new Error().stack, true], + ]; + runTests(tests); + }) +}); + +describe('coerce param from string to number - optional', () => { + context('valid values', () => { + const tests = [ + ['0', NUMBER_SCHEMA, '0', 0, new Error().stack], + ['1', NUMBER_SCHEMA, '1', 1, new Error().stack], + ['-1', NUMBER_SCHEMA, '-1', -1, new Error().stack], + ['1.2', NUMBER_SCHEMA, '1.2', 1.2, new Error().stack], + ['-1.2', NUMBER_SCHEMA, '-1.2', -1.2, new Error().stack], + ]; + runTests(tests); + }) + + context('numbers larger than MAX_SAFE_INTEGER get trimmed', () => { + const tests = [ + ['positive large number', NUMBER_SCHEMA, '2343546576878989879789', 2.34354657687899e+21, new Error().stack], + ['negative large number', NUMBER_SCHEMA, '-2343546576878989879789', -2.34354657687899e+21, new Error().stack], + ]; + runTests(tests); + }) + + context('scientific notations', () => { + const tests = [ + ['positive number', NUMBER_SCHEMA, '1.234e+30', 1.234e+30, new Error().stack], + ['negative number', NUMBER_SCHEMA, '-1.234e+30', -1.234e+30, new Error().stack], + ]; + runTests(tests); + }) + + context('empty value converts to undefined', () => { + const tests = [ + // [], {} are converted to undefined + ['empty value as undefined', NUMBER_SCHEMA, undefined, undefined, new Error().stack] + ] + runTests(tests); + }) + + context('All other non-number values trigger ERROR_BAD_REQUEST', () => { + const tests = [ + // 'false', false, 'true', true, 'text', null, '' are convert to a string + ['invalid value as string', NUMBER_SCHEMA, 'text', ERROR_BAD_REQUEST, new Error().stack, true], + // {a: true}, [1,2] are convert to object + ['invalid value as object', NUMBER_SCHEMA, {a: true}, ERROR_BAD_REQUEST, new Error().stack, true], + ] + runTests(tests); + }) +}); + +describe('OAI3 primitive types', () => { const testCases = [ - ['float', {type: 'number', format: 'float'}, '3.333333', 3.333333, new Error().stack], - ['double', {type: 'number', format: 'double'}, '3.3333333333', 3.3333333333, new Error().stack], + ['float', FLOAT_SCHEMA, '3.333333', 3.333333, new Error().stack], + ['double', DOUBLE_SCHEMA, '3.3333333333', 3.3333333333, new Error().stack], ]; - runTests(testCases); -}); +}) + +context('Number-like string values trigger ERROR_BAD_REQUEST', () => { + // this has to be in serialization acceptance tests + // [{arg: '0'}, ERROR_BAD_REQUEST], + // [{arg: '1'}, ERROR_BAD_REQUEST], + // [{arg: '-1'}, ERROR_BAD_REQUEST], + // [{arg: '1.2'}, ERROR_BAD_REQUEST], + // [{arg: '-1.2'}, ERROR_BAD_REQUEST], + // [{arg: '2343546576878989879789'}, ERROR_BAD_REQUEST], + // [{arg: '-2343546576878989879789'}, ERROR_BAD_REQUEST], + // [{arg: '1.234e+30'}, ERROR_BAD_REQUEST], + // [{arg: '-1.234e+30'}, ERROR_BAD_REQUEST], +}) \ No newline at end of file diff --git a/packages/rest/test/unit/coercion/utils.ts b/packages/rest/test/unit/coercion/utils.ts index 14b2141401c7..6f04d85a925c 100644 --- a/packages/rest/test/unit/coercion/utils.ts +++ b/packages/rest/test/unit/coercion/utils.ts @@ -18,6 +18,7 @@ import { parseOperationArgs, ResolvedRoute, } from '../../..'; +import * as HttpErrors from 'http-errors'; export function givenOperationWithParameters(params?: ParameterObject[]) { return <OperationObject>{ @@ -37,6 +38,7 @@ export interface TestArgs<T> { schema?: SchemaObject; specConfig?: Partial<ParameterObject>; caller: string; + expectError: boolean; } export function givenResolvedRoute( @@ -58,8 +60,18 @@ export async function testCoercion<T>(config: TestArgs<T>) { }, ]); const route = givenResolvedRoute(spec, {aparameter: config.valueFromReq}); - const args = await parseOperationArgs(req, route); - expect(args).to.eql([config.expectedResult]); + + if (config.expectError) { + try { + await parseOperationArgs(req, route); + throw new Error("'parseOperationArgs' should throw error!"); + } catch (err) { + expect(err).to.eql(config.expectedResult); + } + } else { + const args = await parseOperationArgs(req, route); + expect(args).to.eql([config.expectedResult]); + } } catch (err) { throw new Error(`${err} \n Failed ${config.caller.split(/\n/)[1]}`); } @@ -74,7 +86,10 @@ export function runTests(tests: any[][]) { valueFromReq: t[2] as string, expectedResult: t[3], caller: t[4] as string, + expectError: t.length > 5 ? t[5] : false, }); }); } } + +export const ERROR_BAD_REQUEST = new HttpErrors['400']();