aip-dev / google.aip.dev

API Improvement Proposals. https://aip.dev/
Other
1.07k stars 489 forks source link

Clarify usage of field_behavior for OneOfs #1156

Open toumorokoshi opened 1 year ago

toumorokoshi commented 1 year ago

Field behavior for OneOfs should not be required, as they are implicitly optional.

This should be clarified in the AIPs.

noahdietz commented 1 year ago

This should be straight forward to clarify in AIP-203, don't you think? A simple piece of guidance and linter rule.

I'm not sure if we should develop an alternate pattern to address the at least one of these use case though. One random idea is wrap the oneof in a message and make the message field REQUIRED e.g.

message ExportBookDestination {
  oneof destination {
    Library library = 1;

    Auction auction = 2;

    BookShop book_shop = 3;
  }
}

message ExportBookRequest {
  // all the normal fields...
  ExportBookDestination export_book_destination = n;
}

I also used the Import/Export example because this is a well defined pattern that leverages oneofs and would likely benefit from describing said oneof as REQUIRED.

noahdietz commented 1 year ago

Turns out OneofOptions is a thing so we should look at an annotation there rather than a new design pattern.

I think we still want to forbid the use of field_behavior = REQUIRED|OPTIONAL on oneof fields though so I will move forward with that change.

noahdietz commented 1 year ago

Move this to the backlog for now