Skip to content

add missing track_caller to PathRouter method #3308

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

Closed
wants to merge 2 commits into from

Conversation

Laifsyn
Copy link

@Laifsyn Laifsyn commented Apr 7, 2025

Motivation

Make internal panics to return the actual offending line instead of redirecting to somewhere inside the library.

pub fn product() -> axum::Router<()> {
    use axum::routing::post;
    axum::Router::new()
        .route("/", post(|| async {}))
        .route("/", post(|| async {}))
         // The resulting message for panic location will display somewhere 
         // internal in the library instead of the user's offendin line.
}

Solution

Add the missing #[track_caller] attribute in the call stack (?)

make internal panics to return the actual offending line instead of redirecting to somewhere inside the library.
@Laifsyn
Copy link
Author

Laifsyn commented Apr 8, 2025

I embedded a panic in merge_for_path to find what other functions called it yet didn't return the actual callsite at the tests.
So, here's another place that adding the track_caller would allow it to report the callsite of the "offending" test

thread 'routing::tests::merging_routers_with_same_paths_but_different_methods' panicked at axum\src\routing\path_router.rs:198:67: # Inside a protected Method (protected as in `pub(super)`)

Added attribute to protected method so it can redirect to the actual test site which could count as "User site"
@jplatte
Copy link
Member

jplatte commented Apr 24, 2025

Thanks for bringing this up! I took a look, and I think the better fix is to avoid panicking in internal functions. I implemented that and added a test in #3319.

@jplatte jplatte closed this Apr 24, 2025
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