Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue regarding propertynames in constraintviolation. #295

Open
bdunni opened this issue Sep 7, 2023 · 13 comments
Open

Issue regarding propertynames in constraintviolation. #295

bdunni opened this issue Sep 7, 2023 · 13 comments

Comments

@bdunni
Copy link

bdunni commented Sep 7, 2023

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

@lwitkowski
Copy link
Collaborator

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?

@bdunni
Copy link
Author

bdunni commented Sep 7, 2023

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?

@NissenDev
Copy link

@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

@bdunni
Copy link
Author

bdunni commented Sep 8, 2023

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.

@lwitkowski
Copy link
Collaborator

lwitkowski commented Sep 14, 2023

@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.

@bdunni
Copy link
Author

bdunni commented Sep 18, 2023

@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
Copy link
Author

bdunni commented Sep 20, 2023

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

And throwing constraintValidationException manually.

@lwitkowski
Copy link
Collaborator

@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.

@bdunni
Copy link
Author

bdunni commented Nov 16, 2023

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?

@lwitkowski
Copy link
Collaborator

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 :)

@bdunni
Copy link
Author

bdunni commented Nov 17, 2023

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!

@lwitkowski
Copy link
Collaborator

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.

@pazkooda
Copy link
Collaborator

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".

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

No branches or pull requests

4 participants