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

Create AEP-204: Oneofs. #204

wants to merge 2 commits into from

Conversation

rofrankel
Copy link
Collaborator

Creates an AEP that defines generic, IDL-agnostic oneof behavior.

This uses the common components defined in aep-dev/aep-components#16.

🍱 Types of changes

What types of changes does your code introduce to AEP? Put an x in the boxes
that apply

  • Enhancement
  • New proposal
  • Migrated from google.aip.dev
  • Chore / Quick Fix

📋 Your checklist for this pull request

Please review the AEP Style and Guidance for
contributing to this repository.

General

Additional checklist for a new AEP

  • A new AEP should be no more than two pages if printed out.
  • Ensure that the PR is editable by maintainers.
  • Ensure that File structure
    guidelines are met.
  • Ensure that
    Document structure
    guidelines are met.

💝 Thank you!

This uses the common components defined in aep-dev/aep-components#16.
@rofrankel rofrankel requested a review from a team as a code owner July 29, 2024 18:10
@rofrankel rofrankel linked an issue Jul 29, 2024 that may be closed by this pull request
}
```

## 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!

@rofrankel rofrankel requested a review from rambleraptor August 8, 2024 21:03
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.


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?

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?

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Thanks for starting this! I don't really like having our "oneof" match the proto concept - it's not descriptive enough, IMO.

Added a couple in-depth threads we can discuss.

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.

Create oneof AEP
3 participants