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

fix(AIP-203): disallow field_behavior in oneof #1181

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

noahdietz
Copy link
Collaborator

Disallow the use of google.api.field_beahvior on fields in a oneof. Even the immutable, output_only, etc. would be redundant and/or confusing if applied to every field (consistently or otherwise). Instead we should explore defining a google.api.oneof_behavior annotation to mimic field_behavior but for the entire group of oneof fields. That is a separate issue though.

Fixes #1156

@noahdietz noahdietz requested a review from a team as a code owner July 21, 2023 17:29
@toumorokoshi
Copy link
Contributor

@noahdietz the ideas sounds good to me. The only concern I have is those who have already added field_behavior to OneOfs. There's not a lot of them, but since the linter did lint on that for a short period of time, we may randomize some folks.

@noahdietz
Copy link
Collaborator Author

After building a little tool to look into this, I think you are right that we shouldn't disallow this until we can do something like an LSC that migrates google.api.field_behavior on oneof fields into a single annotation like google.api.oneof_behavior.

I'm going to put this PR into draft mode and remove reviewers for the time being.

// Total number of fields with at least one field_behavior annotation and belongs to a oneof
count oneof field_behavior
Total: 1545

// Total number of fields with at least field_behavior=REQUIRED (can include others) and belongs to a oneof
count oneof field_behavior=REQUIRED
Total: 166

// Total number of fields with at least field_behavior=OPTIONAL (can include others) and belongs to a oneof
count oneof field_behavior=OPTIONAL
Total: 802

// Total number of fields with at least field_behavior=OUTPUT_ONLY (can include others) and belongs to a oneof
count oneof field_behavior=OUTPUT_ONLY
Total: 414

// Total number of fields with at least field_behavior=INPUT_ONLY (can include others) and belongs to a oneof
count oneof field_behavior=INPUT_ONLY
Total: 78

// Total number of fields with at least field_behavior=IMMUTABLE (can include others) and belongs to a oneof
count oneof field_behavior=IMMUTABLE
Total: 100

@noahdietz noahdietz marked this pull request as draft July 24, 2023 18:23
Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Possibly explain why? But generally, yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify usage of field_behavior for OneOfs
3 participants