search

nestjs / nest

A progressive Node.js framework for building efficient, scalable, and enterprise-grade server-side applications with TypeScript/JavaScript 🚀
https://nestjs.com
MIT License
67.69k stars 7.63k forks source link

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

Closed karam-mustafa closed 1 year ago

karam-mustafa commented 1 year ago

Is there an existing issue for this?

Current behavior

I have a simple constrictor for a service class as following

constructor( @Inject(REQUEST) public readonly request: Request, runAuthDependincies: Boolean = true, )

after you had released v9.3.1 this request is undefined

Minimum reproduction code

https://codesandbox.io/s/withered-monad-zgg1nu

Steps to reproduce

just updating the packages to 9.3.1

Expected behavior

The request property should not be undefined as before

Package

Other package

No response

NestJS version

9.3.1

Packages versions

"@hapi/hapi": "^21.0.0",
        "@hapi/joi": "^17.1.1",
        "@nestjs-modules/mailer": "^1.8.1",
        "@nestjs/common": "^9.0.0",
        "@nestjs/config": "^2.2.0",
        "@nestjs/core": "^9.0.0",
        "@nestjs/jwt": "^9.0.0",
        "@nestjs/passport": "^9.0.0",
        "@nestjs/platform-express": "^9.0.0",
        "apollo-server-express": "^3.11.1",
        "apollo-server-plugin-base": "^3.7.1",
        "class-transformer": "^0.5.1",
        "class-validator": "^0.13.2",
        "dotenv": "^16.0.3",
        "googleapis": "^109.0.1",
        "nodemailer": "^6.9.0",
        "passport-google-oauth20": "^2.0.0",
        "reflect-metadata": "^0.1.13",
        "rimraf": "^3.0.2",
        "rxjs": "^7.2.0"

Node.js version

16.3

In which operating systems have you tested?

Other

No response

micalevisk commented 1 year ago
jmcdo29 commented 1 year ago

Please provide a minimum reproduction repository (Git repository/StackBlitz/CodeSandbox project).

why reproductions are required

micalevisk commented 1 year ago

@karam-mustafa read this: https://github.com/nestjs/nest/pull/10809

karam-mustafa commented 1 year ago
  • are you using durable providers?
  • did you have some ContextIdStrategy? show us

here is my basic service class

import {
  google,
  Auth,
  GoogleApis,
  admin_directory_v1,
  drive_v3,
  drive_v2,
} from 'googleapis';

import { Request } from 'express';
import { Inject, Injectable, Scope } from '@nestjs/common';
import { REQUEST } from '@nestjs/core';

@Injectable({ scope: Scope.REQUEST })

export class BaseGoogleServices {
  requestProp: Request;

  constructor(
    @Inject(REQUEST) public readonly request: Request,
    runAuthDependincies: Boolean = true,
  ) {
       this.requestProp = request;
    }

    if (runAuthDependincies) {
      this.resolveAuthDependencies();
    }
  }

  resolveAuthDependencies() {
    // do some code
  }
}
jmcdo29 commented 1 year ago

I'll say it one more time.

Please provide a minimum reproduction repository (Git repository/StackBlitz/CodeSandbox project).

why reproductions are required

Otherwise, please use our Discord channel (Support). We are using GitHub to track Bug Reports, Feature Requests, and Regressions.

karam-mustafa commented 1 year ago

I'll say it one more time.

Please provide a minimum reproduction repository (Git repository/StackBlitz/CodeSandbox project).

why reproductions are required

Otherwise, please use our Discord channel (Support). We are using GitHub to track Bug Reports, Feature Requests, and Regressions.

https://codesandbox.io/s/withered-monad-zgg1nu

jmcdo29 commented 1 year ago

Your reproduction doesn't start. @nestjs/core and @nestjs/common are missing and the web preview stays on a 502 bad gateway screen. Please don't make us do the work of fixing your reproduction just so we can see your bug.

micalevisk commented 1 year ago

image

I just tested your repro (after minor changes to make it minimal as possible while valid). Your repro doesn't reproduces the issue.

full code ```ts // main.ts import { NestFactory } from '@nestjs/core'; import type { NestExpressApplication } from '@nestjs/platform-express'; import { AppModule } from './app.module'; async function bootstrap() { const app = await NestFactory.create(AppModule); await app.listen(process.env.PORT || 3000); } bootstrap(); ``` ```ts import { Module, Controller, Get, Inject, Injectable, Scope } from '@nestjs/common'; import { REQUEST } from '@nestjs/core'; import { Request } from 'express'; @Injectable({ scope: Scope.REQUEST }) class Foo { constructor( @Inject(REQUEST) public readonly request: Request, ) { console.log('===================================='); console.log(typeof request === 'undefined'); // false console.log('===================================='); } } class AppService extends Foo { constructor( @Inject(REQUEST) public readonly request: Request, ) { super(request) console.log(typeof request === 'undefined'); // false } } @Controller() class AppController { constructor(private readonly appService: AppService) {} @Get() getHello(): string { return 'ok' } } @Module({ imports: [], controllers: [AppController], providers: [AppService], }) export class AppModule {} ```
karam-mustafa commented 1 year ago

Your reproduction doesn't start. @nestjs/core and @nestjs/common are missing and the web preview stays on a 502 bad gateway screen. Please don't make us do the work of fixing your reproduction just so we can see your bug.

Sorry for this confusion, I will let you know again when the repo could reproduce the issue, thanks for your patience

davidzwa commented 1 year ago

Having the same issue:

https://codesandbox.io/p/sandbox/runtime-star-dpm9rv?file=%2Fsrc%2Fapp.module.ts&selection=%5B%7B%22endColumn%22%3A1%2C%22endLineNumber%22%3A11%2C%22startColumn%22%3A1%2C%22startLineNumber%22%3A11%7D%5D

Any tips on how I can help you refine this issue further from this point?

kamilmysliwiec commented 1 year ago

@micalevisk so I'm guessing this might be related to this PR https://github.com/nestjs/nest/pull/10809 (perhaps isDependencyTreeDurable returns true even though the tree isn't durable for some edge-case scenarios?

Drugg0ff commented 1 year ago

Same problem. Error when migrating from 9.2.1 to 9.3.1 - UnhandledPromiseRejectionWarning: TypeError: this.graphInspector.insertEntrypointDefinition is not a function

micalevisk commented 1 year ago

@Durairaj that's another one. Upgrade your @nestjs/core to 9.3

micalevisk commented 1 year ago

@kamilmysliwiec since no one shared any repro, I guess we should revert that PR for now.

micalevisk commented 1 year ago

hi @davidzwa is there any chance to share your code in a private repository with me?

or maybe you can see what was done at PR #10809 (which is pretty small) and try to debug it locally yourself :)

kamilmysliwiec commented 1 year ago

https://github.com/nestjs/nest/pull/11018

davidzwa commented 1 year ago

@micalevisk in the 9.2.1 edition neither MiddlewareModule::bindHandler or MiddlewareModule::registerHandler function is being called (no breakpoints or console logs that I added seem to trigger in the transpiled js code of nestjs). These are the places your changes seem to reside. I might try 9.3.1 soon for comparison.

Im not in the position to share the repo. Will keep you posted if I have more info to share.

micalevisk commented 1 year ago

@davidzwa another thing you could do: define the env var NEST_DEBUG=true and see if you got occurrences of 'durable' in your logs

davidzwa commented 1 year ago

That tip is really nice! I think this extract might help you:

[server-nest] Info      02/02/2023, 17:06:17 [InstanceWrapper] PrinterController introspected as request-scoped - {} +0ms
[server-nest] Info      02/02/2023, 17:06:17 Starting server at port 4000 - {} +0ms
[server-nest] Info      02/02/2023, 17:06:17 [PrinterGateway] Gateway initialized - {} +1ms
[server-nest] Info      02/02/2023, 17:06:17 [WebSocketsController] PrinterGateway subscribed to the "init-request" message - {} +0ms
[server-nest] Info      02/02/2023, 17:06:17 [AdminGateway] SocketIO AdminUI enabled. This can be disabled with: 'SOCKETIO_ADMINUI_ENABLED=false' - {} +0ms
[server-nest] Info      02/02/2023, 17:06:17 [InstanceWrapper] PrinterController introspected as durable - {} +5ms
[server-nest] Info      02/02/2023, 17:06:17 [InstanceWrapper] PrinterFileController introspected as durable - {} +0ms

Before the listen call we see PrinterController introspected as request-scoped, immediately after the listen(...) call the logs show PrinterController introspected as durable. This is the function that prints that (just making sure that I follow what is happening): https://github.com/nestjs/nest/blob/a439cd9a4120526a95295691e1ebbbce95d64f90/packages/core/injector/instance-wrapper.ts#L255

micalevisk commented 1 year ago

great, thanks

now could you please remove the !item.isDependencyTreeStatic() condition
from L121 node_modules/@nestjs/core/injector/instance-wrapper.js

https://github.com/nestjs/nest/blob/d9c394bb44711ba33cde8aa9199d9fdf21052d7e/packages/core/injector/instance-wrapper.ts#L215-L223

I'm suggesting this because that was another change introduced in 9.3.1 (PR #10698)

davidzwa commented 1 year ago

Problem's gone with your suggestion. (FYI: my post above was 9.3.1 ofcourse)

[server-nest] Info      02/02/2023, 17:58:08 [InstanceWrapper] PrinterFileController introspected as request-scoped - {} +1ms
[server-nest] Info      02/02/2023, 17:58:08 [InjectorLogger] Looking for automapper:nestjs:default in SentryCoreModule - {} +1ms
[server-nest] Info      02/02/2023, 17:58:08 [InjectorLogger] Looking for automapper:nestjs:default in TypeOrmCoreModule - {} +1ms
[server-nest] Info      02/02/2023, 17:58:08 [InjectorLogger] Looking for automapper:nestjs:default in AutomapperModule - {} +0ms
[server-nest] Info      02/02/2023, 17:58:08 [InjectorLogger] Found automapper:nestjs:default in AutomapperModule - {} +1ms
[server-nest] Info      02/02/2023, 17:58:08 [InjectorLogger] Looking for automapper:nestjs:default in CacheModule - {} +1ms
[server-nest] Info      02/02/2023, 17:58:08 [InjectorLogger] Looking for automapper:nestjs:default in SafeEventEmitterModule - {} +1ms
[server-nest] Info      02/02/2023, 17:58:08 [InjectorLogger] Looking for automapper:nestjs:default in OidcModule - {} +0ms
[server-nest] Info      02/02/2023, 17:58:08 [InstanceWrapper] PrinterController introspected as request-scoped - {} +1ms
[server-nest] Info      02/02/2023, 17:58:08 Starting server at port 4000 - {} +1ms
[server-nest] Info      02/02/2023, 17:58:08 [PrinterGateway] Gateway initialized - {} +0ms
[server-nest] Info      02/02/2023, 17:58:08 [WebSocketsController] PrinterGateway subscribed to the "init-request" message - {} +1ms
[server-nest] Info      02/02/2023, 17:58:08 [AdminGateway] SocketIO AdminUI enabled. This can be disabled with: 'SOCKETIO_ADMINUI_ENABLED=false' - {} +1ms
[server-nest] Info      02/02/2023, 17:58:08 [UserModule] Admin user found - skipping automatic creation step - {} +11ms
[server-nest] Info      02/02/2023, 17:58:08 [SettingsService] Cached entity with key settings.1 and id 1 - {} +19ms

No more 'introspected as durable for the PrinterFileController nor PrinterController.

micalevisk commented 1 year ago

@kamilmysliwiec ok so here we go

Can we revert #10698 and then revert #11018 ?

vizio360 commented 1 year ago

Right, will look at it as well as soon as I get some free time. Disappointed my first contribution broke stuff...

vizio360 commented 1 year ago

just reading through the thread, @davidzwa the example on CodeSandbox does it break when you run it locally?

micalevisk commented 1 year ago

@vizio360 it doesn't

we have no repro so far

vizio360 commented 1 year ago

@davidzwa could you check the dependencies chain of PrinterFileController and PrinterController and list them out? What I'm interested in is the scopes of each injected dependency and their dependencies.

Hopefully you don't have too many :)

SocketSomeone commented 1 year ago

Any pipes with scope REQUEST doesnt work 👍🏻

vizio360 commented 1 year ago

What is interesting is that from the log @davidzwa provided the PrinterController does get request scope initially (see first line of the log) and then it gets durable... 

[server-nest] Info      02/02/2023, 17:06:17 [InstanceWrapper] PrinterController introspected as request-scoped - {} +0ms
[server-nest] Info      02/02/2023, 17:06:17 Starting server at port 4000 - {} +0ms
[server-nest] Info      02/02/2023, 17:06:17 [PrinterGateway] Gateway initialized - {} +1ms
[server-nest] Info      02/02/2023, 17:06:17 [WebSocketsController] PrinterGateway subscribed to the "init-request" message - {} +0ms
[server-nest] Info      02/02/2023, 17:06:17 [AdminGateway] SocketIO AdminUI enabled. This can be disabled with: 'SOCKETIO_ADMINUI_ENABLED=false' - {} +0ms
[server-nest] Info      02/02/2023, 17:06:17 [InstanceWrapper] PrinterController introspected as durable - {} +5ms
[server-nest] Info      02/02/2023, 17:06:17 [InstanceWrapper] PrinterFileController introspected as durable - {} +0ms
davidzwa commented 1 year ago

I've done some really extensive digging/hacking (on my project, no NestJS internals) and removed almost everything from except for my printer controller and service. After that I went much further to delete almost everything that I could possibly delete or nullify from the dependencies. Still left with the error.

[server-nest] Info      03/02/2023, 00:01:50 [InstanceWrapper] PrinterController introspected as request-scoped - {} +0ms
[server-nest] Info      03/02/2023, 00:01:50 Starting server at port undefined - {} +3ms
[server-nest] Info      03/02/2023, 00:01:50 [InstanceWrapper] PrinterController introspected as durable - {} +5ms

So far what I think is left is a TypeORM feature called Printer (entity), AutoMapper and the two-layer mixin extensions I use to provide the controller with CRUD operations.

With a fresh mind I should be able to port this into CodeSandbox. Cant promise it tho. Hope this helps.

vizio360 commented 1 year ago

@davidzwa to clarify you are not setting up Durable Providers with a ContextidStrategy right?

davidzwa commented 1 year ago

@davidzwa to clarify you are not setting up Durable Providers with a ContextidStrategy right?

Indeed, none of my services (or anything) follow the durable provider pattern described by https://docs.nestjs.com/fundamentals/injection-scopes#durable-providers

All is purely transient, singleton or request scoped.

davidzwa commented 1 year ago

I think I found a hint towards the problem! :) It is here: this.isTreeDurable = !isTreeNonDurable && this.durable !== false;

In my case this.durable === undefined. How that is I have no idea, but it fails the check as well as the !item.isDependenceyTreeStatic() check.

vizio360 commented 1 year ago

mm that should be ok as the first !isTreeNonDurable should return false so the second should not even be considered

But I'm still confused as I've now got a small app with a controller and a few different providers with different scopes (singleton, request, transient) and I cannot reproduce the issue...

davidzwa commented 1 year ago

Removing AutoMapper has removed the problem, but I must be fair: I've no idea why AutoMapper would be a durable provider or a reason for causing the static check to fail.

vizio360 commented 1 year ago

what does that pesky automapper do???? :)

davidzwa commented 1 year ago

Will attempt to repro this soon, if time allows.

vizio360 commented 1 year ago

I'm a bit ignorant on AutoMapper, could you point me to its repo?

micalevisk commented 1 year ago

https://automapperts.netlify.app/

davidzwa commented 1 year ago

https://www.npmjs.com/package/@automapper/nestjs https://github.com/nartc/mapper/tree/main

davidzwa commented 1 year ago

this.durable === undefined

I still hope you agree that this.durable !== false should not result in true if there is clearly no durability (durable: undefined). That's an edge case in my view.

vizio360 commented 1 year ago

wondering if we could replace that with this.isTreeDurable = !isTreeNonDurable && this.durable

kamilmysliwiec commented 1 year ago

Can we revert https://github.com/nestjs/nest/pull/10698 and then revert https://github.com/nestjs/nest/pull/11018 ?

https://github.com/nestjs/nest/pull/11018 reverted. Now - not sure what we should do about this one https://github.com/nestjs/nest/pull/10698 - sounds like this issue only occurs when AutoMapper is included in the project?

vizio360 commented 1 year ago

@karam-mustafa are you also using AutoMapper?

vizio360 commented 1 year ago

@davidzwa I managed to force the error caused by this.durable !== false. Repro repo here https://github.com/vizio360/nestJSDurableProvidersIssue/tree/nonExplicitDurability with description and instructions in the README file. But this is only when using a ContextIdStrategy.

Will create an issue, even though this is tested on nestjs 9.3.1 and we still don't know if the PRs that have been reverted can be reapplied. I think it will depend on what we understand about the issue with AutoMapper.

vizio360 commented 1 year ago

Can we revert #10698 and then revert #11018 ?

11018 reverted. Now - not sure what we should do about this one #10698 - sounds like this issue only occurs when AutoMapper is included in the project?

@kamilmysliwiec I will try and get automapper in my repro repo, and see if I can figure out what the issue is.

karam-mustafa commented 1 year ago

@karam-mustafa are you also using AutoMapper?

I didn't install it before

micalevisk commented 1 year ago

@karam-mustafa ~it's working now in v9.3.2 right?~ nvm, it shouldn't

kamilmysliwiec commented 1 year ago

@micalevisk note - I didn't revert https://github.com/nestjs/nest/pull/10698 yet

vizio360 commented 1 year ago

@kamilmysliwiec @micalevisk just experienced the problem in our code base with nestjs 9.3.2 and we do not use AutoMapper.

micalevisk commented 1 year ago

that's expected as your PR wasn't reverted yet (I thought it was :p).

So now you have a reproduction of that bug that you can work with, right?