-
Notifications
You must be signed in to change notification settings - Fork 20
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
fix(fieldmask): traverse repeated fields with wildcard in validation #269
base: master
Are you sure you want to change the base?
Conversation
fieldmask/validate.go
Outdated
fd := md.Fields().ByName(protoreflect.Name(field)) | ||
// Parent message is a repeated field. | ||
// Use of AIP161 optional wildcards to target sub-fields | ||
// on repeated fields is not supported |
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.
isn't the spec saying that it is supported on repeated fields, but the field in question must be a wildcard? *
?
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.
Field masks may permit the use of the * character on a repeated field or map to indicate the specification of particular sub-fields in the collection:
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.
It does maybe the comment should be reworded, but it felt worthy of a clarification.
My intention was not to add the optional support for wildcard values in here but tackle the faulty interpretation of fieldmasks that don't traverse using wildcard *
The choice to disallow the use of these wildcards was then to bring it inline with how the fieldmask update function behaves where list traversal is not permitted. The AIP rule is also stated as you MAY support it, meaning we can stick to not supporting it.
I can look into adding the support to traverse with wildcard in both the validate and update functions if that would be something we want to add?
943b5bb
to
3214947
Compare
@odsod I have changed the PR to traverse repeated fields with wildcards to be fully aligned with AIP-161 and updated the PR description to explain that it does not align with how the Looking into how to support this subfield targeting in the |
3214947
to
dc715d2
Compare
This PR has been open for 180 days with no activity. Remove the stale label or add a comment or it will be closed in 14 days. |
fieldmask.Validate
incorrectly allows targeting of subfields on repeated fields aswhereas AIP161 specifies that targeting of subfields MAY be added using wildcards as a suffix to the repeated field, such as:
This PR proposes to allow traversing repeated fields with the use of wildcard
*
when validating a fieldmask.Note that the targeting subfields on repeated fields is not handled in
fieldmask.Update
.