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

fix(fieldmask): traverse repeated fields with wildcard in validation #269

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oscarmuhr
Copy link
Contributor

@oscarmuhr oscarmuhr commented Dec 13, 2023

fieldmask.Validate incorrectly allows targeting of subfields on repeated fields as

repeated.subfield

whereas AIP161 specifies that targeting of subfields MAY be added using wildcards as a suffix to the repeated field, such as:

repeated.*.subfield

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.

...
case field.IsList(), field.IsMap():
	// nested fields in repeated or map not supported
	return
...

@oscarmuhr oscarmuhr requested a review from a team as a code owner December 13, 2023 08:34
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
Copy link
Member

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? *?

Copy link
Member

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:

Copy link
Contributor Author

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?

@oscarmuhr oscarmuhr force-pushed the fieldmask-validate-repeated-fields branch 2 times, most recently from 943b5bb to 3214947 Compare December 19, 2023 13:02
@oscarmuhr oscarmuhr changed the title fix(fieldmask): disallow repeated field traversal in validation fix(fieldmask): traverse repeated fields with wildcard in validation Dec 19, 2023
@oscarmuhr
Copy link
Contributor Author

oscarmuhr commented Dec 19, 2023

@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 fieldmask.Update function treats such paths in the fieldmask.

Looking into how to support this subfield targeting in the fieldmask.Update function, this is not a straightforward thing to do so we should probably avoid to do it in this lib and leave any individual implementer to handle that on their own.

@oscarmuhr oscarmuhr force-pushed the fieldmask-validate-repeated-fields branch from 3214947 to dc715d2 Compare December 19, 2023 13:59
Copy link

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.

@github-actions github-actions bot added the Stale label Jun 17, 2024
@oscarmuhr oscarmuhr removed the Stale label Jun 17, 2024
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