From 776a9a1f7e1d727993dfa20af5765a2ba7f6d2d5 Mon Sep 17 00:00:00 2001 From: Ari Autio Date: Sun, 30 Aug 2020 23:02:26 +0300 Subject: [PATCH 1/2] Fixed test for match() with url params. Input was not validated when testing match & url-params. The api spec was missing a proper path and thus match did not execute. --- .../__snapshots__/integration.test.ts.snap | 20 +++++++++++ test/integration/integration.test.ts | 8 +++++ test/openapi.yaml | 34 +++++++++++++++++++ 3 files changed, 62 insertions(+) diff --git a/test/integration/__snapshots__/integration.test.ts.snap b/test/integration/__snapshots__/integration.test.ts.snap index 62ea749..2e44721 100644 --- a/test/integration/__snapshots__/integration.test.ts.snap +++ b/test/integration/__snapshots__/integration.test.ts.snap @@ -139,3 +139,23 @@ Object { }, } `; + +exports[`Integration tests with real app requests against /match are validated correctly 2`] = ` +Object { + "error": Object { + "data": Array [ + Object { + "dataPath": ".body", + "keyword": "required", + "message": "should have required property 'input'", + "params": Object { + "missingProperty": "input", + }, + "schemaPath": "#/properties/body/required", + }, + ], + "message": "Error while validating request: request.body should have required property 'input'", + "name": "ValidationError", + }, +} +`; diff --git a/test/integration/integration.test.ts b/test/integration/integration.test.ts index 808598e..e647a7b 100644 --- a/test/integration/integration.test.ts +++ b/test/integration/integration.test.ts @@ -66,6 +66,14 @@ describe("Integration tests with real app", () => { expect(res.status).toBe(200); expect(res.body).toEqual({ output: "works-with-url-param" }); expect(validate(res)).toBeUndefined(); + + res = await request(app) + .post("/match/works-with-url-param") + .send({}); + expect(validate(res)).toBeUndefined(); + expect(res.status).toBe(400); + expect(res.body).toHaveProperty("error"); + expect(res.body).toMatchSnapshot(); }); test("requests against /no-match are not validated", async () => { diff --git a/test/openapi.yaml b/test/openapi.yaml index 813df26..982dd98 100644 --- a/test/openapi.yaml +++ b/test/openapi.yaml @@ -144,6 +144,40 @@ paths: - output default: $ref: "#/components/responses/Error" + /match/{optional}: + post: + description: Echo url-param back + requestBody: + content: + application/json: + schema: + type: object + properties: + input: + type: string + required: + - input + parameters: + - name: optional + in: path + required: true + schema: + type: string + responses: + "200": + description: Response + content: + application/json: + schema: + type: object + properties: + output: + type: string + required: + - output + default: + $ref: "#/components/responses/Error" + /internal-ref: post: responses: From dffa3cec38d2eb8d2bf379c4e0818162772af97f Mon Sep 17 00:00:00 2001 From: Ari Autio Date: Thu, 3 Sep 2020 18:47:14 +0300 Subject: [PATCH 2/2] Fix #64. match() throws if paths don't match validator. * This is a breaking change. * To keep previous functionality call match({ allowNoMatch: true }). --- README.md | 14 ++++----- src/OpenApiValidator.ts | 16 ++++++++-- test/OpenApiValidator.test.ts | 29 +++++++++++++++++-- .../__snapshots__/integration.test.ts.snap | 9 ++++++ test/integration/app.ts | 8 ++--- test/integration/integration.test.ts | 7 +++-- 6 files changed, 63 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 4a858bd..6b796fc 100644 --- a/README.md +++ b/README.md @@ -228,18 +228,18 @@ Object][openapi-path-item-object]: `RequestHandler` is an express middleware function with the signature `(req: Request, res: Response, next: NextFunction): any;`. -#### `match(): RequestHandler` +#### `match(options: MatchOptions = { allowNoMatch: false }): RequestHandler` Returns an express middleware function which calls `validate()` based on the request method and path. Using this function removes the need to specify `validate()` middleware for each express endpoint individually. -Note that behaviour is different to `validate()` for routes where no schema is -specified: `validate()` will throw an exception if no matching route -specification is found in the OpenAPI schema. `match()` will not throw an -exception in this case; the request is simply not validated. Be careful to -ensure you OpenAPI schema contains validators for each endpoint if using -`match()`. +`match()` throws an error if matching route specification is not found. This +ensures all requests are validated. + +Use `match({ allowNoMatch: true})` if you want to skip validation for routes +that are not mentioned in the OpenAPI schema. Use with caution: It allows +requests to be handled without any validation. The following examples achieve the same result: diff --git a/src/OpenApiValidator.ts b/src/OpenApiValidator.ts index e6b84c6..76bdd49 100644 --- a/src/OpenApiValidator.ts +++ b/src/OpenApiValidator.ts @@ -64,6 +64,10 @@ export interface PathRegexpObject { regex: RegExp; } +export interface MatchOptions { + allowNoMatch?: boolean; +} + export default class OpenApiValidator { private _ajv: Ajv.Ajv; @@ -151,7 +155,9 @@ export default class OpenApiValidator { return validate; } - public match(): RequestHandler { + public match( + options: MatchOptions = { allowNoMatch: false }, + ): RequestHandler { const paths: PathRegexpObject[] = _.keys(this._document.paths).map( path => ({ path, @@ -160,10 +166,16 @@ export default class OpenApiValidator { ); const matchAndValidate: RequestHandler = (req, res, next) => { const match = paths.find(({ regex }) => regex.test(req.path)); + const method = req.method.toLowerCase() as Operation; if (match) { - const method = req.method.toLowerCase() as Operation; this.validate(method, match.path)(req, res, next); + } else if (!options.allowNoMatch) { + const err = new Error( + `Path=${req.path} with method=${method} not found from OpenAPI document`, + ); + next(err); } else { + // match not required next(); } }; diff --git a/test/OpenApiValidator.test.ts b/test/OpenApiValidator.test.ts index 2eedf33..3512dce 100644 --- a/test/OpenApiValidator.test.ts +++ b/test/OpenApiValidator.test.ts @@ -617,7 +617,7 @@ describe("OpenApiValidator", () => { expect(validateHandler).toBeCalled(); }); - test("match() - does not call validate() if request does not match", async () => { + test("match() - does not call validate() if request does not match and yields error", async () => { const validator = new OpenApiValidator(openApiDocument); const match = validator.match(); @@ -626,11 +626,34 @@ describe("OpenApiValidator", () => { const req = { ...baseReq, + method: "POST", path: "/no-match", }; - // eslint-disable-next-line @typescript-eslint/no-empty-function - match(req, {} as Response, () => {}); + const nextMock = jest.fn(); + + match(req, {} as Response, nextMock); + expect(validateMock).not.toHaveBeenCalled(); + expect(nextMock).toHaveBeenCalledWith(expect.any(Error)); + }); + + test("match({ allowNoMatch: true }) - does not call validate() if request does not match and does not yield error", async () => { + const validator = new OpenApiValidator(openApiDocument); + const match = validator.match({ allowNoMatch: true }); + + const validateMock = jest.fn(); + validator.validate = validateMock; + + const req = { + ...baseReq, + method: "POST", + path: "/no-match", + }; + + const nextMock = jest.fn(); + + match(req, {} as Response, nextMock); expect(validateMock).not.toHaveBeenCalled(); + expect(nextMock).toHaveBeenCalledWith(); }); }); diff --git a/test/integration/__snapshots__/integration.test.ts.snap b/test/integration/__snapshots__/integration.test.ts.snap index 2e44721..f9c183b 100644 --- a/test/integration/__snapshots__/integration.test.ts.snap +++ b/test/integration/__snapshots__/integration.test.ts.snap @@ -159,3 +159,12 @@ Object { }, } `; + +exports[`Integration tests with real app requests against /no-match cause an error 1`] = ` +Object { + "error": Object { + "message": "Path=/no-match with method=post not found from OpenAPI document", + "name": "Error", + }, +} +`; diff --git a/test/integration/app.ts b/test/integration/app.ts index 3670ca6..ae1e601 100644 --- a/test/integration/app.ts +++ b/test/integration/app.ts @@ -16,7 +16,7 @@ import cookieParser from "cookie-parser"; import express from "express"; -import { OpenApiValidator } from "../../dist"; // eslint-disable-line +import { OpenApiValidator, ValidationError } from "../../dist"; // eslint-disable-line import openApiDocument from "../open-api-document"; const app: express.Express = express(); @@ -34,9 +34,7 @@ app.post("/match/:optional?", validator.match(), (req, res, _next) => { res.json({ output: req.params.optional || req.body.input }); }); -app.post("/no-match", validator.match(), (req, res, _next) => { - res.json({ extra: req.body.anything }); -}); +app.post("/no-match", validator.match()); app.get( "/parameters", @@ -74,7 +72,7 @@ app.get( ); const errorHandler: express.ErrorRequestHandler = (err, req, res, _next) => { - res.status(err.statusCode).json({ + res.status(err instanceof ValidationError ? err.statusCode : 500).json({ error: { name: err.name, message: err.message, diff --git a/test/integration/integration.test.ts b/test/integration/integration.test.ts index e647a7b..d5ba198 100644 --- a/test/integration/integration.test.ts +++ b/test/integration/integration.test.ts @@ -76,12 +76,13 @@ describe("Integration tests with real app", () => { expect(res.body).toMatchSnapshot(); }); - test("requests against /no-match are not validated", async () => { + test("requests against /no-match cause an error", async () => { const res = await request(app) .post("/no-match") .send({ anything: "anything" }); - expect(res.status).toBe(200); - expect(res.body).toEqual({ extra: "anything" }); + expect(res.status).toBe(500); + expect(res.body).toHaveProperty("error"); + expect(res.body).toMatchSnapshot(); }); test("path parameters are validated", async () => {