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

Return Value in Error Message when Validation Fails #125

Open
cnoon opened this issue Nov 3, 2023 · 1 comment
Open

Return Value in Error Message when Validation Fails #125

cnoon opened this issue Nov 3, 2023 · 1 comment
Labels
Feature New feature or request

Comments

@cnoon
Copy link

cnoon commented Nov 3, 2023

Feature description:
The proto validation tooling is fantastic! It's super helpful and easy to understand what the core issue is. Something that would make it even better would be to add the actual value to the message that failed validation.

Problem it solves or use case:
While it's currently very helpful to be able to see exactly which field failed validation, it's VERY difficult to figure out what the original value was. We've had to go to some extreme lengths to print out the entire proto object client-side when a field validation error occurs. Instead, it would be SO much easier if the failure message included the original value that failed the validation. It may make sense as well to make the option configurable in case there are concerns with PII being in the failure message.

Proposed implementation or solution:
Here's a simple example of an orderId not passing the regex validation that would be very helpful to add the failing orderId to the message.

Original Message

"invalid ClickstreamEvent.OrderEvent: embedded message failed validation | caused by: invalid OrderEvent.OrderId: embedded message failed validation | caused by: invalid OrderId.Value: value does not match regex pattern \"^[0-9a-fA-F]{2,64}$\""

Modified Message

"invalid ClickstreamEvent.OrderEvent: embedded message failed validation | caused by: invalid OrderEvent.OrderId: embedded message failed validation | caused by: invalid OrderId.Value: \"INVALID_ORDER_ID\" does not match regex pattern \"^[0-9a-fA-F]{2,64}$\""

In the event that the orderId was empty, it would be helpful to explicitly state that as well.

@cnoon cnoon added the Feature New feature or request label Nov 3, 2023
@rodaine
Copy link
Member

rodaine commented Nov 3, 2023

Hi @cnoon! Thanks for praise and feedback. 😁

You hit the nail on the head with the value not being included in the error to avoid PII or security concerns. We want users of protovalidate to choose how they present their errors for the best and have it be safe by default. This value might also be ambiguous, such as when the violation results from a custom CEL expression at the message level that compares two fields or a message field custom constraint that might operate against subfields.

That said, internally we've also encountered this issue and have talked through some potential solutions.

We definitely want to provide a way to go from a violation error and message to the value in question. This can be done in the protovalidate implementations as a function that takes a violation error & message, and returns the value. There've been similar requests to be able to resolve the constraint definition(s) as well.

With that available, we can evaluate if it makes sense to provide configuration that automatically wraps errors with this information in the validator. I can see this getting messy when people want to conditionally include the information or not depending on a PII annotation. Wrapping the validator is still always possible to apply this feature with the helper function above, though.

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

No branches or pull requests

2 participants