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

EntityManager injection broken >5.1.2 with custom factory using PinoLogger and built via ncc #128

Open
AndKiel opened this issue Jul 18, 2023 · 17 comments
Assignees
Labels
bug Something isn't working

Comments

@AndKiel
Copy link

AndKiel commented Jul 18, 2023

Describe the bug
EntityManager (MongoEntityManager) dependency injection cannot be resolved when using @mikro-orm/nestjs >5.1.2 with .forRootAsync and PinoLogger. It works fine without injecting the logger into custom useFactory. Happens only when executing built code.

Stack trace

$ node ./dist/index.js
[Nest] 28664  - 07/18/2023, 11:02:59 AM     LOG [NestFactory] Starting Nest application...
[Nest] 28664  - 07/18/2023, 11:02:59 AM     LOG [InstanceLoader] AppModule dependencies initialized +97ms
[Nest] 28664  - 07/18/2023, 11:02:59 AM     LOG [InstanceLoader] MikroOrmModule dependencies initialized +0ms
[Nest] 28664  - 07/18/2023, 11:02:59 AM   ERROR [ExceptionHandler] Nest can't resolve dependencies of the MongoService (?). Please make sure that the argument MongoEntityManager at index [0] is available in the MongoModule context.

Potential solutions:
- Is MongoModule a valid NestJS module?
- If MongoEntityManager is a provider, is it part of the current MongoModule?
- If MongoEntityManager is exported from a separate @Module, is that module imported within MongoModule?
  @Module({
    imports: [ /* the Module containing MongoEntityManager */ ]
  })

Error: Nest can't resolve dependencies of the MongoService (?). Please make sure that the argument MongoEntityManager at index [0] is available in the MongoModule context.

Potential solutions:
- Is MongoModule a valid NestJS module?
- If MongoEntityManager is a provider, is it part of the current MongoModule?
- If MongoEntityManager is exported from a separate @Module, is that module imported within MongoModule?
  @Module({
    imports: [ /* the Module containing MongoEntityManager */ ]
  })

    at Injector.lookupComponentInParentModules (/Users/akieltyka/RubymineProjects/DeepCrawl/mikro-orm-nestjs-bug/webpack:/mikro-orm-nestjs-bug/node_modules/@nestjs/core/injector/injector.js:254:1)
    at Injector.resolveComponentInstance (/Users/akieltyka/RubymineProjects/DeepCrawl/mikro-orm-nestjs-bug/webpack:/mikro-orm-nestjs-bug/node_modules/@nestjs/core/injector/injector.js:207:1)
    at resolveParam (/Users/akieltyka/RubymineProjects/DeepCrawl/mikro-orm-nestjs-bug/webpack:/mikro-orm-nestjs-bug/node_modules/@nestjs/core/injector/injector.js:128:1)
    at async Promise.all (index 0)
    at Injector.resolveConstructorParams (/Users/akieltyka/RubymineProjects/DeepCrawl/mikro-orm-nestjs-bug/webpack:/mikro-orm-nestjs-bug/node_modules/@nestjs/core/injector/injector.js:143:1)
    at Injector.loadInstance (/Users/akieltyka/RubymineProjects/DeepCrawl/mikro-orm-nestjs-bug/webpack:/mikro-orm-nestjs-bug/node_modules/@nestjs/core/injector/injector.js:70:1)
    at Injector.loadProvider (/Users/akieltyka/RubymineProjects/DeepCrawl/mikro-orm-nestjs-bug/webpack:/mikro-orm-nestjs-bug/node_modules/@nestjs/core/injector/injector.js:97:1)
    at /Users/akieltyka/RubymineProjects/DeepCrawl/mikro-orm-nestjs-bug/webpack:/mikro-orm-nestjs-bug/node_modules/@nestjs/core/injector/instance-loader.js:56:1
    at async Promise.all (index 3)
    at InstanceLoader.createInstancesOfProviders (/Users/akieltyka/RubymineProjects/DeepCrawl/mikro-orm-nestjs-bug/webpack:/mikro-orm-nestjs-bug/node_modules/@nestjs/core/injector/instance-loader.js:55:1)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

To Reproduce
package.json

{
  "name": "mikro-orm-nestjs-bug",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "scripts": {
    "compile": "tsc",
    "build": "ncc build --out dist --source-map ./reproduction.ts",
    "run:ts-node": "yarn ts-node ./reproduction.ts",
    "run:built": "node ./dist/index.js"
  },
  "resolutions": {
    "mongodb": "4.1.4"
  },
  "dependencies": {
    "@mikro-orm/core": "5.7.13",
    "@mikro-orm/mongodb": "5.7.13",
    "@mikro-orm/nestjs": "5.2.0",
    "@nestjs/common": "10.1.0",
    "@nestjs/config": "3.0.0",
    "@nestjs/core": "10.1.0",
    "nestjs-pino": "3.3.0",
    "pino-http": "8.3.3",
    "reflect-metadata": "0.1.13",
    "source-map-support": "0.5.21",
    "tslib": "2.6.0"
  },
  "devDependencies": {
    "@nestjs/cli": "10.1.4",
    "@nestjs/schematics": "10.0.1",
    "@nestjs/testing": "10.0.5",
    "@types/node": "18.16.19",
    "@vercel/ncc": "0.36.1",
    "ts-node": "10.9.1",
    "typescript": "5.1.6"
  }
}

tsconfig.json

{
  "compilerOptions": {
    "target": "es2021",
    "lib": ["es2021"],
    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,
    "module": "commonjs",
    "esModuleInterop": true,
    "forceConsistentCasingInFileNames": true,
    "strict": true,
    "skipLibCheck": true
  }
}

reproduction.ts

import "reflect-metadata";

import { BaseEntity, Entity, PrimaryKey, SerializedPrimaryKey } from "@mikro-orm/core";
import { EntityManager, ObjectId } from "@mikro-orm/mongodb";
import { MikroOrmModule } from "@mikro-orm/nestjs";
import { INestApplicationContext, Injectable, Module, Scope } from "@nestjs/common";
import { NestFactory } from "@nestjs/core";
import { LoggerModule, PinoLogger } from "nestjs-pino";

@Entity({ collection: "entities" })
export class MongoEntity extends BaseEntity<MongoEntity, "id"> {
  @PrimaryKey()
  public _id!: ObjectId;

  @SerializedPrimaryKey()
  public id!: string;
}


@Injectable({ scope: Scope.TRANSIENT })
export class MongoService {
  private entityManager: EntityManager;

  constructor(entityManager: EntityManager) {
    this.entityManager = entityManager.fork();
  }
}

@Module({ providers: [MongoService] })
export class MongoModule {}

@Module({
  imports: [
    LoggerModule.forRoot(),
    MikroOrmModule.forRootAsync({
      inject: [PinoLogger],
      useFactory: (logger: PinoLogger) => {
        logger.setContext("MikroOrm");

        return {
          type: "mongo",
          clientUrl: "mongodb://127.0.0.1:27017/dbName", // run a local dockerized MongoDB v3 instance
          dbName: "dbName",
          entities: [MongoEntity],
          debug: ["query"], // Log all queries
          logger: message => logger.info(message),
        };
      },
    }),
    MongoModule,
  ],
})
class AppModule {}

let applicationContext: INestApplicationContext;

if (require.main === module) {
  (async () => {
    applicationContext = await NestFactory.createApplicationContext(AppModule);
    applicationContext = await applicationContext.init();
    await applicationContext.resolve(MongoService);
    await applicationContext.close();
    process.exit(0)
  })().catch(async error => {
    await applicationContext?.close();
    throw error;
  });
}
yarn install
yarn build
yarn run:built

Expected behavior
I expect the EntityManager to get injected correctly and work as it was working before in version 5.1.2.

Additional context
The error happens only when running a built .js file. It does not occur when running .ts file via ts-node. The mongodb dependency is locked to version 4.1.4 because of an old MongoDB v3 instance that needs to be connected to.

Versions

Dependency Version
node 18.16.1
typescript 5.1.6
mikro-orm 5.7.13
mongodb 4.1.4
@AndKiel AndKiel added the bug Something isn't working label Jul 18, 2023
@B4nan
Copy link
Member

B4nan commented Jul 18, 2023

Can you please provide a complete repro as a GH repository?

Have you tried it without ncc? Could be some issue in there rather than here, since things seem to be working fine with regular tsc build on my end.

constructor(entityManager: EntityManager) {
this.entityManager = entityManager.fork();
}

This is a pretty bad idea, you are just getting around the validation, but still using a single global EM fork. This will almost certainly have unwanted side effects.

@AndKiel
Copy link
Author

AndKiel commented Jul 18, 2023

The reproduction above is complete (package.json, tsconfig.json, reproduction.ts). You'd find nothing more in a repository. The only thing that can be added is any MongoDB v3 instance which can be easily created via Docker:

docker-compose.yml

version: "3.7"
services:
  db-mongo:
    image: "public.ecr.aws/docker/library/mongo:3"
    healthcheck:
      test: ["CMD-SHELL", 'mongo --eval ''db.runCommand("ping").ok'' localhost:27017/test --quiet']
      interval: 10s
      timeout: 2s
      retries: 10
    ports:
      - "27017:27017"

If the version 5.1.2 works fine with ncc and any version above does not, then I'm inclined to say some change here may be at fault.

This reproduction merely shows the error and how to cause it. It does not show the actual business use case which is AWS EventBridgeEvent lambda that gets/resolves various services using cached and initialized application context assigned to let.

@B4nan
Copy link
Member

B4nan commented Jul 18, 2023

The reproduction above is complete (package.json, tsconfig.json, reproduction.ts). You'd find nothing more in a repository. The only thing that can be added is any MongoDB v3 instance which can be easily created via Docker:

I am asking because I want to spare my time, as I have a huge amount of other issues I need to review these days. I don't want to copy past things and create files.

Sounds like you value your time more than mine. That's fine, but don't expect me to also put more value on yours...

@AndKiel
Copy link
Author

AndKiel commented Jul 19, 2023

@B4nan
Copy link
Member

B4nan commented Jul 19, 2023

Thanks!

Maybe this one is related? vercel/ncc#776

@AndKiel
Copy link
Author

AndKiel commented Jul 19, 2023

Doesn't seem related. I have no issues with decorators in different projects using exactly the same ncc version and other dependencies but with sequelize instead mikro-orm. And @mikro-orm/nestjs v5.1.2 worked fine before.

@B4nan
Copy link
Member

B4nan commented Jul 19, 2023

You keep talking about @mikro-orm/nestjs v5.1.2, but this package hasn't been released for quite some time, so I guess you mean the ORM packages, not the nest adapter?

@AndKiel
Copy link
Author

AndKiel commented Jul 19, 2023

No, I mean exactly what I said above and I wrote the same in the issue description. @mikro-orm/nestjs v5.1.2 works. Upgrading to a higher version breaks built code execution.

@B4nan
Copy link
Member

B4nan commented Jul 19, 2023

Oh ok, so the problem is in this package. I was suspecting some changes I did in v5.7.13 released a few days ago. Will try to look into this later this week.

@csechrist
Copy link

It appears that the issue is that it is calling the useFactory method without actually injecting any of the injected values. I ran into this with the config service not being injected after updating. I am not sure why it broke only on 5.7.13 (for me).

@AndKiel
Copy link
Author

AndKiel commented Jul 20, 2023

I've added an alternative reproduction to the reproduction repository - one using InjectRepository instead of EntityManager. This variant throws the error even when using ts-node to execute the file.


constructor(entityManager: EntityManager) {
this.entityManager = entityManager.fork();
}

This is a pretty bad idea, you are just getting around the validation, but still using a single global EM fork. This will almost certainly have unwanted side effects.

Regarding this, what would be the proper way of doing things for applications using NestFactory.createApplicationContext? What I mean is for example EventBridgeEvent lambdas where there are no HTTP requests so there is also no request context. This manual forking was my "workaround" because with InjectRepository it threw ValidationError. Is passing the scope property to the forRootAsync the proper way?

@B4nan
Copy link
Member

B4nan commented Jul 20, 2023

Is passing the scope property to the forRootAsync the proper way?

I am not sure if that would help, it controls the nestjs DI scopes, and since there are no requests, I'd expect there won't be any request context in the DI either, but maybe I am wrong, haven't used nest in a long time.

You could create the context explicitly via RequestContext.createAsync() or use the @UseRequestContext decorator.

https://mikro-orm.io/docs/usage-with-nestjs#request-scoped-handlers-in-queues

@B4nan
Copy link
Member

B4nan commented Jul 20, 2023

The second test is missing forFeature call, which registers the repositories to the DI. If you import that in the MongoModule where you are using it, then it starts to work (including the built version).

@Module({
  providers: [MongoService],
  imports: [
    MikroOrmModule.forFeature([MongoEntity]),
  ],
})
export class MongoModule {}

@AndKiel
Copy link
Author

AndKiel commented Jul 21, 2023

Ah, you're right, silly mistake.

I fiddled with scope a bit and found out that:

  • If scope is not passed, using the repository throws ValidationError. More or less expected because there is no request scope as mentioned in previous comments.
  • If scope is passed:
    • applicationContext.get works but repository becomes undefined (it's not injected) and .resolve must be used instead. This seems like a bug.
    • Each resolved MongoService instance gets EntityManager with _id incremented by 1. This seems correct and wanted behaviour so that application behaves as if there was a request context even if there isn't one.

It seems to be working fine both with ts-node and with code built via ncc. Maybe usage with NestFactory.createApplicationContext should be documented somehow if the above is how it's supposed to work.

Now, should the initial reproduction be considered an actual bug or an incorrect usage due to the above? If it's incorrect usage, then .get behaviour may need investigation as to why repository is not being injected at all when scope is present.

@B4nan
Copy link
Member

B4nan commented Jul 21, 2023

The thing is I am not completely sure how it is supposed to be working in nest di, it's been years since I used it in an actual project (same for any web app, I no longer develop those). I checked how the official nest/typeorm adapter registers things, and it feels pretty much the same as we do here, especially when it comes to exports field.

It seems to be working fine both with ts-node and with code built via ncc. Maybe usage with NestFactory.createApplicationContext should be documented somehow if the above is how it's supposed to work.

That's good news, could you PR that somewhere? e.g. the readme here, I can propagate it to mikro-orm.io myself.

@AndKiel
Copy link
Author

AndKiel commented Jul 21, 2023

There are some edge cases around this. For example, if I make the whole MongoService in my reproduction have a scope but skip it in MikroOrmModule.forRootAsync, then I still get ValidationError when using the repository. As if injection ignored the fact that the whole service is scoped. No idea if it should somehow propagate without explicit MikroOrmModule scope being defined or not. At least I found a way to make it work with InjectRepository and solve my use-case. And it doesn't require manual forking as a workaround.

@B4nan
Copy link
Member

B4nan commented Jul 21, 2023

For example, if I make the whole MongoService in my reproduction have a scope but skip it in MikroOrmModule.forRootAsync, then I still get ValidationError when using the repository. As if injection ignored the fact that the whole service is scoped.

I am more than sure you need to use the scope option of the forRoot call, otherwise the services are registered as singletons, its not relevant that your MongoService (which is a leaf, not a dependency of those) is request scoped - you don't import that into the ORM services, so they don't have to he request scoped at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants