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

feat(core,common,testing): support overriding middleware for testing #12541

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

schiemon
Copy link

@schiemon schiemon commented Oct 9, 2023

closes: #4073

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

The TestingModuleBuilder in the testing package currently has no method available to override middleware à la overrideProvider or overrideModule.

Issue Number: #4073

What is the new behavior?

Now, it is possible to override middleware as it is configured by the configure method of a module:

@Module({})
class AppModule {
  configure(consumer: MiddlewareConsumer) {
    return consumer
      .apply(MiddlewareA, MiddlewareB, MiddlewareC)
      .forRoutes('*');
  }
}

const testingModule = await Test.createTestingModule({
      imports: [AppModule],
})
  .overrideMiddleware(MiddlewareA)
  .use(MiddlewareAOverride)
  .overrideMiddleware(MiddlewareC)
  .use(MiddlewareC1Override, MiddlewareC2Override)
  .compile();

As a by-product, the MiddlewareBuilder is extended by a replace method that allows for swapping out middleware:

builder
  .apply(MiddlewareA, MiddlewareB, MiddlewareC)
  .exclude(route)
  .forRoutes("*")
  .replace(MiddlewareA, MiddlewareAOverride)
  .replace(MiddlewareC, MiddlewareC1Override, MiddlewareC2Override);

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@coveralls
Copy link

coveralls commented Oct 9, 2023

Pull Request Test Coverage Report for Build 1ff17a34-ae9b-45c7-afcc-8778ae899c7c

  • 10 of 10 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 92.645%

Totals Coverage Status
Change from base Build 79434c05-5367-4a7a-a04c-0f91cc807577: 0.006%
Covered Lines: 6474
Relevant Lines: 6988

💛 - Coveralls

Copy link
Contributor

@Tony133 Tony133 left a comment

Choose a reason for hiding this comment

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

Let's leave one line space, for the rest I would say it can be fine

@schiemon
Copy link
Author

@kamilmysliwiec Any updates on this?

*
* @returns {MiddlewareConsumer}
*/
replace(
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Author

@schiemon schiemon Dec 29, 2023

Choose a reason for hiding this comment

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

To remove that method, as far as I see, we need a way to swap the MiddlewareConsumer instance with an implementation that can support middleware replacements. One could accomplish that through the following changes:

First, one enables the injection of MiddlewareModule into a NestApplication:

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 MiddlewareModule itself, we then allow the insertion of a custom MiddlewareBuilder in loadConfiguration like that:

  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 TestMiddlewareModule that inherits from MiddlewareModule into our NestApplication. Our custom TestMiddlewareModule can then override loadConfiguration which we call the loadConfiguration from the parent MiddlewareModule with a custom TestMiddlewareBuilder. The TestMiddlewareBuilder inherits from the MiddlewareBuilder, which finally supports the replace method we can use.

I am unsure of the consequences of the changes on central classes like NestApplication, but in contrast to the replace method in MiddlewareBuilder, they make sense in the respective classes (I assume this is the reason why you want the method to be removed).

What do you (or others!) think about that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OverrideMiddleware is missing
4 participants