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(rest): improve validation errors for invalid parameter value #6105

Merged
merged 1 commit into from
Aug 17, 2020
Merged

feat(rest): improve validation errors for invalid parameter value #6105

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
29 changes: 29 additions & 0 deletions packages/rest/src/__tests__/acceptance/coercion/coercion.acceptance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
import {
Client,
createRestAppClient,
expect,
givenHttpServerConfig,
sinon,
} from '@loopback/testlab';
Expand Down Expand Up @@ -253,6 +254,34 @@ describe('Coercion', () => {
sinon.assert.calledWithExactly(spy, {...inclusionFilter});
});

it('returns AJV validation errors in error details', async () => {
const filter = {
where: 'string-instead-of-object',
};
const response = await client
.get(`/nested-inclusion-from-query`)
.query({filter: JSON.stringify(filter)})
.expect(400);

expect(response.body.error).to.containDeep({
code: 'INVALID_PARAMETER_VALUE',
details: [
{
code: 'type',
info: {
type: 'object',
},
message: 'should be object',
path: '/where',
},
],
});

expect(response.body.error.message).to.match(
/Invalid data.* for parameter "filter"/,
);
});

async function givenAClient() {
app = new RestApplication({rest: givenHttpServerConfig()});
app.controller(MyController);
Expand Down
2 changes: 1 addition & 1 deletion packages/rest/src/coercion/coerce-parameter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ async function coerceObject(
data,
schema,
{},
{...options, coerceTypes: true, source: 'parameter'},
{...options, coerceTypes: true, source: 'parameter', name: spec.name},
);
}

Expand Down
8 changes: 6 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
Expand Up @@ -11,7 +11,7 @@ export namespace RestHttpErrors {
name: string,
extraProperties?: Props,
): HttpErrors.HttpError & Props {
const msg = `Invalid data ${JSON.stringify(data)} for parameter ${name}!`;
const msg = `Invalid data ${JSON.stringify(data)} for parameter "${name}".`;
return Object.assign(
new HttpErrors.BadRequest(msg),
{
Expand Down Expand Up @@ -51,11 +51,15 @@ export namespace RestHttpErrors {

export const INVALID_REQUEST_BODY_MESSAGE =
'The request body is invalid. See error object `details` property for more info.';
export function invalidRequestBody(): HttpErrors.HttpError {

export function invalidRequestBody(
details: ValidationErrorDetails[],
Copy link
Member

Choose a reason for hiding this comment

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

would there be backward compatibility issue for adding a parameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

In the strict interpretation, this is a breaking change because existing callers of invalidRequestBody must be updated. However, I consider invalidRequestBody as an internal helper that's not a part of our public API, therefore I think it's ok to make this change in a semver-minor release.

): HttpErrors.HttpError & {details: ValidationErrorDetails[]} {
return Object.assign(
new HttpErrors.UnprocessableEntity(INVALID_REQUEST_BODY_MESSAGE),
{
code: 'VALIDATION_FAILED',
details,
},
);
}
Expand Down
5 changes: 5 additions & 0 deletions packages/rest/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,11 @@ export interface ValueValidationOptions extends ValidationOptions {
* 'query', 'cookie', etc...
*/
source?: string;

/**
* Parameter name, as provided in `ParameterObject#name` property.
*/
name?: string;
}

/**
Expand Down
35 changes: 18 additions & 17 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 @@ -12,7 +12,6 @@ import {
} from '@loopback/openapi-v3';
import ajv, {Ajv} from 'ajv';
import debugModule from 'debug';
import _ from 'lodash';
import util from 'util';
import {HttpErrors, RequestBody, RestHttpErrors} from '..';
import {
Expand Down Expand Up @@ -180,30 +179,32 @@ export async function validateValueAgainstSchema(

// Throw invalid request body error
if (options.source === 'body') {
const error = RestHttpErrors.invalidRequestBody();
addErrorDetails(error, validationErrors);
const error = RestHttpErrors.invalidRequestBody(
buildErrorDetails(validationErrors),
);
throw error;
}

// Throw invalid value error
const error = new HttpErrors.BadRequest('Invalid value.');
addErrorDetails(error, validationErrors);
const error = RestHttpErrors.invalidData(value, options.name ?? '(unknown)', {
details: buildErrorDetails(validationErrors),
});
throw error;
}

function addErrorDetails(
// eslint-disable-next-line @typescript-eslint/no-explicit-any
error: any,
function buildErrorDetails(
validationErrors: ajv.ErrorObject[],
) {
error.details = _.map(validationErrors, e => {
return {
path: e.dataPath,
code: e.keyword,
message: e.message,
info: e.params,
};
});
): RestHttpErrors.ValidationErrorDetails[] {
return validationErrors.map(
(e: ajv.ErrorObject): RestHttpErrors.ValidationErrorDetails => {
return {
path: e.dataPath,
code: e.keyword,
message: e.message ?? `must pass validation rule ${e.keyword}`,
info: e.params,
};
},
);
}

/**
Expand Down