anatine / zod-plugins

Plugins and utilities for Zod
656 stars 91 forks source link

Swagger and Validation Not working for Query params and URL params. #35

Closed alichampion closed 2 years ago

alichampion commented 2 years ago

extendApi and createZodDto is not working for Query and URL params.

Here is the code

const Schema = extendApi(
  z.object({
    compTicketNum: z.string().length(111),
  }),
  {
    example: { compTicketNum: 'C3001200025' },
  },
);

export class GetComplaint_Query_DTO extends createZodDto(Schema) {}

  // Route
  @Get(':cnic')
  getComplaint(
    @Param() param: GetComplaint_DTO,
    @Query() query: GetComplaint_Query_DTO,
  ) {
    return this.complaintLodgmentService.getComplaint(
      param,
      query,
      headers,
      logger,
    );
  }
fpoppinga commented 2 years ago

We're facing the same problem. I would be willing to have a look at fixing this, but I was not immediately able to find the right place in the code for a start.

Brian-McBride commented 2 years ago

@fpoppinga This is the code that creates the DTO: https://github.com/anatine/zod-plugins/blob/d6a59471f299e3b3298f90e85cfd4ca4ab077823/libs/zod-nestjs/src/lib/create-zod-dto.ts#L29

Then swagger is patched. Keep in mind that this runs the current Nest Swagger plugin, then patches it up. If I were to guess, this might be a place to look if the Swagger Doc is not matching. https://github.com/anatine/zod-plugins/blob/d6a59471f299e3b3298f90e85cfd4ca4ab077823/libs/zod-nestjs/src/lib/patch-nest-swagger.ts#L15

When you have @Query() query: GetComplaint_Query_DTO, as the example above. Maybe the DTO needs to use the query param> Remember, this is creating a class. I'm not saying this will work, but maybe?

const Schema = extendApi(
  z.object({
    query: z.object({
      compTicketNum: z.string().length(111),
    }),
  }),
  {
    example: { query: { compTicketNum: 'C3001200025' } },
  },
);

export class GetComplaint_Query_DTO extends createZodDto(Schema) {}

But here is the real issue. The whole process here is using a validation pipe for data coming in. @UsePipes(ZodValidationPipe)

Validation pipes are at a higher level.

This code was taking from another repo, which has credit in all the source code. Unfortunately, it doesn't support anything better. I moved it here as I was adding in some better error handling and updating some other deps. If you can find a similar example where another validation lib is being used with Nest and the swagger generator, I can use that to infer a fix.

I don't love decorators as they abstract away a lot of logic making a system harder to extend. When I moved from Node from Java I was so happy not to deal with all these magical decorators. lol.

maxs-rose commented 2 years ago

I have got zod to validate the schema properly for query params and URL params (all the examples are for query params but path params work similarly. I am also using express and haven't tested this with Fastlify but it should work).

To define my DTO I have:

const DataSchema = extendApi(
  z.object({
    data: z.string().max(4).optional(),
    moreData: z.string().optional(),
  }),
);

export class DataDto extends createZodDto(DataSchema) {}

Im my controller I define the endpoint as

@Get('test')
@ApiQuery({ name: 'data', type: 'string', required: false })
@ApiQuery({ name: 'moreData', type: 'string', required: false })
getTest(@Query() inputData: DataDto) { return inputData; }

This causes a swagger definition to be generated that is correct and zod will properly validate the inputs.

There are issues with this if you would like to use non-string types for example simply making an object with:

const DataSchema = extendApi(
  z.object({
    data: z.number().optional(),
  }),
);

Will cause the input to fail validation as Nest seems to be converting all input values to strings.

If you need to take non string inputs you can use preprocess:

const DataSchema = extendApi(
  z.object({
    data: z.preprocess(a => parseInt(z.string().parse(a as string), 10), z.number()).optional(),
  }),
);

This will mean zod properly transforms the query parameter into a number but causes swagger to fail to generate the proper definition for the object (this is actually a problem for all places we can put Dto's so I assume that the swagger plugin is having a hard time deciding the type so it just fails to generate since the actual generated DTO works as expected). But you can get around this by creating two objects:

export const DataSchema = extendApi(
  z.object({
    data: z.preprocess(a => parseInt(z.string().parse(a as string), 10), z.number()).optional(),
  }),
);

const DataDtoSchema = extendApi(
  z.object({
    data: z.number().optional(),
  }),
);

export class DataInput extends createZodDto(DataSchema) {}
export class DataDto extends createZodDto(DataDtoSchema) {}

// Controller
@Get('test')
@ApiQuery({ name: 'data', type: 'number', required: false })
getTest(@Query() inputData: DataInput): DataDto { return inputData; }

None of this is ideal but it does work 😅.

Brian-McBride commented 2 years ago

A downside of NestJS, in general, is the excessive decorators. It sure would be great if the Nest team did an overhaul to create ONE decorator that handled API, Swagger, GraphQL, etc... instead of having to define each with basically the same info again and again.

maxs-rose commented 2 years ago

I have actually been playing with this a bit more today and found that it is actually possible to get the Dto to be created properly without having to resort to making the same thing twice

const isFormattedDate = (possibleDate: string) => new RegExp('^[0-9]{4}-[0-9]{2}-[0-9]{2}$').test(possibleDate) && isValid(parseISO(possibleDate));

const DataSchema = extendApi(
  z.object({
    test: z.preprocess(a => parseInt(String(a), 10), z.number()),
    date: z
      .string()
      .refine(isFormattedDate, 'Date must be in format YYYY-MM-DD')
      .transform(date => parseISO(date))
      .optional(),
  }),
  {
    properties: {
      test: { type: 'number', format: 'int64' },
      date: { type: 'string', format: 'YYYY-MM-DD' },
    },
  },
);

export class DataDto extends createZodDto(DataSchema) {}

Adding the properties that the swagger plugin fails to generate manually seems to work perfectly and you can also omit properties it will properly generate a schema for and the manually created schema and the auto-generated ones will get properly merged.

Yea I agree there does seem to be a heavy reliance on magical decorators but it is what it is.

alichampion commented 2 years ago

:> you can do so by combining decorators.

kohanian commented 2 years ago

@alichampion I wasn't able to get your method to work, but I was able to dig DEEP into the NestJS swagger repo and came across a couple of findings that prompted me to come up with something that works for the most part. @Brian-McBride I'm not sure if you want something like this merged or not, but here's a link to my PR as a starter: https://github.com/anatine/zod-plugins/pull/62

Brian-McBride commented 2 years ago

Function added via #62 and #63

jonsoku-dev commented 1 year ago

@Brian-McBride @Query is not working yet...