gnostic icon indicating copy to clipboard operation
gnostic copied to clipboard

OneOf support in OpenAPIv3

Open fenos opened this issue 3 years ago • 26 comments

Hello there! Thanks for the magical tool! It took a while to get my head around but it is great!

I was testing out the capability of the proto ->OpenAPIV3 and it works nicely. However I noticed that it doesn't use the latest oneOf keywords to annotate a oneof type in the proto message.

For example given this proto file:

syntax = "proto3";

package twirp.example.haberdasher;

import "google/api/annotations.proto";
option go_package="twirp/example/haberdasher";


// Haberdasher service makes hats for clients.
service Haberdasher {
  // MakeHat produces a hat of mysterious, randomly-selected color!
  rpc MakeHat(Size) returns (Hat) {
    option(google.api.http) = {
      post: "/v1/create"
      body: "text"
    };
  };
}

// Size of a Hat, in inches.
message Size {
  int32 inches = 1; // must be > 0
}

// A Hat is a piece of headwear made by a Haberdasher.
message Hat {
  int32 inches = 1;
  string color = 2; // anything but "invisible"
  string name = 3; // i.e. "bowler"
  oneof type {
    Type1 type1 = 4;
    Type1 type2 = 5;
  }
}

message Type1 {
  string id = 1;
}

message Type2 {
  string id = 1;
}

I get this:

# Generated with protoc-gen-openapi
# https://github.com/googleapis/gnostic/tree/master/apps/protoc-gen-openapi

openapi: 3.0.3
info:
    title: Haberdasher
    description: Haberdasher service makes hats for clients.
    version: 0.0.1
paths:
    /v1/create:
        post:
            summary: MakeHat produces a hat of mysterious, randomly-selected color!
            operationId: Haberdasher_MakeHat
            parameters:
                - name: inches
                  in: query
                  schema:
                    type: string
            requestBody:
                content:
                    application/json: {}
                required: true
            responses:
                "200":
                    description: OK
                    content:
                        application/json:
                            schema:
                                $ref: '#/components/schemas/Hat'
components:
    schemas:
        Hat:
            properties:
                inches:
                    type: integer
                    format: int32
                color:
                    type: string
                name:
                    type: string
                type1:
                    $ref: '#/components/schemas/Type1'
                type2:
                    $ref: '#/components/schemas/Type2'
            description: A Hat is a piece of headwear made by a Haberdasher.
        Type1:
            properties:
                id:
                    type: string
        Type2:
            properties:
                id:
                    type: string

note the type1 and type2 fields are flat in the schema instead of being something like:

type:
     oneOf:
                - $ref: '#/components/schemas/Type1'
                - $ref: '#/components/schemas/Type2'

Maybe I'm missing something?

fenos avatar Jun 14 '21 21:06 fenos

I have the same issue going in the other direction:

  • Define a oneOf in an Openapi3 YAML file
  • Run gnostic --grpc-out=xxx openapi.yaml
  • The resulting .proto file flattens everything that was originally defined in the oneOf

Expected:

  • oneOf type definition in the .proto file

bctsui avatar Aug 31 '21 23:08 bctsui

I have the same issue going in the other direction:

  • Define a oneOf in an Openapi3 YAML file
  • Run gnostic --grpc-out=xxx openapi.yaml
  • The resulting .proto file flattens everything that was originally defined in the oneOf

Expected:

  • oneOf type definition in the .proto file

For the generated proto file it is actually expected behavior. The generation is similar to OpenAPI generator. This one flattens everything as well.

LorenzHW avatar Sep 02 '21 17:09 LorenzHW

I have been trying to understand the differences and similarities between protobuf oneof/anyof and JSON Schema oneOf/anyOf and wanted to share my findings, in case it can help anybody else reading this.

TL;DR: It looks similar, but is not the same in almost any aspect. JSON Schema can describe multiple valid types for a property, while protobuf values can ONLY be 1 type. JSON Schema describes the type for a value to be one of multiple. Protocol Buffers describes a set of fields, of which only one of them may be set.

To make it simple, I will try to describe my findings, using an event that can be either a string or a number, as an example. The differences actually becomes rather self-evident, already in trying to describe the event in the 2 "languages".

JSON Schema. This says: event's value should match one of the schemas (type number or string). Or: The value of event should be either a number or a string.

{
  "event": {
    "oneOf": [
      { "type": "number" },
      { "type": "string" }      
    ]
  }
}

Protocol Buffers. This says: event have the fields event_number and event_string of which only one may be set. It is NOT possible to have a field be one of multiple types.

message Event {
  oneof event {
    float event_number = 1;
    string event_string = 2;
  }
}

So JSON Schema describes the value of a property, while protobuf describes a field with sub fields, of which only one may be set.

This is reflected in how Envoy Proxy + the gRPC transcoder interprets data for the above protobuf message. Transcoded to JSON, it will look something like this: EDIT: Fixed this - it was not correct before.

{
  "event_number": 213.4
}

While a correct JSON object for the above JSON Schema would look something like this:

{
  "event": 213.4
}

JSON Schema is a description of constraints, that is completely implementation ignorant. A correct implementation of a JSON Schema validator is expected to check a value against all oneOfs to ensure that the value only matches one of the descriptions. If the value is valid for more than one of the oneOf schemas, it should be handled as an invalid value.

Protocol Buffers is used to write implementations that share the knowledge of the shape of data. The actual value being passed around, only makes sense, when also knowing the shape. In other words: there is no guess-work or double checking involved. A value can be set for event_number, then it is a number. A value can be set for event_string then it is a string. If both holds a value, then the entire message is invalid.

Hope this helps :)

morphar avatar Jan 03 '22 05:01 morphar

I think the current schema output is still a little too loose though. For instance, there is nothing in the schema that makes the type1 and type2 (or in your example @morphar, event_number and event_string) properties mutually exclusive, and a client could in theory specify both which is not correct. I think protoc-gen-openapi should be updated to properly support mutually exclusive properties in this case...doing something like: https://stackoverflow.com/a/49199240/2233608

jeffsawatzky avatar Apr 04 '22 16:04 jeffsawatzky

I would love for this to be more like OpenAPI / JSON Schema oneOf, but:

TL;DR: It looks similar, but is not the same in almost any aspect.

I think the explanation I gave, pretty thoroughly explains what the problem is.

Though the following may be a bit confusing as there was an error (fixed in the above comment as well):

message Event {
  oneof event {
    float event_number = 1;
    string event_string = 2;
  }
}

This is reflected in how Envoy Proxy + the gRPC transcoder interprets data for the above protobuf message. Transcoded to JSON, it will look something like this: EDIT: Fixed this - it was not correct before.

{
  "event_number": 213.4
}

So I'll try to make it more clear:

message Event {
  string id = 1;
  string name = 2;
  oneof call_it_whatever_you_want {
    float event_number = 3;
    string event_string = 4;
  }
}

Is transcoded by Envoy to:

{
  "id": "123abc",
  "name": "some name",
  "event_number": 213.4
}

or

{
  "id": "123abc",
  "name": "some name",
  "event_string": "some string"
}

It is correct, that having more than one of the proto oneofs is an error, but how to express that in JSON Schema?

JSON Schema oneOf describes constraints of a field, while proto oneof describes fields, where only one of them may be set.

morphar avatar Apr 04 '22 18:04 morphar

@morphar the link I provided gives an example of how to define mutually exclusive properties. Here is another example:

https://stackoverflow.com/questions/24023536/how-do-i-require-one-field-or-another-or-one-of-two-others-but-not-all-of-them

The schema would look something like this:

{
  "type": "object",
  "properties": {
    "id": {
      "type": "string"
    },
    "name": {
      "type": "string"
    },
    "event_number": {
      "type": "number"
    },
    "event_string": {
      "type": "string"
    }
  },
  "additionalProperties": false,
  "oneOf": [
    {
      "required": [
        "event_number"
      ]
    },
    {
      "required": [
        "event_string"
      ]
    }
  ]
}

That would allow (and require) only one of event_number or event_string to be set. If you try to set both properties it won't validate against the schema. You can try that here: https://www.jsonschemavalidator.net/

This will validate:

{
  "id": "id",
  "name": "name",
  "event_string": "str"
}

As will this:

{
  "id": "id",
  "name": "name",
  "event_number": 1
}

But not if you set both like this:

{
  "id": "id",
  "name": "name",
  "event_string": "str",
  "event_number": 1
}

As it will fail the "oneOf" which only allows ONE of the options to be true. This currently does not allow NEITHER of them properties to be set. Perhaps with some extra JSON schema modifications it might be possible?

jeffsawatzky avatar Apr 04 '22 19:04 jeffsawatzky

This schema seems to do what I think we'd want:

{
  "type": "object",
  "properties": {
    "id": {
      "type": "string"
    },
    "name": {
      "type": "string"
    },
    "event_number": {
      "type": "number"
    },
    "event_string": {
      "type": "string"
    }
  },
  "anyOf": [
    {
      "oneOf": [
        {
          "required": ["event_number"]
        },
        {
          "required": ["event_string"]
        }
      ]
    },
    {
      "not": { "required": ["event_number", "event_string"] }
    }
  ],
  "additionalProperties": false
}

As it looks like it allows either event_string XOR event_number or neither.

jeffsawatzky avatar Apr 04 '22 20:04 jeffsawatzky

YES! You're absolutely right @jeffsawatzky! 👍 Sorry I missed your point in the link you provided. How have I missed this...

That's awesome, then it should actually be possible to get to a point where everything maps perfectly, with all the other work you have done so far.

As far as I can see, the last schema should work without the not. So instead of:

  "oneOf": [
    {
      "oneOf": [
        {
          "required": ["event_number"]
        },
        {
          "required": ["event_string"]
        }
      ]
    },
    {
      "not": { "required": ["event_number", "event_string"] }
    }
  ]

It should be enough with:

      "oneOf": [
        {
          "required": ["event_number"]
        },
        {
          "required": ["event_string"]
        }
      ]

It seemed to work correctly, when I tested this:

{
  "type": "object",
  "properties": {
    "id": {
      "type": "string"
    },
    "name": {
      "type": "string"
    },
    "event_number": {
      "type": "number"
    },
    "event_string": {
      "type": "string"
    }
  },
  "additionalProperties": false,
  "oneOf": [
    {
      "required": [
        "event_number"
      ]
    },
    {
      "required": [
        "event_string"
      ]
    }
  ]
}

With Hyperjump

morphar avatar Apr 04 '22 20:04 morphar

@morphar the "not" was there to allow NEITHER of them to be specified...as I couldn't find any documentation that states at least one of the oneofs needs to be set. If there is something that states at least one of the oneofs need to be present then we don't need the "not"

jeffsawatzky avatar Apr 04 '22 20:04 jeffsawatzky

Aaahh... I can easily test that against Envoy - give me a couple of minutes, then I'll know.

morphar avatar Apr 04 '22 20:04 morphar

@jeffsawatzky It seems like you're right again 👍

morphar avatar Apr 04 '22 20:04 morphar

This seems to fail validation with the double oneOfs though:

{
"name": "name",
"event_number": 123
}

morphar avatar Apr 04 '22 20:04 morphar

@morphar oops, sorry. That first "oneOf" should have been an "anyOf". I copied and pasted an older schema. I updated the comment above.

jeffsawatzky avatar Apr 04 '22 20:04 jeffsawatzky

This seems to do the trick:

{
    "type": "object",
    "properties": {
        "id": {
            "type": "string"
        },
        "name": {
            "type": "string"
        },
        "event_number": {
            "type": "number"
        },
        "event_string": {
            "type": "string"
        }
    },
    "oneOf": [
        {
            "required": [
                "event_number"
            ]
        },
        {
            "required": [
                "event_string"
            ]
        },
        {
            "not": {
                "oneOf": [
                    {
                        "required": [
                            "event_number"
                        ]
                    },
                    {
                        "required": [
                            "event_string"
                        ]
                    }
                ]
            }
        }
    ],
    "additionalProperties": false
}

morphar avatar Apr 04 '22 20:04 morphar

It seems that the deepest oneOf could be an anyOf as well... But I don't know if that's more or less confusing :)

morphar avatar Apr 04 '22 20:04 morphar

@jeffsawatzky sorry, just saw your last comment now :)

morphar avatar Apr 04 '22 20:04 morphar

Great! So any of the 3 combinations seems to work 👍

morphar avatar Apr 04 '22 20:04 morphar

I think yours makes most sense 👍

morphar avatar Apr 04 '22 20:04 morphar

This may also need to change when there is more than one oneof field

jeffsawatzky avatar Apr 04 '22 21:04 jeffsawatzky

Oh yeah, you're right. That would not work.

morphar avatar Apr 04 '22 21:04 morphar

Then it would need something like:

{
    "type": "object",
    "properties": {
        "id": {
            "type": "string"
        },
        "name": {
            "type": "string"
        },
        "event_number": {
            "type": "number"
        },
        "event_string": {
            "type": "string"
        }
    },
    "allOf": [
        {
            "anyOf": [
                {
                    "oneOf": [
                        {
                            "required": [
                                "id"
                            ]
                        },
                        {
                            "required": [
                                "name"
                            ]
                        }
                    ]
                },
                {
                    "not": {
                        "required": [
                            "id",
                            "name"
                        ]
                    }
                }
            ]
        },
        {
            "anyOf": [
                {
                    "oneOf": [
                        {
                            "required": [
                                "event_number"
                            ]
                        },
                        {
                            "required": [
                                "event_string"
                            ]
                        }
                    ]
                },
                {
                    "not": {
                        "required": [
                            "event_number",
                            "event_string"
                        ]
                    }
                }
            ]
        }
    ],
    "additionalProperties": false
}

It's really becoming extremely elaborate....

morphar avatar Apr 04 '22 21:04 morphar

Yeah, but I'm not sure how often people have more than 1 oneof in a single message. We should support it (if we choose to go this route) but in these rare cases if the schema is elaborate is it that big a deal?

Also, does gnostic favour simplicity over correctness? Or correctness over simplicity?

Also, if the generated schema did include these restrictions would it help clear up confusion for other developers when they try to use oneof?

These are all just guesses and hunches though...

jeffsawatzky avatar Apr 06 '22 12:04 jeffsawatzky

Here is an example of another protoc-gen plugin that makes it optional: https://github.com/chrusty/protoc-gen-jsonschema/issues/56

jeffsawatzky avatar Apr 06 '22 14:04 jeffsawatzky

@jeffsawatzky it's definitely my understanding, that gnostic favours correctness over simplicity. I don't think that it's a problem with elaborate definitions, if that's what it takes to be correct, it was more of a thought, than "stating a problem" :)

It seems that the elaborate version, captures the proto oneof restrictions correctly.

morphar avatar Apr 06 '22 21:04 morphar

I can't find any implementation for generating oneOf in OAS3 from proto files.

Having this

message Foo {
  oneof a {
    string b = 1;
    int32 c = 2;
    bool d = 3;
  };
}

The generated output is flattened as written above.

Is there a way to enforce mutual exclusivity defined by proto in the generated openapi spec?

nadilas avatar Sep 23 '23 07:09 nadilas

Any update on this? We are bumping into the same issue

Dimitris-Christodoulou avatar Feb 20 '24 10:02 Dimitris-Christodoulou