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.

Sign up for GitHub

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

Jump to bottom

Fix missing Allow when middleware are applied to MethodRouter #1773

Merged
merged 2 commits into from Feb 20, 2023
Merged

Fix missing Allow when middleware are applied to MethodRouter #1773

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17e0895
Fix missing `Allow` when middleware are applied to `MethodRouter`
davidpdrsn Feb 20, 2023
5219f59
this should fix it :pray:
davidpdrsn Feb 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion axum/CHANGELOG.md
View file
Open in desktop
Expand Up @@ -7,7 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

# Unreleased

- None.
- **fixed:** Fix `Allow` missing from routers with middleware

# 0.6.7 (17. February, 2023)

Expand Down
11 changes: 11 additions & 0 deletions axum/src/routing/method_routing.rs
View file
Open in desktop
Expand Up @@ -1487,6 +1487,17 @@ mod tests {
assert_eq!(headers[ALLOW], "GET,POST");
}

#[crate::test]
async fn allow_header_noop_middleware() {
let mut svc = MethodRouter::new()
.get(ok)
.layer(tower::layer::util::Identity::new());

let (status, headers, _) = call(Method::DELETE, &mut svc).await;
assert_eq!(status, StatusCode::METHOD_NOT_ALLOWED);
assert_eq!(headers[ALLOW], "GET,HEAD");
}

#[crate::test]
#[should_panic(
expected = "Overlapping method route. Cannot add two method routes that both handle `GET`"
Expand Down
13 changes: 0 additions & 13 deletions axum/src/routing/route.rs
View file
Open in desktop
Expand Up @@ -155,9 +155,6 @@ where

#[inline]
fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
#[derive(Clone, Copy)]
struct AlreadyPassedThroughRouteFuture;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe dig up the PR that introduced this via git blame?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did that here

tbh I'm not totally sure why everything still worked after removing this. Doesn't seem related to this change because it also works on main if you remove this. So must be something else that changed 🤷

#755 did add a regression test and I also tried the repro from #747. Both worked correctly.

and tested specifically the repro from the issue and everything worked fine. So something else must have changed that made AlreadyPassedThroughRouteFuture unnecessary.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, great 🙂


let this = self.project();

let mut res = match this.kind.project() {
Expand All @@ -171,16 +168,6 @@ where
}
};

if res
.extensions()
.get::<AlreadyPassedThroughRouteFuture>()
.is_some()
{
return Poll::Ready(Ok(res));
} else {
res.extensions_mut().insert(AlreadyPassedThroughRouteFuture);
}

set_allow_header(res.headers_mut(), this.allow_header);

// make sure to set content-length before removing the body
Expand Down