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

Updates to context are not propagated to all operational providers #2460

Closed
fridaystreet opened this issue Nov 2, 2023 · 2 comments
Closed
Assignees
Labels
waiting-for-answer Waiting for answer from author

Comments

@fridaystreet
Copy link

When using operational scope, updates to the context via middleware or schema directives is not passed to all providers in all modules. This becomes apparent for example when you have a directive on a query who's resolver is a provider in one module but the response type has child filed resolvers on another module

Say you have something like

Task module

Query {
   myTasks: Tasks! @auth
}

type Task {
  id: ID
  notifications: [Notification]
} 

(sudo resolver)
myQuery -> MyTaskProvider-> myTasks

Notifications Module

type Notification {
  id: ID
  message: String
}

(sudo resolver)
Notification -> NotificationProvider-> getNotification

When you query myTasks, the @auth directive runs and adds the user to the context. The task provider uses the user in the context no problem, but once it's handed to the the notification provider, by that point the Notification provider has already been instatiated for the operation and when getModuleContext is called, it's context is populated from the cache which was created when all the operational providers where instantiated, which doesn't include the updates that were applied from the @auth directive. This is just one example, there are other scenarios where the updated context is then not available, for instance using another module which is a service that isn't actually directly involved in the resolver chain.

Seems to be a couple of things going on from what we can see and just wanted to get some clarity on what The getModuleContext function is trying to achieve.

Firstly as described above it pulls the context from the cached context all the time and never checks to see if the context has been updated.

Secondly, when it doesn set the context, it only sets the context for the operational providers of the currently requested module.

The current function is here

Below is our current workaround. But as I mentioned, we would like to understand a bit more about the intent here as we're not entirley sure what else this could be affecting as yet. Aside from what seems to be a performance improvment by not calculating the context everytime, which our below fix lacks. Possibly improvement would be to compare the incoming context first, we just haven't gone that far yet.

It's worth noting we are running this in an AWS lambda environment with yoga. We have tried running in singleton mode without luck and were hitting this issue #2227. Going back to operational mode works with our fix to the cached context.

workaround

function getModuleContext(
      moduleId: string,
      ctx: GraphQLModules.GlobalContext
    ): GraphQLModules.ModuleContext {
      // Reuse a context or create if not available

      //update: don't use the cache
      //if (!contextCache[moduleId]) {
        // We're interested in operation-scoped providers only
       // const providers = modulesMap.get(moduleId)?.operationProviders!;

        //update: get all the providers not just the ones for this module
         const providers = flatten(Array.from(modulesMap.values()).map(a => a.operationProviders));
        
        // Create module-level Operation-scoped Injector
        const operationModuleInjector = ReflectiveInjector.createFromResolved({
          name: `Module "${moduleId}" (Operation Scope)`,
          providers: providers.concat(
            ReflectiveInjector.resolve([
              {
                provide: CONTEXT,
                useFactory() {
                 //update: always inject the latest context not the cached context
                  return ctx
                 // return contextCache[moduleId];
                },
              },
            ])
          ),
          // This injector has a priority
          parent: modulesMap.get(moduleId)!.injector,
          // over this one
          fallbackParent: operationAppInjector,
        });

        // Same as on application level, we need to collect providers with OnDestroy hooks
        registerProvidersToDestroy(operationModuleInjector);

        contextCache[moduleId] = merge(ctx, {
          injector: operationModuleInjector,
          moduleId,
        });
      //}

      return contextCache[moduleId];
    }

Any assistance or thoughts would be greatly appreciated

@enisdenjo
Copy link
Collaborator

enisdenjo commented Nov 2, 2023

Hey @fridaystreet, thanks for reporting! Can you try out graphql-modules@2.2.1-alpha-20231102140257-e36ba0ac please (it fixes #2227)? #2461 has potential to fix your issue too. Thanks!

@enisdenjo enisdenjo self-assigned this Nov 2, 2023
@Urigo Urigo added the waiting-for-answer Waiting for answer from author label Nov 19, 2023
@enisdenjo
Copy link
Collaborator

Closing due to staleness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-answer Waiting for answer from author
Projects
None yet
Development

No branches or pull requests

3 participants