Implement structured field and rule paths for violations #217
Conversation
|
Note: This can not be merged yet because it depends on changes that are not merged. However, the changes this PR depends on should be solidified by this point, as the proto changes that impact runtimes have been reviewed (sans for one small change made after review.) |
| if violation.HasField("field"): | ||
| violation.field.elements.reverse() | ||
| if violation.HasField("rule"): | ||
| violation.rule.elements.reverse() |
There was a problem hiding this comment.
I'm assuming we're reversing these because we have a list from root -> this nested field, and we want to render these as field -> root — is that right?
separately, is the structure of these fields covered by the conformance tests, or is that something we ought to separately add?
There was a problem hiding this comment.
I'm assuming we're reversing these because we have a list from root -> this nested field, and we want to render these as field -> root — is that right?
Field paths are constructed in reverse, as we're returning violations up through the stack. Prior to this PR, the string-based field path was prepended to at each step. After this PR, lists are appended to instead, and then those lists get reversed at the end. Prepending to a list is O(n) since it must shift all of the elements, so instead we append then reverse later (reverse is theoretically very cheap since it just needs to swap a bunch of pointers around.)
If we had our choice of data structure we could use a deque here instead, but we don't.
separately, is the structure of these fields covered by the conformance tests, or is that something we ought to separately add?
Absolutely! Painstakingly, the WIP conformance tests add field path and rule path tests to all existing violation cases. The entire field path and rule path are tested to ensure they match the conformance test cases. This PR passes right now.
protovalidate/validator.py
Outdated
| return ctx.violations | ||
|
|
||
| @classmethod | ||
| def _field_path_string(cls, path: validate_pb2.FieldPath) -> str: |
There was a problem hiding this comment.
nit: should we move this to protovalidate.internal and import it instead? doesn't need to be a classmethod AFAICS
There was a problem hiding this comment.
Sure, SGTM. Moved to internal.
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
|
Thanks for the quick reviews. I think this is good to go. |
Implements the changes necessary to propagate field and rule paths in Violation messages, passing conformance.
Depends on bufbuild/protovalidate#265.
DO NOT MERGE until the following is done: