Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: localize error in details #1511

Merged
merged 1 commit into from
Jul 13, 2018
Merged

feat: localize error in details #1511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions packages/rest/src/rest-http-error.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as HttpErrors from 'http-errors';

export namespace RestHttpErrors {
export function invalidData<T>(data: T, name: string) {
const msg = `Invalid data ${JSON.stringify(data)} for parameter ${name}!`;
Expand All @@ -12,7 +13,33 @@ 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);
export const INVALID_REQUEST_BODY_MESSAGE =
'The request body is invalid. See error object `details` property for more info.';
export function invalidRequestBody(): HttpErrors.HttpError {
return new HttpErrors.UnprocessableEntity(INVALID_REQUEST_BODY_MESSAGE);
}
/**
* An invalid request body error contains a `details` property as the machine-readable error.
* Each entry in `error.details` contains 4 attributes: `path`, `code`, `info` and `message`.
* `ValidationErrorDetails` defines the type of each entry, which is an object.
* The type of `error.details` is `ValidationErrorDetails[]`.
*/
export interface ValidationErrorDetails {
/**
* A path to the invalid field.
*/
path: string;
/**
* A single word code represents the error's type.
*/
code: string;
/**
* A human readable description of the error.
*/
message: string;
/**
* Some additional details that the 3 attributes above don't cover.
*/
info: object;
}
}
21 changes: 14 additions & 7 deletions packages/rest/src/validation/request-body.validator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import * as debugModule from 'debug';
import * as util from 'util';
import {HttpErrors} from '..';
import {RestHttpErrors} from '..';
import {AnyObject} from '@loopback/repository';
import * as _ from 'lodash';

const toJsonSchema = require('openapi-schema-to-json-schema');
const debug = debugModule('loopback:rest:validation');
Expand Down Expand Up @@ -82,10 +82,10 @@ function convertToJsonSchema(openapiSchema: SchemaObject) {
function validateValueAgainstJsonSchema(
// tslint:disable-next-line:no-any
body: any,
jsonSchema: AnyObject,
jsonSchema: object,
globalSchemas?: SchemasObject,
) {
const schemaWithRef = Object.assign({}, jsonSchema);
const schemaWithRef = Object.assign({components: {}}, jsonSchema);
schemaWithRef.components = {
schemas: globalSchemas,
};
Expand All @@ -103,8 +103,15 @@ function validateValueAgainstJsonSchema(
}

debug('Invalid request body: %s', util.inspect(ajv.errors));
const message = ajv.errorsText(ajv.errors, {dataVar: body});
// FIXME add `err.details` object containing machine-readable information
// see LB 3.x ValidationError for inspiration
throw RestHttpErrors.invalidRequestBody(message);

const error = RestHttpErrors.invalidRequestBody();
error.details = _.map(ajv.errors, e => {
return {
path: e.dataPath,
code: e.keyword,
message: e.message,
info: e.params,
};
});
throw error;
}
129 changes: 116 additions & 13 deletions packages/rest/test/unit/request-body.validator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,16 @@

import {expect} from '@loopback/testlab';
import {validateRequestBody} from '../../src/validation/request-body.validator';
import {RestHttpErrors} from '../../';
import {aBodySpec} from '../helpers';
import {
RequestBodyObject,
SchemaObject,
SchemasObject,
} from '@loopback/openapi-v3-types';

const INVALID_MSG = RestHttpErrors.INVALID_REQUEST_BODY_MESSAGE;

const TODO_SCHEMA = {
title: 'Todo',
properties: {
Expand Down Expand Up @@ -46,8 +49,17 @@ describe('validateRequestBody', () => {
});

it('rejects data missing a required property', () => {
const details: RestHttpErrors.ValidationErrorDetails[] = [
{
path: '',
code: 'required',
message: "should have required property 'title'",
info: {missingProperty: 'title'},
},
];
verifyValidationRejectsInputWithError(
/required property 'title'/,
INVALID_MSG,
details,
{
description: 'missing required "title"',
},
Expand All @@ -56,8 +68,17 @@ describe('validateRequestBody', () => {
});

it('rejects data containing values of a wrong type', () => {
const details: RestHttpErrors.ValidationErrorDetails[] = [
{
path: '.isComplete',
code: 'type',
message: 'should be boolean',
info: {type: 'boolean'},
},
];
verifyValidationRejectsInputWithError(
/isComplete should be boolean/,
INVALID_MSG,
details,
{
title: 'todo with a string value of "isComplete"',
isComplete: 'a string value',
Expand All @@ -67,8 +88,23 @@ describe('validateRequestBody', () => {
});

it('reports all validation errors', () => {
const details: RestHttpErrors.ValidationErrorDetails[] = [
{
path: '',
code: 'required',
message: "should have required property 'title'",
info: {missingProperty: 'title'},
},
{
path: '.isComplete',
code: 'type',
message: 'should be boolean',
info: {type: 'boolean'},
},
];
verifyValidationRejectsInputWithError(
/required property 'title'.*isComplete should be boolean/,
INVALID_MSG,
details,
{
description: 'missing title and a string value of "isComplete"',
isComplete: 'a string value',
Expand All @@ -78,8 +114,17 @@ describe('validateRequestBody', () => {
});

it('resolves schema references', () => {
const details: RestHttpErrors.ValidationErrorDetails[] = [
{
path: '',
code: 'required',
message: "should have required property 'title'",
info: {missingProperty: 'title'},
},
];
verifyValidationRejectsInputWithError(
/required property/,
INVALID_MSG,
details,
{description: 'missing title'},
aBodySpec({$ref: '#/components/schemas/Todo'}),
{Todo: TODO_SCHEMA},
Expand All @@ -88,7 +133,8 @@ describe('validateRequestBody', () => {

it('rejects empty values when body is required', () => {
verifyValidationRejectsInputWithError(
/body is required/,
'Request body is required',
undefined,
null,
aBodySpec(TODO_SCHEMA, {required: true}),
);
Expand All @@ -99,20 +145,37 @@ describe('validateRequestBody', () => {
});

it('rejects invalid values for number properties', () => {
const details: RestHttpErrors.ValidationErrorDetails[] = [
{
path: '.count',
code: 'type',
message: 'should be number',
info: {type: 'number'},
},
];
const schema: SchemaObject = {
properties: {
count: {type: 'number'},
},
};
verifyValidationRejectsInputWithError(
/count should be number/,
INVALID_MSG,
details,
{count: 'string value'},
aBodySpec(schema),
);
});

context('rejects array of data with wrong type - ', () => {
it('primitive types', () => {
const details: RestHttpErrors.ValidationErrorDetails[] = [
{
path: '.orders[1]',
code: 'type',
message: 'should be string',
info: {type: 'string'},
},
];
const schema: SchemaObject = {
type: 'object',
properties: {
Expand All @@ -125,28 +188,52 @@ describe('validateRequestBody', () => {
},
};
verifyValidationRejectsInputWithError(
/orders\[1\] should be string/,
INVALID_MSG,
details,
{orders: ['order1', 1]},
aBodySpec(schema),
);
});

it('first level $ref', () => {
const details: RestHttpErrors.ValidationErrorDetails[] = [
{
path: '[1]',
code: 'required',
message: "should have required property 'title'",
info: {missingProperty: 'title'},
},
];
const schema: SchemaObject = {
type: 'array',
items: {
$ref: '#/components/schemas/Todo',
},
};
verifyValidationRejectsInputWithError(
/required property/,
INVALID_MSG,
details,
[{title: 'a good todo'}, {description: 'a todo item missing title'}],
aBodySpec(schema),
{Todo: TODO_SCHEMA},
);
});

it('nested $ref in schema', () => {
const details: RestHttpErrors.ValidationErrorDetails[] = [
{
path: '.todos[1]',
code: 'required',
message: "should have required property 'title'",
info: {missingProperty: 'title'},
},
{
path: '.todos[2].title',
code: 'type',
message: 'should be string',
info: {type: 'string'},
},
];
const schema: SchemaObject = {
type: 'object',
properties: {
Expand All @@ -159,11 +246,13 @@ describe('validateRequestBody', () => {
},
};
verifyValidationRejectsInputWithError(
/todos\[1\] should have required property \'title\'/,
INVALID_MSG,
details,
{
todos: [
{title: 'a good todo'},
{description: 'a todo item missing title'},
{description: 'a todo with wrong type of title', title: 2},
],
},
aBodySpec(schema),
Expand All @@ -172,6 +261,14 @@ describe('validateRequestBody', () => {
});

it('nested $ref in reference', () => {
const details: RestHttpErrors.ValidationErrorDetails[] = [
{
path: '.accounts[0].address.city',
code: 'type',
message: 'should be string',
info: {type: 'string'},
},
];
const schema: SchemaObject = {
type: 'object',
properties: {
Expand All @@ -184,7 +281,8 @@ describe('validateRequestBody', () => {
},
};
verifyValidationRejectsInputWithError(
/accounts\[0\]\.address\.city should be string/,
INVALID_MSG,
details,
{
accounts: [
{title: 'an account with invalid address', address: {city: 1}},
Expand All @@ -201,13 +299,18 @@ describe('validateRequestBody', () => {

function verifyValidationRejectsInputWithError(
errorMatcher: Error | RegExp | string,
details: RestHttpErrors.ValidationErrorDetails[] | undefined,
body: object | null,
spec: RequestBodyObject | undefined,
schemas?: SchemasObject,
) {
// workaround for Function.prototype.bind not preserving argument types
function validateRequestBodyWithBoundArgs() {
try {
validateRequestBody(body, spec, schemas);
throw new Error(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should wrap this error in a variable and assert this in the catch block: expect(err).to.not.eql(noErrorThrown)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shimks that error doesn't have the same error.message and error.details as the catch block expect to have, so if it happens the 1st assertion expect(err.message).to.equal(errorMatcher); will faill :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I think it's cleaner to directly assert for the error instance created at the moment the validation (unexpectedly) passes, you're right in the sense that the error thrown by AJV is extremely unlikely to have errorMatcher as its message.

Feel free to ignore this comment

"expected Function { name: 'validateRequestBody' } to throw exception",
);
} catch (err) {
expect(err.message).to.equal(errorMatcher);
expect(err.details).to.deepEqual(details);
}
expect(validateRequestBodyWithBoundArgs).to.throw(errorMatcher);
}