Support OpenAPI 3.1

[czechboy0]

Today, we assume OpenAPI 3.0.x, but we should explore how we can also support 3.1, and continue to support both in parallel for some time, as 3.0.x is still the most adopted one out in the wild.

[mattpolzin]

I'd be curious to hear your thoughts on how close to the mark OpenAPIKit's new compatibility layer comes for this use-case.

The idea (described in the release notes for Alpha 6 is that you would ideally have just one file that imported all of OpenAPI 3.0.x support, OpenAPI 3.1.x support, and OpenAPIKit compatibility layer -- that file would attempt to parse an OpenAPI Document as 3.0.x and convert the result to 3.1.x but then if it failed it would attempt to parse the same document as OpenAPI 3.1.x. In the release notes I give a low fidelity example w/r/t error handling because you'd probably want to give a better error if the document was supposed to be 3.0.x but failed to parse than just attempting the guaranteed failed parsing as 3.1.x.

The big caveat to this approach: you need to migrate this codebase to use the OpenAPIKit module everywhere (except for in that location that handles attempting to parse as 3.0.x). Although most differences between the OpenAPIKit30 and OpenAPIKit modules are minor, this could easily become a bit of a chore.

Aside from that caveat, I think this approach would be well suited especially to this use-case where you are taking OpenAPI in but not then re-serializing a new OpenAPI Document because you won't be hurt by "losing" the information that the original document was imported as 3.0.x.

[EDIT] RE handling of the parsing of the document as one version or another, I now see your code already takes a preemptive step of determining the document spec version prior to having OpenAPIKit parse the document, so that code would already be well equipped to decide whether to use OpenAPIKit's 3.0.x or 3.1.x module for parsing.

[czechboy0]

Yeah I'd like us to take that approach, but I haven't played with the conversion enough yet to be confident that we won't lose fidelity in some cases, and wouldn't want to handle 3.0 and 3.1 differently in code generation. It'd be good for someone to prototype this approach and see what issues are hit. Contributions are always very welcome 🙂

[Kyle-Ye]

Use case about OpenAPI 3.1: discourse_api_docs/openapi.json

More info can be found at #72.

[Kyle-Ye]

For anyone eager to try OpenAPI 3.1 with swift-openapi-generator, you can give https://github.com/Kyle-Ye/swift-openapi-generator/tree/openapi_31x a try.

Disclaimer: This branch only fixes the compilation problem for testing openapi 3.1.0. It has not been fully tested. Please do not use it for production purposes.

[czechboy0]

Looking at @Kyle-Ye's experimental converter branch, I'm concerned that we'd have to make such a large change atomically, in one go.

The phases I'd like us to be able to add OpenAPI 3.1 support (while also continuing to support 3.0) in would instead look like:

1. Start with the leaves: convert Swift OpenAPI Generator logic that operates just on e.g. parameters from OpenAPIKit30 to OpenAPIKit. This wouldn't yet add support for parsing OpenAPI 3.1 documents, but it'd update one part of our codebase to use the latest currency types from OpenAPIKit, and we can discuss any 3.0 -> 3.1 changes in smaller, focused PRs, especially topics around the changes to JSON schema will warrant thoughtful discussion.
2. Until all of the Swift OpenAPI Generator codebase is migrated, migrate small pieces of the codebase in individual PRs.
3. Only once the whole codebase works with OpenAPIKit (and not OpenAPIKit30) currency types we can update our document parsing logic and for 3.0 convert the whole document, passing it into the generator logic, and for 3.1 pass it as-is, as it doesn't require conversion. Only at this point would the generator accept 3.1 documents, in addition to 3.0.

But for this piecemeal approach to be possible, we'd need OpenAPIKit's OpenAPIKitCompat to make it possible to convert just bits of the document, instead of the whole document, here: https://github.com/mattpolzin/OpenAPIKit/blob/3.0.0-alpha.7/Sources/OpenAPIKitCompat/Compat30To31.swift The conversion functions are currently fileprivate, would you be open to making them public, @mattpolzin? It'd go a long way for projects that deeply integrate with OpenAPIKit 🙂

[Kyle-Ye]

Looking at @Kyle-Ye's experimental converter branch, I'm concerned that we'd have to make such a large change atomically, in one go.

The phases I'd like us to be able to add OpenAPI 3.1 support (while also continuing to support 3.0) in would instead look like:

1. Start with the leaves: convert Swift OpenAPI Generator logic that operates just on e.g. parameters from OpenAPIKit30 to OpenAPIKit. This wouldn't yet add support for parsing OpenAPI 3.1 documents, but it'd update one part of our codebase to use the latest currency types from OpenAPIKit, and we can discuss any 3.0 -> 3.1 changes in smaller, focused PRs, especially topics around the changes to JSON schema will warrant thoughtful discussion.
2. Until all of the Swift OpenAPI Generator codebase is migrated, migrate small pieces of the codebase in individual PRs.
3. Only once the whole codebase works with OpenAPIKit (and not OpenAPIKit30) currency types we can update our document parsing logic and for 3.0 convert the whole document, passing it into the generator logic, and for 3.1 pass it as-is, as it doesn't require conversion. Only at this point would the generator accept 3.1 documents, in addition to 3.0.

But for this piecemeal approach to be possible, we'd need OpenAPIKit's OpenAPIKitCompat to make it possible to convert just bits of the document, instead of the whole document, here: https://github.com/mattpolzin/OpenAPIKit/blob/3.0.0-alpha.7/Sources/OpenAPIKitCompat/Compat30To31.swift The conversion functions are currently fileprivate, would you be open to making them public, @mattpolzin? It'd go a long way for projects that deeply integrate with OpenAPIKit 🙂

Can we do the same like the upstream OpenAPIKit? Provide _OpenAPIGeneratorCore with dependency of OpenAPIKit and _OpenAPIGeneratorCore30 with dependency of OpenAPIKit30. Or even maintain 2 branch one for 3.0.x and one for 3.1.x.

This way we'll make the implementation simpler but make the maintain harder.

[Kyle-Ye]

Looking at @Kyle-Ye's experimental converter branch, I'm concerned that we'd have to make such a large change atomically, in one go.

The phases I'd like us to be able to add OpenAPI 3.1 support (while also continuing to support 3.0) in would instead look like:

1. Start with the leaves: convert Swift OpenAPI Generator logic that operates just on e.g. parameters from OpenAPIKit30 to OpenAPIKit. This wouldn't yet add support for parsing OpenAPI 3.1 documents, but it'd update one part of our codebase to use the latest currency types from OpenAPIKit, and we can discuss any 3.0 -> 3.1 changes in smaller, focused PRs, especially topics around the changes to JSON schema will warrant thoughtful discussion.
2. Until all of the Swift OpenAPI Generator codebase is migrated, migrate small pieces of the codebase in individual PRs.
3. Only once the whole codebase works with OpenAPIKit (and not OpenAPIKit30) currency types we can update our document parsing logic and for 3.0 convert the whole document, passing it into the generator logic, and for 3.1 pass it as-is, as it doesn't require conversion. Only at this point would the generator accept 3.1 documents, in addition to 3.0.

But for this piecemeal approach to be possible, we'd need OpenAPIKit's OpenAPIKitCompat to make it possible to convert just bits of the document, instead of the whole document, here: https://github.com/mattpolzin/OpenAPIKit/blob/3.0.0-alpha.7/Sources/OpenAPIKitCompat/Compat30To31.swift The conversion functions are currently fileprivate, would you be open to making them public, @mattpolzin? It'd go a long way for projects that deeply integrate with OpenAPIKit 🙂

One reason of the extra adaptive code is due to the change between OpenAPI.reference and JSONReference. OpenAPIKit repo updates some in OpenAPIKit module but not OpenAPIKit30 module. And according to the author, this is the expected behavior.

See mattpolzin/OpenAPIKit#276

[czechboy0]

Can we do the same like the upstream OpenAPIKit? Provide _OpenAPIGeneratorCore with dependency of OpenAPIKit and _OpenAPIGeneratorCore30 with dependency of OpenAPIKit30. Or even maintain 2 branch one for 3.0.x and one for 3.1.x.

I'd like to avoid duplicating the whole generator logic module, as we want to fully migrate over to the OpenAPIKit module from OpenAPIKit30, we just don't want to do so in a single PR.

[mattpolzin]

I'm open to making the conversion logic public. I don't think I want to properly support them as public API, but this feels like a pretty reasonable request all things considered so I'll just think on how best to express that. It may just be documentation that indicates this as a valid piecemeal approach for now but I might make them private again in version 4.x of OpenAPIKit perhaps since that's a release I tentatively plan for code cleanup and minimum swift version bump anyway.

I would advise against the multiple module strategy (which was already sounding like an unlikely option in comments above). OpenAPIKits logic and/or code structure would have been drastically more complicated if I had tried to make the same code handle both OpenAPI 3.0 and 3.1, but two modules has definitely had a high cost as well. It still might have been the right move for me, but it would be very unfortunate if that move resulted in downstream projects bifurcating their codebases as well.

[czechboy0]

Thanks @mattpolzin, totally understood. Yeah, we would use the piecemeal compat APIs only during the transition period, so you could even consider only making them public for a single alpha release, and remove them before 3.0 goes out of alpha.

[mattpolzin]

OpenAPIKit 3.0.0-alpha.8 exposes most of the to31() methods publicly. Hopefully your efforts won't suffer for not having access to the remaining functions (e.g. converting an JSONReference from one module to the other in isolation) because those operations are probably more granular than makes sense for a migration plan, but let me know if I am wrong and I will expose more of the API.

https://github.com/mattpolzin/OpenAPIKit/releases/tag/3.0.0-alpha.8

[czechboy0]

So I have a PR ready, took only about 30mins to migrate the codebase to use the OpenAPIKit module instead of OpenAPIKit30, and now we can load either! Thanks so much for the seamless transition, @mattpolzin!

I do have one question, @mattpolzin though - one of the drivers of us adding 3.1 support is the ability to have description on next to $refs - but they don't seem to be getting propagated by OpenAPIKit today. Is that expected or am I holding it wrong? Hopefully won't be a large amount of work, and it's my only remaining blocker of calling initial 3.1 support done once #219 lands. Thanks 🙏

[czechboy0]

Specifically, I'm looking for this to work (properties in object schemas):

Foo:
  type: object
  properties:
    bar:
      description: This is a bar.
      $ref: '#/components/schemas/Baz`

I need to get This is a bar generated on the property, but right now the JSONSchema's description is empty. Might it be because properties are String -> JSONSchema instead of String -> OpenAPI.Reference?

[mattpolzin]

Interesting. It turns out that when I first wrote my JSONSchema implementation it was not permitted to specify annotations like description next to $refs (well, it was stated that any annotations would be ignored) but now the newer versions do allow it. Even more interestingly, even the newer versions say that any given application can choose to use those annotations however it wants (it may choose to ignore them in favor of annotations within the referenced schema). However, given that OpenAPI is explicit about its support for overriding description and title alongside references, I think it is certainly the right move to carry that behavior forward into JSONSchema.

Hopefully doing so will be as simple as swapping the JSONReference out for an OpenAPI.Reference (as you suggested above). I'll look into that soon. If you get a chance to throw a ticket on OpenAPIKit that would help me remember, but I think I'll get to it soon enough.

[mattpolzin]

Tracking need for overriding reference descriptions within schemas here: mattpolzin/OpenAPIKit#298

[mattpolzin]

Ok, a new release has been cut with a few bug fixes and improvements including the ability to hang properties like description next to $refs.

I'm happy to answer questions or field suggestions regarding the changes: https://github.com/mattpolzin/OpenAPIKit/releases/tag/3.0.0-beta.2

[czechboy0]

@mattpolzin I just tried it out and it works great, thank you for the quick turnaround! 🙏