From 949a5f010968ab891e05f2aa680c76302e5b85c1 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Wed, 17 Jul 2024 12:47:32 +0200 Subject: [PATCH] fix: Update OpenAPI error converter to handle query param errors too (#7609) This PR updates the OpenAPI error converter to also work for errors with query parameters. We previously only sent the body of the request along with the error, which meant that query parameter errors would show up incorrectly. For instance given a query param with the date format and the invalid value `01-2020-01`, you'd previously get the message: > The `from` value must match format "date". You sent undefined With this change, you'll get this instead: > The `from` value must match format "date". You sent "01-2020-01". The important changes here are two things: - passing both request body and query params - the 3 lines in `fromOpenApiValidationError` that check where we should get the value you sent from. The rest of it is primarily updating tests to send the right arguments and some slight rewording to more accurately reflect that this can be either request body or query params. --- src/lib/error/bad-data-error.ts | 20 +++--- src/lib/error/unleash-error.test.ts | 105 +++++++++++++++++++++------- src/lib/services/openapi-service.ts | 2 +- 3 files changed, 94 insertions(+), 33 deletions(-) diff --git a/src/lib/error/bad-data-error.ts b/src/lib/error/bad-data-error.ts index 5baf4a7318..9b4aeb6ee6 100644 --- a/src/lib/error/bad-data-error.ts +++ b/src/lib/error/bad-data-error.ts @@ -117,10 +117,14 @@ const enumMessage = ( }; export const fromOpenApiValidationError = - (requestBody: object) => + (request: { body: object; query: object }) => (validationError: ErrorObject): ValidationErrorDescription => { const { instancePath, params, message } = validationError; - const propertyName = instancePath.substring('/body/'.length); + const [errorSource, substringOffset] = instancePath.startsWith('/body') + ? [request.body, '/body/'.length] + : [request.query, '/query/'.length]; + + const propertyName = instancePath.substring(substringOffset); switch (validationError.keyword) { case 'required': @@ -139,28 +143,28 @@ export const fromOpenApiValidationError = message, params.allowedValues, getProp( - requestBody, - instancePath.substring('/body/'.length).split('/'), + errorSource, + instancePath.substring(substringOffset).split('/'), ), ); case 'oneOf': return oneOfMessage(propertyName, validationError.message); default: - return genericErrorMessage(requestBody, propertyName, message); + return genericErrorMessage(errorSource, propertyName, message); } }; export const fromOpenApiValidationErrors = ( - requestBody: object, + request: { body: object; query: object }, validationErrors: [ErrorObject, ...ErrorObject[]], ): BadDataError => { const [firstDetail, ...remainingDetails] = validationErrors.map( - fromOpenApiValidationError(requestBody), + fromOpenApiValidationError(request), ); return new BadDataError( - "Request validation failed: the payload you provided doesn't conform to the schema. Check the `details` property for a list of errors that we found.", + "Request validation failed: your request doesn't conform to the schema. Check the `details` property for a list of errors that we found.", [firstDetail, ...remainingDetails], ); }; diff --git a/src/lib/error/unleash-error.test.ts b/src/lib/error/unleash-error.test.ts index a2ecb0412a..0fde995731 100644 --- a/src/lib/error/unleash-error.test.ts +++ b/src/lib/error/unleash-error.test.ts @@ -56,7 +56,9 @@ describe('OpenAPI error conversion', () => { message: "should have required property 'enabled'", }; - const result = fromOpenApiValidationError({})(error); + const result = fromOpenApiValidationError({ body: {}, query: {} })( + error, + ); expect(result).toMatchObject({ message: @@ -83,7 +85,10 @@ describe('OpenAPI error conversion', () => { const parameterValue = []; const result = fromOpenApiValidationError({ - parameters: parameterValue, + body: { + parameters: parameterValue, + }, + query: {}, })(error); expect(result).toMatchObject({ @@ -111,9 +116,12 @@ describe('OpenAPI error conversion', () => { }; const result = fromOpenApiValidationError({ - secret: 'blah', - username: 'string2', - type: 'admin', + body: { + secret: 'blah', + username: 'string2', + type: 'admin', + }, + query: {}, })(error); expect(result).toMatchObject({ @@ -148,7 +156,10 @@ describe('OpenAPI error conversion', () => { const requestDescription = 'A pattern that does not match.'; const result = fromOpenApiValidationError({ - description: requestDescription, + body: { + description: requestDescription, + }, + query: {}, })(error); expect(result).toMatchObject({ @@ -167,14 +178,17 @@ describe('OpenAPI error conversion', () => { message: 'must be equal to one of the allowed values', }; - const request = [ - { - name: 'variant', - weight: 500, - weightType: 'party', - stickiness: 'userId', - }, - ]; + const request = { + body: [ + { + name: 'variant', + weight: 500, + weightType: 'party', + stickiness: 'userId', + }, + ], + query: {}, + }; const result = fromOpenApiValidationError(request)(error); @@ -202,7 +216,10 @@ describe('OpenAPI error conversion', () => { const requestDescription = 'Longer than the max length'; const result = fromOpenApiValidationError({ - description: requestDescription, + body: { + description: requestDescription, + }, + query: {}, })(error); expect(result).toMatchObject({ @@ -233,7 +250,10 @@ describe('OpenAPI error conversion', () => { const propertyValue = 6; const result = fromOpenApiValidationError({ - newprop: propertyValue, + body: { + newprop: propertyValue, + }, + query: {}, })(error); expect(result).toMatchObject({ @@ -278,7 +298,10 @@ describe('OpenAPI error conversion', () => { // create an error and serialize it as it would be shown to the end user. const serializedUnleashError: ApiErrorSchema = - fromOpenApiValidationErrors({ newprop: 7 }, errors).toJSON(); + fromOpenApiValidationErrors( + { body: { newprop: 7 }, query: {} }, + errors, + ).toJSON(); expect(serializedUnleashError).toMatchObject({ name: 'BadDataError', @@ -305,9 +328,10 @@ describe('OpenAPI error conversion', () => { message: 'should NOT have additional properties', }; - const error = fromOpenApiValidationError({ bogus: 5 })( - openApiError, - ); + const error = fromOpenApiValidationError({ + body: { bogus: 5 }, + query: {}, + })(openApiError); expect(error).toMatchObject({ message: expect.stringContaining( @@ -322,9 +346,12 @@ describe('OpenAPI error conversion', () => { it('gives useful messages for nested properties', () => { const request2 = { - nestedObject: { - nested2: { extraPropertyName: 'illegal property' }, + body: { + nestedObject: { + nested2: { extraPropertyName: 'illegal property' }, + }, }, + query: {}, }; const openApiError = { keyword: 'additionalProperties', @@ -360,7 +387,10 @@ describe('OpenAPI error conversion', () => { }; const result = fromOpenApiValidationError({ - nestedObject: { a: { b: [] } }, + body: { + nestedObject: { a: { b: [] } }, + }, + query: {}, })(error); expect(result).toMatchObject({ @@ -382,7 +412,10 @@ describe('OpenAPI error conversion', () => { const illegalValue = 'illegal string'; const result = fromOpenApiValidationError({ - nestedObject: { a: { b: illegalValue } }, + body: { + nestedObject: { a: { b: illegalValue } }, + }, + query: {}, })(error); expect(result).toMatchObject({ @@ -392,6 +425,30 @@ describe('OpenAPI error conversion', () => { expect(result.message).toMatch(/\bnestedObject\/a\/b\b/); }); + + it('handles query parameter errors', () => { + const error = { + instancePath: '/query/from', + schemaPath: '#/properties/query/properties/from/format', + keyword: 'format', + params: { format: 'date' }, + message: 'must match format "date"', + }; + const query = { from: '01-2020-01' }; + + const result = fromOpenApiValidationError({ body: {}, query })(error); + + expect(result).toMatchObject({ + message: + // it tells the user what the format is + expect.stringContaining(error.params.format), + }); + + // it tells the user which property it pertains to + expect(result.message).toContain('from'); + // it tells the user what they provided + expect(result.message).toContain(query.from); + }); }); describe('Error serialization special cases', () => { diff --git a/src/lib/services/openapi-service.ts b/src/lib/services/openapi-service.ts index 612fc38c78..810935a6a6 100644 --- a/src/lib/services/openapi-service.ts +++ b/src/lib/services/openapi-service.ts @@ -64,7 +64,7 @@ export class OpenApiService { app.use((err, req, res, next) => { if (err?.status && err.validationErrors) { const apiError = fromOpenApiValidationErrors( - req.body, + req, err.validationErrors, );