Validation not working for new entry in replaceOne route

[cleivson]

Hi, I was currently struggling with data validation in the PUT method since it Creates or Updates an entity I thought it would respect the corresponding CrudValidationGrup depending on the actual action it's going to perform.

But after some digging, I realized that the "problem" was in this line and occurred me that is too hard to decide what validation group it's going to use at the route creation.

Since the validation occurs before the endpoint actually getting hit and therefore before it can check entity existence, I'm not sure what would be the best approach to this.

From the top of my head what I can think of is:

• Change the mentioned line to make PUT use the same validations as POST (probably the easiest way to change, but I'm not sure if it can bring any hidden bugs. I will open a CR for this).
• Change replaceOne to be a route that does no validation and just checks user existence and redirects to the appropriate route with the appropriate validations (may increase the overhead for each call)
• Create a new CrudValidationGroup (I don't think that helps very much since it will probably be used at the same places as the CREATE)

For now I can overcome this by hiding the replaceOneBase route, creating one of my own and redirecting the call to replaceOneBase after validation. The problem is that I don't know an easy way of adding the ValidationPipe with the correct configurations besides of creating a new controller and overriding createOneBase (what is terrible).

[michaelyali]

@cleivson so you suggesting adding a new validation group? Yeah, probably we should consider that.
Meanwhile, you could use dto options to provide a dto for a replace action

[jonahsnider]

I have a similar issue with CRUD validation groups where it seemed the groups were being triggered on all write/create/update requests. It's causing a major headache because the CREATE validation is being triggered on PATCH requests.

[cleivson]

@cleivson so you suggesting adding a new validation group? Yeah, probably we should consider that.

I don't know if a new validation group would be the best experience, probably validating PUT as a create method would be better since the methods idea is to replace a resource, it's basically inserting an entity over another one.

Meanwhile, you could use dto options to provide a dto for a replace action

That's my workaround for the time being.

[michaelyali]

@cleivson so you indeed can validate replace action with dto, right? So why an issue though?

[cleivson]

@zMotivat0r I believe that despite being able to work around the issue, the framework should behave as expected out of the box, right?

Think of it as if the createOne method didn't respect the CREATE validation group, it would be the same type of issue since there is a feature in the framework and it wouldn't be working, don't you agree?

[michaelyali]

I see only one solution here is to add replace validation group. As per your point here:

Create a new CrudValidationGroup (I don't think that helps very much since it will probably be used at the same places as the CREATE)

in general, it should be in the same places as the CREATE, except maybe it also should cover some primary columns, or auto-generated ones, because PUT method means you need to send the whole resource but not the partial resource as you might send it on PATCH request.
So adding REPLACE validation group will definitely help here.
Actually I thought about adding it, but there were so much complains about this whole idea of validation and a lot of user requests for adding the ability to set DTO for each action so I started thinking that sooner or later the majority of the lib users will switch to the normal DTO validation

[cleivson]

I agree that the custom DTO would come first in terms of prioritization as it adds more flexibility in terms of validation and data hiding.

in general, it should be in the same places as the CREATE, except maybe it also should cover some primary columns, or auto-generated ones, because PUT method means you need to send the whole resource but not the partial resource as you might send it on PATCH request.

I partially agree on that, I just don't understand why should PUT validate more fields than create, but I accept that it could.

If we had to add a replace validation group, I think it should be complimentary to create since create defines the minimum requirements for registering an entity, no method should allow less than that.

[dawinter]

#503 has been closed as duplicate from this item here. @cleivson Can you verify that the described issue here is the same as in #503?

@cleivson A comment to your statement about validation groups for POST, PUT and PATCH.
Yes there are differences in validation logic for POST, PUT and PATCH.
For example in case of the id or revision, these fields are not madatory in POST but required in PUT because they will be created during the POST request processing.
Same with PUT and PATCH, PUT is to replace the full entry and require all fields as mandatory. But PATCH is to partial change properties and just need partial information, means all fields are optional.
Therefore PATCH needs a own validation group and a change in the method signature to updateOne(req: CrudRequest, dto: Partial).

I recognized the tag PR open. Does the PR already contain the mentioned changes?

[cleivson]

@dawinter it looks essentially the same problem, but I was having problems with PUT and you apparently had problems with PATCH. The problem is that both methods use the same validation group so it's hard to define what's mandatory in each method.

About your statement that some fields are required in POST and not required in UPDATE, you probably have to pass the id in the route for update so I think you could use the paramsOverride to fill this field even if the client doesn't fill it.

I think having 3 method groups would be cumbersome and make the entities crowded with annotations, so my proposal was to use the create group in update method.

I don't remember opening a CR for this, so I cannot confirm if your required changes are implemented.