Brakebein / prisma-generator-nestjs-dto

Generates NestJS DTO classes from Prisma Schema
Apache License 2.0
45 stars 24 forks source link

Add type to @ApiProperty configuration #20

Closed gwesterman closed 1 year ago

gwesterman commented 1 year ago

This is a simplified version of my model:

// schema.prisma
model Profile {
  id String @id() @default(cuid())
  traits Trait[]
}

The generated entity looks like this:

// profile.entity.ts
export class Profile {
    @ApiProperty({
    required: false
  })
  id!: string;

  @ApiProperty({
    isArray: true,
    required: false
  })
  traits?: Trait[];
}

The generated Swagger JSON schema looks like this (note: No schema is created for Trait):

"Profile":{
  "type":"object",
  "properties":{
    "id":{
      "type":"string"
    }
    "traits":{
      "type":"array",
      "items":{
        "type":"array"
      }
    }
  }
}

I would expect traits to be an Array of Trait, not an Array of Array.

When I do the following manual change to the entity:

// profile.entity.ts
export class Profile {
    @ApiProperty({
    required: false
  })
  id!: string;

  @ApiProperty({
    type: Trait, // <-- added explicit type
    isArray: true,
    required: false
  })
  traits?: Trait[];
}

This is the resulting JSON schema (note: A schema for Trait is created, too):

"Profile":{
  "type":"object",
  "properties":{
    "id":{
      "type":"string"
    }
    "traits":{
      "type":"array",
      "items":{
        "$ref":"#/components/schemas/Trait"
      }
    }
  }
}

I cannot make manual edits to generated files and I haven't managed to make the changes necessary for the generator to create the correct annotations.

The documentation claims, that the types should be inferred automatically, but this does not happen. The only time the type is set correctly in the annotation is for something like a Create___BlockingRelationInputDto or the special data types mentioned in the documentation.

I've tried adding /// @type Trait and /// @DtoCastType(Trait) but neither worked.

My request: Please make it possible to explicitly set the type value within the @ApiProperty annotation, either inferring it by default or by accepting something like /// @type Trait.

Brakebein commented 1 year ago

Thanks for pointing out! I just noticed that in one of my projects this issue leads to incorrect swagger.json as well. Of couse, this should be automatically inferred.

gwesterman commented 1 year ago

Another thing to consider are circular dependencies which occur most times when two entities are related.

This is handled by using the following syntax, which is mentioned here in the docs:

// profile.entity.ts
export class Profile {
    @ApiProperty({
    required: false
  })
  id!: string;

  @ApiProperty({
    type: () => Trait, // <-- added lazy function that resolves the type
    isArray: true,
    required: false
  })
  traits?: Trait[];
}

So far, after manually adding this to every generated type my JSON seems to be valid and does not cause any problems when generating a library via OpenAPITools/openapi-generator.

I also haven't run into any problems when using this syntax for all relationships, even where it's not explicitly needed. Perhaps it could be the configuration default or at least an optional default.

Brakebein commented 1 year ago

Sorry, that it took that long. The type property should now be properly set for list fields. Please check out: https://www.npmjs.com/package/@brakebein/prisma-generator-nestjs-dto/v/1.18.0-beta0

gwesterman commented 1 year ago

Thanks for the update! I've identified two issues with the current code (v1.18.0-beta0):

  1. The generated Entity files reference DTOs not other Entities. The type property should reference the same type as the field that the Decorator is attached to.

  2. For arrays of Enum values the type property is set even though it shouldn't be. The referenced DTO does not exist and shouldn't since it's an Enum. This happens in Entities and DTOs.

// schema.prisma
model Profile {
  id String @id() @default(cuid())
  traits Trait[]
  hairLengths HairLength[]
}

enum HairLength {
  SHORT
  MEDIUM
  LONG
}
// profile.entity.ts
export class Profile {
    @ApiProperty({
    required: false
  })
  id!: string;

  @ApiProperty({
    type: () => TraitDto, // <-- the DTO is referenced even though it should reference the entity (Trait)
    isArray: true,
    required: false
  })
  traits?: Trait[];

  @ApiProperty({
     type: () => HairLengthDto, // <-- this is not needed, also no DTO is generated for the Enum
     isArray: true,
     enum: HairLength
  })
  hairLengths!: HairLength[];
}
Brakebein commented 1 year ago

The line traits?: Trait[]; should be actually traits?: TraitDto[];. Or is it really just generating Trait[]?

It can't reference TraitEntity, because it isn't generated. For composite types, it only generates create, update, and plain dtos. That's because the composite type dtos are already referencing all related composite types. An entity dto file wouldn't look any different. The difference between the plain dto and the entity for normal models is that the entity contains all the relations fields as well. But for composite types, we have no relation fields. Correct me, if I'm missing something.

Enums are something I haven't even looked at yet. But this issue should be fixed of course.

gwesterman commented 1 year ago

Trait is a model in the schema, I've left it out for brevity. The DTOs and an Entity are also generated.

Also I'm not quite sure what you mean by entity dto file. A file will either contain a DTO or an Entity, wouldn't it?

DTOs won't contain relations, so no other DTO should be imported or referenced. This currently holds true, it's just the (non-existing) Enum DTOs that are referenced.

Entities on the other hand will contain all of the DTO's fields, plus all scalar fields and relation fields. And these should point to other Entities, not DTOs. This is necessary because without nested Entities you wouldn't be able to make use of deeply nested relations, since a DTO couldn't reference the next layer.

This is the result I would be expecting:

// profile.entity.ts
export class Profile {
    @ApiProperty({
    required: false
  })
  id!: string;

  @ApiProperty({
    type: () => Trait,
    isArray: true,
    required: false
  })
  traits?: Trait[];

  @ApiProperty({
     isArray: true,
     enum: HairLength
  })
  hairLengths!: HairLength[];
}

Another thing to mention is that the DTOs that are now being referenced in the Entity files are not imported. I didn't deem this to be necessary information because I wouldn't have expected them to be referenced in the first place. So even if the Entity file should reference DTOs, it's currently not working.

Brakebein commented 1 year ago

Also I'm not quite sure what you mean by entity dto file. A file will either contain a DTO or an Entity, wouldn't it?

Sorry, wrong naming from my side.

Trait is a model in the schema, I've left it out for brevity.

Ok, I wasn't sure if Trait is a model or a composite type (available for mongodb schemas). It would be good to extend your schema from above by Trait. And maybe I was confused because in my examples the entity class is suffixed by Entity, e.g. ProfileEntity.

So, in your case Trait is the entity. And it is correctly imported import { Trait } from 'path/to/trait.entity.ts';? So, the only issue is that type: () => TraitDto should be type: () => Trait?

gwesterman commented 1 year ago

Trait is a model in the schema, I've left it out for brevity.

Ok, I wasn't sure if Trait is a model or a composite type (available for mongodb schemas). It would be good to extend your schema from above by Trait. And maybe I was confused because in my examples the entity class is suffixed by Entity, e.g. ProfileEntity.

My bad, here's the (abbreviated) schema for Trait, next time I'll include it from the get-go.

// schema.prisma
model Trait {
  id              String        @id
  name            String
  translationKey  String
  profiles        Profile[]
}

So, in your case Trait is the entity. And it is correctly imported import { Trait } from 'path/to/trait.entity.ts';? So, the only issue is that type: () => TraitDto should be type: () => Trait?

Exactly, that would solve it. type: () => Trait should reference the same type as traits: Trait[].

Brakebein commented 1 year ago

Please check out new release: https://www.npmjs.com/package/@brakebein/prisma-generator-nestjs-dto/v/1.18.0-beta1

It should now generate the entities and dtos as you expected.

gwesterman commented 1 year ago

Excellent stuff, it works just as I hoped it would :)

But one error has found its way into this release. @ApiProperty() for RelationInput DTOs now contain two type declarations. Here's an example from a create DTO:

// create.profile.dto
export class CreateProfileTraitsRelationInputDto {
   @ApiProperty({
      type: Trait, // <-- this should not exist
      isArray: true,
      type: ConnectTraitDto
   })
   @IsNotEmpty()
   @IsArray()
   @ValidateNested({ each: true })
   @Type(() => ConnectTraitDto)
   connect!: ConnectTraitDto[];
}

I think once this is fixed code generation will run flawlessly in my case.

Brakebein commented 1 year ago

Thanks for pointing out the edge cases! New release: https://www.npmjs.com/package/@brakebein/prisma-generator-nestjs-dto/v/1.18.0-beta2

Brakebein commented 1 year ago

@gwesterman Did you get a chance to test the latest beta release if everything works as expected now?

gwesterman commented 1 year ago

Please excuse the late response, I forgot to confirm. Yes, everything is working as expected, thank you!

omarkhatibco commented 1 year ago

I can confirm that is working with me as well