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

Add proposal SOAR-0008: OpenAPI document filtering #303

Merged
merged 8 commits into from Oct 19, 2023

Conversation

simonjbeaumont
Copy link
Collaborator

@simonjbeaumont simonjbeaumont commented Sep 28, 2023

Motivation

We'd like to run a proposal for filtering the OpenAPI document for just the required parts prior to generating.

Modifications

  • Add SOAR-0008: OpenAPI document filtering

(See the proposal itself for details)1

Result

n/a

Test Plan

n/a

Related Issues

#285

Footnotes

  1. https://github.com/apple/swift-openapi-generator/pull/303/files

Signed-off-by: Si Beaumont <beaumont@apple.com>
@czechboy0
Copy link
Collaborator

Thanks, @simonjbeaumont!

This proposal is now In Review, which will run until October 5 - feel free to leave your feedback either on this PR or on the corresponding Swift Forums post.

@porglezomp
Copy link

Should it be possible to exclude tags as well as include them?
I'm thinking of cases like an API where some endpoints are tagged as higher privileged, and a low-privilege client might want to exclude those endpoints.

@simonjbeaumont
Copy link
Collaborator Author

Should it be possible to exclude tags as well as include them? I'm thinking of cases like an API where some endpoints are tagged as higher privileged, and a low-privilege client might want to exclude those endpoints.

Thanks for taking the time to provide feedback.

I think this is a compelling example of tag-based filtering (e.g. filtering for only readonly). However, this would only be useful in conjunction with finer-grain filtering that can filter at the operation level. Currently the smallest unit of inclusion is a path, and filtering a path for specific operations was called out in the proposal as a future direction.

I'm keen to explore if this can be delivered incrementally, on top of this proposal. Specifically, can we can confirm this additional functionality be added in a backwards-compatible way?

For filtering for operations, this seems like it would compose pretty cleanly: just add an operations key to the filter config.

For excluding, we could consider using a prefix character to negate the pattern, e.g. !, which is similar to how gitignore works. The alternative here would be to move each of the current filters into an explicit "include", e.g.

filter:
  tags:
    include:
    - "t"
    exclude:
    - "rw"

Personally, I'm not sure it's worth the additional nesting, and would prefer the negation prefix, e.g.

filter:
  tags:
    - "t"
    - "!rw"

In either event, we would need to consider the precedence between include and exclude, which is why I'd prefer for this to come in its own feature.

On that note, @porglezomp, we would certainly consider a followup proposal to this one, to add this functionality, if that's something you wanted to drive. The process for this is documented here.

@czechboy0
Copy link
Collaborator

I quite like the "!" prefix, and I agree this should all compose nicely, so folks are encouraged to propose further extensions of this feature.

@porglezomp
Copy link

Another possible option that doesn't add nesting but keeps out-of-band signaling would be:

filter:
  tags:
    - "t"
    - not: "rw"

Although I think ! is safe because tags are required to be valid URI characters.

Signed-off-by: Si Beaumont <beaumont@apple.com>
@simonjbeaumont
Copy link
Collaborator Author

@porglezomp On reflection, I think that we want to include filtering at the operation level (cf. path) from the start. I have updated the proposal to bring that part forward from the "Future directions" section into the proposed changes and updated the PoC branch with an implementation.

At this time I haven't added support for excludes or complement filters.

As this feedback was incorporated pretty late in the review period, I'm proposing we extend it for a few days.

Please let me know your thoughts on the updates.

@czechboy0
Copy link
Collaborator

Thanks @simonjbeaumont, looks great - okay, let's extend the review period into next week.

@czechboy0
Copy link
Collaborator

Hi folks,

this proposal did not receive additional feedback after the extension last week, so it's now Ready for Implementation and considered Approved.

Thank you, @simonjbeaumont!

simonjbeaumont added a commit that referenced this pull request Oct 13, 2023
### Motivation

When generating client code, Swift OpenAPI Generator generates code for
the entire OpenAPI document, even if the user only makes use of a subset
of its types and operations.

Generating code that is unused constitutes overhead for the adopter:
- The overhead of generating code for unused types and operations
- The overhead of compiling the generated code
- The overhead of unused code in the users codebase (AOT generation)

This is particularly noticeable when working with a small subset of a
large API, which can result in O(100k) lines of unused code and long
generation and compile times.

For a more detailed motivation and design, see the proposal in #303.

### Modifications

- Add document filter to the generator config.
- Run filter as a post-transition hook in the generator pipeline after
parsing the document.
- Provide a CLI command that outputs the filtered document to stdout.

### Result

Users can filter a document before code-generation. For large APIs, this
can result in >90% speedup (see proposal).

### Test Plan

- Unit tests.

---------

Signed-off-by: Si Beaumont <beaumont@apple.com>
Co-authored-by: Honza Dvorsky <honza@apple.com>
@simonjbeaumont simonjbeaumont marked this pull request as ready for review October 13, 2023 12:24
Copy link
Collaborator

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually wait - please add it to the Proposals.md page so that it's linkable.

@simonjbeaumont simonjbeaumont enabled auto-merge (squash) October 13, 2023 12:25
@simonjbeaumont simonjbeaumont merged commit c67892a into apple:main Oct 19, 2023
8 checks passed
@czechboy0 czechboy0 added the semver/none No version bump required. label Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants