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

fix(core): middleware does not apply to a controller that has versioning #10835

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
47 changes: 47 additions & 0 deletions integration/versioning/e2e/middleware-versioning.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { INestApplication, VersioningType } from '@nestjs/common';
import { Test } from '@nestjs/testing';
import * as request from 'supertest';
import { expect } from 'chai';
import { AppWithMiddlewareModule } from '../src/app-with-middleware.module';
import * as sinon from 'sinon';
import { SpyInjectToken } from '../src/versioning-middleware';

describe('MiddlewareVersioning', () => {
let app: INestApplication;
let spy = sinon.spy();

before(async () => {
const moduleRef = await Test.createTestingModule({
imports: [AppWithMiddlewareModule],
}).compile();

app = moduleRef.createNestApplication();
app.enableVersioning({
type: VersioningType.URI,
defaultVersion: '-default-version',
});
await app.init();

spy = app.get<sinon.SinonSpy>(SpyInjectToken);
});

beforeEach(() => spy.resetHistory());

['/v-default-version/foo/bar', '/v1/', '/v3', '/v4'].forEach(path => {
it('should call middleware for route with version', async () => {
const response = await request(app.getHttpServer()).get(path);

expect(response.status).to.eq(200);
expect(spy.called).to.be.true;
});
});

it('should not call middleware if the controller route is not defined', async () => {
const response = await request(app.getHttpServer()).get('/v2/');

expect(response.status).to.eq(200);
expect(spy.called).to.be.false;
});

after(() => app.close());
});
17 changes: 17 additions & 0 deletions integration/versioning/src/app-v3.controller.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { Controller, Get, Version } from '@nestjs/common';

@Controller({
version: '3',
})
export class AppV3Controller {
@Get()
paramV3() {
return 'V3';
}

@Version('4')
@Get()
paramV4() {
return 'V4';
}
}
29 changes: 29 additions & 0 deletions integration/versioning/src/app-with-middleware.module.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { MiddlewareConsumer, Module, NestModule } from '@nestjs/common';
import { SpyInjectToken, VersioningMiddleware } from './versioning-middleware';
import { AppV1Controller } from './app-v1.controller';
import * as sinon from 'sinon';
import { AppV3Controller } from './app-v3.controller';
import { NoVersioningController } from './no-versioning.controller';
import { AppV2Controller } from './app-v2.controller';

@Module({
providers: [
{
provide: SpyInjectToken,
useValue: sinon.spy(),
},
],
controllers: [
NoVersioningController,
AppV1Controller,
AppV2Controller,
AppV3Controller,
],
})
export class AppWithMiddlewareModule implements NestModule {
configure(consumer: MiddlewareConsumer) {
consumer
.apply(VersioningMiddleware)
.forRoutes(NoVersioningController, AppV1Controller, AppV3Controller);
}
}
15 changes: 15 additions & 0 deletions integration/versioning/src/versioning-middleware.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { Inject, Injectable, NestMiddleware } from '@nestjs/common';
import * as sinon from 'sinon';
import { NextFunction } from 'express';

export const SpyInjectToken = 'SpyInjectToken';

@Injectable()
export class VersioningMiddleware implements NestMiddleware {
constructor(@Inject(SpyInjectToken) private readonly spy: sinon.SinonSpy) {}

use(req: any, res: any, next: NextFunction): any {
this.spy(req, res);
next();
}
}
5 changes: 4 additions & 1 deletion packages/core/middleware/routes-mapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ export class RoutesMapper {
private readonly pathsExplorer: PathsExplorer;

constructor(private readonly container: NestContainer) {
this.pathsExplorer = new PathsExplorer(new MetadataScanner());
this.pathsExplorer = new PathsExplorer(
new MetadataScanner(),
container.applicationConfig?.getVersioning(),
);
}

public mapRouteToRouteInfo(
Expand Down
20 changes: 16 additions & 4 deletions packages/core/router/paths-explorer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ import {
} from '@nestjs/common/constants';
import { RequestMethod } from '@nestjs/common/enums';
import { Controller } from '@nestjs/common/interfaces/controllers/controller.interface';
import { VersionValue } from '@nestjs/common/interfaces/version-options.interface';
import {
VersioningOptions,
VersionValue,
} from '@nestjs/common/interfaces/version-options.interface';
import {
addLeadingSlash,
isString,
Expand All @@ -23,7 +26,10 @@ export interface RouteDefinition {
}

export class PathsExplorer {
constructor(private readonly metadataScanner: MetadataScanner) {}
constructor(
private readonly metadataScanner: MetadataScanner,
private readonly versioningOptions?: VersioningOptions,
) {}

public scanForPaths(
instance: Controller,
Expand Down Expand Up @@ -55,20 +61,26 @@ export class PathsExplorer {
METHOD_METADATA,
prototypeCallback,
);
const version: VersionValue | undefined = Reflect.getMetadata(
const methodVersion: VersionValue | undefined = Reflect.getMetadata(
VERSION_METADATA,
prototypeCallback,
);
const controllerVersion: VersionValue | undefined = Reflect.getMetadata(
VERSION_METADATA,
prototype.constructor,
);
const path = isString(routePath)
? [addLeadingSlash(routePath)]
: routePath.map((p: string) => addLeadingSlash(p));

const globalVersion = this.versioningOptions?.defaultVersion;

return {
path,
requestMethod,
targetCallback: instanceCallback,
methodName,
version,
version: methodVersion || controllerVersion || globalVersion,
Copy link
Member

Choose a reason for hiding this comment

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

AFAIR this may cause some unexpected side-effects.

RouterExplorer#applyPathsToRouterProxy assumes that "version" attribute represents the method-level version https://github.com/nestjs/nest/pull/10835/files#diff-652791ce58d92c15826b2be408ac4b6b0b1c7269664536c3d2e89b9e6f49699fR144 while now it equals to "method"/"controller"/"global"-level.

Ideally, to avoid regressions, we should make sure not to update the "scanForPaths" method (leave it as is) :(

};
}
}
5 changes: 4 additions & 1 deletion packages/core/router/router-explorer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,10 @@ export class RouterExplorer {
private readonly routePathFactory: RoutePathFactory,
private readonly graphInspector: GraphInspector,
) {
this.pathsExplorer = new PathsExplorer(metadataScanner);
this.pathsExplorer = new PathsExplorer(
metadataScanner,
config.getVersioning(),
);

const routeParamsFactory = new RouteParamsFactory();
const pipesContextCreator = new PipesContextCreator(container, config);
Expand Down
67 changes: 66 additions & 1 deletion packages/core/test/middleware/routes-mapper.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Version } from '../../../common';
import { Version, VersioningType } from '../../../common';
import { MiddlewareConfiguration } from '../../../common/interfaces';
import { expect } from 'chai';
import { Controller } from '../../../common/decorators/core/controller.decorator';
Expand All @@ -9,6 +9,7 @@ import {
import { RequestMethod } from '../../../common/enums/request-method.enum';
import { NestContainer } from '../../injector/container';
import { RoutesMapper } from '../../middleware/routes-mapper';
import { ApplicationConfig } from '../../../core/application-config';

describe('RoutesMapper', () => {
@Controller('test')
Expand All @@ -25,8 +26,17 @@ describe('RoutesMapper', () => {
}

let mapper: RoutesMapper;
let mapperWithGlobalVersioning: RoutesMapper;
beforeEach(() => {
kamilmysliwiec marked this conversation as resolved.
Show resolved Hide resolved
mapper = new RoutesMapper(new NestContainer());

const applicationConfig = new ApplicationConfig();
applicationConfig.enableVersioning({
defaultVersion: 'defaultVersion1',
type: VersioningType.URI,
});
const container = new NestContainer(applicationConfig);
mapperWithGlobalVersioning = new RoutesMapper(container);
});

it('should map @Controller() to "ControllerMetadata" in forRoutes', () => {
Expand Down Expand Up @@ -81,4 +91,59 @@ describe('RoutesMapper', () => {
{ path: '/test2/another', method: RequestMethod.DELETE },
]);
});

@Controller({
path: 'test',
version: '1',
})
class TestRouteWithControllerVersion {
@RequestMapping({ path: 'test' })
public getTest() {}

@Version('2')
@RequestMapping({ path: 'another', method: RequestMethod.DELETE })
public getAnother() {}
}

it('should map @Controller with controller version to "ControllerMetadata"', () => {
const config = {
middleware: 'Test',
forRoutes: [TestRouteWithControllerVersion],
};

expect(mapper.mapRouteToRouteInfo(config.forRoutes[0])).to.deep.equal([
{ path: '/test/test', method: RequestMethod.GET, version: '1' },
{ path: '/test/another', method: RequestMethod.DELETE, version: '2' },
]);
});

it('should create ControllerMetadata from version defined in @Controller rather than global default version', () => {
const config = {
middleware: 'Test',
forRoutes: [TestRoute, TestRouteWithControllerVersion],
};

expect(
mapperWithGlobalVersioning.mapRouteToRouteInfo(config.forRoutes[0]),
).to.deep.equal([
{
path: '/test/test',
method: RequestMethod.GET,
version: 'defaultVersion1',
},
{
path: '/test/another',
method: RequestMethod.DELETE,
version: 'defaultVersion1',
},
{ path: '/test/versioned', method: RequestMethod.GET, version: '1' },
]);

expect(
mapperWithGlobalVersioning.mapRouteToRouteInfo(config.forRoutes[1]),
).to.deep.equal([
{ path: '/test/test', method: RequestMethod.GET, version: '1' },
{ path: '/test/another', method: RequestMethod.DELETE, version: '2' },
]);
});
});
2 changes: 1 addition & 1 deletion packages/core/test/router/routes-resolver.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ describe('RoutesResolver', () => {
versioningOptions: {
type: VersioningType.URI,
},
methodVersion: undefined,
methodVersion: '1',
methodPath: '/',
};

Expand Down