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 to grab REQUEST_CONTEXT_ID #3105

Closed
kdubb opened this issue Oct 4, 2019 · 9 comments
Closed

Allow to grab REQUEST_CONTEXT_ID #3105

kdubb opened this issue Oct 4, 2019 · 9 comments

Comments

@kdubb
Copy link

@kdubb kdubb commented Oct 4, 2019

Bug Report

Current behavior

ModuleRef.resolve doesn't work correctly when resolving REQUEST scope dependencies from a controller/service. The call to resolve generates a new contextId because there is seemingly no method to get the "current" context id that generated the requesting controller.

This has the effect of creating new dependencies for a scope that will be discarded nearly immediately and items in that scope will not wire to the current request's scope. This causes problems when a dependency tries to get the current request (using @Inject(REQUEST)) because it doesn't exist in this temporary isolated scope.

Input Code

    @Injectable({scope: Scope.REQUEST))
    class Logger {
      constructor(@Inject(REQUEST) request) {}
    }

    @Injectable({scope: Scope.REQUEST))  //Scope implicit due to Logger dependency
    class SomeCommandHandler implements CommandHandler {
      constructor(private readonly logger: Logger) {...}
    }

    @Injectable({scope: Scope.REQUEST))
    class SomeController {
      constructor(private readonly module: ModuleRef) {}

      dispatch(@Request() req: any) {
        const handler = await this.module.resolve<CommandHandler>(getCommandHandlerToken(command.command));  // Maps to `SomeCommandHandler` using a custom provider
    }

Expected behavior

ModuleRef.resolve would produce a properly wired SomeCommandHandler with a Logger that has access to the current request.

Possible Solution

First when a context id is generated, if one is not already assigned to the request object...

const contextId = req[REQUEST_CONTEXT_ID] || createContextId();

it never gets assigned back to the request. This seems incorrect because it is now inaccessible anywhere outside this usage. Also, in MiddlewhereModule.registerHandler, the only other place a context is generated, it is assigned back to the request.

Second, REQUEST_CONTEXT_ID seems like it should be injectable like its sibling REQUEST is allowing easy access to it; although if the above is fixed it will be accessible via the request object itself.

Finally, ModuleRef should carry the REQUEST_CONTEXT_ID that generated it inside itself (if it was injected into a request scoped object) and resolve should use that context id if none is passed into the resolve argument. As was laid out above it currently just generates a new context id which causes all the issues.

Environment


Nest version: 6.6.3
 
For Tooling issues:
- Node version: 12.10.8
- Platform:  Mac

Others:
@kdubb kdubb added the needs triage label Oct 4, 2019
@kdubb

This comment has been minimized.

Copy link
Author

@kdubb kdubb commented Oct 4, 2019

FYI, I hacked my RouterExplorer to assign the generated context id to the request and then I passed that to ModuleRef.resolve using the request object. It then produces the correct object with all dependencies as they should be.

@hovonako

This comment has been minimized.

Copy link

@hovonako hovonako commented Oct 7, 2019

It's could be better if we have method: ModuleRef.resolveByCurrentContextId(TOKEN)

@kamilmysliwiec kamilmysliwiec changed the title ModuleRef.resolve fails with REQUEST scoped dependencies Allow to grab REQUEST_CONTEXT_ID Oct 8, 2019
@kdubb

This comment has been minimized.

Copy link
Author

@kdubb kdubb commented Oct 8, 2019

@kamilmysliwiec You changed this to "enhancement" but the problem is related to resolving dependencies from request scoped beans. Currently resolve is broken so enhancement is not the correct label.

Also, Is there any workaround that doesn't require a core change?

@kdubb

This comment has been minimized.

Copy link
Author

@kdubb kdubb commented Oct 8, 2019

For anybody experiencing the same issue, a workaround is to assign a REQUEST_CONTEXT_ID before NestJS begins handling the request.

When using the FastifyAdapter it looks like this...

fastifyInstance
  .addHook('onRequest',
           (req: FastifyRequest, reply: FastifyReply<any>, done: (err?: Error) => void) => {
             req[REQUEST_CONTEXT_ID] = { id: randomStringGenerator() } as ContextId;
             done();
           });
@kamilmysliwiec

This comment has been minimized.

Copy link
Member

@kamilmysliwiec kamilmysliwiec commented Oct 9, 2019

@kdubb resolve() isn't broken, this is how it is supposed to work. The only thing we should provide is the way to grab current request context id.

@kdubb

This comment has been minimized.

Copy link
Author

@kdubb kdubb commented Oct 9, 2019

@kamilmysliwiec First, NestJS is awesome and I am grateful for all the hardwork the team has put into it. We've rearchitected a new version of our app using it and couldn't be more pleased with it.

That being said... If I try to use resolve() (without workarounds) it crashes the current request due to missing dependencies with no warnings or error explanations. That's only after you work past an error about the undefined context id.

It required hours of debugging (considering I have no previous knowledge of NestJS) followed by a couple hours more to figure out a hack. Finally, another half day of tinkering to discover a workaround that could be shipped.

Sincerely, with all due respect... it's broken.

@kamilmysliwiec

This comment has been minimized.

Copy link
Member

@kamilmysliwiec kamilmysliwiec commented Oct 9, 2019

Can you provide a minimal repository which reproduces your issue?

@kdubb

This comment has been minimized.

Copy link
Author

@kdubb kdubb commented Oct 24, 2019

@kamilmysliwiec I've added an example to my issue example repo @ https://github.com/kdubb/nest-fastify-req-scoped

Specifically, in RequestScopedController.postResolve1 you'll notice that, in its current state, it fails to inject dependencies into the "resolved" SomeService.

You can fix the injection problem for SomeService by generating a contextId and passing it to the resolve method. Although, since it's not the same contextId as the current request is using then OtherService fails because one of its dependencies is @Inject(REQUEST), of which there is no current value.

@kamilmysliwiec

This comment has been minimized.

Copy link
Member

@kamilmysliwiec kamilmysliwiec commented Jan 28, 2020

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.