Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2898683002:
Default unspecified value added to enum in proto (Closed)
DescriptionDefault unspecified value added to enum in proto
Updated the enum in proto to be according to the rules from
https://engdoc.corp.google.com/eng/doc/devguide/proto/index.md?cl=head
BUG=722371
Review-Url: https://codereview.chromium.org/2898683002
Cr-Commit-Position: refs/heads/master@{#473862}
Committed: https://chromium.googlesource.com/chromium/src/+/193a454cb1ee78b5a6cecc6c6e6fb63c1598c40c
Patch Set 1 : Merged #
Total comments: 4
Patch Set 2 : Changed default to UNSET #Messages
Total messages: 21 (6 generated)
Patchset #1 (id:1) has been deleted
igorcov@chromium.org changed reviewers: + atwilson@chromium.org, bartfab@chromium.org
https://codereview.chromium.org/2898683002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/2898683002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:787: MIGRATION_STRATEGY_UNSPECIFIED = 0; I'm not sure that's what we want. What behavior do you expect if MIGRATION_STRATEGY_UNSPECIFIED is provided? Is the implication that DISALLOW_ARC should not be the default for unspecified values?
On 2017/05/22 12:30:57, Andrew T Wilson (Slow) wrote: > https://codereview.chromium.org/2898683002/diff/20001/chrome/browser/chromeos... > File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right): > > https://codereview.chromium.org/2898683002/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:787: > MIGRATION_STRATEGY_UNSPECIFIED = 0; > I'm not sure that's what we want. What behavior do you expect if > MIGRATION_STRATEGY_UNSPECIFIED is provided? Is the implication that DISALLOW_ARC > should not be the default for unspecified values? Just paste-ing Bartosz comment from https://critique.corp.google.com/#review/156424271/depot/google3/ccc/hosted/p...: "The best practice sounds reasonable to me but we never followed it for policies. We do not have "0 = unspecified" in any of our device or user policy enums. They always start with 0 being the first actual value, typically the safe default. From the client's POV, DISALLOW_ARC is the safe default and it is OK to send this for any devices you are not sure about. However, I foresee a future where we may want to distinguish between devices where the admin explicitly chose DISALLOW_ARC and those where no explicit choice was made. The client does not care, but the server will. Since we share the same proto, there probably is no good way for you to distinguish these two cases other than an UNSET value." The way I see it, we could put for zero something like DEFAULT_DISALLOW_ARC but it might be confusing considering the default functionality for consumer owned devices is to migrate.
Isn't that what has_migration_strategy() is for - detecting "I didn't actually set anything here"? On 22 May 2017 at 14:55, <igorcov@chromium.org> wrote: > On 2017/05/22 12:30:57, Andrew T Wilson (Slow) wrote: > > > https://codereview.chromium.org/2898683002/diff/20001/ > chrome/browser/chromeos/policy/proto/chrome_device_policy.proto > > File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto > (right): > > > > > https://codereview.chromium.org/2898683002/diff/20001/ > chrome/browser/chromeos/policy/proto/chrome_device_policy.proto#newcode787 > > chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:787: > > MIGRATION_STRATEGY_UNSPECIFIED = 0; > > I'm not sure that's what we want. What behavior do you expect if > > MIGRATION_STRATEGY_UNSPECIFIED is provided? Is the implication that > DISALLOW_ARC > > should not be the default for unspecified values? > > Just paste-ing Bartosz comment from > https://critique.corp.google.com/#review/156424271/depot/ > google3/ccc/hosted/policies/services/chrome/chrome_device_ > settings.proto&v=s1: > "The best practice sounds reasonable to me but we never followed it for > policies. We do not have "0 = unspecified" in any of our device or user > policy > enums. They always start with 0 being the first actual value, typically > the safe > default. > > From the client's POV, DISALLOW_ARC is the safe default and it is OK to > send > this for any devices you are not sure about. However, I foresee a future > where > we may want to distinguish between devices where the admin explicitly chose > DISALLOW_ARC and those where no explicit choice was made. The client does > not > care, but the server will. Since we share the same proto, there probably > is no > good way for you to distinguish these two cases other than an UNSET value." > > The way I see it, we could put for zero something like > DEFAULT_DISALLOW_ARC but > it might be confusing considering the default functionality for consumer > owned > devices is to migrate. > > https://codereview.chromium.org/2898683002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/05/22 13:00:08, Andrew T Wilson (Slow) wrote: > Isn't that what has_migration_strategy() is for - detecting "I didn't > actually set anything here"? Not in proto3 [1]: "Removal of field presence logic for primitive value fields, removal of required fields, and removal of default values." You can no longer tell whether a field is unset or set to its default value. It will always be "there" and will always have a value when you read it. - Bartosz [1] https://github.com/google/protobuf/releases?after=v3.0.0-javalite
https://codereview.chromium.org/2898683002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/2898683002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:787: MIGRATION_STRATEGY_UNSPECIFIED = 0; On 2017/05/22 12:30:56, Andrew T Wilson (Slow) wrote: > I'm not sure that's what we want. What behavior do you expect if > MIGRATION_STRATEGY_UNSPECIFIED is provided? Is the implication that DISALLOW_ARC > should not be the default for unspecified values? We (the client) do not want or need an UNSET value. What you are seeing here are implementation details leaking from server to client. On the server, they want to keep track of domains/OUs that explicitly set the policy vs. ones that never set it. They need a sepecial UNSET value for that. Since server-side storage uses the same proto as the client-server protocol (a long-known WTF in the design), we have to add the value here if we want to help out the server guys. Igor, let's call this simply UNSET and note in the comment that this triggers version- and board-specific, undefined behavior and should thus never be sent by the server.
https://codereview.chromium.org/2898683002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/2898683002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:787: MIGRATION_STRATEGY_UNSPECIFIED = 0; On 2017/05/22 13:23:47, bartfab (slow) wrote: > On 2017/05/22 12:30:56, Andrew T Wilson (Slow) wrote: > > I'm not sure that's what we want. What behavior do you expect if > > MIGRATION_STRATEGY_UNSPECIFIED is provided? Is the implication that > DISALLOW_ARC > > should not be the default for unspecified values? > > We (the client) do not want or need an UNSET value. What you are seeing here are > implementation details leaking from server to client. On the server, they want > to keep track of domains/OUs that explicitly set the policy vs. ones that never > set it. They need a sepecial UNSET value for that. Since server-side storage > uses the same proto as the client-server protocol (a long-known WTF in the > design), we have to add the value here if we want to help out the server guys. > > Igor, let's call this simply UNSET and note in the comment that this triggers > version- and board-specific, undefined behavior and should thus never be sent by > the server. Changed to UNSET. Thanks for explanations.
tnagel@chromium.org changed reviewers: + tnagel@chromium.org
https://codereview.chromium.org/2898683002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/2898683002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:787: MIGRATION_STRATEGY_UNSPECIFIED = 0; > We (the client) do not want or need an UNSET value. What you are seeing here are > implementation details leaking from server to client. On the server, they want > to keep track of domains/OUs that explicitly set the policy vs. ones that never > set it. They need a sepecial UNSET value for that. Since server-side storage > uses the same proto as the client-server protocol (a long-known WTF in the > design), we have to add the value here if we want to help out the server guys. Imho this is just enforcing proto best practices (which I think is fair, BTW). I don't think there's a specific need for this on the server side.
Can anybody actually lgtm this? I think it needs to land before being synchronized in google3.
On 2017/05/23 10:04:39, igorcov wrote: > Can anybody actually lgtm this? I think it needs to land before being > synchronized in google3. Following Bin's suggestion, I've just committed the previous version to google3. I'll commit an update once your CL has landed, but there's no rush.
lgtm
The CQ bit was checked by igorcov@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/23 10:07:33, Thiemo Nagel wrote: > On 2017/05/23 10:04:39, igorcov wrote: > > Can anybody actually lgtm this? I think it needs to land before being > > synchronized in google3. > > Following Bin's suggestion, I've just committed the previous version to google3. > I'll commit an update once your CL has landed, but there's no rush. The numerical values in enum change from one version to other. Don't think we can have any implementation on top until we don't have a final land. Thanks for review.
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495537190119280,
"parent_rev": "bfce56092d9d5dcecf353b18113db68835d1e5bd", "commit_rev":
"193a454cb1ee78b5a6cecc6c6e6fb63c1598c40c"}
Message was sent while issue was closed.
Description was changed from ========== Default unspecified value added to enum in proto Updated the enum in proto to be according to the rules from https://engdoc.corp.google.com/eng/doc/devguide/proto/index.md?cl=head BUG=722371 ========== to ========== Default unspecified value added to enum in proto Updated the enum in proto to be according to the rules from https://engdoc.corp.google.com/eng/doc/devguide/proto/index.md?cl=head BUG=722371 Review-Url: https://codereview.chromium.org/2898683002 Cr-Commit-Position: refs/heads/master@{#473862} Committed: https://chromium.googlesource.com/chromium/src/+/193a454cb1ee78b5a6cecc6c6e6f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/193a454cb1ee78b5a6cecc6c6e6f... |
