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

Define common components for oneof parity across protobuf and OAS. #16

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
21 changes: 21 additions & 0 deletions json_schema/oneof.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
$schema: https://json-schema.org/draft/2020-12/schema
$id: https://aep.dev/component.oneof.json
title: x-aep-oneof
description: |
Represents a mutual exclusion constraint over multiple properties.

It is an error for any properties specified by a oneof to be required.
type: object
required: [properties]
additionalProperties: false
properties:
properties:
description: |
A set of properties that are mutually exclusive.
type: array
items:
type: string
required:
description: |
If set, exactly one property from the `properties` array must be present on the object.
type: string
26 changes: 26 additions & 0 deletions proto/aep-api/aep/api/oneof_behavior.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
syntax = "proto3";

package aep.api;

option cc_enable_arenas = true;
option go_package = "aep.dev/api";
option java_multiple_files = true;
option java_outer_classname = "OneOfBehaviorProto";
option java_package = "dev.aep.api";
option objc_class_prefix = "AEP";

import "google/protobuf/descriptor.proto";

extend google.protobuf.OneofOptions {
// An option for specifying API-specific behavior of a `oneof`.
OneofBehavior oneof_behavior = 10521;
}

// Used to specify the behavior of a `oneof`.
message OneofBehavior {
// If set, one field within the `oneof` must be set.
//
// When unset, the `oneof` is implicitly optional (the default behavior in
// protobuf).
bool required = 1;
}
65 changes: 65 additions & 0 deletions schemas/oneof.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# Oneof

As used by the [AEPs](http://aep.dev), a `oneof` represents a set of mutually
exclusive fields.

## Schema

Rather than a generic schema for `oneof`, we provide common components
supplementing the language features of specific IDLs. Using these enables
uniform behavior (and JSON serialization) across IDLs.

### protobuf

Protobuf's built-in `oneof` supports mutual exclusion of fields. However, it
does not have a way for an API indicate that at least one of the fields must be
set. For this purpose, a protobuf `oneof` may be annotated with
[`aep.api.OneofBehavior`](../proto/aep-api/aep/api/oneof_behavior.proto):

```proto
message Document {
rofrankel marked this conversation as resolved.
Show resolved Hide resolved
// ...

// The owner of the document.
oneof owner {
// The user who owns the document.
string user = 1 [(google.api.resource_reference) = {
type: "apis.example.com/User",
}];

// The group that owns the document.
string group = 2 [(google.api.resource_reference) = {
type: "apis.example.com/Group",
}];
} [(aep.api.OneofBehavior) = {required: true}];
}
```

### OAS

OAS 3 has a concept called `oneOf`, but it has different semantics than the
`oneof` defined by this document; the OAS concept says that a single field must
match exactly one of multiple possible schemas, rather than specifying mutual
exclusion over multiple properties.

In order to represent a `oneof` in OAS, in a way that produces JSON equivalent
to that of protobuf, we define an extension `x-aep-oneof`:

```yaml
Copy link
Member

Choose a reason for hiding this comment

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

the examples in aep.dev or json - shouldn't this also be json?

components:
schemas:
Document:
type: object
Copy link

Choose a reason for hiding this comment

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

In proto, oneof is not a nested object. Will this schema be wire compatible with proto?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. I'm not aware of a way to solve that, unfortunately. Wire compatibility would be great but I'm not sure it's feasible, and I'm also not sure banning oneofs (and other things) in the AEPs is worth the benefit of wire compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

yeah this is a tricky one. We'd end up having to write our own generator to map the concepts appropriately.

I don't think those who write native HTTP / JSON APIs will like the idea of nested fields as a way of defining oneofs - I sure don't. But it is possible to use the oneof annotation in such a way that proto -> json mappings will still be compliant (just with an additional nested layer).

I think it's best to have OpenAPI / HTTP+JSON have freedom to define it's own interface for now from the resource model (aepc) - then see the discrepancies and make a decision on how much compatibility we want from there.

properties:
user: { type: string }
group: { type: string }
x-aep-oneof:
Copy link

Choose a reason for hiding this comment

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

Will OAS introduce native support for oneof?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe in OAS 4, but not sure. Good question for @hudlow or @earth2marsh.

Copy link
Member

Choose a reason for hiding this comment

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

this should be a list, so there can be multiple oneofs. something like:

properties:
    x-aep-oneofs:
      - properties: [user, group]
        required: true

also do we need a name for this oneof?

name: owner
properties: [user, group]
required: true
```

The `x-aep-oneof` extension is used to indicate that at least one of the fields
in the `oneOf` array must be set.

**Note:** OAS extension not yet defined.
Loading