[johanbrandhorst]

Adds a minimal protoc-gen-openapiv3 generator that can generate OpenAPI v3.1 specifications from protobuf service definitions. No annotations or options are supported yet.

Builds on the work of @abhipranay in #6431.

Fixes #441

[johanbrandhorst]

CC @abhipranay @JackBogart @aidandj @commiterate. I think even if you try this and find it isn't suitable for you right now that I'd rather merge this and create issues for each outstanding request to work on as a follow-up. What do you think?

[aidandj]

I guess I don't totally see the difference between merging this, immediately following up with a bunch of changes to support additional features. When they are already supported in another PR. I get that the initial review may be simpler, but when a working set of options is already implemented it seems a bit redundant.

What about stripping the adapter pattern in the other PR, so it's just 3.1. I think that won't be that challenging to review, while still supporting all the options.

[johanbrandhorst]

Thanks for your comment, to me there is a pretty big difference, since this code base is small enough that I (and hopefully others) can review it and agree on it as a basis for the new plugin. I think we should carefully consider every new flag we want to add. What I am looking for from practitioners such as yourself right now is:

1. Does this generator cause any issues with your current set of files?
2. Do the new defaults make sense? E.g. we might add flags for omit_default_error, etc, but I'm hoping the defaults chosen here (which are different from the openapiv2 defaults) make sense in isolation.

I have to take a long view on the maintenance of this new tool and I am far more comfortable with this smaller step-by-step attempt at adding this new logic. If the end result is the same over the course of a few weeks of adding features that users are asking for, I think that gives us the best of both worlds.

[aidandj]

Oneof's don't seem to be working.

Given:

message PingResponse {
  optional string field_world = 1 [json_name = "fieldW_rld"];

  message Inner1 {
    optional string field_inner1 = 1;
  }

  message Inner2 {
    optional string field_inner2 = 1;
  }

  message Inner3 {
    optional string field_inner3 = 1;
  }

  oneof inner_message {
    Inner1 inner1 = 4;
    Inner2 inner2 = 5;
    Inner3 inner3 = 6;
  }
}

I get:

With the other branch:

[johanbrandhorst]

Thanks for trying it! I can't reproduce the any nested message rendering - could you share a more complete example? I'm seeing:

{
  "example.v1.PingResponse": {
    "type": "object",
    "properties": {
      "fieldW_rld": {
        "type": "string"
      },
      "inner1": {
        "$ref": "#/components/schemas/example.v1.PingResponse.Inner1"
      },
      "inner2": {
        "$ref": "#/components/schemas/example.v1.PingResponse.Inner2"
      },
      "inner3": {
        "$ref": "#/components/schemas/example.v1.PingResponse.Inner3"
      }
    },
    "oneOf": [
      {
        "not": {
          "anyOf": [
            {
              "required": [
                "inner1"
              ]
            },
            {
              "required": [
                "inner2"
              ]
            },
            {
              "required": [
                "inner3"
              ]
            }
          ]
        }
      },
      {
        "required": [
          "inner1"
        ]
      },
      {
        "required": [
          "inner2"
        ]
      },
      {
        "required": [
          "inner3"
        ]
      }
    ]
  },
  "example.v1.PingResponse.Inner1": {
    "type": "object",
    "properties": {
      "fieldInner1": {
        "type": "string"
      }
    }
  },
  "example.v1.PingResponse.Inner2": {
    "type": "object",
    "properties": {
      "fieldInner2": {
        "type": "string"
      }
    }
  },
  "example.v1.PingResponse.Inner3": {
    "type": "object",
    "properties": {
      "fieldInner3": {
        "type": "string"
      }
    }
  }
}

With regards to the not: anyOf wrapping, that's what I believe to be a more correct way of handling the case where none of the oneof messages is set. It seems like the other generator inserts a none message in this case, but that's not logically correct AFAICT.

[aidandj]

If you load the a_bit_of_everything.openapi.json into a swagger editor you can see it

[johanbrandhorst]

If you load the a_bit_of_everything.openapi.json into a swagger editor you can see it

Thanks, I see it now too! I think this is a bug in the SwaggerEditor rendering, because the schema of the oneOf types is defined in the properties, but I've updated the generator to include the schema in each oneOf definition as well, and it's now rendering correctly for me.

[abhipranay]

CC @abhipranay @JackBogart @aidandj @commiterate. I think even if you try this and find it isn't suitable for you right now that I'd rather merge this and create issues for each outstanding request to work on as a follow-up. What do you think?

Yea sounds like a good plan to me. As long as we are motivated to make this a reality ( even in mvp form ) it's a win.
I am damn sure people needed this and switching to other solutions like connectrpc etc is not that easy for every team. 🧑‍💻

[johanbrandhorst]

I had a look at the spec, specifically the Schema Object, which references the JSON Schema Draft 2020-12. Sections §7.7 and §10.1 establishes that adjacent keywords are independently evaluated and their results combined, which means that having both properties and oneOf at the same level simply means the instance must satisfy both. The JSON Schema example page contains an example (device type) with the oneOf schema defined in both the properties and the oneOf sections.

Is there a particular semantic difference you are aware of that this will trigger? To me it sounds like we can safely define the field schema in both places.

[abhipranay]

I re-read the references in detail. You are right.

adjacent keywords are independently evaluated and their results combined, which means that having both properties and oneOf at the same level simply means the instance must satisfy both. is true.

[aidandj]

Should we create an issue (issues?) for the next round of features?

High priority for me include

• merging files (new option)
• enable deprecation (imo default true)
• omit internal comments (imo default true)
• custom annotations
• disable default errors (option?)
• visibility restrictors (needs to be an option)

Yaml output is a nice to have

[johanbrandhorst]

Should we create an issue (issues?) for the next round of features?

High priority for me include

* merging files (new option)

* enable deprecation (imo default true)

* omit internal comments (imo default true)

* custom annotations

* disable default errors (option?)

* visibility restrictors (needs to be an option)

Yaml output is a nice to have

Lets try and decide on reasonable defaults here, and where we want options, yes, lets create an issue for each option. The current implementation has deprecation and internal comments (per AIP-192) handled already. The other 4 I think we could do 4 separate issues for. I'd like each issue to have an opportunity for users to discuss the need and ideally express their own support for it. I'd like at least 2 users supporting an option before we add it, though when we're this early in development I expect we can just add back some obvious ones, like the ones you suggested, without needing 2 individual supporters. If you'd like to get started on those issues, please do!

[aidandj]

Should we create an issue (issues?) for the next round of features?

High priority for me include

* merging files (new option)

* enable deprecation (imo default true)

* omit internal comments (imo default true)

* custom annotations

* disable default errors (option?)

* visibility restrictors (needs to be an option)

Yaml output is a nice to have

Lets try and decide on reasonable defaults here, and where we want options, yes, lets create an issue for each option. The current implementation has deprecation and internal comments (per AIP-192) handled already. The other 4 I think we could do 4 separate issues for. I'd like each issue to have an opportunity for users to discuss the need and ideally express their own support for it. I'd like at least 2 users supporting an option before we add it, though when we're this early in development I expect we can just add back some obvious ones, like the ones you suggested, without needing 2 individual supporters. If you'd like to get started on those issues, please do!

Will do.

I'm on mobile and can't search through the PR. Is file level deprecation supported (like the change we just added to the v2 generator)

[johanbrandhorst]

I'm on mobile and can't search through the PR. Is file level deprecation supported (like the change we just added to the v2 generator)

I'll make sure to get it in if it isn't, no reason it shouldn'd be.

[aidandj]

Issues created

[aidandj]

Should we mark the v3 generator as alpha. And indicate that output may change as we work through options?

I had some contributions to the original PR, if I could also be marked as a co-author.

[johanbrandhorst]

Yes, it would make sense to have an alpha phase for early adopters. I will add you to the contributors on the commit.

[aidandj]

OpenAPI 3.1 allows deprecating parameters. When I deprecate a field in a request message I'm not seeing the parameter get deprecated.

Small change: #6646

[JackBogart]

Should we create an issue (issues?) for the next round of features?

High priority for me include

* merging files (new option)

* enable deprecation (imo default true)

* omit internal comments (imo default true)

* custom annotations

* disable default errors (option?)

* visibility restrictors (needs to be an option)

Yaml output is a nice to have

Just wanna say that I'd definitely like most of these options as well. I actively use visibility restrictors, omitting internal comments, and deprecation. I can also easily see the use case of the others.

Not saying that the first iteration of the generator needs these, but wanted to show there's a general desire for them eventually.

[johanbrandhorst]

Thanks everyone for your contribution to this PR 🎉. Now we can start adding back some of the other features everyone wants 😄.

[abhipranay]

Thanks @johanbrandhorst for proactiveness on this long pending feature.
Now we have a starting point. 🥳