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

enable cors/tests.rs and make it pass #473

Merged
merged 3 commits into from
Feb 22, 2024
Merged

Conversation

GlenDC
Copy link
Contributor

@GlenDC GlenDC commented Feb 22, 2024

Motivation

cors/tests.rs was added somewhere between now and the 0.5 release.
However the file wasn't enabled, and this seemed to also hide a bug,
where we did not in fact preserve the already set Vary (http response) header.

Found while porting over the updated/added logic since 0.5 to https://github.com/plabayo/rama

(as that codebase uses ported code from tower-async, which is ported from tower,
since then I have abandoned tower-async as it did its job to proof that it was possible, but more drastic changes
were desired on my side for rama. I do keep however rama in sync with improvements made to codebases such as tower-http, which is one of many reasons why I love to contribute to tower where I can and where it is desired)

Solution

  1. enable the test by adding the mod to cors/mod.rs
  2. fix the bug in cors/mod.rs where we remove from headers instead of response_headers

As flagged on Discord it does also mean we need a third change where the order is not preserved,
and instead the custom (already set) Vary header will be at the end.

Copy link
Collaborator

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! In addition to the change requested below, can you also add a changelog entry?

tower-http/src/cors/mod.rs Outdated Show resolved Hide resolved
@GlenDC GlenDC requested a review from jplatte February 22, 2024 21:03
GlenDC pushed a commit to plabayo/rama that referenced this pull request Feb 22, 2024
based on feedback given by jplatte in
tower-rs/tower-http#473
tower-http/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Thanks again!

@jplatte jplatte enabled auto-merge (squash) February 22, 2024 21:23
@jplatte jplatte merged commit 18677a9 into tower-rs:main Feb 22, 2024
11 checks passed
@GlenDC GlenDC deleted the patch/cors-tests branch February 22, 2024 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants