Skip to content
This repository has been archived by the owner on May 6, 2024. It is now read-only.

DTO is not a class, all fields typed as optional #66

Closed
nolawnchairs opened this issue Aug 31, 2023 · 2 comments
Closed

DTO is not a class, all fields typed as optional #66

nolawnchairs opened this issue Aug 31, 2023 · 2 comments

Comments

@nolawnchairs
Copy link

Hi.

First off, I'm happy there is a project using Zod instead of having to deal with class-validator. However, the createZodDto function does not create a class instance, but rather an object.

const createEnvironmentSchema = z.object({
  label: z.string().max(191).nonempty(),
  fromName: z.string().max(191).nonempty(),
  fromEmail: z.string().max(191).email(),
  smtpHost: z.string().max(191).nonempty(),
  smtpPort: z.number().int().max(65535),
  smtpUser: z.string().max(191).nonempty(),
  smtpPass: z.string().max(191).nonempty(),
})

export class CreateEnvironmentDto extends createZodDto(createEnvironmentSchema) {

  mailbox(): string {
    return `"${this.fromName}" <${this.fromEmail}>`
  }
}

I expected the function to return an instance of the class, so I can add additional methods and acessors, and be able to use them as well as instanceof calls downstream, as I've been accustomed to do using class-transfromer. However, the value returned to the controller is a POJO.

@Controller('environments')
export class EnvironmentsController {

  @Post()
  async create(@Body() dto: CreateEnvironmentDto) {
    console.log(dto, dto instanceof CreateEnvironmentDto)
  }
}

The above console statement produces { ... }, false instead of CreateEnvironmentDto { ... }, true

Another issue is that the object properties are all optional whereas the Zod schema has no optional properties. The LSP reports the shape of the object as such:

label?: string;
fromName?: string;
fromEmail?: string;
smtpHost?: string;
smtpPort?: number;
smtpUser?: string;
smtpPass?: string;

The actual type shown is this:

(alias) createZodDto<{
    label?: string;
    fromName?: string;
    fromEmail?: string;
    smtpHost?: string;
    smtpPort?: number;
    smtpUser?: string;
    smtpPass?: string;
}, z.ZodObjectDef<{
    label: z.ZodString;
    fromName: z.ZodString;
    fromEmail: z.ZodString;
    smtpHost: z.ZodString;
    smtpPort: z.ZodNumber;
    smtpUser: z.ZodString;
    smtpPass: z.ZodString;
}, "strip", z.ZodTypeAny>, {
    ...;
}>(schema: z.ZodType<...>): ZodDto<...>
import createZodDto

This keeps it from being useful for downstream API's where it should be assumed that all required properties are present in the DTO, which of course they are, but the LSP doesn't know that.

Using the most recent versions of this library and NestJS:

"@nestjs/common": "^10.0.0",
"@nestjs/core": "^10.0.0",
"nestjs-zod": "^2.3.3",
"zod": "^3.22.2",

Hopefully there is a fix? I REALLY do not want to go back to using class-validator and vlass-transformer which are just tedious to deal with.

@xrebelox
Copy link

xrebelox commented Aug 31, 2023

Can't really say much regarding the first issue, but in regards to the second issue where all properties are presented as being optional, this can be solved by setting strictNullChecks to true in your tsconfig.json file.

Update: Actually zod's documentation requires you to set strict: true in your tsconfig.json, you can read it here.

@nolawnchairs
Copy link
Author

Thanks, @xrebelox - I changed the strict level and the second issue is gone. I never really ever had the need to enforce strict mode before, although it is recommended. I'm just stubborn.

As for the first issue, I've found a fix:

In the src/pipe.ts module, within the transform method, there's this line:

return validate(value, metatype.schema, createValidationException)

As expected, if the validation passed, the object is returned. However, I kind of hacked the JS code directly, and I changed it to:

const result = validate(value, metatype.schema, createValidationException)
return Object.assign(new metatype(), result)

This creates a class instance of the same shape as the object. Since the DTO classes are created on-the-fly, this is safe to do. However, with a public library the choice on whether or not to transform the object to a class instance should be left to the end user, as it is in Nest's native SerializerInterceptor implementation (which uses class-transformer under the hood)

The metatype value will always be the class constructor reference to whatever the end-user adds as the DTO type.

This begs the question on WHERE to set this user configurable value. The best option would be to add an options object directly to the interceptor's constructor like so:

{
  provide: APP_INTERCEPTOR,
  useValue: new ZodSerializerInterceptor({ transform: true }),
}

Of course, the interceptor's constructor has an @Inject for the Reflector class so there are ways around this, but it should be noted that the Reflector class doesn't always need to be injected, and can be instantiated normally, as it's just a utility class with no DI of its own.

const reflector = new Reflector()

If you still prefer to inject it, there are ways to do that as well:

{
  provide: APP_INTERCEPTOR,
  inject: [Reflector],
  useFactory: (reflector: Reflector) => 
     new ZodSerializerInterceptor(reflector, { transform: true }),
}

I should note that for many use cases, the object is just fine and classes are not needed. However, this would be more of a semantic choice. If the type is defined as a certain class type, then the resulting value should conform to the class API specification including all the methods and accessors. This is why it should be the choice of the implementing party.

I also notice there is no top-level module to initialize. This is also a good way to introduce user-specific configuration for all the moving parts to have access to.

There's no contributing guide, so I'm curious as to the author's preferred protocols regarding PR's and such. I have loads of experience building NestJS libraries (all in-house libraries, but experience none the less), so I can make meaningful contributions to this project if it's welcome.

Cheers.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants