fix: improve error mssage

loopbackio · Jun 7, 2018 · 05bb648 · 05bb648
1 parent a471fd4
commit 05bb648
Show file tree

Hide file tree

Showing 6 changed files with 47 additions and 30 deletions.
diff --git a/packages/rest/src/coercion/coerce-parameter.ts b/packages/rest/src/coercion/coerce-parameter.ts
@@ -7,6 +7,7 @@ import {ParameterObject, isReferenceObject} from '@loopback/openapi-v3-types';
 import {Validator} from './validator';
 import * as debugModule from 'debug';
 import {HttpErrors} from '..';
+import {HttpErrorMessage} from '../';
 
 const debug = debugModule('loopback:rest:coercion');
 
@@ -48,7 +49,10 @@ export function coerceParameter(data: string, spec: ParameterObject) {
     case 'number':
       coercedResult = data ? Number(data) : undefined;
       if (coercedResult === undefined) break;
-      if (isNaN(coercedResult)) throw new HttpErrors['400']();
+      if (isNaN(coercedResult))
+        throw new HttpErrors['400'](
+          HttpErrorMessage.INVALID_DATA(data, spec.name),
+        );
       break;
     case 'long':
       coercedResult = Number(data);

diff --git a/packages/rest/src/coercion/http-error-message.ts b/packages/rest/src/coercion/http-error-message.ts
@@ -0,0 +1,9 @@
+// tslint:disable:no-any
+export namespace HttpErrorMessage {
+  export const INVALID_DATA = (data: any, name: any) => {
+    return `Invalid data ${data} of parameter ${name}!`;
+  };
+  export const MISSING_REQUIRED = (name: any) => {
+    return `Required parameter ${name} is missing!`;
+  };
+}
diff --git a/packages/rest/src/coercion/validator.ts b/packages/rest/src/coercion/validator.ts
@@ -5,6 +5,7 @@
 
 import {ParameterObject} from '@loopback/openapi-v3-types';
 import * as HttpErrors from 'http-errors';
+import {HttpErrorMessage} from '../';
 
 /**
  * A set of options to pass into the validator functions
@@ -17,7 +18,7 @@ export type ValidationOptions = {
  * The context information that a validator needs
  */
 export type ValidationContext = {
-  parameterSpec?: ParameterObject;
+  parameterSpec: ParameterObject;
 };
 
 /**
@@ -41,7 +42,8 @@ export class Validator {
   ) {
     if (this.isAbsent(value)) {
       if (this.isRequired(opts)) {
-        throw new HttpErrors['400']();
+        const name = this.ctx.parameterSpec.name;
+        throw new HttpErrors['400'](HttpErrorMessage.MISSING_REQUIRED(name));
       } else {
         return;
       }
@@ -54,8 +56,7 @@ export class Validator {
    * @param opts
    */
   isRequired(opts?: ValidationOptions) {
-    if (this.ctx && this.ctx.parameterSpec && this.ctx.parameterSpec.required)
-      return true;
+    if (this.ctx.parameterSpec.required) return true;
     if (opts && opts.required) return true;
     return false;
   }

diff --git a/packages/rest/src/index.ts b/packages/rest/src/index.ts
@@ -39,3 +39,4 @@ export * from './rest.component';
 export * from './rest.server';
 export * from './sequence';
 export * from '@loopback/openapi-v3';
+export * from './coercion/http-error-message';
diff --git a/packages/rest/test/unit/coercion/paramStringToNumber.unit.ts b/packages/rest/test/unit/coercion/paramStringToNumber.unit.ts
@@ -3,9 +3,10 @@
 // This file is licensed under the MIT License.
 // License text available at https://opensource.org/licenses/MIT
 
-import {test, ERROR_BAD_REQUEST} from './utils';
+import {test} from './utils';
 import {ParameterLocation} from '@loopback/openapi-v3-types';
 import * as HttpErrors from 'http-errors';
+import {HttpErrorMessage} from './../../../';
 
 const NUMBER_PARAM = {
   in: <ParameterLocation>'path',
@@ -48,7 +49,14 @@ describe('coerce param from string to number - required', () => {
 
   context('empty values trigger ERROR_BAD_REQUEST', () => {
     // null, '' sent from request are converted to raw value ''
-    test<HttpErrors.HttpError>(REQUIRED_NUMBER_PARAM, '', ERROR_BAD_REQUEST);
+    const errorMsg = HttpErrorMessage.MISSING_REQUIRED(
+      REQUIRED_NUMBER_PARAM.name,
+    );
+    test<HttpErrors.HttpError>(
+      REQUIRED_NUMBER_PARAM,
+      '',
+      new HttpErrors['400'](errorMsg),
+    );
   });
 });
 
@@ -77,30 +85,25 @@ describe('coerce param from string to number - optional', () => {
   });
 
   context('All other non-number values trigger ERROR_BAD_REQUEST', () => {
+    let errorMsg = HttpErrorMessage.INVALID_DATA('text', NUMBER_PARAM.name);
     // 'false', false, 'true', true, 'text' sent from request are convert to a string
-    test<HttpErrors.HttpError>(NUMBER_PARAM, 'text', ERROR_BAD_REQUEST);
+    test<HttpErrors.HttpError>(
+      NUMBER_PARAM,
+      'text',
+      new HttpErrors['400'](errorMsg),
+    );
+
+    errorMsg = HttpErrorMessage.INVALID_DATA({a: true}, NUMBER_PARAM.name);
     // {a: true}, [1,2] are convert to object
-    test<HttpErrors.HttpError>(NUMBER_PARAM, {a: true}, ERROR_BAD_REQUEST);
+    test<HttpErrors.HttpError>(
+      NUMBER_PARAM,
+      {a: true},
+      new HttpErrors['400'](errorMsg),
+    );
   });
 });
 
 describe('OAI3 primitive types', () => {
   test<number>(FLOAT_PARAM, '3.333333', 3.333333);
   test<number>(DOUBLE_PARAM, '3.3333333333', 3.3333333333);
 });
-
-// Review notes: just add them for the purpose of review,
-// will remove it when merge code
-context('Number-like string values trigger ERROR_BAD_REQUEST', () => {
-  // Copied from strong-remoting
-  // these have 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],
-});
diff --git a/packages/rest/test/unit/coercion/utils.ts b/packages/rest/test/unit/coercion/utils.ts
@@ -16,8 +16,6 @@ import {
 } from '../../..';
 import * as HttpErrors from 'http-errors';
 
-export const ERROR_BAD_REQUEST = new HttpErrors['400']();
-
 function givenOperationWithParameters(params?: ParameterObject[]) {
   return <OperationObject>{
     'x-operation-name': 'testOp',
@@ -39,16 +37,16 @@ function givenResolvedRoute(
 }
 
 export interface TestArgs<T> {
-  expectedResult: T;
-  rawValue: string | undefined | object;
   paramSpec: ParameterObject;
+  rawValue: string | undefined | object;
+  expectedResult: T;
   caller: string;
   expectError: boolean;
+  opts: TestOptions;
 }
 
 export type TestOptions = {
   testName?: string;
-  expectError?: boolean;
 };
 
 export async function testCoercion<T>(config: TestArgs<T>) {
@@ -91,6 +89,7 @@ export function test<T>(
       expectedResult,
       caller,
       expectError: expectedResult instanceof HttpErrors.HttpError,
+      opts: opts || {},
     });
   });
 }