nestjs/mongoose

InjectModel decorator for multiple databases is broken since version 9.1.0

Closed this issue · 1 comments

shuki770 commented

Did you read the migration guide?

  • I have read the whole migration guide

Is there an existing issue that is already proposing this?

  • I have searched the existing issues

Potential Commit/PR that introduced the regression

#1023

Versions

9.1.0

Describe the regression

Since version 9.1.0, the InjectModel decorator for multiple databases is malfunctioning, resulting in an error in resolving dependencies. The only functional approach is to use the connectionName parameter as follows: @InjectModel(modelName, connectionName). It's imperative that this parameter remains optional.

Root cause analysis: The function getModelToken was changed (referenced in PR #1023), with the connectionName becoming a part of the provider name in the form of ${getConnectionToken(connectionName)}/${model}Model.

One potential solution involves introducing two provider tokens for each model, one with a connectionName and the other without.

Minimum reproduction code

@Module({
  imports: [
    MongooseModule.forFeature(
      [
        { name: SomeModel1.name, schema: Schema1,  },
      ],
      mongoConnection1,
    ),
    MongooseModule.forFeature(
      [
        { name: SomeModel2.name, schema: Schema2,  },
      ],
      mongoConnection2,
    ),
  ],
})
export class AppModule {}

// Not working: Nest can't resolve dependencies of the SomeService (?)
@Injectable()
export class SomeService {
  constructor(@InjectModel(SomeModel1.name) private readonly model: Model<SomeDocument>) {}
}

// Working
@Injectable()
export class SomeService {
  constructor(@InjectModel(SomeModel1.name, mongoConnection1) private readonly model: Model<SomeDocument>) {}
}

Expected behavior

connectionName parameter of InjectModel decorator should be optional

@Injectable()
export class SomeService {
  constructor(@InjectModel(SomeModel1.name) private readonly model: Model<SomeDocument>) {}
}

Other

No response

jmcdo29 commented

In my opinion, it shouldn't remain optional if you have multiple databases. If you have Connection1 and Connection2 and you are @InjectModel(SomeModel1.name) and SomeModel1 is in both connections, are you injecting it for Connection1 or Connection2? By adding the connection name, you are making the explicit, conscious choice on which connection you are using, and leading to less guesswork by other devs, and by the framework. If you want to not need to pass a connection name by default, and use one of the X connections, leave the default connection without a name, and the framework will properly use the connection name default. This is how it has been working in both @nestjs/sequelize and @nestjs/typeorm and has pretty much been accepted as the right way to go about multiple connections

  • 👍1