Repository navigation
Add guidance on reporting errors in implementations - #536
Conversation
| protobuf messages for validate.proto itself, though usually it is useful to provide a higher level | ||
| wrapper for additional features like accessing failed fields via reflection. | ||
|
|
||
| The string message should be a summary of the violations rather than describing the entire |
There was a problem hiding this comment.
Note that protovalidate-go generally doesn't follow this by printing out every violation. I don't know if it's safe to change the protovalidate-go error message, but I think it's still worth recommending the summary form as the general guidance.
There was a problem hiding this comment.
I'm not sure failing with a summary as the default is the right recommendation. The default validation behavior (at least in pv-go) is to fail open, checking all fields and accumulating all the errors, so that the caller can respond to all violations in one shot, not to drip feed the errors after repeated validation passes. If the user is presented with a summary, that's equivalent to it failing fast/closed (but slower since it really ran open), especially as the rest of this patch implies access to all of the violation details are optional for an implementation to expose.
That said, giving the user the choice of how these errors are presented is reasonable. As an example, golangci-lint defaults to summarizing (though it's not as aggressive as the suggestion here), but on my own projects I immediately remove this behavior.
There was a problem hiding this comment.
I think the main point is that the error details are still available in the structured violations, which the user can still use, including constructing a large message if really desired so it's not quite equivalent to fail-fast. When discussing with Timo, we didn't see a good reason to duplicate all the content into the message as well while the negative effect of newlines in standard deployments like container logs can be quite large.
Note I didn't intend for the structured details to be optional in practice. I wasn't sure if this doc would use i.e. must instead of should but would it help?
That being said an option of course would be fine to allow both. If others also think it's a good idea I can add it, I would consider softer language like may provide an option since not sure it fits well in all languages.
There was a problem hiding this comment.
I went ahead and stated implementations must provide the structured details, and may provide an option for a full error message
|
Related discussion: bufbuild/protovalidate-py#546 |
| structrued violations. For example, a common format is to print the first validation error message, | ||
| and if there are more violations, the count of remaining violations, e.g., | ||
|
|
||
| `user.email: must be a valid email address [string.email], and 2 more violations` |
There was a problem hiding this comment.
protovalidate-go only appends the rule id ([string.email]) if there is a message, see bufbuild/protovalidate-go#291.
The PR had the same motivation as bufbuild/protovalidate-py#546: improve error strings.
Wouldn't it make sense to incorporate that here?
There was a problem hiding this comment.
Ah thanks, yeah that sounds good added it
|
|
||
| ### Reporting errors | ||
|
|
||
| A failed violation is reported to the user as the error type for the language with a string message. |
There was a problem hiding this comment.
A failed validation?
| If neither is available, print `[unknown]`. | ||
|
|
||
| An option may be provided to generate a message string containing all errors instead of a summary. | ||
|
|
There was a problem hiding this comment.
I think this is a reasonable idea. Let me suggest some slightly different language:
All validation failures found during validation must be reported to the user via the language's error mechanism. This error must include a string message that contains the field name with the first validation failure found, the rule ID and description of that validation failure, and a count of the remaining validation failures. The error may also contain a string message that lists the field names, rule IDs and descriptions of all validation failures.
The error must provide access to all validation failures found during validation. How these validation failures are accessed is language-specific.
Also, we should standardize on using the RFC 2119 meanings of MUST, MUST NOT, SHOULD, SHOULD NOT, and MAY.
There was a problem hiding this comment.
Thanks, reworded along those lines.
Also, we should standardize on using the RFC 2119 meanings
Yeah intending to use those words here informally, probably we need to move this out of CONTRIBUTING.md to really formalize things
There was a problem hiding this comment.
One more edit:
All validation failures found during validation must be reported to the user via the language's error mechanism. This error must include a string message that contains the field name with the first validation failure found, the rule ID and description of that validation failure, and a count of the remaining validation failures. The error may also contain a string message that lists the field names, rule IDs and descriptions of all detected validation failures.
A protovalidate implementation may choose to limit the number of validation failures detected and reported during validation. If an implementation does so, it must indicate in the error message that there are unreported validation failures.
The error must provide access to all validation failures found during validation. How these validation failures are accessed is language-specific.
There was a problem hiding this comment.
Thanks, a couple of tweaks to it
- Clarified that the "all violations" that are returned are post-limit which isn't only for the string message
- I said "rule ID or description of the validation failure", this is to reflect the feedback from Add guidance on reporting errors in implementations #536 (comment) unless we'd like to go ahead and change that
timostamm
left a comment
timostamm
left a comment
There was a problem hiding this comment.
I took a closer look at the implementations and what's already specified and documented. IMO field path formatting needs to be specified before we can put an example for the summary here.
To unblock the protovalidate-py ask, and to make an incremental step towards better specs, I suggest the following wording, which deliberately leaves out string formatting:
### Reporting validation failures
Validation failures must be reported to the user, either via the language's
error mechanism or as a result value that the user checks, whichever is
idiomatic for the language.
The error or result value must provide access to the structured failures: each
time a value does not meet a rule, the implementation provides a "violation"
that describes which rule failed, where it failed, and why.
The error or result value may provide a single string representation, either
listing every violation or summarizing them, for example the first violation
and the number of remaining violations.
Implementations may choose to limit the number of violations reported during
validation. Implementations may also provide an option to "fail fast", and
immediately return the result on the first rule violation encountered.
The canonical representation of a violation is the Protobuf message
`buf.validate.Violation`. Implementations may wrap violations in their own type,
for example to add a string representation, access to the failing value, or
other conveniences, but must provide access to the canonical representation.This documents and clarifies what implementations already do, without adding new requirements. The only exception is protovalidate-py's invalid <MessageName> message, which is what the protovalidate-py ask is about.
| any contribution that introduces a new function or variable must add its | ||
| signature there. | ||
|
|
||
| ### Reporting errors |
There was a problem hiding this comment.
Protovalidate implementations report validation failures, runtime errors, and compilation errors. I suggest to scope this change to validation failures:
| ### Reporting errors | |
| ### Reporting validation failures |
| ### Reporting errors | ||
|
|
||
| All validation failures found during validation must be reported to the user via | ||
| the language's error mechanism. This error must include a string message that |
There was a problem hiding this comment.
The first sentence does not fit Java, TypeScript, and C++ - they all return a validation result:
- class ValidationResult in protovalidate-java
- type ValidationResult in protovalidate-es
- class ValidationResult in protovalidate-cc
I do not think that these APIs can change, so this requires wording that fits both models.
|
|
||
| All validation failures found during validation must be reported to the user via | ||
| the language's error mechanism. This error must include a string message that | ||
| contains the field name with the first validation failure found, the rule ID or |
There was a problem hiding this comment.
"field name" is too loose. The example user.email we give below is a "field path", see the FieldPath message and fieldpath.go. Message rules don't always have a field path.
| detected and reported during validation. If an implementation does so, it must | ||
| indicate in the error message that there are unreported validation failures. |
There was a problem hiding this comment.
Indicating unreported sounds like a good idea. But I wonder if buf.validate.Violations should indicate it too. I don't have an answer for that, and think we're better off leaving that undefined until we know better.
|
Realized I had missed pushing up changes yesterday. Thanks for the help @timostamm, I agree reducing the scope seems to make sense here, applied the change |
This adds guidance on how to structure errors to return to the user in implementations, in CONTRIBUTING.md which is our current home for such language.