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
66.88k stars 7.56k forks source link

Fix possible Denpendency Injection issue #1277

Closed cschroeter closed 4 years ago

cschroeter commented 5 years ago

I'm submitting a...


[ ] Regression 
[x] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

This ticket is related to the latest Prisma client and the NestJS DI mechanism.

So when the Prisma Client is extended

import { Injectable } from '@nestjs/common';
import { Prisma } from './client';

@Injectable()
export class PrismaService extends Prisma  {
}

NestJS terminates with

[nodemon] clean exit - waiting for changes before restart

Expected behavior

Well if the Prisma client is extended the NestJs app should no terminate 🤪

Minimal reproduction of the problem with instructions

https://github.com/cschroeter/nestjs-prisma-client-bug

Here is the bug

What is the motivation / use case for changing the behavior?

Environment


Nest version: 5.4.0


For Tooling issues:
- Node version: 11.0.0
- Platform: All

marcus-sa commented 5 years ago

A constructor cannot be async, so what exactly do you mean by "fixing async constructor functions"?

cschroeter commented 5 years ago

A constructor can not be async but a function can. And this is how the client is instantiated.

marcus-sa commented 5 years ago

Well then that's not how you should instantiate it, since a class can't extend an async function constructor? That's basic logic as you can't asynchronously instantiate a new class :)

Instantiate it using FactoryProvider instead

prisma.service.ts


import { Injectable } from '@nestjs/common';

@Injectable() // should extend the actual returned class from the function for type safety export class PrismaService {}

> prisma.module.ts

```ts
import { Module } from '@nestjs/common';

import { PrismaService } from './prisma.service';
import { Prisma } from './client';

@Module({
  providers: [
    {
      provide: PrismaService,
      useFactory: Prisma,
    },
  ],
})
export class PrismaModule
cschroeter commented 5 years ago

Yeah my title is a bit missleading here. So normally Dependency Injection works like a charm. I tried to figure out whats different here compared to other 3rd party libraries I use.

What is different is the way the client gets instantiated.

import { Client as BaseClient } from './Client'
import { BaseClientOptions } from './types'

// the not so async constrcutor function 😁
export function makePrismaClientClass<T>({
  typeDefs,
  endpoint,
  secret,
}: {
  typeDefs: string
  endpoint: string
  secret?: string
}): T {
  return class Client extends BaseClient {
    constructor(options: BaseClientOptions) {
      super({ typeDefs, endpoint, secret, ...options })
    }
  } as any
}

export interface ClientConstructor<T> {
  new (options?: BaseClientOptions): T;
}

export const Prisma = makePrismaClientClass<ClientConstructor<Prisma>>({
  typeDefs,
  endpoint: `https://eu1.prisma.sh/public-agatepuma-476/my-app/dev`
});
export const prisma = new Prisma();

So I tried to get ridd of this makePrismaClientClass by doing something like this.

import { Injectable } from '@nestjs/common';
import { Client } from 'prisma-client-lib';

export interface PrismaClientOption {
  typeDefs: string;
  endpoint: string;
  secret?: string;
}

@Injectable()
export class PrimsaClient extends Client {
  constructor(options: PrismaClientOption) {
    super(options);
  }
}

// module
@Module({
  providers: [
    {
      provide: PrimsaClient,
      useFactory: () => {
        return new PrimsaClient({
          typeDefs,
          endpoint: 'https://eu1.prisma.sh/public-agatepuma-476/my-app/dev',
        });
      },
    },
  ],
  exports: [PrimsaClient],
})
export class PrismaModule {}

But the same error happens. Now I feel that this must have something to do with the Client itself. Also not having any error message don't help either 🧐

itsMapleLeaf commented 5 years ago

Just ran into the same issue. I was following along with the documentation. Here's my code:

```ts // prisma.service.ts import { Injectable } from "@nestjs/common" import { Prisma } from "./index" @Injectable() export class PrismaService extends Prisma { constructor() { super({ endpoint: "...", debug: false, }) } } ``` ```ts // prisma.module.ts import { Module } from "@nestjs/common" import { PrismaService } from "./prisma.service" @Module({ providers: [PrismaService], exports: [PrismaService], }) export class PrismaModule {} ``` ```ts // character.controller.ts import { Controller, Get, Param } from "@nestjs/common" import { PrismaService } from "../prisma/prisma.service" @Controller("characters") export class CharacterController { constructor(private readonly prisma: PrismaService) {} @Get() async getAllCharacters() { return await this.prisma.characters({}) } @Get(":id") async getCharacter(@Param("id") id: string) { return await this.prisma.character({ id }) } } ``` ```ts // character.module.ts import { Module } from "@nestjs/common" import { PrismaModule } from "../prisma/prisma.module" import { CharacterController } from "./character.controller" @Module({ imports: [PrismaModule], controllers: [CharacterController], providers: [], }) export class CharacterModule {} ``` ```ts // app.module.ts import { Module } from "@nestjs/common" import { CharacterModule } from "./character/character.module" @Module({ imports: [CharacterModule], controllers: [], providers: [], }) export class AppModule {} ```

The app starts, then abruptly stops.

$ yarn start
yarn run v1.12.1
$ ts-node src/main.ts
[Nest] 22928   - 11/10/2018, 9:17:17 PM   [NestFactory] Starting Nest application...
[Nest] 22928   - 11/10/2018, 9:17:17 PM   [InstanceLoader] AppModule dependencies initialized +10ms
Done in 1.66s.

This Prisma class is the one created by the prisma generate CLI.

The prisma instance itself seems to be the cause of the issue. Here's a simple factory provider which returns a prisma instance. This causes the server to stop.

const prisma = new Prisma({
  endpoint: `...`,
})

const prismaProvider = {
  provide: "Prisma",
  useFactory: () => prisma,
}

@Module({
  providers: [prismaProvider],
  exports: [prismaProvider],
})
export class PrismaModule {}

If I turn the factory into a thunk function, the server runs as normal.

const prismaProvider = {
  provide: "Prisma",
  useFactory: () => () => prisma,
}

Then, I can do this.prisma().characters(...) in my controller, and everything works.

picosam commented 5 years ago

Hello! So has it been confirmed that this is an issue on the nestjs side of the story?

marcus-sa commented 5 years ago

If it works with a FactoryProvider as @kingdaro showed above, then it's an issue with Nest, most likely because of the way its DI/IOC can't handle abstract classes properly. #inversifymasterrace

itsMapleLeaf commented 5 years ago

In case it helps, this is how the Prisma class is created:

import { makePrismaClientClass } from "prisma-client-lib";
import { typeDefs } from "./prisma-schema";

export interface Prisma {
  // a bunch of prisma query/mutation properties here
}

export const Prisma = makePrismaClientClass<ClientConstructor<Prisma>>({
  typeDefs,
  endpoint: `...`
});

Could it have to do with the fact that it's both a variable and an interface? 🤔

Note that this file is generated, so while I could change this, it wouldn't be a very clean way to solve the issue

matt328 commented 5 years ago

I can confirm @kingdaro's thunk solution works for me too.

picosam commented 5 years ago

Thanks @matt328, me too! However, this is definitely a work around, not a solution.

matt328 commented 5 years ago

Right. From a quick glance, the generated prisma constructor seems like it may do some heavy lifting. So at a minimum, we'd probably want to inject the constructor function, then initialize and store off a prisma client instance to minimize calls to the prisma constructor. Even doing that though, the prisma ctor is still called once per injection point rather than just once, period.

That being said, that does sound exactly like something a DI/IOC container can do; it's more than just syntactic sugar in this case.

HNicolas commented 5 years ago

Hello, Here is a dirty workaround I found to this problem. I created a PrismaService service :

import { Injectable } from "@nestjs/common";
import { Prisma } from "generated/prisma-client";

@Injectable()
export class PrismaService {
  prisma: Prisma;

  constructor() {
    this.prisma = new Prisma({
      endpoint: ...,
    });
  }
}

Then I can access prisma in my other services / controllers :

import { Injectable } from '@nestjs/common';
import { PrismaService } from './prisma.service';

@Injectable()
export class AppService {
  constructor(
    protected prismaService: PrismaService,
  ) {}

  async root() {
    const users = await this.prismaService.prisma.users();
    ...
  }
}
kamilmysliwiec commented 5 years ago

Have somebody tried to reproduce this issue in 5.4.1 (some injector changes were introduced in this release)?

marcus-sa commented 5 years ago

@kamilmysliwiec now it seems to be stuck when it comes down to creating the module where the Prisma service is being provided without throwing any errors.

EDIT: Even if you use the service as a provide token, and then use the already instantiated client from the generated file as a value, it still fails.

Seems like Nest can't handle having the Prisma client as a provider/service at all.

ullalaaron commented 5 years ago

Any news on this?

ZenSoftware commented 5 years ago

@kingdaro

In case it helps, this is how the Prisma class is created:

import { makePrismaClientClass } from "prisma-client-lib";
import { typeDefs } from "./prisma-schema";

export interface Prisma {
  // a bunch of prisma query/mutation properties here
}

export const Prisma = makePrismaClientClass<ClientConstructor<Prisma>>({
  typeDefs,
  endpoint: `...`
});

Could it have to do with the fact that it's both a variable and an interface? 🤔

Note that this file is generated, so while I could change this, it wouldn't be a very clean way to solve the issue

I was wondering that myself. Though, I tried modifying the generated code to rename one of the Prisma to PrismaClient as follows. Unfortunately it still breaks Nest on startup when trying to make prisma-client a provider.

import { makePrismaClientClass } from "prisma-client-lib";
import { typeDefs } from "./prisma-schema";

export interface Prisma {
  // a bunch of prisma query/mutation properties here
}

// Here I renamed Prisma to PrismaClient
export const PrismaClient = makePrismaClientClass<ClientConstructor<Prisma>>({
  typeDefs,
  models,
  endpoint: `...`
});
export const prisma = new PrismaClient();

It is a bit of a mystery to me with what makePrismaClientClass({...}) is doing, which is making things a bit difficult to diagnose. Anyone have any idea what this is generally doing?

ekalinichev commented 5 years ago

Issue is still present with current Nest.js version, here's a little utility class to avoid writing this.prismaService.prisma ... all the time:

import { Inject } from '@nestjs/common'
import { Prisma } from './client'
import { PrismaService } from './prisma.service'

export class PrismaConsumer {
  @Inject(PrismaService)
  private readonly prismaService: PrismaService

  protected get prisma(): Prisma {
    return this.prismaService.prisma
  }
}

With that you can just extend PrismaConsumer in any class where you need to Prisma, and use this.prisma to access it

lock[bot] commented 4 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.