search

aip-dev / google.aip.dev

API Improvement Proposals. https://aip.dev/
Other
1.1k stars 500 forks source link

Seeking additional information about read_mask deprecation #912

Open btc opened 2 years ago

btc commented 2 years ago

Hi. I noticed that the read_mask field is no longer advised on in-band request messages. I am hoping to locate more information about the motivation behind deprecating this practice.

related: https://github.com/aip-dev/google.aip.dev/commit/2ad98dc7c5c8973ffacd34621ad5f9913e0334f0 and https://github.com/aip-dev/google.aip.dev/issues/814 and https://github.com/aip-dev/google.aip.dev/issues/902

enzam-db commented 1 year ago

Bumping this up. Can you please share the discussion/reasoning behind deprecating read_mask and the alternatives suggested (like a view?)?

enzam-db commented 1 year ago

Okay, answering myself, at least the new guidance is here - https://google.aip.dev/157

The motivation - not so clear. One clear win I can see is - for Update requests, we can now specify both update_mask and read_mask easily - update_mask as part of the request, and read_mask can be passed as described in AIP-157.

shwoodard commented 1 year ago

We're working on it.

jankrynauw commented 1 year ago

Also interested to understand the thinking behind this. Could it be related to the 'fields' attribute that have been used fairly consistently across Google's APIs? Here are a few examples:

toumorokoshi commented 1 year ago

Hi! yes, the rationale to remove read_mask in the request is because it overlapped with the fields system parameter.

We are looking for feedback on this choice. But as fields is supported globally, it would be great to hear how we benefit from an additional field with similar behavior.

jankrynauw commented 1 year ago

ah, thank you this makes sense!

On Mon, 1 May 2023 at 17:23, Yusuke Tsutsumi @.***> wrote:

Hi! yes, the rationale to remove read_mask in the request is because it overlapped with the fields system parameter https://cloud.google.com/apis/docs/system-parameters.

We are tacking feedback on this choice at the moment. But as fields is supported globally, it would be great to hear how we expect to understand the benefit of an additional field with similar behavior.

— Reply to this email directly, view it on GitHub https://github.com/aip-dev/google.aip.dev/issues/912#issuecomment-1529830959, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEJOSVRFVZPZWGC7OJTXGH3XD7IOXANCNFSM5ZEBPQSA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

zchenyu commented 2 months ago

It makes sense in the context Google's existing fields parameter, but AFAICT that's not an "AIP" concept. So for new APIs based on AIPs, it feels a bit awkward. Especially since update_mask is specified in the request body.