feat(146): add oas-specific guidance on mutually exclusive fields#274
feat(146): add oas-specific guidance on mutually exclusive fields#274GeoffreyXue wants to merge 1 commit intoaep-dev:mainfrom
Conversation
toumorokoshi
left a comment
toumorokoshi
left a comment
There was a problem hiding this comment.
Thank you, nice first round!
The content on union types looks good as is - but we should leave the purpose of the overall AEP alone (generic fields, not shrink that down to union types specifically).
| @@ -1,28 +1,29 @@ | |||
| # Generic fields | |||
| # Mutually exclusive fields | |||
There was a problem hiding this comment.
note: generic fields covers much more than mutually exclusive fields. It refers to structs and the Any type at minimum, and we may want to cover more things like sets.
I'd suggest creating a sub-section for mutually exclusive fields, or perhaps splitting it into it's own AEP (150? since aip.dev/149 exists).
| However, occasionally it is appropriate to have a generic or polymorphic field | ||
| of some kind that can conform to multiple schemata, or even be entirely | ||
| free-form. | ||
| However, occasionally it is appropriate to have a mutually exclusive or |
There was a problem hiding this comment.
I won't leave a comment everywhere, but I think it'll be important to revert the nomenclature in this PR to continue to refer to the larger design area of generic fields.
| the field needs to be; in general, APIs **should** attempt to introduce the | ||
| "least generic" approach that is able to satisfy the use case. | ||
| While mutually exclusive fields are generally rare, a API **may** introduce | ||
| mutually exclusive field where necessary. There are several approaches to this |
There was a problem hiding this comment.
| mutually exclusive field where necessary. There are several approaches to this | |
| mutually exclusive fields where necessary. There are several approaches, |
| For example, an API **should not** use a completely mutually exclusive field | ||
| (such as `google.protobuf.Struct` in protobuf APIs) when the value of the field | ||
| must correspond to one of a known number of schemas. Instead, the API | ||
| **should** use a [`oneof`](./oneof) to represent the known schemas. |
There was a problem hiding this comment.
I don't think most users would use a protobuf struct to define a mutually exclusive set of fields - perhaps it's better to start with OneOf?
We can always call out bad patterns below, in a new section like Rationale: https://aep.dev/8/#document-structure
| `oneof` in protobuf APIs. Due to the schema's complexity, validation for this | ||
| schema **should** be handled server-side, and it is fine to not document the | ||
| schema explicitly. |
There was a problem hiding this comment.
| `oneof` in protobuf APIs. Due to the schema's complexity, validation for this | |
| schema **should** be handled server-side, and it is fine to not document the | |
| schema explicitly. | |
| `oneof` in protobuf APIs. Due to the schema's complexity, validation for this | |
| schema **should** be handled server-side, and OpenAPI schemas do not need to document the | |
| schema explicitly. |
"it is fine" feels a bit colloquial here - I'd say something along the lines of "do not need" or "are not required".
| {% tab proto %} | ||
|
|
||
| #### Oneof | ||
| #### Type Union |
There was a problem hiding this comment.
| #### Type Union | |
| #### Union Type |
I think most descriptions of types describe this as "Union Types":
https://docs.python.org/3/library/typing.html#typing.Union
https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html#use-union-types
There was a problem hiding this comment.
Also I don't think protobuf oneofs are union types really - they are a set of fields that are mutually exclusive.
A union type is "field foo can be X or Y" - in this case it's "field foo of type X, or bar of type Y can be set". So really I'd say "mutually exclusive fields" is a better term.
2 participants
🍱 Types of changes
What types of changes does your code introduce to AEP? Put an
xin the boxesthat apply
📋 Your checklist for this pull request
Please review the AEP Style and Guidance for
contributing to this repository.
General
references AEPs
correctly.
(usually
prettier -w .)💝 Thank you!