Implement structured field and rule paths, add field and rule values to ValidationErrors #154
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / validate-protos (pull_request).
|
jchadwick-buf
changed the title
|
Well, it took a while, but I think this is the most bang-for-our-buck possible. I think it's not the best possible code by any single measure, but I tried to balance concerns of performance, simplicity and API surface. A lot of thought was put into exactly how paths are constructed: as before, we avoid constructing paths unless a validation error is actually encountered. A careful adjustment is made to the builder code to add just enough information to be able to produce the rule paths accurately in as many edge cases as I could find, without requiring new special cases to be distributed in more places. To minimize the resulting API surface of changing the error type, the protobuf form of a It would be difficult to convey the number of different considerations that wound up needing to be made, but hopefully the resulting code does not make this difficulty apparent. :) |
jchadwick-buf
changed the title
This PR introduces a new structured field path format, and uses it to provide a structured path to the field and rule of a violation. - The new message `buf.validate.FieldPathElement` is added. - It describes a single path segment, e.g. equivalent to a string like `repeated_field[1]` - Both the text name and field number of the field is provided; this allows the field path to be rendered into a string trivially without the need for descriptor lookups, and will work for e.g. unknown fields. (Example: A new field is marked required; old clients can still print the field path, even if they do not have the new field in their schema.) - It also contains the kind of field, to make it possible to interpret unknown field values. - Finally, it contains a subscript oneof. This contains either a repeated field index or a map key. This is needed because maps in protobuf are unordered. There are multiple map key entries, one for each distinctly encoded valid kind of map key. - The new message `buf.validate.FieldPath` is added. It just contains a repeated field of `buf.validate.FieldPathElement` - It would be possible to just have `repeated buf.validate.FieldPathElement` anywhere a path is needed to save a level of pointer chasing, but it is inconvenient for certain uses, e.g. comparing paths with `proto.Equal`. - Two new `buf.validate.Violation` fields are added: `field` and `rule`, both of type `buf.validate.FieldPath`. The old `field_path` field is left for now, but deprecated. - The conformance tests are updated to match the expectations. Note that there are a number of very subtle edge cases: - In one specific case, field paths point to oneofs. In this case, the last element of the fieldpath will contain only the field name, set to the name of the oneof. The field number, field type and subscript fields will all be unset. This is only intended to be used for display purposes. - Only field constraints will output rule paths, because it is a relative path to the `FieldConstraints` message. (In other cases, `constraint_id` is always sufficient anyways, but we can change this behavior later.) - Custom constraints will not contain rule paths, since they don't have a corresponding rule field. (Predefined constraints will contain rule paths, of course.) Implementations: - bufbuild/protovalidate-go#154 - bufbuild/protovalidate-python#217 - bufbuild/protovalidate-cc#63
Implements the changes needed to pass conformance after bufbuild/protovalidate#265. This is basically a straight port of what is done in bufbuild/protovalidate-go#154, owing to the similarities between protovalidate-go and protovalidate-java. Unfortunately this means it inherits some of the trickier maneuvers needed to weave the right data into the right places. This version is also not as efficient as a result of differences between Go and Java protobufs, but effort was made to try to prevent big regressions in the case of successful validation. Some of this is probably not as pretty as it could be. I'm open to improvements if there are any obvious things (I am certainly not a Java expert after all) but if possible I'd like to defer anything that would require major rethinking to down the road. (Happy to add TODOs for those, though.) DO NOT MERGE until the following is done: - [x] bufbuild/protovalidate#265 is merged - [x] Dependencies updated to stable version release (waiting on manage module push)
|
After some discussion the API was updated to use a struct for the |
|
Wound up making a few more tweaks:
|
Adds the ability to access the captured rule and field value from a
`Violation`.
**This is a breaking change.** The API changes in the following ways:
- `ValidationError` has changed:
- Old accesses to `ValidationError.violations` should call
`ValidationError.to_proto()` instead, to get a `buf.validate.Violations`
message.
- `ValidationError.errors` was removed. Switch to using
`ValidationError.violations` instead.
- `ValidationError.violations` provides a list of the new `Violation`
wrapper type instead of a list of `buf.validate.Violation`.
- The new `Violation` wrapper type contains the `buf.validate.Violation`
message under the `proto` field, as well as `field_value` and
`rule_value` properties that capture the field and rule values,
respectively.
- `Validator.collect_violations` now operates on and returns
`list[Violation]` instead of the protobuf `buf.validate.Violations`
message.
This API mirrors the changes being made in protovalidate-go in
bufbuild/protovalidate-go#154.
There was a problem hiding this comment.
Can you add some example tests using the Violations to demonstrate how a user would leverage them? Don't need to be too fancy, but give a jumping point for someone to implement i18n or other custom behaviors.
There was a problem hiding this comment.
I've added an example, ExampleValidationError_localized, to validator_example_test.go. (I had to look up how you were supposed to add multiple tests for one type, and in case you are not familiar, it is in fact standard to use an underscore and lowercase character here. Wild!)
| case protoreflect.EnumKind, protoreflect.FloatKind, protoreflect.DoubleKind, | ||
| protoreflect.BytesKind, protoreflect.MessageKind, protoreflect.GroupKind: |
There was a problem hiding this comment.
This was what I originally did, but note that our golangcilint configuration prohibits not listing a case:
internal/evaluator/map.go:77:4: missing cases in switch of type protoreflect.Kind: protoreflect.EnumKind, protoreflect.FloatKind, protoreflect.DoubleKind, protoreflect.BytesKind, protoreflect.MessageKind, protoreflect.GroupKind
But I can still add a default case and use a fallthrough here, for good measure.
internal/expression/compile.go
Outdated
| GetId() string | ||
| GetMessage() string | ||
| GetExpression() string | ||
| // Expression is a container for the information needed to compile and evaluate |
There was a problem hiding this comment.
| // Expression is a container for the information needed to compile and evaluate | |
| // Expressions are a container for the information needed to compile and evaluate |
There was a problem hiding this comment.
Fixed, although I went slightly different to make it sound imo a bit better. (An Expressions instance is...)
|
Thanks for the review, I think we're in good enough shape to merge now. |
This implements the structured field and rule paths, passing the conformance test changes in bufbuild/protovalidate#265.
In addition, it also adds the ability to get the field and rule values. These are not present in the protobuf form of the violation.
The API is still heavily centered around the protobuf form of violations, but the
ValidationErrortype is no longer an alias to the protobufbuf.validate.Violationsmessage. The intention is that users needing access to the error information will still use the proto violations message, only using the "native" interface methods to access in-memory data that is not feasible to transport over the wire. (e.g.protoreflect.Valuepointers.)DO NOT MERGE until the following is done: