Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Oct 16, 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:

@github-actions
Copy link

github-actions bot commented Oct 16, 2024

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedNov 25, 2024, 5:23 PM

@ghost ghost marked this pull request as ready for review October 17, 2024 15:34
@ghost ghost marked this pull request as draft October 17, 2024 15:36
@ghost
Copy link
Author

ghost commented Oct 22, 2024

I finished adding rule_path to all violation cases in the conformance test suite, but unfortunately doing so did reveal a mistaken assumption I've been making: I've been thinking that we can just follow the field path and find the rule on its options, but, somewhat obviously in retrospect, the field the violation is on might not be the field that has the rule. Particularly in the repeated.items, map.keys and map.values cases. It might be possible to still work backwards by parsing the rule path and using it to back track from the field path...

@ghost ghost changed the title Add rule path to Violation message Add structured field and rule paths to Violation Nov 6, 2024
@ghost ghost marked this pull request as ready for review November 6, 2024 23:59
@ghost ghost requested a review from rodaine November 6, 2024 23:59
@ghost
Copy link
Author

ghost commented Nov 25, 2024

JH realized that our idea for handling the unknown map key types was not quite enough so I made a slight tweak: now FieldPathElement doesn't have a special case for sint* types and instead the map key and value protobuf kinds are conveyed directly. This is a little simpler anyways since it closer mirrors what you would want for both the reflection and unknown wire data cases. Adding map value type isn't strictly necessary, but it brings paths referring to primitives inside map values to parity with paths referring to primitives inside messages and repeated fields: it will be possible to display them even if the field is unknown. Seems like it's worth it for 2 bytes of wire data.

@ghost ghost merged commit 41573d9 into main Nov 26, 2024
6 checks passed
@ghost ghost deleted the jchadwick/add-rule-path branch November 26, 2024 21:00
// value inside unknown fields through wire data.
optional google.protobuf.FieldDescriptorProto.Type value_type = 5;

// `subscript` contains a repeated index or map key, if this path element nests into a repeated or map field.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

repeated?

Copy link
Author

@ghost ghost Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FieldPathElement is meant to be logically equivalent to a path segment, e.g. repeated[1] or map["key"]. The subscript contains either a repeated field index or a map key. If this wording is confusing, I'm happy to update the documentation.

WDYT about this? (would "list index" be better?)

Suggested change
// `subscript` contains a repeated index or map key, if this path element nests into a repeated or map field.
// `subscript` contains a repeated field index or map key, if this path element nests into a repeated or map field.

ghost pushed a commit to bufbuild/protovalidate-python that referenced this pull request Nov 27, 2024
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:
- [x] bufbuild/protovalidate#265 is merged
- [x] Dependencies updated to stable version release
ghost pushed a commit to bufbuild/protovalidate-cc that referenced this pull request Nov 27, 2024
Implements the changes needed to pass conformance after
bufbuild/protovalidate#265.

DO NOT MERGE until the following is done:
- [x] bufbuild/protovalidate#265 is merged
- [x] Dependencies updated to stable version release
ghost pushed a commit to bufbuild/protovalidate-java that referenced this pull request Dec 2, 2024
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)
ghost pushed a commit to bufbuild/protovalidate-go that referenced this pull request Dec 12, 2024
…to ValidationErrors (#154)

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:
- [x] bufbuild/protovalidate#265 is merged
- [x] Dependencies updated to stable version release
This pull request was closed.
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.

2 participants