Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Exclude webhooks when filter is provided #734

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

czechboy0
Copy link
Contributor

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.

@simonjbeaumont
Copy link
Collaborator

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]1, 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.

Footnotes

  1. https://spec.openapis.org/oas/latest.html#fixed-fields

@czechboy0
Copy link
Contributor Author

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.

@mattpolzin
Copy link

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

@mattpolzin
Copy link

mattpolzin commented Feb 12, 2025

Fixed in OpenAPIKit 3.4.1 and OpenAPIKit 4.0.0-beta.2.

@simonjbeaumont
Copy link
Collaborator

Once again @mattpolzin, thank you! 🙏

@czechboy0
Copy link
Contributor Author

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 czechboy0 marked this pull request as draft February 13, 2025 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenAPI docs with webhooks + schema refs fail with filtering
3 participants