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

Add structured field and rule paths to Violation #265

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

Conversation

jchadwick-buf
Copy link
Member

@jchadwick-buf jchadwick-buf 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.)

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 7, 2024, 7:20 PM

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

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...

@jchadwick-buf jchadwick-buf changed the title Add rule path to Violation message Add structured field and rule paths to Violation Nov 6, 2024
@jchadwick-buf jchadwick-buf marked this pull request as ready for review November 6, 2024 23:59
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