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

Don't allow extracting MatchedPath in middleware for nested routes #1462

Merged
merged 10 commits into from Nov 8, 2022

Conversation

davidpdrsn
Copy link
Member

This changes MatchedPath so it cannot be extracted in middleware of nested routes. This means users wont see the internal wildcard route (/nested/*__private__axum_nest_tail_param) that actually matched. This is an implementation detail and shouldn't be exposed.

I didn't make MatchedPath accessible in response extensions like discussed in #1441 because thats not a breaking change and shouldn't block shipping 0.6.0

I also reworked the tests so there are multiple smaller tests instead of one big one. Should be easier to read.

Part of #1441

@davidpdrsn davidpdrsn added this to the 0.6 milestone Oct 9, 2022
axum/src/extract/matched_path.rs Show resolved Hide resolved
axum/src/extract/matched_path.rs Outdated Show resolved Hide resolved
axum/src/extract/matched_path.rs Outdated Show resolved Hide resolved
axum/src/extract/matched_path.rs Outdated Show resolved Hide resolved
axum/src/extract/matched_path.rs Outdated Show resolved Hide resolved
@dcormier
Copy link

dcormier commented Oct 10, 2022

It would be nice if the error returned by impl FromRequestParts for MatchedPath made it clear that it failed because there's a rested router. It would give people some quick guidance that it will work if they refactor their router stack, if they can (this is what I did based on the conversation in #1441).

@davidpdrsn
Copy link
Member Author

It would be nice if the error returned by impl FromRequestParts for MatchedPath made it clear that it failed because there's a rested router. It would give people some quick guidance that it will work if they refactor their router stack, if they can (this is what I did based on the conversation in #1441).

That would mean adding support for MatchedPath in nested routers would be a breaking change. I'm also not sure how users would actually handle the error differently from the general MatchedPathRejection. Personally I think documenting it is sufficient.

@dcormier
Copy link

Yes, I agree. I don't think they would be able to handle it differently. I was thinking purely for discoverability of the fact that it won't work with nested routers.

Copy link
Member

@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.

Outside of some small code style things, looks good!

axum/src/extract/matched_path.rs Outdated Show resolved Hide resolved
axum/src/extract/matched_path.rs Outdated Show resolved Hide resolved
axum/src/extract/matched_path.rs Outdated Show resolved Hide resolved
axum/src/extract/matched_path.rs Outdated Show resolved Hide resolved
@davidpdrsn davidpdrsn enabled auto-merge (squash) November 8, 2022 20:40
@davidpdrsn davidpdrsn merged commit 0e3f9d0 into main Nov 8, 2022
@davidpdrsn davidpdrsn deleted the rework-matched-path branch November 8, 2022 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants