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

Regression: ContextIdStrategy and Injection of Request into Custom Provider No Longer Works #10504

Closed
2 tasks done
ccaspers opened this issue Nov 2, 2022 · 3 comments
Closed
2 tasks done
Labels
needs triage This issue has not been looked into type: bug 😭

Comments

@ccaspers
Copy link

ccaspers commented Nov 2, 2022

Did you read the migration guide?

  • I have read the whole migration guide

Is there an existing issue that is already proposing this?

  • I have searched the existing issues

Potential Commit/PR that introduced the regression

No response

NestJS version

9.0.11 -> 9.1.1+

Describe the regression

Injecting the request into a factory provider no longer works if the provider is subject to a custom context id strategy. Request object is undefined.

When you look at the implementation in the linked repo, there is one module that deals with multitenancy. It contains a ContextIdStrategy that is more or less a copy of the one that can be found in the official documentation. It is used to create a DI subtree based on a tenant-id http header.

The module also contains a factory provider that is used to inject the tenant id, when it needs to be used by a service. In the linked repo, the echo-service is injected with the tenant-id from the request via the factory provider. Then the echo service exposes the tenantId to the controller which sends it back to the caller.

When using nestjs 9.0.11, the code of the repo works without issues. When using 9.1.1+ the code breaks.

Minimum reproduction code

https://github.com/ccaspers/nestjs-context-bug

Input code

No response

Expected behavior

The factory provider is injected with the request.

Other

No response

@ccaspers ccaspers added needs triage This issue has not been looked into type: bug 😭 labels Nov 2, 2022
@CodyTseng
Copy link
Contributor

CodyTseng commented Nov 2, 2022

IMO, the problem stems from this PR #10299. And I think this is a breaking change. 🤔

this.container.registerRequestProvider(
contextId.getParent ? contextId.payload : request,
contextId,
);

Currently, you can let your MultiTenantStrategy return a ContextIdResolver object to resolve the problem.

return {
  resolve: (info: HostComponentInfo) => {
    const context = info.isTreeDurable ? tenantSubTreeId : contextId;
    return context;
  },
  payload: request,
};

@CodyTseng
Copy link
Contributor

this.container.registerRequestProvider(
contextId.getParent ? contextId.payload : request,
contextId,
);

I think contextId.payload should be judged here instead of contextId.getParent to avoid BREAKING CHANGE like this:

 this.container.registerRequestProvider( 
   contextId.payload ?? request, 
   contextId, 
 ); 

Not sure if there will be other effects.

@kamilmysliwiec
Copy link
Member

request object was never meant to be auto-registered for durable trees as that would simply lead to memory leaks. This behavior was never documented & described in the docs. We fixed this issue in 9.1.1.

I just updated the docs with instructions on how to register a per-durable-tree request provider, using the payload object https://docs.nestjs.com/fundamentals/injection-scopes#durable-providers

image

@nestjs nestjs locked and limited conversation to collaborators Nov 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs triage This issue has not been looked into type: bug 😭
Projects
None yet
Development

No branches or pull requests

3 participants