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.

Sign up for GitHub

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

feat(AIP-203): clarify immutable optional, required fields #1120

Merged
merged 2 commits into from
Jun 1, 2023
Merged

feat(AIP-203): clarify immutable optional, required fields #1120

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29a67b3
feat(AIP-203): clarify immutable optional, required fields
toumorokoshi May 24, 2023
dec6035
address feedback
toumorokoshi May 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 24 additions & 22 deletions aip/general/0203.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,12 @@ field behavior, such as a field being required or immutable.
RecognitionAudio audio = 2 [(google.api.field_behavior) = REQUIRED];
```

- APIs **must** apply the `google.api.field_behavior` annotation on every field.
- APIs **must** apply the `google.api.field_behavior` annotation on every field
Copy link
Contributor

Choose a reason for hiding this comment

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

When did this become a requirement? It shouldn't be one. Leaving off a field_behavior annotation is equivalent to specifying OPTIONAL.

Copy link
Contributor Author

@toumorokoshi toumorokoshi May 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

See #1100, there's a separate thread you opened we could discuss this too.

on a message or sub-message used in a request.
toumorokoshi marked this conversation as resolved.
Show resolved Hide resolved
- The annotation **must** include any [google.api.FieldBehavior][] values that
accurately describe the behavior of the field.
- `FIELD_BEHAVIOR_UNSPECIFIED` **must not** be used.
- APIs **must** at minimum use one of `REQUIRED`, `OPTIONAL`, or `OUTPUT_ONLY`.

**Warning:** Although `field_behavior` does not impact proto-level behavior,
many clients (e.g. CLIs and SDKs) rely on them to generate code. Thoroughly
Expand Down Expand Up @@ -79,6 +81,14 @@ integers, or the unspecified value for enums) are indistinguishable from unset
values, and therefore setting a required field to a falsy value yields an
error. A corollary to this is that a required boolean must be set to `true`.

### Optional

The use of `OPTIONAL` indicates that a field is not required.

A field **may** be described as optional if it is a field on a request message
(a message that is an argument to an RPC, usually ending in `Request`), or a
field on a submessage.

### Output only

The use of `OUTPUT_ONLY` indicates that the field is provided in responses, but
Expand Down Expand Up @@ -122,13 +132,9 @@ before use.

### Immutable

The use of `IMMUTABLE` indicates that a field may be set once in a request to
create a resource but may not be changed thereafter.

Additionally, a field **should** only be described as immutable if it is a
field on a request message (a message that is an argument to an RPC, usually
ending in `Request`), or a field of a message included within a request
message.
The use of `IMMUTABLE` indicates that a field on a resource cannot be changed
after it's creation. This can apply to either fields that are input or outputs,
required or optional.

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 if
Expand All @@ -142,20 +148,6 @@ Potential use cases for immutable fields (this is not an exhaustive list) are:
**Note:** Fields which are "conditionally immutable" **must not** be given the
immutable annotation.

### Optional

The use of `OPTIONAL` indicates that a field is not required, nor covered by
Copy link
Contributor

Choose a reason for hiding this comment

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

The reason that "not covered by any of the above descriptions" was included is because when the annotation was being debated, API producers wanted to avoid the outcome of having to specify field_behavior on every field. We almost didn't provide OPTIONAL at all, but Docgen wanted it for cases where documentation needed to call out that the field was optional (e.g. if it was counter-intuitive).

Copy link
Contributor Author

@toumorokoshi toumorokoshi May 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Great, that matches my understanding as well. There's been a lot of discussion internally around this and the conclusion (with ATL review) was that we should require these, as the cost to API producers is worth the benefit to clients.

any of the above descriptions.

A field **may** be described as optional if none of the above vocabulary
applies, and it is a field on a request message (a message that is an argument
to an RPC, usually ending in `Request`), or a field on a submessage, but it is
not mandatory to describe optional fields in this way.

If you do choose to explicitly describe a field as optional, ensure that every
optional field on the message has this indicator. Within a single message,
either all optional fields should be indicated, or none of them should be.

### Unordered List

The use of `UNORDERED_LIST` on a repeated field of a resource indicates that
Expand All @@ -171,6 +163,15 @@ A resource with an unordered list **may** return the list in a stable order, or

## Rationale

### Required set of annotations
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this section should exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you clarify which section, and clarify why you think it shouldn't exist? If you're referring to the rationale section, there was several requests internally and unanimous support among the current API design team. Public discussion is at: #1058.


A field used in a request message must be either an input or an output.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section explains why some combination of annotations are required, and was trying to explain that a field is either and input or and output, and therefore a full description of the behavior should include at least one of required, optional, or output_only.


In the case of an output, the `OUTPUT_ONLY` annotation is sufficient.

In the case of an input, a field is either required or optional, and therefore
Copy link
Contributor

Choose a reason for hiding this comment

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

We specifically didn't want to mandate OPTIONAL. (There was debate on whether to even provide it.)

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, and after a decent amount of discussion it was agreed that field_behavior should be required: #1100. See the rationale section for more details, but this choice has lead to a lot of incorrectly documented fields since developers did not think through the field behavior choice. field behavior is being used as forcing function.

should have at least the `REQUIRED` or `OPTIONAL` annotation, respectively.

### Requiring field behavior

By including the field behavior annotation for each field, the overall behavior
Expand Down Expand Up @@ -203,6 +204,7 @@ surpass the costs to clients and API users of not doing so.

## Changelog

- **2023-05-24**: Clarify that `IMMUTABLE` does not imply input nor required.
- **2023-05-10**: Added guidance to require the annotation.
- **2020-12-15**: Added guidance for `UNORDERED_LIST`.
- **2020-05-27**: Clarify behavior when receiving an immutable field in an
Expand Down