Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AIP-203] Clarify behavior when updating immutable fields. #516

Merged
merged 4 commits into from Jun 5, 2020

Conversation

lukesneeringer
Copy link
Contributor

This came up in an internal discussion with @erictune.
This does not represent a change in guidance, only a clarification
to what we already tell people.

This came up in an internal discussion with @erictune.
This does not represent a change in guidance, only a clarification
to what we already tell people.
@lukesneeringer lukesneeringer requested a review from a team May 27, 2020 16:17
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 27, 2020
aip/0203.md Outdated
Comment on lines 122 to 125
When a service receives an immutable field in an update request (or similar),
even if included in the update mask, the service **should** ignore the field
and leave the existing value alone, but **should not** error.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this saying that it is never an error to try to change an immutable field using an update, but that it just doesn't do anything? If so, that seems confusing to users to debug what is going on.

Or is it saying something more subtle?

I would have expected it to say something like:

If an update request asks to change to an immutable field, then it should return an error. A change is defined as the value in the update differs from the value in the current state of the resources on the server, and that the field was included in the update mask.
If an update request mentions an immutable field but does not ask to change it, this should be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an update request asks to change to an immutable field, then it should return an error. A change is defined as the value in the update differs from the value in the current state of the resources on the server, and that the field was included in the update mask.
If an update request mentions an immutable field but does not ask to change it, this should be ignored.

I just realized I was thinking of output only, and not immutable, when I wrote that. For output only, we ignore all the time (because of things like update_time), but I agree that for immutable, we should error if a change is requested. Fixing.

Copy link

@erictune erictune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Potential use cases for immutable fields (this is not an exhaustive list) are:

- Names or IDs which are set on creation and then used as a primary key.

**Note:** Fields which are "conditionally immutable" **must not** be given the

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "conditionally immutable" defined elsewhere? Does that mean things like "GCE instance size", which is immutable while the instance is powered on, but mutable if the instance is powered off?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link

@nat-henderson nat-henderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, this will be helpful for us.

lukesneeringer pushed a commit that referenced this pull request Jun 4, 2020
As usual, I will update this before merging with the PR numbers for
public comment. This PR is also contingent on #518 and #516 being
merged.

**Note:** I somehow goofed in the May newsletter and wrote
"Friday, May 31" everywhere. The last Friday of May was May 29.
I actually did the merge on May 29 and I am engaging in willful
revisionist history.
@lukesneeringer lukesneeringer mentioned this pull request Jun 4, 2020
@lukesneeringer lukesneeringer merged commit 5a771e6 into master Jun 5, 2020
@lukesneeringer lukesneeringer deleted the immutable-behavior branch June 5, 2020 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants