apple / swift-openapi-generator

Generate Swift client and server code from an OpenAPI document.
https://swiftpackageindex.com/apple/swift-openapi-generator/documentation
Apache License 2.0
1.26k stars 91 forks source link

Add the ability to skip generation of deprecated API in client code #106

Closed PadraigK closed 10 months ago

PadraigK commented 11 months ago

Recently, support was added to deprecation annotations to generated code, where the API document's API/models are marked as deprecated.

On the server side, it is necessary to support all deprecated models until they are obsoleted (removed from the spec) but on the client side it can be useful to skip generation of items the are marked as deprecated rather than generating them and annotating. Using the client generator this way can be part of a strategy that allows a service to obsolete old endpoints safely rather than needing to maintain them forever.

Skipping deprecated API allows a project to maintain zero warnings, since the generated code will reference deprecated models otherwise.

simonjbeaumont commented 11 months ago

That's an interesting idea.

I appreciate that the introduction of the deprecated annotations makes it more challenging to maintain a warning-free codebase—especially for adopters wishing to generate a client for an API that they don't author.

The Swift compiler also doesn't support suppressing these warnings piecemeal^0.

What will make this interesting is the fact that the API types (generated with the types mode) are shared between the client and the server.

IIUC the deprecation warnings are only generated if you use the deprecated code and that point of use is not also marked as deprecated.

Are we missing a deprecation annotation on some of the generated client code that's causing a warning to propagate even if the adopter doesn't make use of that operation on the generated Client type?

czechboy0 commented 11 months ago

Plus, a straightforward way to remove the types and operations you want to skip is to process the OpenAPI document before passing it to the generator, so I imagine some invocation of yq could be all you need here.

PadraigK commented 10 months ago

IIUC the deprecation warnings are only generated if you use the deprecated code and that point of use is not also marked as deprecated.

Deprecated operations use deprecated models, so the generated code itself can end up emitting lots of these warnings. There is a compiler flag that can hide all of these warnings that can be set on the module though... but I think a stronger reason for providing this is the point about enabling a strategy to allow a service to obsolete old endpoints safely rather than needing to maintain them forever.

When we can be assured that the minimum supported shipping version of the client does not call an operation, we can safely remove support from the server side without auditing through git history to determine if it's safe. But if the operations/models are shipped (even if deprecated) they may still be used by clients.

What will make this interesting is the fact that the API types (generated with the types mode) are shared between the client and the server.

That is interesting alright and would complicate the proposal somewhat. It would need to be made clear that this is a client only feature I suppose; maybe it would be an error to set this while also generating server components?

Plus, a straightforward way to remove the types and operations you want to skip is to process the OpenAPI document before passing it to the generator, so I imagine some invocation of yq could be all you need here.

I agree that using yq or post processing the generated code to filter out the deprecated operations/models would work as a work-around here but it's definitely more work for consumers to build this themselves.

PadraigK commented 10 months ago

Additionally (this may be a bug?) it seems that the Client implementation of the deprecated operations are not marked as deprecated, just the protocol definition. This means that calling a deprecated operation on Client doesn't emit a warning.

czechboy0 commented 10 months ago

There are existing open source tools that allow filtering operations and models based on various criteria.

I think this comes down to the scope of this project, which explicitly sets guard rails so that this project composes well with the rest of the OpenAPI ecosystem (in various languages) rather than try to replace multiple focused tools with a single broader scoped tool.

I think using a separate tool is ultimately a better adopter experience as well, as presumably clients in other languages, not just Swift, might have the same exact needs, and more people can help maintain it.

This being an example of something that you can fully achieve in a preprocessing step on the OpenAPI document strongly suggests it doesnt need to become part of the generator, and should instead be in a separate tool (which can be also written in Swift and use OpenAPIKit, like Swift OpenAPI Generator does), with its own clear project scope.

I'll let @simonjbeaumont chime in as well, but I'm leaning towards not adding this feature to the generator, as it seems to already be solved by the wider OpenAPI ecosystem.

czechboy0 commented 10 months ago

Additionally (this may be a bug?) it seems that the Client implementation of the deprecated operations are not marked as deprecated, just the protocol definition. This means that calling a deprecated operation on Client doesn't emit a warning.

Oh that's interesting, can you file a separate issue for that? I genuinely don't know whether Swift automatically marks the implementations of deprecated protocol methods as deprecated or not. If you're not getting a warning when calling a deprecated operation, it's definitely a bug somewhere that we should address.

czechboy0 commented 10 months ago

Update: @PadraigK graciously filed #149 🙏

simonjbeaumont commented 10 months ago

I'll let @simonjbeaumont chime in as well, but I'm leaning towards not adding this feature to the generator, as it seems to already be solved by the wider OpenAPI ecosystem.

This is fine with me.

However, for every feature we choose not to provide (opt-in) support for, we present users with a discoverability issue.

In our documentation we highlight some of the consequences of our decisions and advise how users should workaround them^1. We may want to consider extending this over time with common workflows for such goals.

czechboy0 commented 10 months ago

That's true, but I think that's a natural part of being a "good citizen" in a larger ecosystem, where many more things are "out of scope" than are "in scope". Recommending other specific tools, or even documenting how to use other tools, runs the risk of being partially on the hook for their quality and putting alternative tools at a disadvantage.

We can instead recommend a class of tools, and there are websites that list various tools in these categories (or even a web search provides good results). In this case, I'd suggest adopters look for an "OpenAPI filter" tools, there are a few.

czechboy0 commented 10 months ago

Maybe we can document the general workflow with some diagrams, without getting into specific tools, but conceptually showing that a preprocessing step might be used, which takes an OpenAPI doc as input, produces OpenAPI doc as output, and the second one is the the input into Swift OpenAPI Generator.

czechboy0 commented 10 months ago

Closing this for now as it's considered out of scope for this tool, and seems to already by covered by the wider OpenAPI ecosystem.