fix(core,microservices): inject the context when the tree is not durable #10809

micalevisk · 2023-01-05T21:51:34Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

regression introduce at v9.1.1

What is the current behavior?

When injecting REQUEST for non-durable providers, its value is always being context payload even when the injectable chain is not durable, like this:

@Injectable({ scope: Scope.REQUEST, durable: false })
class AppService {
  constructor(@Inject(REQUEST) private readonly req: any) {
    console.log('Is req fasly?', !!req === false) // true but should be false
  }

  isReqDefined() {
    return typeof this.req !== 'undefined' // false but should be true
  }
}

@Controller() // request-scoped but not durable, due to its dependency
class AppController {
  constructor(private readonly appService: AppService) {}

  @Get()
  getHello() {
    return {
      is_req_defined: this.appService.isReqDefined(),
    }
  }
}

What is the new behavior?

non-durable providers can inject the REQUEST (or CONTEXT. I didn't tested this patch with CONTEXT tho) regardless of the ContextIdStrategy#attach. I'm under the assumption that the context id being resolved doesn't affect the REQUEST provider for a non-durable provider.

You can try this patch:

git clone -b with-10809 https://gitlab.com/micalevisk/nestjs-bug-pr-10793
npm ci
npm start

./request.sh

Does this PR introduce a breaking change?

Yes
No

Other information

After digging a bit into the source, I didn't find any other way to fix that bug other than checking if the tree is durable in order to define what would be the value for REQUEST/CONTEXT provider.

coveralls · 2023-01-05T21:56:24Z

Pull Request Test Coverage Report for Build eb39fb09-4213-4d70-b662-1e7d6091c253

6 of 9 (66.67%) changed or added relevant lines in 2 files are covered.
11 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.002%) to 93.407%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/microservices/listeners-controller.ts	4	5	80.0%
packages/core/router/router-explorer.ts	2	4	50.0%

Files with Coverage Reduction	New Missed Lines	%
packages/microservices/listener-metadata-explorer.ts	2	87.1%
packages/core/injector/container.ts	9	88.89%

Totals
Change from base Build 3995954d-8a28-41bd-aed6-d1836b737f1e:	-0.002%
Covered Lines:	6205
Relevant Lines:	6643

💛 - Coveralls

packages/core/middleware/middleware-module.ts

vizio360 · 2023-01-12T15:27:11Z

@micalevisk I've created a small test for this but it seems to be working as expected in the most recent nest master branch.

Code here https://github.com/vizio360/nest/pull/1/files

Unless I misunderstood the problem.

vizio360 · 2023-01-12T17:15:52Z

@micalevisk I've created a small test for this but it seems to be working as expected in the most recent nest master branch.

Code here https://github.com/vizio360/nest/pull/1/files

Unless I misunderstood the problem.

Actually I didn't really test it properly. I've not set up the ContextIdStrategy, let me do that and get back to you.

And maybe I should start with your test first :)

vizio360 · 2023-01-12T17:25:34Z

@micalevisk

Ok, cloned the repo, ran npm ci and then ran the app and the request.sh script and this is what I get

> $ ./request.sh test                                                                                                                             ⬡ 16.13.2 [±with-10809 ✓]
{"is_req_defined":true}%

that is without your patch applied and nestjs core 9.2.1, so it seems to work as expected without changing anything and even without my changes in the other PR...

Am I doing something wrong?

micalevisk · 2023-01-12T19:08:53Z

@vizio360 did you use the branch main? so git clone -b main https://gitlab.com/micalevisk/nestjs-bug-pr-10793

vizio360 · 2023-01-13T08:20:04Z

@vizio360 did you use the branch main? so git clone -b main https://gitlab.com/micalevisk/nestjs-bug-pr-10793

Just tested it with main and it seems to work. But the branch of your repo should not really matter as nestjs is installed as an npm.

simone.vicentini@Simones-MacBook-Pro ~/dev/repos/vizio360/nestjs-bug-pr-10793-main                                                                                [8:18:55]
> $ ./request.sh test                                                                                                                                   ⬡ 16.13.2 [±main ✓]
{"is_req_defined":true}%

micalevisk · 2023-01-13T11:19:45Z

@vizio360 the branch do matter as one has a patch that will be applied after install while the other doesn't.

git clone -b main https://gitlab.com/micalevisk/nestjs-bug-pr-10793
cd nestjs-bug-pr-10793
npm ci # even using npm i will reproduce the same
npm start

./request.sh foo
{"is_req_defined":false}

this is what I got on linux rn. Pretty weird 🤔

vizio360 · 2023-01-13T11:39:23Z

@vizio360 the branch do matter as one has a patch that will be applied after install while the other doesn't.
git clone -b main https://gitlab.com/micalevisk/nestjs-bug-pr-10793
cd nestjs-bug-pr-10793
npm ci # even using npm i will reproduce the same
npm start

./request.sh foo
{"is_req_defined":false}
this is what I got on linux rn. Pretty weird 🤔

Oh ok, did not know it would automagically apply the patch (never used that before).

And sorry I've been switching context too many times and I see what I have done wrong... I was running the patched nestjs app from the other repo and the request script from the main repo. Apologies.

Doing it again now.

vizio360 · 2023-01-13T11:41:04Z

@micalevisk yeap same result.

simone.vicentini@Simones-MacBook-Pro ~/dev/repos/vizio360/nestjs-bug-pr-10793-main                                                                               [11:40:24]
> $ ./request.sh test                                                                                                                                   ⬡ 16.13.2 [±main ✓]
{"is_req_defined":false}%

Will properly look into it asap.

vizio360 · 2023-01-13T13:27:25Z

I did some digging and I think I know what the problem is. I do have a solution but I need to test it further.

The issue seems to be in the RouterExplorer

nest/packages/core/router/router-explorer.ts

Line 428 in 308599f

contextId.getParent ? contextId.payload : request,

If the ContextIdStrategy::attach method returns a function ContextIdResolverFn and not an object ContextIdResolver then the payload on the context is not set. See ContextIdFactory::getByRequest :

    const contextId = createContextId();
    const resolverObjectOrFunction = this.strategy.attach(contextId, request);
    if (this.isContextIdResolverWithPayload(resolverObjectOrFunction)) {
      contextId.getParent = resolverObjectOrFunction.resolve;
      contextId.payload = resolverObjectOrFunction.payload;
    } else {
      contextId.getParent = resolverObjectOrFunction; //Here the payload is not set
    }
    return contextId;

But then in the RouterExplorer::getContextId method, it checks if the context.getParent exists and if so it passes the context.payload object to the registerRequestProvider function, but that payload is empty.

The fix I tried was to set the payload in the ContextIdFactory::getByRequest method to the request object, like so:

    const contextId = createContextId();
    const resolverObjectOrFunction = this.strategy.attach(contextId, request);
    if (this.isContextIdResolverWithPayload(resolverObjectOrFunction)) {
      contextId.getParent = resolverObjectOrFunction.resolve;
      contextId.payload = resolverObjectOrFunction.payload;
    } else {
      contextId.getParent = resolverObjectOrFunction; 
      contextId.payload = request //<-- HERE
    }
    return contextId;

But I need to run all the tests to make sure this will not break other things. Plus the same logic as per RouterExplorer is in MiddlewareModule and ListenersController.

micalevisk · 2023-01-13T13:32:42Z

@vizio360 yeah that's was my first solution but see: #10793 (comment)

vizio360 · 2023-01-13T14:42:39Z

@micalevisk PR is ok, it does solve the issue.

kamilmysliwiec · 2023-02-01T11:56:49Z

lgtm

kamilmysliwiec reviewed Jan 6, 2023

View reviewed changes

packages/core/middleware/middleware-module.ts Outdated Show resolved Hide resolved

micalevisk force-pushed the fix/durable-payload-regression branch from 82518f4 to dda7ded Compare January 6, 2023 16:17

micalevisk mentioned this pull request Jan 10, 2023

fix(core): Making Request scope non durable win over scope durable #10698

Merged

12 tasks

micalevisk requested a review from kamilmysliwiec January 27, 2023 16:04

micalevisk added 2 commits January 27, 2023 12:08

fix(core,microservices): inject the context when the tree is not durable

aafdac6

refactor(core): extract get context id logic into its own method

f396dc1

micalevisk force-pushed the fix/durable-payload-regression branch from 637ceed to f396dc1 Compare January 27, 2023 16:08

kamilmysliwiec added type: bug 😭 scope: core labels Feb 1, 2023

kamilmysliwiec merged commit 6d88e3c into nestjs:master Feb 1, 2023

micalevisk deleted the fix/durable-payload-regression branch February 1, 2023 11:58

micalevisk mentioned this pull request Feb 1, 2023

request property in a service class is undefined after v9.3.1 #11002

Closed

15 tasks

kamilmysliwiec mentioned this pull request Feb 2, 2023

Revert "fix(core,microservices): inject the context when the tree is not durable" #11018

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core,microservices): inject the context when the tree is not durable #10809

fix(core,microservices): inject the context when the tree is not durable #10809

micalevisk commented Jan 5, 2023 •

edited

Loading

coveralls commented Jan 5, 2023 •

edited

Loading

vizio360 commented Jan 12, 2023 •

edited

Loading

vizio360 commented Jan 12, 2023 •

edited

Loading

vizio360 commented Jan 12, 2023

micalevisk commented Jan 12, 2023

vizio360 commented Jan 13, 2023 •

edited

Loading

micalevisk commented Jan 13, 2023 •

edited

Loading

vizio360 commented Jan 13, 2023 •

edited

Loading

vizio360 commented Jan 13, 2023

vizio360 commented Jan 13, 2023 •

edited

Loading

micalevisk commented Jan 13, 2023

vizio360 commented Jan 13, 2023

kamilmysliwiec commented Feb 1, 2023

fix(core,microservices): inject the context when the tree is not durable #10809

fix(core,microservices): inject the context when the tree is not durable #10809

Conversation

micalevisk commented Jan 5, 2023 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

coveralls commented Jan 5, 2023 • edited Loading

Pull Request Test Coverage Report for Build eb39fb09-4213-4d70-b662-1e7d6091c253

💛 - Coveralls

vizio360 commented Jan 12, 2023 • edited Loading

vizio360 commented Jan 12, 2023 • edited Loading

vizio360 commented Jan 12, 2023

micalevisk commented Jan 12, 2023

vizio360 commented Jan 13, 2023 • edited Loading

micalevisk commented Jan 13, 2023 • edited Loading

vizio360 commented Jan 13, 2023 • edited Loading

vizio360 commented Jan 13, 2023

vizio360 commented Jan 13, 2023 • edited Loading

micalevisk commented Jan 13, 2023

vizio360 commented Jan 13, 2023

kamilmysliwiec commented Feb 1, 2023

micalevisk commented Jan 5, 2023 •

edited

Loading

coveralls commented Jan 5, 2023 •

edited

Loading

vizio360 commented Jan 12, 2023 •

edited

Loading

vizio360 commented Jan 12, 2023 •

edited

Loading

vizio360 commented Jan 13, 2023 •

edited

Loading

micalevisk commented Jan 13, 2023 •

edited

Loading

vizio360 commented Jan 13, 2023 •

edited

Loading

vizio360 commented Jan 13, 2023 •

edited

Loading