swift-openapi-generator icon indicating copy to clipboard operation
swift-openapi-generator copied to clipboard

[Bug] Exclude webhooks when filter is provided

Open czechboy0 opened this issue 10 months ago • 6 comments

Motivation

Fixes #712.

When an OpenAPI document includes webhooks that include schema references, and a filter is provided, the generator would fail unless all the schemas needed by all webhooks were explicitly included in the filter.

Modifications

Since we don't currently support filtering webhooks, just exclude them always when a filter is provided. This fixes the bug.

In the future, if we add support for webhooks, we can add support for filtering them as well.

Result

Unblocks generating GitHub's OpenAPI 3.1.0 doc + filters.

Test Plan

Adapted the filtering unit test, verified it fails without the fix, and passes with it.

czechboy0 avatar Feb 09 '25 21:02 czechboy0

Since we don't currently support filtering webhooks, just exclude them always when a filter is provided. This fixes the bug.

In the future, if we add support for webhooks, we can add support for filtering them as well.

It's true that we don't explicitly support filtering only webhooks, but looking at the OAS, webhooks: is a Map[string, Path Item Object]^0, and path items contain operations, which, in turn can have operation IDs and/or tags.

The filtering feature IMO is document-centric and so should respect the document, especially since we provide a standalone command for filter.

IMO the bug here is that we do not filter the webhooks according to the filter parameters, which we should.

simonjbeaumont avatar Feb 11 '25 11:02 simonjbeaumont

If we want to properly support filtering of webhooks in this PR, then it'll be a bit more work. For example, the allOperationIds property on Document in OpenAPIKit doesn't include operations from webhooks, which will break for example this invariant, if we allow operationIds from webhook operations. Not sure if that means we should wait for a fix in OpenAPIKit (cc @mattpolzin) or if we should try to reimplement it on our end.

I guess that means that the standalone filter command on the CLI doesn't work today on documents that contain webhooks.

czechboy0 avatar Feb 11 '25 12:02 czechboy0

Happy to fix allOperationIds to include webhooks. I’ve got the work tracked here.

mattpolzin avatar Feb 12 '25 02:02 mattpolzin

Once again @mattpolzin, thank you! 🙏

simonjbeaumont avatar Feb 12 '25 18:02 simonjbeaumont

Thanks @mattpolzin!

I started implementing the webhook filtering properly, but it's quickly growing into more work, so I'll have to put this aside for now and come back to when I have more time. Or someone else can pick it up if we need it sooner. For now we'll need to recommend adopters use the 3.0 GitHub OpenAPI doc, rather than 3.1, so they shouldn't be blocked.

czechboy0 avatar Feb 13 '25 09:02 czechboy0