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

Create AEP-204: Oneofs. #204

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
67 changes: 67 additions & 0 deletions aep/general/0204/aep.md.j2
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
# Oneofs

Multiple IDLs have a concept called "oneof". For example, in protobuf, a
`oneof` specifies that a set of fields within a message are mutually exclusive
(i.e. at most one field can be set). In OAS 3, `oneOf` specifies that the value
of a _single_ field must match _exactly one_ of a set of schemas.

In order to smooth over such discrepancies, this AEP defines a generic `oneof`
concept; when other AEPs refer to `oneof` in a generic context (i.e. outside
IDL-specific sections or tabs), they are using this definition:

- A `oneof` is a set of fields within a message that are mutually exclusive.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think "oneof" is a great name for mutually exclusive fields. I'd argue we should call this AEP "mutually exclusive fields" and use the term "x-mutually-exclusive-fields" for OAS.

mutually-exclusive also implies at most one of. "one of" does not.

_At most one_ field can be set at a time.
- If the `oneof` is required, that means that _exactly one_ field must be set.

## protobuf

Protobuf APIs **must** use the built-in `oneof` feature. In order to specify
that a `oneof` is required, they **should** use the `aep.api.OneofBehavior`
extension:

```proto
import "aep/api/oneof_behavior.proto";

message Book {
// ...

// The author of the book.
oneof author {
// The individual author of a book.
string individual_author = 1 [(google.api.resource_reference) = {
type: "library.example.com/Author",
}];

// The organization that authored a book.
string organization_author = 2 [(google.api.resource_reference) = {
type: "library.example.com/Organization",
}];
} [(aep.api.oneof_behavior) = {required: true}];
}
```

## OAS
Copy link
Member

Choose a reason for hiding this comment

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

If possible, we should avoid expressing oneof behavior as a OAS extension. Extensions aren’t going to be understood by any other parts of the OAS ecosystem

(I’m sure this came up in group conversations, but still documenting here)

Copy link
Collaborator Author

@rofrankel rofrankel Aug 8, 2024

Choose a reason for hiding this comment

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

Yeah it did come up in conversation, and this was unfortunately the best solution we could think of. Protobuf and OAS have different approaches to oneof, and we think the AEPs need some unified oneof concept (independent of IDL support).

We also concluded that proto's approach is better, because JSON clients can figure out which case it is by looking at which field is set. There are ways to make that happen in JSON Schema (e.g. by adding a constant field to each possible schema with different values) but that's clunkier and less reliable/"free".

So this PR (and the companion PR aep-dev/aep-components#16) take the approach of explaining how to express AEP's oneof concept in both proto and OAS.

Open to suggestions for improvement though!


OAS 3's `oneOf` does not represent the same concept as a generic AEP `oneof`.
Therefore, while APIs **may** use OAS `oneOf` when otherwise applicable, in
order to specify an AEP `oneof`, they **should** use the [`x-aep-oneof`][]
Copy link
Member

Choose a reason for hiding this comment

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

we should probably add a link to the annotation. Where do we link to custom proto annotations?

extension:

```yaml
components:
schemas:
Book:
type: object
properties:
individual_author: { type: string }
organization_author: { type: string }
x-aep-oneof:
name: owner
Copy link
Member

Choose a reason for hiding this comment

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

why do oneofs need a name?

properties: [individual_author, organization_author]
required: true
```

<!-- prettier-ignore-start -->
[`aep.api.OneofBehavior`]: https://buf.build/aep/api/docs/main:aep.api#aep.api.OneofBehavior
[`x-aep-oneof`]: https://github.com/aep-dev/aep/tree/main/json_schema/oneof.yaml
<!-- prettier-ignore-end -->
7 changes: 7 additions & 0 deletions aep/general/0204/aep.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
id: 204
state: approved
slug: oneofs
created: 2024-07-29
placement:
category: design-patterns
Loading