Additional api protocol conformances

peterringset commented 1 month ago

Motivation

The generated code from this tool offers a very good starting point for integrating an API into a larger project. One challenge that I've run into when using this tool is testability and mocking. While it is possible to write mocks by hand I have found it very useful to use code generation tools (e.g. Sourcery) to do most of the heavy lifting. To enable behavior like this it is often necessary to annotate the client/server source in some way. This change offers a way to let the APIProtocol declaration have a conformance to one or more user-defined protocols to achieve this.

Modifications

Added an option to the generate command to list protocols that the user wishes the API to conform to
Added entry to the config-file to list protocols that the user wishes the API to conform to
Added protocol conformance of user-defined protocols to the APIProtocol
Added test to verify correct behavior

Result

The generate command and config file will get an extra option (additionalAPIProtocols) for the user to list additional protocols they wish that the APIProtocol type conforms to.

Test Plan

Added a reference test that generates an empty API with conformance to an additional example protocol
Modified existing config file test to include this change

simonjbeaumont commented 1 month ago

@peterringset thanks for putting up this patch and your interest in the project!

We've heard of folks wanting this before (e.g. https://github.com/apple/swift-openapi-generator/issues/573, https://github.com/apple/swift-openapi-generator/issues/383).

While I'm not against it, I'd like to understand the motivation more. Unlike the issue that asked to attach user-defined macros, IIUC this PR would allow users only to declare a protocol conformance on the generated protocol APIProtocol. Some questions:

What's the developer experience when the protocol doesn't exist? Presumably the generated code just fails to compile but we need to make sure this surfaces well, because that conformance is now in a derived source file.

What's next for the adopter? Are we relying on them having to go and provide the protocol witnesses to actually conform to the protocol? If this is the case then does this bring much value? It seems like it will just separate the conformance from the conformance declaration in user code, which might actually be more confusing?

Potentially this is mainly focused on protocols that rely only on default implementations so declaring conformance is enough?

peterringset commented 1 month ago

Hi! Thanks for the quick feedback. I'll write my replies one by one below:

What's the developer experience when the protocol doesn't exist? Presumably the generated code just fails to compile but we need to make sure this surfaces well, because that conformance is now in a derived source file.

Yes, that's true. My initial thought would be that this is something that the developer opts in to, and it's hopefully not something that they would unintentionally add to their code generation. With that in mind I hope that all developers adopting this will understand how to use this option and declare the necessary protocol(s) in a different file that's available in the generated code's scope. I don't know if there's anything more that could/should be done? Maybe adding some comments to document what's happening?

What's next for the adopter? Are we relying on them having to go and provide the protocol witnesses to actually conform to the protocol? If this is the case then does this bring much value? It seems like it will just separate the conformance from the conformance declaration in user code, which might actually be more confusing?

This usage would in most cases mean that the protocol(s) to be added already exists somehow in the package that we're generating code in. In my particular case it's necessary for APIProtocol to inherit from the custom protocols (AutoMockable in my case with Sourcery) for the mocks generation to work properly. I.e. protocol APIProtocol: Sendable, AutoMockable { ... works, but extension APIProtocol: AutoMockable { ... does not work, since protocols cannot declare inheritance to other protocols like that. This allows me to generate a mock for APIProtocol, and not for Client.

Another use case could be that the custom protocol requirements exactly match the APIProtocols signatures, such that APIProtocol is already implementing the protocol by just declaring conformance. Although, this could also be solved other ways.

Potentially this is mainly focused on protocols that rely only on default implementations so declaring conformance is enough?

Yep, I think that is the most common use-case for a feature like this. The Sourcery use-case for instance has an empty protocol AutoMockable that works kind of like an annotation, and allows Sourcery to perform the mock generation for protocols that inherit from AutoMockable.

czechboy0 commented 1 month ago

Hi @peterringset,

yeah this is an interesting situation. I'm also not necessarily against landing this PR, but I'd like to explore how much this really helps compared to:

public typealias AutoMockableAPIProtocol = APIProtocol & AutoMockable

and then using AutoMockableAPIProtocol in your codebase, which has the advantage of not mixing the generated code from the two separate tools? Or would Sourcery not generate the code if the conformance is declared in the typealias?

peterringset commented 1 month ago

@czechboy0 No, unfortunately Sourcery doesn't pick up the AutoMockableAPIProtocol typealias as an eligible type for mocks. AFAIK it only cares about protocols directly inheriting AutoMockable, so composition will also not work in this particular case.

czechboy0 commented 1 month ago

I see. From briefly skimming its sources, I wonder if support for inspecting typealiases could be added, something around https://github.com/krzysztofzablocki/Sourcery/blob/8acc3bf2ba1828e5f1126c1acc23a9e79561652d/Templates/Templates/AutoMockable.stencil#L335C10-L335C55 ?

apple / swift-openapi-generator