Brakebein / prisma-generator-nestjs-dto

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

@ApiProperty({ required: false}) set for required field #18

Closed gwesterman closed 1 year ago

gwesterman commented 1 year ago

My model's ID field is defined like this:

id String @id() @default(cuid())

My config contains:

definiteAssignmentAssertion = "true"

This results in the following definition in both the *.entity* and .dto.ts** files:

@ApiProperty({ required: false })
id!: string;

I'm using another library to create an Angular client based on the Swagger definitions. This leads to the ID field being optional in the frontend, which it isn't. A value for ID can always be expected when an entry is read, since it's automatically generated when the entry is created.

When I manually set required to true like this, everything works as expected:

@ApiProperty({ required: true })
id!: string;

But since automatically generated files shouldn't be edited manually I don't see this as a solution.

Brakebein commented 1 year ago

As I understood it semantically, required is something for request bodies (CreateDto, UpdateDto). For response bodies, optional fields have nullable: true property. I'm using Redoc to render the API docs, and I found it irritating if in the response body fields are marked as required. If they are optional they can be null and it prints, for example, string or null. That's at least my use case. I know that my understanding of required may be wrong.

Nevertheless, all optional fields in the swagger.json have nullable: true, otherwise it defaults to false. So your library for the frontend client is not considering the nullable property?

I might need to add an additional config flag to cover both use cases.

gwesterman commented 1 year ago

I get your reasoning, though I wasn't able to find a source for it on the quick, and if OpenAPI defines it as such, then it's probably best to follow suit. I'm also sure that your understanding of required is better than mine in any case.

One might argue though, that required is just as much an obligation for the API that returns an object as it is for the consumer that sends a request body. In both cases the other side can expect the object to look exactly as defined. And if a field cannot be null since an ID will always be generated and will always be returned, then the specification could just as well reflect that.

I've run both ng-openapi-gen and openapi-generator (for typescript-angular). Both will generate a nullable ID field.

In some cases this requires me to do a non-null assertion, when passing the ID to a function for instance, even though the ID will never be null. It's an easy fix at this point, though I'd prefer an accurate model over it.

Brakebein commented 1 year ago

With v1.18.0-beta0 it now uses the default behavior: the required property is not set (defaults to true) unless it is an optional field. With the flag requiredResponseApiProperty = "false" I can use the old behavior adding always required: false to the response dtos.

gwesterman commented 1 year ago

I'm currently experiencing an issue with the following config:

generator nestjsDto {
  provider                        = "prisma-generator-nestjs-dto"
  output                          = "../../libs/api/models/generated"
  outputToNestJsResourceStructure = "true"
  reExport                        = "true"
  flatResourceStructure           = "false"
  exportRelationModifierClasses   = "true"
  createDtoPrefix                 = "Create"
  updateDtoPrefix                 = "Update"
  dtoSuffix                       = "Dto"
  entityPrefix                    = ""
  entitySuffix                    = ""
  classValidation                 = "true"
  fileNamingStyle                 = "kebab"
  noDependencies                  = "false"
  outputType                      = "class"
  definiteAssignmentAssertion     = "true"
  prettier                        = "true"
}

The following column definition:

   avatarUrl String?

results in the following output within the entity:

   @ApiProperty({
      nullable: true
   })
   avatarUrl!: string | null;

whereas I would expect something like this:

   @ApiProperty({
      required: false
   })
   avatarUrl?: string;

After reading the docs I understand that this intentional, but it is causing me problems because I generate a proxy library for my frontend and the property is generated like this:

   avatarUrl: string | null;

instead of this:

   avatarUrl?: string;

In Angular using optional chaining (?.) or the nullish coalescing operator (??) on fields that are not optional, e.g. marked with the definite assignment assertion operator (!) will at the very least cause linting issues and warnings to be thrown in the console at runtime.

Brakebein commented 1 year ago

Usually, an API or a database query returns null for a field that is optional and not set, instead of just omitting it (at least those I work with). So yes, this is intentional so far.

However, I don't see where the difference is when checking the field against null or undefined. Optional chaining (?.) and the nullish coalescing operator (??) work for both null and undefined. (I don't get any linting issues either).

If this is still a problem (maybe you have small code snippet pointing it out), I might consider an extra config option to change this behavior.

gwesterman commented 1 year ago

My bad, I found the cause and should stop creating issues at 2AM.

As you say there's no difference in using ? or ?? on fields that are defined using field? or field: string | null, but when I was looking for the cause of the issue I first saw the DTO created by this library instead of the one created by @openapitools/openapi-generator-cli for use in the frontend.

When I saw the definite assignment assertion operator used in the DTO for the backend I thought I had found the problem, even though my comment clearly shows that the DTO generated for the frontend looks perfectly fine: avatarUrl: string | null;

The cause of the error was a remnant null-check in my template (abbreviated example):

<ng-container *ngIf="profile.avatarUrl; else noAvatarTemplate">
   <img [src]="profile.avatarUrl" />
</ng-container>
<ng-template #noAvatarTemplate>
   <img [src]="'https://ui-avatars.com/api/?size=400&name=' + profile.name" />
</ng-template>

went on to become:

<ng-container *ngIf="profile.avatarUrl">
   <img [src]="profile.avatarUrl" />
</ng-container>

then:

<!-- warning NG8102: The left side of this nullish coalescing operation does not include 'null' or 'undefined' in its type, therefore the '??' operator can be safely removed. -->
<ng-container *ngIf="profile.avatarUrl">
   <img [src]="profile.avatarUrl ?? 'https://ui-avatars.com/api/?size=400&name=' + profile.name" />
</ng-container>

which should have been:

<img [src]="profile.avatarUrl ?? 'https://ui-avatars.com/api/?size=400&name=' + profile.name" />

Using the nullish coalescing operator within the *ngIf that already checked if the field in question had a value was of course unnecessary and that is what the warning was all about.