fmutils icon indicating copy to clipboard operation
fmutils copied to clipboard

support for wildcards

Open willfindlay opened this issue 2 years ago • 10 comments

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

willfindlay avatar Sep 09 '22 18:09 willfindlay

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.

mennanov avatar Sep 09 '22 18:09 mennanov

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

awltr avatar Mar 01 '23 08:03 awltr

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 https://github.com/mennanov/fmutils/issues/6#issue-1368180963 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.

mennanov avatar Mar 02 '23 08:03 mennanov

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

awltr avatar Mar 12 '23 21:03 awltr

Repeated fields are already supported, example: https://github.com/mennanov/fmutils/blob/d05c935c65dcb2a27fe40179cc5bb091bb14b633/fmutils_test.go#L265

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

mennanov avatar Mar 13 '23 22:03 mennanov

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 avatar Mar 14 '23 11:03 awltr

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

awltr avatar Mar 24 '23 21:03 awltr

There is an example on how to use fmutils.Filter() and proto.Merge(): https://github.com/mennanov/fmutils/blob/d05c935c65dcb2a27fe40179cc5bb091bb14b633/examples_test.go#L60

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.

mennanov avatar Mar 25 '23 21:03 mennanov

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

awltr avatar Mar 25 '23 23:03 awltr

@awltr please take a look at https://github.com/mennanov/fmutils/pull/10 and see whether it solves this issue

mennanov avatar Mar 20 '24 18:03 mennanov