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

Implement structured field and rule paths, add field and rule values to ValidationErrors #154

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

jchadwick-buf
Copy link
Member

@jchadwick-buf jchadwick-buf commented Oct 15, 2024

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 ValidationError type is no longer an alias to the protobuf buf.validate.Violations message. 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.Value pointers.)

DO NOT MERGE until the following is done:

Copy link

github-actions bot commented Oct 15, 2024

The latest Buf updates on your PR. Results from workflow Buf / validate-protos (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedNov 27, 2024, 6:53 PM

@jchadwick-buf jchadwick-buf marked this pull request as draft October 22, 2024 16:02
@jchadwick-buf jchadwick-buf changed the title Refactor validation errors Add structured field and rule paths, field and rule values to ValidationErrors Nov 6, 2024
@jchadwick-buf jchadwick-buf marked this pull request as ready for review November 7, 2024 00:10
@jchadwick-buf
Copy link
Member Author

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 Violation is still the primary way to consume a Violation, and we only need to add a utility function and an additional type to the protovalidate package.

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 jchadwick-buf changed the title Add structured field and rule paths, field and rule values to ValidationErrors Implement structured field and rule paths, add field and rule values to ValidationErrors Nov 25, 2024
jchadwick-buf added a commit to bufbuild/protovalidate that referenced this pull request Nov 26, 2024
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
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

Successfully merging this pull request may close these issues.

1 participant