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

Refactor storing URL params in extensions #833

Merged
merged 3 commits into from
Mar 6, 2022
Merged

Conversation

davidpdrsn
Copy link
Member

I've been working on #678 but did refactoring of path params first. Thought I'd submit that as its own PR to make reviewing easier.

@davidpdrsn davidpdrsn requested a review from jplatte March 5, 2022 14:18
@davidpdrsn davidpdrsn enabled auto-merge (squash) March 5, 2022 14:19
axum/src/routing/url_params.rs Outdated Show resolved Hide resolved

let params = params
.iter()
.filter(|(key, _)| !key.starts_with(super::NEST_TAIL_PARAM))
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit weird, will this special-cased parameter name go away with the next PR that normalizes path params?

Copy link
Member Author

Choose a reason for hiding this comment

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

This has to do with nested opaque services. Its implemented as a wildcard route like /path/*NEST_TAIL_PARAM. So NEST_TAIL_PARAM is reserved by axum and shouldn't pollute the path params the user might see if using Path<HashMap<String, String>>.

Copy link
Member

Choose a reason for hiding this comment

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

I see. I think it could use being more obviously special in that case, something like __private__axum_nest_tail_param.

@davidpdrsn davidpdrsn merged commit a438e6b into main Mar 6, 2022
@davidpdrsn davidpdrsn deleted the refactor-path-params branch March 6, 2022 11:41
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

2 participants