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

support for wildcards #6

Open
willfindlay opened this issue Sep 9, 2022 · 10 comments
Open

support for wildcards #6

willfindlay opened this issue Sep 9, 2022 · 10 comments

Comments

@willfindlay
Copy link

willfindlay commented Sep 9, 2022

Hi there.

I was wondering if you would be interested in a PR to add support for wildcards in field filters. For example, something like "*.a.b.c" would match all paths with any field followed by a nested a, b, and c field. Perhaps this could be exposed through a separate WildcardNestedMask type to avoid changing existing behaviour.

If you would be interested in this, please let me know, and I would be happy to make a PR. :)

@mennanov
Copy link
Owner

mennanov commented Sep 9, 2022

Sounds interesting!

As you mentioned this should be implemented separately from the existing API as the wildcard does not seem to be a valid input there.

@awltr
Copy link

awltr commented Mar 1, 2023

Is a PR planned? Repeated wildcards seem to be intended https://google.aip.dev/161#wildcards

@mennanov
Copy link
Owner

mennanov commented Mar 2, 2023

Is a PR planned? Repeated wildcards seem to be intended https://google.aip.dev/161#wildcards

That doc mentions the wildcards in the context of repeated fields only as a way to describe all possible indexes in a list.

If I got the initial request right #6 (comment) the wildcards are wanted on all types of fields, not just repeated fields.

In both cases this should be possible to implement.
I may try to come up with something within the next few weeks or so.

@awltr
Copy link

awltr commented Mar 12, 2023

Sounds great. If you are interested in a PR regarding the repeated field wildcard just let me know.

@mennanov
Copy link
Owner

Repeated fields are already supported, example:

name: "mask with repeated nested fields keeps the listed fields",

If the wildcard is what you need then your PR is welcome (keep in mind that the existing behavior should also be supported).
Thanks!

@awltr
Copy link

awltr commented Mar 14, 2023

Yes, had a look into the sources on mobile and also thought that it already should be possible just without wildcard asterisk. Should be fine, will try it in a few days. Thanks

@awltr
Copy link

awltr commented Mar 24, 2023

@mennanov Works well, thanks.

But I don't understand why the recommendation is to normalize and validate paths since normalize would remove fields of repeated messages.

And in fieldmask-utils you referring to this project for simple usage with protobuf messages but this project does not cover merge with fieldmask (which is needed for update masks).

Or am I missing something?

edit: I will consider a PR for Merge. Shouldn't be a big deal, then we can discuss it there.

edit2: Since proto.Merge accepts populated fields only, a combination of fmutils.Filter and proto.Merge should do the job for update masks. Maybe some special handling is needed if you want to explicitly overwrite fields with empty values. But that probably should not be part of this package.

@mennanov
Copy link
Owner

There is an example on how to use fmutils.Filter() and proto.Merge():

fmutils.Filter(updateProfileRequest.GetProfile(), updateProfileRequest.Fieldmask.GetPaths())

But I don't understand why the recommendation is to normalize and validate paths since normalize would remove fields of repeated messages.

Yeah, that's true. Looks like repeated messages are not well supported by the standard go proto package: they are considered invalid. However this library supported them.

@awltr
Copy link

awltr commented Mar 25, 2023

Thanks for your reply!

Although we're leaving the topic of this issue. In my opinion this merge approach does not solve two requirements:

  • Override a value with it's zero/nil value
  • Override a repeated field instead of appending it

@mennanov
Copy link
Owner

@awltr please take a look at #10 and see whether it solves this issue

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

No branches or pull requests

3 participants