mennanov / fmutils

Golang protobuf FieldMask missing utils
MIT License
97 stars 10 forks source link

support for wildcards #6

Open willfindlay opened 1 year ago

willfindlay commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

mennanov commented 1 year ago

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.

awltr commented 1 year ago

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

mennanov commented 1 year ago

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!

awltr commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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.

awltr commented 1 year ago

Thanks for your reply!

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

mennanov commented 4 months ago

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