feat: body validation

jannyHou · bajtos · Janny · commit d284ad8ce707 · 2018-07-05T12:25:05.000-04:00
Co-authored-by: Miroslav Bajtos &lt;mbajtoss@gmail.com&gt;
diff --git a/examples/todo/src/controllers/todo.controller.ts b/examples/todo/src/controllers/todo.controller.ts
@@ -5,16 +5,7 @@
 
 import {inject} from '@loopback/core';
 import {repository} from '@loopback/repository';
-import {
-  HttpErrors,
-  del,
-  get,
-  param,
-  patch,
-  post,
-  put,
-  requestBody,
-} from '@loopback/rest';
+import {del, get, param, patch, post, put, requestBody} from '@loopback/rest';
 import {Todo} from '../models';
 import {TodoRepository} from '../repositories';
 import {GeocoderService} from '../services';
@@ -27,12 +18,6 @@ export class TodoController {
 
   @post('/todos')
   async createTodo(@requestBody() todo: Todo) {
-    // TODO(bajtos) This should be handled by the framework
-    // See https://github.com/strongloop/loopback-next/issues/118
-    if (!todo.title) {
-      throw new HttpErrors.BadRequest('title is required');
-    }
-
     if (todo.remindAtAddress) {
       // TODO(bajtos) handle "address not found"
       const geo = await this.geoService.geocode(todo.remindAtAddress);
diff --git a/examples/todo/test/acceptance/application.acceptance.ts b/examples/todo/test/acceptance/application.acceptance.ts
@@ -47,6 +47,15 @@ describe('Application', () => {
     expect(result).to.containDeep(todo);
   });
 
+  it('rejects requests to create a todo with no title', async () => {
+    const todo = givenTodo();
+    delete todo.title;
+    await client
+      .post('/todos')
+      .send(todo)
+      .expect(422);
+  });
+
   it('creates an address-based reminder', async function() {
     // Increase the timeout to accommodate slow network connections
     // tslint:disable-next-line:no-invalid-this
diff --git a/examples/todo/test/unit/controllers/todo.controller.unit.ts b/examples/todo/test/unit/controllers/todo.controller.unit.ts
@@ -47,8 +47,6 @@ describe('TodoController', () => {
   let aChangedTodo: Todo;
   let aTodoList: Todo[];
 
-  const noError = 'No error was thrown!';
-
   beforeEach(resetRepositories);
 
   describe('createTodo', () => {
@@ -59,20 +57,6 @@ describe('TodoController', () => {
       sinon.assert.calledWith(create, aTodo);
     });
 
-    it('throws if the payload is missing a title', async () => {
-      const todo = givenTodo();
-      delete todo.title;
-      try {
-        await controller.createTodo(todo);
-      } catch (err) {
-        expect(err).to.match(/title is required/);
-        sinon.assert.notCalled(create);
-        return;
-      }
-      // Repository stub should not have been called!
-      throw new Error(noError);
-    });
-
     it('resolves remindAtAddress to a geocode', async () => {
       geocode.resolves([aLocation.geopoint]);
 
diff --git a/packages/openapi-v3/src/controller-spec.ts b/packages/openapi-v3/src/controller-spec.ts
@@ -178,16 +178,34 @@ function resolveControllerSpec(constructor: Function): ControllerSpec {
         if (!spec.components.schemas) {
           spec.components.schemas = {};
         }
+        if (p.name in spec.components.schemas) {
+          // Preserve user-provided definitions
+          debug(
+            '    skipping parameter type %j as already defined',
+            p.name || p,
+          );
+          continue;
+        }
         const jsonSchema = getJsonSchema(p);
         const openapiSchema = jsonToSchemaObject(jsonSchema);
+        const outputSchemas = spec.components.schemas;
         if (openapiSchema.definitions) {
           for (const key in openapiSchema.definitions) {
-            spec.components.schemas[key] = openapiSchema.definitions[key];
+            // Preserve user-provided definitions
+            if (key in outputSchemas) continue;
+            const relatedSchema = openapiSchema.definitions[key];
+            debug(
+              '    defining referenced schema for %j: %j',
+              key,
+              relatedSchema,
+            );
+            outputSchemas[key] = relatedSchema;
           }
           delete openapiSchema.definitions;
         }
 
-        spec.components.schemas[p.name] = openapiSchema;
+        debug('    defining schema for %j: %j', p.name, openapiSchema);
+        outputSchemas[p.name] = openapiSchema;
         break;
       }
     }
diff --git a/packages/openapi-v3/src/decorators/request-body.decorator.ts b/packages/openapi-v3/src/decorators/request-body.decorator.ts
@@ -79,6 +79,10 @@ export const REQUEST_BODY_INDEX = 'x-parameter-index';
  */
 export function requestBody(requestBodySpec?: Partial<RequestBodyObject>) {
   return function(target: Object, member: string, index: number) {
+    debug('@requestBody() on %s.%s', target.constructor.name, member);
+    debug('  parameter index: %s', index);
+    debug('  options: %s', inspect(requestBodySpec, {depth: null}));
+
     // Use 'application/json' as default content if `requestBody` is undefined
     requestBodySpec = requestBodySpec || {content: {}};
 
@@ -89,11 +93,12 @@ export function requestBody(requestBodySpec?: Partial<RequestBodyObject>) {
     const methodSig = MetadataInspector.getDesignTypeForMethod(target, member);
     const paramTypes = (methodSig && methodSig.parameterTypes) || [];
 
-    let paramType = paramTypes[index];
-    let schema: SchemaObject;
+    const paramType = paramTypes[index];
+    const schema = getSchemaForRequestBody(paramType);
+    debug('  inferred schema: %s', inspect(schema, {depth: null}));
     requestBodySpec.content = _.mapValues(requestBodySpec.content, c => {
       if (!c.schema) {
-        c.schema = schema = schema || getSchemaForRequestBody(paramType);
+        c.schema = schema;
       }
       return c;
     });
@@ -104,9 +109,7 @@ export function requestBody(requestBodySpec?: Partial<RequestBodyObject>) {
       requestBodySpec[REQUEST_BODY_INDEX] = index;
     }
 
-    debug('requestBody member: ', member);
-    debug('requestBody index: ', index);
-    debug('requestBody spec: ', inspect(requestBodySpec, {depth: null}));
+    debug('  final spec: ', inspect(requestBodySpec, {depth: null}));
     ParameterDecoratorFactory.createDecorator<RequestBodyObject>(
       OAI3Keys.REQUEST_BODY_KEY,
       requestBodySpec as RequestBodyObject,
diff --git a/packages/openapi-v3/src/index.ts b/packages/openapi-v3/src/index.ts
@@ -6,3 +6,5 @@
 export * from './decorators';
 export * from './controller-spec';
 export * from './json-to-schema';
+
+export * from '@loopback/repository-json-schema';
diff --git a/packages/openapi-v3/test/unit/decorators/request-body/request-body.decorator.unit.ts b/packages/openapi-v3/test/unit/decorators/request-body/request-body.decorator.unit.ts
@@ -70,7 +70,7 @@ describe('requestBody decorator', () => {
       expect(requestBodySpec.content).to.deepEqual(expectedContent);
     });
 
-    it('schema in requestBody overrides the generated schema', () => {
+    it('preserves user-provided schema in requestBody', () => {
       const expectedContent = {
         'application/json': {
           schema: {type: 'object'},
@@ -93,6 +93,29 @@ describe('requestBody decorator', () => {
       expect(requestBodySpec.content).to.deepEqual(expectedContent);
     });
 
+    it('preserves user-provided reference in requestBody', () => {
+      const expectedContent = {
+        'application/json': {
+          schema: {$ref: '#/components/schemas/MyModel'},
+        },
+      };
+
+      class MyModel {}
+
+      class MyController {
+        @post('/MyModel')
+        createMyModel(
+          @requestBody({content: expectedContent})
+          inst: Partial<MyModel>,
+        ) {}
+      }
+
+      const requestBodySpec = getControllerSpec(MyController).paths['/MyModel'][
+        'post'
+      ].requestBody;
+      expect(requestBodySpec.content).to.deepEqual(expectedContent);
+    });
+
     it('reports error if more than one requestBody are found for the same method', () => {
       class MyController {
         @post('/greeting')
diff --git a/packages/rest/package.json b/packages/rest/package.json
@@ -31,13 +31,15 @@
     "@types/cors": "^2.8.3",
     "@types/express": "^4.11.1",
     "@types/http-errors": "^1.6.1",
+    "ajv": "^6.5.1",
     "body": "^5.1.0",
     "cors": "^2.8.4",
     "debug": "^3.1.0",
     "express": "^4.16.3",
     "http-errors": "^1.6.3",
     "js-yaml": "^3.11.0",
     "lodash": "^4.17.5",
+    "openapi-schema-to-json-schema": "^2.1.0",
     "path-to-regexp": "^2.2.0",
     "validator": "^10.4.0"
   },
diff --git a/packages/rest/src/coercion/rest-http-error.ts b/packages/rest/src/coercion/rest-http-error.ts
@@ -12,4 +12,7 @@ export namespace RestHttpErrors {
     const msg = `Parameters with "in: ${location}" are not supported yet.`;
     return new HttpErrors.NotImplemented(msg);
   }
+  export function invalidRequestBody(msg: string): HttpErrors.HttpError {
+    return new HttpErrors.UnprocessableEntity(msg);
+  }
 }
diff --git a/packages/rest/src/http-handler.ts b/packages/rest/src/http-handler.ts
@@ -55,7 +55,9 @@ export class HttpHandler {
   }
 
   findRoute(request: Request): ResolvedRoute {
-    return this._routes.find(request);
+    const route = this._routes.find(request);
+    Object.assign(route.schemas, this.getApiDefinitions());
+    return route;
   }
 
   protected async _handleRequest(
diff --git a/packages/rest/src/index.ts b/packages/rest/src/index.ts
@@ -20,16 +20,8 @@ export {
 
 export * from './providers';
 
-// import all errors from external http-errors package
-import * as HttpErrors from 'http-errors';
-
 export * from './parser';
-
-export {writeResultToResponse} from './writer';
-
-// http errors
-export {HttpErrors};
-
+export * from './writer';
 export * from './http-handler';
 export * from './request-context';
 export * from './types';
@@ -38,5 +30,10 @@ export * from './rest.application';
 export * from './rest.component';
 export * from './rest.server';
 export * from './sequence';
-export * from '@loopback/openapi-v3';
 export * from './coercion/rest-http-error';
+
+// export all errors from external http-errors package
+import * as HttpErrors from 'http-errors';
+export {HttpErrors};
+
+export * from '@loopback/openapi-v3';
diff --git a/packages/rest/src/parser.ts b/packages/rest/src/parser.ts
@@ -9,14 +9,18 @@ import {
   OperationObject,
   ParameterObject,
   isReferenceObject,
+  SchemasObject,
 } from '@loopback/openapi-v3-types';
 import {REQUEST_BODY_INDEX} from '@loopback/openapi-v3';
 import {promisify} from 'util';
 import {OperationArgs, Request, PathParameterValues} from './types';
 import {ResolvedRoute} from './router/routing-table';
 import {coerceParameter} from './coercion/coerce-parameter';
+import {validateRequestBody} from './validation/request-body.validator';
 import {RestHttpErrors} from './index';
 type HttpError = HttpErrors.HttpError;
+import * as debugModule from 'debug';
+const debug = debugModule('loopback:rest:parser');
 
 // tslint:disable-next-line:no-any
 type MaybeBody = any | undefined;
@@ -52,10 +56,17 @@ export async function parseOperationArgs(
   request: Request,
   route: ResolvedRoute,
 ): Promise<OperationArgs> {
+  debug('Parsing operation arguments for route %s', route.describe());
   const operationSpec = route.spec;
   const pathParams = route.pathParams;
   const body = await loadRequestBodyIfNeeded(operationSpec, request);
-  return buildOperationArguments(operationSpec, request, pathParams, body);
+  return buildOperationArguments(
+    operationSpec,
+    request,
+    pathParams,
+    body,
+    route.schemas,
+  );
 }
 
 async function loadRequestBodyIfNeeded(
@@ -65,13 +76,15 @@ async function loadRequestBodyIfNeeded(
   if (!operationSpec.requestBody) return Promise.resolve();
 
   const contentType = getContentType(request);
+  debug('Loading request body with content type %j', contentType);
   if (contentType && !/json/.test(contentType)) {
     throw new HttpErrors.UnsupportedMediaType(
       `Content-type ${contentType} is not supported.`,
     );
   }
 
   return await parseJsonBody(request).catch((err: HttpError) => {
+    debug('Cannot parse request body %j', err);
     err.statusCode = 400;
     throw err;
   });
@@ -81,7 +94,8 @@ function buildOperationArguments(
   operationSpec: OperationObject,
   request: Request,
   pathParams: PathParameterValues,
-  body?: MaybeBody,
+  body: MaybeBody,
+  globalSchemas: SchemasObject,
 ): OperationArgs {
   let requestBodyIndex: number = -1;
   if (operationSpec.requestBody) {
@@ -107,6 +121,10 @@ function buildOperationArguments(
     const coercedValue = coerceParameter(rawValue, spec);
     paramArgs.push(coercedValue);
   }
+
+  debug('Validating request body - value %j', body);
+  validateRequestBody(body, operationSpec.requestBody, globalSchemas);
+
   if (requestBodyIndex > -1) paramArgs.splice(requestBodyIndex, 0, body);
   return paramArgs;
 }
diff --git a/packages/rest/src/providers/reject.provider.ts b/packages/rest/src/providers/reject.provider.ts
@@ -9,6 +9,8 @@ import {HttpError} from 'http-errors';
 import {writeErrorToResponse} from '../writer';
 import {RestBindings} from '../keys';
 
+const debug = require('debug')('loopback:rest:reject');
+
 export class RejectProvider implements Provider<Reject> {
   constructor(
     @inject(RestBindings.SequenceActions.LOG_ERROR)
@@ -23,6 +25,16 @@ export class RejectProvider implements Provider<Reject> {
     const err = <HttpError>error;
     const statusCode = err.statusCode || err.status || 500;
     writeErrorToResponse(response, err);
+
+    // Always log the error in debug mode, even when the application
+    // has a custom error logger configured (e.g. in tests)
+    debug(
+      'HTTP request %s %s was rejected: %s %s',
+      request.method,
+      request.url,
+      statusCode,
+      err.stack || err,
+    );
     this.logError(error, statusCode, request);
   }
 }
diff --git a/packages/rest/src/rest.server.ts b/packages/rest/src/rest.server.ts
@@ -36,6 +36,8 @@ import {RestBindings} from './keys';
 import {RequestContext} from './request-context';
 import * as express from 'express';
 
+const debug = require('debug')('loopback:rest:server');
+
 export type HttpRequestListener = (
   req: ServerRequest,
   res: ServerResponse,
@@ -251,8 +253,11 @@ export class RestServer extends Context implements Server, HttpServerLike {
       const apiSpec = getControllerSpec(ctor);
       if (!apiSpec) {
         // controller methods are specified through app.api() spec
+        debug('Skipping controller %s - no API spec provided', controllerName);
         continue;
       }
+
+      debug('Registering controller %s', controllerName);
       if (apiSpec.components && apiSpec.components.schemas) {
         this._httpHandler.registerApiDefinitions(apiSpec.components.schemas);
       }
@@ -593,6 +598,7 @@ export class RestServer extends Context implements Server, HttpServerLike {
     this.bind(RestBindings.PORT).to(this._httpServer.port);
     this.bind(RestBindings.HOST).to(this._httpServer.host);
     this.bind(RestBindings.URL).to(this._httpServer.url);
+    debug('RestServer listening at %s', this._httpServer.url);
   }
 
   /**
diff --git a/packages/rest/src/router/routing-table.ts b/packages/rest/src/router/routing-table.ts
diff --git a/packages/rest/src/sequence.ts b/packages/rest/src/sequence.ts
diff --git a/packages/rest/src/validation/request-body.validator.ts b/packages/rest/src/validation/request-body.validator.ts
diff --git a/packages/rest/test/acceptance/validation/validation.acceptance.ts b/packages/rest/test/acceptance/validation/validation.acceptance.ts
diff --git a/packages/rest/test/helpers.ts b/packages/rest/test/helpers.ts
diff --git a/packages/rest/test/unit/coercion/utils.ts b/packages/rest/test/unit/coercion/utils.ts
diff --git a/packages/rest/test/unit/request-body.validator.test.ts b/packages/rest/test/unit/request-body.validator.test.ts

Original file line number	Diff line number	Diff line change
`@@ -12,4 +12,7 @@ export namespace RestHttpErrors {`
`12`	`12`	const msg = `Parameters with "in: ${location}" are not supported yet.`;
`13`	`13`	`return new HttpErrors.NotImplemented(msg);`
`14`	`14`	`}`
	`15`	`+ export function invalidRequestBody(msg: string): HttpErrors.HttpError {`
	`16`	`+ return new HttpErrors.UnprocessableEntity(msg);`
	`17`	`+ }`
`15`	`18`	`}`
Original file line number	Diff line number	Diff line change
`@@ -55,7 +55,9 @@ export class HttpHandler {`
`55`	`55`	`}`
`56`	`56`
`57`	`57`	`findRoute(request: Request): ResolvedRoute {`
`58`		`- return this._routes.find(request);`
	`58`	`+ const route = this._routes.find(request);`
	`59`	`+ Object.assign(route.schemas, this.getApiDefinitions());`
	`60`	`+ return route;`
`59`	`61`	`}`
`60`	`62`
`61`	`63`	`protected async _handleRequest(`