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

fix(core): merge request context with tenant context payload in the request singleton #13485

Closed

Conversation

DylanVeldra
Copy link

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?

Doesn't create the request singleton with the contextId payload when the first request isn't to a durable tree.

Issue Number: #13477

What is the new behavior?

The payload generated by the contextIdFactory will be always merged to the request singleton, not matter if the the tree is durable or not.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

I would like to know which tests should I updated, if needed.

Also the patch doesn't merge payload from the contextIdFactory in the request passed to canActivate method for AuthGuard and exception filter.

To patch the case above:

packages/core/router/router-explorer.ts lines 389 and 402

  public createRequestScopedHandler(
    instanceWrapper: InstanceWrapper,
    requestMethod: RequestMethod,
    moduleRef: Module,
    moduleKey: string,
    methodName: string,
  ) {
    const { instance } = instanceWrapper;
    const collection = moduleRef.controllers;

    const isTreeDurable = instanceWrapper.isDependencyTreeDurable();

    return async <TRequest extends Record<any, any>, TResponse>(
      req: TRequest,
      res: TResponse,
      next: () => void,
    ) => {
      try {
        const contextId = this.getContextId(req, isTreeDurable);
        const contextInstance = await this.injector.loadPerContext(
          instance,
          moduleRef,
          collection,
          contextId,
        );
        await this.createCallbackProxy(
          contextInstance,
          contextInstance[methodName],
          methodName,
          moduleKey,
          requestMethod,
          contextId,
          instanceWrapper.id,
        )(req, res, next);
      } catch (err) {
        let exceptionFilter = this.exceptionFiltersCache.get(
          instance[methodName],
        );
        if (!exceptionFilter) {
          exceptionFilter = this.exceptionsFilter.create(
            instance,
            instance[methodName],
            moduleKey,
          );
          this.exceptionFiltersCache.set(instance[methodName], exceptionFilter);
        }
        const host = new ExecutionContextHost([req, res, next]);
        exceptionFilter.next(err, host);
      }
    };
  }

..., instanceWrapper.id, )(req, res, next);

should be replace by

..., instanceWrapper.id, )(Object.assign(req, contextId.payload), res, next);

and

const host = new ExecutionContextHost([req, res, next]);

should be replace by

const host = new ExecutionContextHost([Object.assign(req, contextId.payload), res, next]);

But I don't want to do it on my own since I would like a quick fix first, moreover maybe it will imply edge-case, I don't know the code base well 🤷‍♂️

@coveralls
Copy link

coveralls commented Apr 23, 2024

Pull Request Test Coverage Report for Build e79cdc3c-762e-40cd-acb3-21e17deb1cd9

Details

  • 1 of 2 (50.0%) changed or added relevant lines in 2 files are covered.
  • 74 unchanged lines in 13 files lost coverage.
  • Overall coverage decreased (-0.09%) to 92.124%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/core/router/router-explorer.ts 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
packages/core/router/router-explorer.ts 1 83.2%
packages/common/pipes/parse-uuid.pipe.ts 1 95.45%
packages/core/router/sse-stream.ts 1 96.77%
packages/core/scanner.ts 2 88.46%
packages/microservices/server/server-redis.ts 3 95.77%
packages/microservices/server/server.ts 4 92.19%
packages/microservices/server/server-kafka.ts 5 94.78%
packages/microservices/server/server-mqtt.ts 6 91.76%
packages/microservices/server/server-tcp.ts 6 89.47%
packages/microservices/client/client-nats.ts 8 89.47%
Totals Coverage Status
Change from base Build ce4d7ee2-3275-4dce-b5f8-4b602a80cf12: -0.09%
Covered Lines: 6737
Relevant Lines: 7313

💛 - Coveralls

@kamilmysliwiec
Copy link
Member

Also the patch doesn't merge payload from the contextIdFactory in the request passed to canActivate method for AuthGuard and exception filter.

This isn't required anyway

Copy link

@benjGam benjGam left a comment

Choose a reason for hiding this comment

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

Won't introduce any side effects ?

@DylanVeldra
Copy link
Author

I will take care of tests this week.

@DylanVeldra DylanVeldra force-pushed the fix-tenant-context-payload branch 2 times, most recently from 1bc6679 to ff09b98 Compare August 16, 2024 16:17
@DylanVeldra
Copy link
Author

I added an integration test @kamilmysliwiec let me know what do you think about it

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.

4 participants