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.

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

Allow optional use of req.url #857

Merged
merged 3 commits into from
Nov 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/framework/openapi.context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,23 @@ export class OpenApiContext {
public readonly openApiRouteMap = {};
public readonly routes: RouteMetadata[] = [];
public readonly ignoreUndocumented: boolean;
public readonly useRequestUrl: boolean;
private readonly basePaths: string[];
private readonly ignorePaths: RegExp | Function;

constructor(spec: Spec, ignorePaths: RegExp | Function, ignoreUndocumented: boolean = false) {
constructor(
spec: Spec,
ignorePaths: RegExp | Function,
ignoreUndocumented: boolean = false,
useRequestUrl = false,
) {
this.apiDoc = spec.apiDoc;
this.basePaths = spec.basePaths;
this.routes = spec.routes;
this.ignorePaths = ignorePaths;
this.ignoreUndocumented = ignoreUndocumented;
this.buildRouteMaps(spec.routes);
this.useRequestUrl = useRequestUrl;
}

public isManagedRoute(path: string): boolean {
Expand Down
1 change: 1 addition & 0 deletions src/framework/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ export interface OpenApiValidatorOpts {
ignoreUndocumented?: boolean;
securityHandlers?: SecurityHandlers;
coerceTypes?: boolean | 'array';
useRequestUrl?: boolean;
/**
* @deprecated
* Use `formats` + `validateFormats` to ignore specified formats
Expand Down
16 changes: 11 additions & 5 deletions src/middlewares/openapi.metadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,12 @@ export function applyOpenApiMetadata(
if (openApiContext.shouldIgnoreRoute(path)) {
return next();
}
const matched = lookupRoute(req);
const matched = lookupRoute(req, openApiContext.useRequestUrl);
if (matched) {
const { expressRoute, openApiRoute, pathParams, schema } = matched;
if (!schema) {
// Prevents validation for routes which match on path but mismatch on method
if(openApiContext.ignoreUndocumented) {
if (openApiContext.ignoreUndocumented) {
return next();
}
throw new MethodNotAllowed({
Expand All @@ -54,7 +54,10 @@ export function applyOpenApiMetadata(
// add the response schema if validating responses
(<any>req.openapi)._responseSchema = (<any>matched)._responseSchema;
}
} else if (openApiContext.isManagedRoute(path) && !openApiContext.ignoreUndocumented) {
} else if (
openApiContext.isManagedRoute(path) &&
!openApiContext.ignoreUndocumented
) {
throw new NotFound({
path: req.path,
message: 'not found',
Expand All @@ -63,8 +66,11 @@ export function applyOpenApiMetadata(
next();
};

function lookupRoute(req: OpenApiRequest): OpenApiRequestMetadata {
const path = req.originalUrl.split('?')[0];
function lookupRoute(
req: OpenApiRequest,
useRequestUrl: boolean,
): OpenApiRequestMetadata {
const path = useRequestUrl ? req.url : req.originalUrl.split('?')[0];
const method = req.method;
const routeEntries = Object.entries(openApiContext.expressRouteMap);
for (const [expressRoute, methods] of routeEntries) {
Expand Down
10 changes: 8 additions & 2 deletions src/openapi.validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export class OpenApiValidator {
if (options.$refParser == null) options.$refParser = { mode: 'bundle' };
if (options.validateFormats == null) options.validateFormats = true;
if (options.formats == null) options.formats = {};
if (options.useRequestUrl == null) options.useRequestUrl = false;
Copy link
Owner

@cdimascio cdimascio Aug 23, 2023

Choose a reason for hiding this comment

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

can you help me understand why this is an option controlled by the client? what is the downside using this behavior i.e. req.path and req.url for all requests? for instance, can we make both paths work withour requiring the client to configure the behavior

Choose a reason for hiding this comment

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

req.path and req.url are localized by express to be router specific, so if we nest router A in router B req.path/req.url will only have router A data (/test), no router B data (/api/). #211 introduced the current default behavior as the desired use case was that users were validating against a single spec that included router B data. I don't think there's an easy way without naive searching of the spec to handle both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cdimascio what @sennyeya said - using req.path or req.url is scoped to the router using it and is unaware of the url defined by the parent router. The flip side of this is that you can't use separate schemas in the parent router and child router, because originalUrl is always set to the one defined by the parent router i.e /api. But using req.url in the child router allows us to decouple child and parent routers and schemas. Also req.url is intended to be used like so by the express framework I believe.
Screenshot 2023-08-26 at 14 38 31


if (typeof options.operationHandlers === 'string') {
/**
Expand Down Expand Up @@ -103,7 +104,12 @@ export class OpenApiValidator {
resOpts,
).preProcess();
return {
context: new OpenApiContext(spec, this.options.ignorePaths, this.options.ignoreUndocumented),
context: new OpenApiContext(
spec,
this.options.ignorePaths,
this.options.ignoreUndocumented,
this.options.useRequestUrl,
),
responseApiDoc: sp.apiDocRes,
error: null,
};
Expand Down Expand Up @@ -201,7 +207,7 @@ export class OpenApiValidator {
return resmw(req, res, next);
})
.catch(next);
})
});
}

// op handler middleware
Expand Down
272 changes: 272 additions & 0 deletions test/user-request-url.router.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,272 @@
import { expect } from 'chai';
import type {
Express,
IRouter,
Response,
NextFunction,
Request,
} from 'express';
import * as express from 'express';
import { OpenAPIV3 } from '../src/framework/types';
import * as request from 'supertest';
import { createApp } from './common/app';

import * as OpenApiValidator from '../src';
import { Server } from 'http';

interface HTTPError extends Error {
status: number;
text: string;
method: string;
path: string;
}

describe('when useRequestUrl is set to "true" on the child router', async () => {
let app: Express & { server?: Server };

before(async () => {
const router = makeRouter({ useRequestUrl: true });
app = await makeMainApp();
app.use(router);
});

after(() => app?.server?.close());

it('should apply parent app schema to requests', async () => {
const result = await request(app).get('/api/pets/1');
const error = result.error as HTTPError;
expect(result.statusCode).to.equal(400);
expect(error.path).to.equal('/api/pets/1');
expect(error.text).to.contain(
'Bad Request: request/params/petId must NOT have fewer than 3 characters',
);
});

it('should apply child router schema to requests', async () => {
const result = await request(app).get('/api/pets/not-uuid');
const error = result.error as HTTPError;
expect(result.statusCode).to.equal(400);
expect(error.path).to.equal('/api/pets/not-uuid');
expect(error.text).to.contain(
'Bad Request: request/params/petId must match format &quot;uuid&quot',
);
});

it('should return a reponse if request is valid', async () => {
const validId = 'f627f309-cae3-46d2-84f7-d03856c84b02';
const result = await request(app).get(`/api/pets/${validId}`);
expect(result.statusCode).to.equal(200);
expect(result.body).to.deep.equal({
id: 'f627f309-cae3-46d2-84f7-d03856c84b02',
name: 'Mr Sparky',
tag: "Ain't nobody tags me",
});
});
});

describe('when useRequestUrl is set to "false" on the child router', async () => {
let app: Express & { server?: Server };

before(async () => {
const router = makeRouter({ useRequestUrl: false });
app = await makeMainApp();
app.use(router);
});

after(() => app?.server?.close());

it('should throw not found', async () => {
const result = await request(app).get('/api/pets/valid-pet-id');
const error = result.error as HTTPError;
expect(result.statusCode).to.equal(404);
expect(error.path).to.equal('/api/pets/valid-pet-id');
expect(error.text).to.contain('Not Found');
});
});

function defaultResponse(): OpenAPIV3.ResponseObject {
return {
description: 'unexpected error',
content: {
'application/json': {
schema: {
type: 'object',
required: ['code', 'message'],
properties: {
code: {
type: 'integer',
format: 'int32',
},
message: {
type: 'string',
},
},
},
},
},
};
}

/*
represents spec of the "public" entrypoint to our application e.g gateway. The
type of id in path and id in the response here defined as simple string
with minLength
*/
const gatewaySpec: OpenAPIV3.Document = {
openapi: '3.0.0',
info: { version: '1.0.0', title: 'test bug OpenApiValidator' },
servers: [{ url: 'http://localhost:3000/api' }],
paths: {
'/pets/{petId}': {
get: {
summary: 'Info for a specific pet',
operationId: 'showPetById',
tags: ['pets'],
parameters: [
{
name: 'petId',
in: 'path',
required: true,
description: 'The id of the pet to retrieve',
schema: {
type: 'string',
minLength: 3,
},
},
],
responses: {
'200': {
description: 'Expected response to a valid request',
content: {
'application/json': {
schema: {
type: 'object',
required: ['id', 'name'],
properties: {
id: {
type: 'string',
},
name: {
type: 'string',
},
tag: {
type: 'string',
},
},
},
},
},
},
default: defaultResponse(),
},
},
},
},
};

/*
represents spec of the child router. We route request from main app (gateway) to this router.
This router has its own schema, routes and validation formats. In particular, we force id in the path and id in the response to be uuid.
*/
const childRouterSpec: OpenAPIV3.Document = {
openapi: '3.0.0',
info: { version: '1.0.0', title: 'test bug OpenApiValidator' },
servers: [{ url: 'http://localhost:3000/' }],
paths: {
'/internal/api/pets/{petId}': {
get: {
summary: 'Info for a specific pet',
operationId: 'showPetById',
tags: ['pets'],
parameters: [
{
name: 'petId',
in: 'path',
required: true,
description: 'The id of the pet to retrieve',
schema: {
type: 'string',
format: 'uuid',
},
},
],
responses: {
'200': {
description: 'Expected response to a valid request',
content: {
'application/json': {
schema: {
type: 'object',
required: ['id', 'name'],
properties: {
id: {
type: 'string',
format: 'uuid',
},
name: {
type: 'string',
},
tag: {
type: 'string',
},
},
},
},
},
},
},
},
},
},
};

function redirectToInternalService(
req: Request,
_res: Response,
next: NextFunction,
): void {
req.url = `/internal${req.originalUrl}`;
next();
}

function makeMainApp(): ReturnType<typeof createApp> {
return createApp(
{
apiSpec: gatewaySpec,
validateResponses: true,
validateRequests: true,
},
3000,
(app) => {
app
.get(
'/api/pets/:petId',
function (_req: Request, _res: Response, next: NextFunction) {
next();
},
)
.use(redirectToInternalService);
},
false,
);
}

function makeRouter({ useRequestUrl }: { useRequestUrl: boolean }): IRouter {
return express
.Router()
.use(
OpenApiValidator.middleware({
apiSpec: childRouterSpec,
validateRequests: true,
validateResponses: true,
useRequestUrl,
}),
)
.get('/internal/api/pets/:petId', function (req, res) {
res.json({
id: req.params.petId,
name: 'Mr Sparky',
tag: "Ain't nobody tags me",
});
});
}