-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
feat(core,common,testing): support overriding middleware for testing #12541
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
base: master
Are you sure you want to change the base?
Changes from all commits
2b85c08
5ad75c0
20697bb
b07819e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,192 @@ | ||
import { | ||
Injectable, | ||
MiddlewareConsumer, | ||
Module, | ||
NestMiddleware, | ||
} from '@nestjs/common'; | ||
import { Test } from '@nestjs/testing'; | ||
import * as request from 'supertest'; | ||
import { expect } from 'chai'; | ||
|
||
describe('Middleware overriding', () => { | ||
@Injectable() | ||
class MiddlewareA implements NestMiddleware { | ||
use(req, res, next) { | ||
middlewareAApplied = true; | ||
next(); | ||
} | ||
} | ||
|
||
function MiddlewareAOverride(req, res, next) { | ||
middlewareAOverrideApplied = true; | ||
next(); | ||
} | ||
|
||
function MiddlewareB(req, res, next) { | ||
middlewareBApplied = true; | ||
next(); | ||
} | ||
|
||
@Injectable() | ||
class MiddlewareBOverride implements NestMiddleware { | ||
use(req, res, next) { | ||
middlewareBOverrideApplied = true; | ||
next(); | ||
} | ||
} | ||
|
||
@Injectable() | ||
class MiddlewareC implements NestMiddleware { | ||
use(req, res, next) { | ||
middlewareCApplied = true; | ||
next(); | ||
} | ||
} | ||
|
||
@Injectable() | ||
class MiddlewareC1Override implements NestMiddleware { | ||
use(req, res, next) { | ||
middlewareC1OverrideApplied = true; | ||
next(); | ||
} | ||
} | ||
|
||
function MiddlewareC2Override(req, res, next) { | ||
middlewareC2OverrideApplied = true; | ||
next(); | ||
} | ||
|
||
@Module({}) | ||
class AppModule { | ||
configure(consumer: MiddlewareConsumer) { | ||
return consumer | ||
.apply(MiddlewareA) | ||
.forRoutes('a') | ||
.apply(MiddlewareB) | ||
.forRoutes('b') | ||
.apply(MiddlewareC) | ||
.forRoutes('c'); | ||
} | ||
} | ||
|
||
let middlewareAApplied: boolean; | ||
let middlewareAOverrideApplied: boolean; | ||
|
||
let middlewareBApplied: boolean; | ||
let middlewareBOverrideApplied: boolean; | ||
|
||
let middlewareCApplied: boolean; | ||
let middlewareC1OverrideApplied: boolean; | ||
let middlewareC2OverrideApplied: boolean; | ||
|
||
const resetMiddlewareApplicationFlags = () => { | ||
middlewareAApplied = | ||
middlewareAOverrideApplied = | ||
middlewareBApplied = | ||
middlewareBOverrideApplied = | ||
middlewareCApplied = | ||
middlewareC1OverrideApplied = | ||
middlewareC2OverrideApplied = | ||
false; | ||
}; | ||
|
||
beforeEach(() => { | ||
resetMiddlewareApplicationFlags(); | ||
}); | ||
|
||
it('should override class middleware', async () => { | ||
const testingModule = await Test.createTestingModule({ | ||
imports: [AppModule], | ||
}) | ||
.overrideMiddleware(MiddlewareA) | ||
.use(MiddlewareAOverride) | ||
.overrideMiddleware(MiddlewareC) | ||
.use(MiddlewareC1Override, MiddlewareC2Override) | ||
.compile(); | ||
|
||
const app = testingModule.createNestApplication(); | ||
await app.init(); | ||
|
||
await request(app.getHttpServer()).get('/a'); | ||
|
||
expect(middlewareAApplied).to.be.false; | ||
expect(middlewareAOverrideApplied).to.be.true; | ||
expect(middlewareBApplied).to.be.false; | ||
expect(middlewareBOverrideApplied).to.be.false; | ||
expect(middlewareCApplied).to.be.false; | ||
expect(middlewareC1OverrideApplied).to.be.false; | ||
expect(middlewareC2OverrideApplied).to.be.false; | ||
resetMiddlewareApplicationFlags(); | ||
|
||
await request(app.getHttpServer()).get('/b'); | ||
|
||
expect(middlewareAApplied).to.be.false; | ||
expect(middlewareAOverrideApplied).to.be.false; | ||
expect(middlewareBApplied).to.be.true; | ||
expect(middlewareBOverrideApplied).to.be.false; | ||
expect(middlewareCApplied).to.be.false; | ||
expect(middlewareC1OverrideApplied).to.be.false; | ||
expect(middlewareC2OverrideApplied).to.be.false; | ||
resetMiddlewareApplicationFlags(); | ||
|
||
await request(app.getHttpServer()).get('/c'); | ||
|
||
expect(middlewareAApplied).to.be.false; | ||
expect(middlewareAOverrideApplied).to.be.false; | ||
expect(middlewareBApplied).to.be.false; | ||
expect(middlewareBOverrideApplied).to.be.false; | ||
expect(middlewareCApplied).to.be.false; | ||
expect(middlewareC1OverrideApplied).to.be.true; | ||
expect(middlewareC2OverrideApplied).to.be.true; | ||
resetMiddlewareApplicationFlags(); | ||
|
||
await app.close(); | ||
}); | ||
|
||
it('should override functional middleware', async () => { | ||
const testingModule = await Test.createTestingModule({ | ||
imports: [AppModule], | ||
}) | ||
.overrideMiddleware(MiddlewareB) | ||
.use(MiddlewareBOverride) | ||
.compile(); | ||
|
||
const app = testingModule.createNestApplication(); | ||
await app.init(); | ||
|
||
await request(app.getHttpServer()).get('/a'); | ||
|
||
expect(middlewareAApplied).to.be.true; | ||
expect(middlewareAOverrideApplied).to.be.false; | ||
expect(middlewareBApplied).to.be.false; | ||
expect(middlewareBOverrideApplied).to.be.false; | ||
expect(middlewareCApplied).to.be.false; | ||
expect(middlewareC1OverrideApplied).to.be.false; | ||
expect(middlewareC2OverrideApplied).to.be.false; | ||
resetMiddlewareApplicationFlags(); | ||
|
||
await request(app.getHttpServer()).get('/b'); | ||
|
||
expect(middlewareAApplied).to.be.false; | ||
expect(middlewareAOverrideApplied).to.be.false; | ||
expect(middlewareBApplied).to.be.false; | ||
expect(middlewareBOverrideApplied).to.be.true; | ||
expect(middlewareCApplied).to.be.false; | ||
expect(middlewareC1OverrideApplied).to.be.false; | ||
expect(middlewareC2OverrideApplied).to.be.false; | ||
resetMiddlewareApplicationFlags(); | ||
|
||
await request(app.getHttpServer()).get('/c'); | ||
|
||
expect(middlewareAApplied).to.be.false; | ||
expect(middlewareAOverrideApplied).to.be.false; | ||
expect(middlewareBApplied).to.be.false; | ||
expect(middlewareBOverrideApplied).to.be.false; | ||
expect(middlewareCApplied).to.be.true; | ||
expect(middlewareC1OverrideApplied).to.be.false; | ||
expect(middlewareC2OverrideApplied).to.be.false; | ||
resetMiddlewareApplicationFlags(); | ||
|
||
await app.close(); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
{ | ||
"compilerOptions": { | ||
"module": "commonjs", | ||
"declaration": true, | ||
"removeComments": true, | ||
"emitDecoratorMetadata": true, | ||
"experimentalDecorators": true, | ||
"allowSyntheticDefaultImports": true, | ||
"target": "ES2021", | ||
"sourceMap": true, | ||
"outDir": "./dist", | ||
"baseUrl": "./", | ||
"incremental": true, | ||
"skipLibCheck": true | ||
}, | ||
"include": ["src/**/*"] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,4 +16,17 @@ export interface MiddlewareConsumer { | |
* @returns {MiddlewareConfigProxy} | ||
*/ | ||
apply(...middleware: (Type<any> | Function)[]): MiddlewareConfigProxy; | ||
|
||
/** | ||
* Replaces the currently applied middleware with a new (set of) middleware. | ||
* | ||
* @param {Type | Function} middlewareToReplace middleware class/function to be replaced. | ||
* @param {(Type | Function)[]} middlewareReplacement middleware class/function(s) that serve as a replacement for {@link middlewareToReplace}. | ||
* | ||
* @returns {MiddlewareConsumer} | ||
*/ | ||
replace( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm generally OK with this PR except for this change. Is there any chance we could get rid of this method? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively, we could perform following changes to facilitate middleware replacement: First, one enables the injection of constructor(
container: NestContainer,
private readonly httpAdapter: HttpServer,
private readonly config: ApplicationConfig,
private readonly graphInspector: GraphInspector,
private readonly middlewareModule: MiddlewareModule = new MiddlewareModule(), // `middlewareModule` is now injected and not internally initialized
appOptions: NestApplicationOptions = {},
) {
... In the public async loadConfiguration(
middlewareContainer: MiddlewareContainer,
moduleRef: Module,
moduleKey: string,
middlewareBuilder?: MiddlewareBuilder, // We now can specify the builder we want to use to configure the modules
) {
...
middlewareBuilder =
middlewareBuilder ??
new MiddlewareBuilder(
this.routesMapper,
this.httpAdapter,
this.routeInfoPathExtractor,
);
... With that in place, we can then inject a I am unsure of the consequences of the changes on central classes like What do you (or others!) think about that? |
||
middlewareToReplace: Type<any> | Function, | ||
...middlewareReplacement: (Type<any> | Function)[] | ||
): MiddlewareConsumer; | ||
} |
Uh oh!
There was an error while loading. Please reload this page.