-
Notifications
You must be signed in to change notification settings - Fork 37
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
Conversation
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
I finished adding |
… jchadwick/add-rule-path
JH realized that our idea for handling the unknown map key types was not quite enough so I made a slight tweak: now |
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repeated?
There was a problem hiding this comment.
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?)
// `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. |
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
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
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)
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.
buf.validate.FieldPathElement
is added.repeated_field[1]
buf.validate.FieldPath
is added. It just contains a repeated field ofbuf.validate.FieldPathElement
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 withproto.Equal
.buf.validate.Violation
fields are added:field
andrule
, both of typebuf.validate.FieldPath
. The oldfield_path
field is left for now, but deprecated.Note that there are a number of very subtle edge cases:
FieldConstraints
message. (In other cases,constraint_id
is always sufficient anyways, but we can change this behavior later.)Implementations: