quarkus-resteasy-problem icon indicating copy to clipboard operation
quarkus-resteasy-problem copied to clipboard

Issue regarding propertynames in constraintviolation.

Open bdunni opened this issue 1 year ago • 13 comments

When I throw a plain constraing violation, violating as an example: email address. I recieve the message in the issue, but I do not get the fieldname or what the actual value is.

I reviewed the code and found the below implementation quite weird.

Why do we visit the iterator twice to get the propertyname?

https://github.com/TietoEVRY/quarkus-resteasy-problem/blob/da642a0aad2d5892fc1ecc466edf4005c468e8db/runtime/src/main/java/com/tietoevry/quarkus/resteasy/problem/validation/ConstraintViolationExceptionMapper.java#L64-L78

bdunni avatar Sep 07 '23 12:09 bdunni

Hi @bdunni, it may seem weird, but hibernate validator puts controller's class name and method name as first 2 elements of the property path - we decided we don't want to leak this implementation detail outside of REST api - api clients don't care if it's java or php, if it's handled by ResourceA, or ResourceB.

This may interfer with your use case - would you mind posting reproducer/example here?

lwitkowski avatar Sep 07 '23 13:09 lwitkowski

Hey @lwitkowski thanks for the quick reply.

I will see if I can make a reproducer example.

It also has me questioning line 75. We compare the parameter name in the resource to the parameter name in violation. Am I wrong to assume that this is also interfering with my use case?

In my case I have validation on "Email" it is part of the body of my request, but all my resourceInfo method parameters delivers is the name of the DTO which email is a part of. Hence this will never be a valid?

as an example:

public class PersonDTO { @NotNull String email }

Then the parameters returned by line 75 will return PersonDTO, but never email as it is inside the object?

bdunni avatar Sep 07 '23 14:09 bdunni

@lwitkowski I have to post from another account.

I reproduced the problem here: https://github.com/NissenDev/Http-problem-reproduce

Simply make a post with the following Json: { "email": 222222, "title": "45654565" }

To http://localhost:8080/hello

I used POSTMAN

NissenDev avatar Sep 07 '23 20:09 NissenDev

So it seems to be that we do not support the @RequestBody as a param of the ResourceInfo methods. Furthermore the 2 first elements of the controller and name and class are not present when doing the post.

bdunni avatar Sep 08 '23 07:09 bdunni

@NissenDev @bdunni thanks for reproducer, now I see that you use Validator programmatically, instead of simply using @Valid annotation on Book request param.

@Valid usage is covered with tests, and works as designed. Have a look at this test: https://github.com/TietoEVRY/quarkus-resteasy-problem/blob/master/integration-test/src/main/java/com/tietoevry/quarkus/resteasy/problem/ValidationExceptionsResource.java#L34

We would need to find a way to detect if this exception comes from @Valid annotation or from programmatic usage of Validator, as they produce different ConstraintViolationException. I strongly suggest using @Valid when possible though.

lwitkowski avatar Sep 14 '23 14:09 lwitkowski

@lwitkowski It works when I use @Valid directly on the requestParam.

We do run into a problem because we're using validation groups. Which are added programatically later on. So in this case we have to use it as is. It produces the same empty fields and unknown variables as the example above.

In our case it has something to do with external and internal calls or Business or private users. They are validated differently.

bdunni avatar Sep 18 '23 11:09 bdunni

I am thinking we should support: https://quarkus.io/guides/validation#validation-groups-for-rest-endpoint-or-service-method-validation.

And throwing constraintValidationException manually.

bdunni avatar Sep 20 '23 09:09 bdunni

@bdunni support for validation groups is a nice feature request, I've never used them. I'll try have a closer look.

Regarding original issue, after merging #313 we could add a config flag, something like: quarkus.resteasy.problem.constraint-violation.sanitize-parameters=true|false

This way one could disable removing first two segments of the param path, and that may potentially solve your problem.

lwitkowski avatar Oct 28 '23 10:10 lwitkowski

I wouldn't be against that what so ever.

Sounds like a decent solution.

However what if they were to use the constraints programatically and from the annotation? Wouldn't this be an issue?

bdunni avatar Nov 16 '23 10:11 bdunni

However what if they were to use the constraints programatically and from the annotation? Wouldn't this be an issue?

When you opt-in for not sanitizing parameters, you'll get everything that is in ConstraintViolationException.getConstraintViolations(). getPropertyPath(), unchanged in any way. So it will work as good as the code that creats these exception :)

lwitkowski avatar Nov 17 '23 09:11 lwitkowski

Would it also know when it should ignore the 2 first propertynames you mention here:

hibernate validator puts controller's class name and method name as first 2 elements of the property path

And when not to?

If so, lets go!

bdunni avatar Nov 17 '23 11:11 bdunni

No, it would never ignore anything if this flag is active - it would work similar to default Quarkus implementation. The consequence would be that Class and method name will leak outside rest api, which we tried to avoid with this 'remove 2 first segments from path' logic - but I don't see other option.

I have no idea how to reliably detect different usecases without increasing complexity too much.

lwitkowski avatar Nov 17 '23 11:11 lwitkowski

If I correctly recall at the moment we wrote this implementaiton there were not means to determine how exception has been triggered. We had to make "one-way" decission based on "most common use-case" for scenarios we recognized at that moment of time.

I'm not sure if API changed since then so there might not be an option with "increased complexity".

pazkooda avatar Nov 17 '23 12:11 pazkooda