Issue #10198: Utility func in FieldMaskUtil.java to clear fields specified in FieldMask

[selotape]

Hi,

I implemented a simple function on FieldMaskUtil to clear the fields specified in the mask instead of "copy only these fields", as described in Issue #10198.

For this, I extended the existing recursive FieldMaskTree.merge() method using a MergeOption.clearField. This is of course covered by tests I deemed relevant.

I'm not 100% confident about my choice of method names, nor about any performance/waste issues in destination.mergeFrom(source), nor about my decision to reuse and extend FieldMaskTree.merge() rather than copy+pasting+editing it.

I'd love to hear your thoughts.

Thanks!

[selotape]

Hey, can I help in any way? Maybe schedule a short VC with the reviewer to explain the changes so it's easier to review?

[shaod2]

Hi selotape, thanks for you contribution and sorry for the delay.

This is a sound PR. We're still evaluating it and its implication to other protobuf languages from our internal codebase.

[selotape]

Thank you!

I'd love to extend this CL as you think is needed (within my abilities).

Looking forward to news from you.

[selotape]

Hey, any news? Thanks!

[shaod2]

After having a closer look over our fieldmask API, doesn't trim() meet what you want? I know there's definitely a reason for "clearing" instead of "trimming", but just want to make sure that method was considered. Can you justify it with your use case?

[selotape]

Sure, using trim() is a possibility. It means I'll create+maintain a long list of all the ""whitelisted"" fields my consumers care about. This is certainly valid and more secure, but I think less suitable for me because:

• I'm sharing data with trusted backend services. I prefer the faster development, easier maintanence and "future-proofing" of a black list.
• The proto I'm sharing is large and nested and I'm presently not even sure of all fields my consumers care about. I am however sure of the large/costly fields I'd rather not share.
• For me this is "just" a traffic/bandwidth optimization. Even if I accidentally send a fields that's large, it's not critical and will fix it when an issue is detected.

So a list of "these fields should be deleted" is more suitable for my situation

[selotape]

Any thoughts shaod2?

[shaod2]

I've started a proposal but currently interrupted by some other tasks. I hope I can send it for review this week

[selotape]

Hey shaod2, please see my fixes

[selotape]

Hey @shaod2 , anything else you think needs attention?

[shaod2]

Hi sorry again for the delay. Unfortunately, we have to suspend this PR until further notice.

Protobuf is dealing with some ownership issues about the FieldMask API. Google has officially built up Google API Improvement Proposals, which contains some approved proposals (e.g. https://google.aip.dev/161) for FieldMask, but most of these proposals diverge from Protobuf and historically, updates to FieldMask libraries have caused confusions and inconsistency. Therefore, we are resolving this unclear ownership and stop any updates to it.