dbartholomae / middy-middleware-class-validator

A middy validation middleware using class-validator.
MIT License
13 stars 5 forks source link

All fields optional makes the library fail #22

Closed tkgalk closed 5 months ago

tkgalk commented 4 years ago

Currently, if all fields are marked as @IsOptional the library will fail with:

{
    "message": "Incorrect object param type! Only string, plain object and array of plain objects are valid.",
    "name": "Error",
    "statusCode": 400
}

Is there something we might do with reflect-metadata to check if all fields are @IsOptional and then not validate the body at all?

dbartholomae commented 4 years ago

Could you share your validator class? Sounds like there might be something funky going on. If it is really about the validation itself, then you might have to open a ticket with class-validator, though, as this is just a wrapper.

tkgalk commented 4 years ago

I'll try to dig into it more. I tried to create a reproducible scenario and something different happened. So it might be caused by each: true, actually. What I first though was happening was the library providing body to class-validator as undefined when no body was provided by the HTTP call.

But it doesn't seem to be the case. I'll let you know if I isolate the problem.

tkgalk commented 4 years ago

export class ListTransactionsDTO {
  @IsString()
  @IsOptional()
  a?: string;

  @IsString()
  @IsOptional()
  b?: string;

  @IsString()
  @IsOptional()
  c?: string;
}

For this validator class when making this call: http :3000 without any body I get the error mentioned earlier.

When I add http :3000 a=test (essentially { "a": "test" } in httpie) it works correctly. I think what is happening is that when there is no body provided the middleware wrapper, that is this library, doesn't have anything to pass to class-validator and class-validator fails with Incorrect object param type! Only string, plain object and array of plain objects are valid..

EDIT: https://github.com/dbartholomae/middy-middleware-class-validator/blob/da906b5661f6767d4dffc5dc7742c9de758a74f7/src/ClassValidatorMiddleware.ts#L37 issue might be lying here. I'll quickly get something working and I'll create a PR for you.

dbartholomae commented 4 years ago

If I understand correctly, then this is currently intended behavior. With the class above, '{}' is a valid body, but '' isn't.

tkgalk commented 4 years ago

Hm. This is counterintuitive to how other validation libraries work (ie. joi). But I understand the point. What would be your approach to accepting cases where body can be optional but we still need to validate particular props when they are passed? Currently it seems like it is either validating body or not, without the ability to validate it IF there is something present. 🤔

dbartholomae commented 4 years ago

To be honest I did not encounter that situation yet. Could you elaborate why this is needed? If there's a use case, then we could add a step that converts an empty body to an empty object before validation.

tkgalk commented 4 years ago

The main example I can come up with is POSTing an API to prepare big data export. It's not a GET, as it usually has some side effects. You can, but not necessarily, add a lot of filters to the body to limit the data exported. But no filters should still provide a valid response.

It can be overridden by sending empty {filters: []} though so it is not something that cannot be worked around. And is maybe more "correct" so I don't really have strong opinion here.

dbartholomae commented 4 years ago

I've played around and integrated it in a different middleware project I am using now instead of middy: https://github.com/dbartholomae/lambda-middleware/

It does make sense to treat '' body as '{}', and I'm open to a PR.

dbartholomae commented 5 months ago

Closed due to inactivity