ronantakizawa/codereview-bench huggingface dataset:
Repository: tokio-rs/axum
File: examples/reqwest-response/src/main.rs (Rust)

Code before:
        Err(err) => {
            tracing::error!(%err, "request failed");
            return (StatusCode::BAD_REQUEST, Body::empty()).into_response();
        }
    };

    let mut response_builder = Response::builder().status(reqwest_response.status().as_u16());

    // This unwrap is fine because we haven't insert any headers yet so there can't be any invalid
    // headers
    let headers = response_builder.headers_mut().unwrap();
    for (header_name, header_value) in reqwest_response.headers().clone() {
        headers.insert(
            header_name
                .to_owned()
                .unwrap()
                .as_str()
                .parse::<HeaderName>()
                .unwrap(),
            header_value
                .to_str()
                .unwrap()
                .parse::<HeaderValue>()
                .unwrap(),
        );
    }

    response_builder
        .body(Body::from_stream(reqwest_response.bytes_stream()))
        // Same goes for this unwrap
        .unwrap()
}

async fn stream_some_data() -> Body {
    let stream = tokio_stream::iter(0..5)
        .throttle(Duration::from_secs(1))
        .map(|n| n.to_string())
        .map(Ok::<_, Infallible>);
    Body::from_stream(stream)
}


Review comment (performance):
It should be possible to convert the headers like this https://github.com/tokio-rs/axum/blob/main/axum/src/extract/multipart.rs#L98-L103. Thats a bit more efficient and doesn't require utf-8 encoded headers.

Code after:
        Err(err) => {
            tracing::error!(%err, "request failed");
            return (StatusCode::BAD_REQUEST, Body::empty()).into_response();
        }
    };

    let response_builder = Response::builder().status(reqwest_response.status().as_u16());

    // here the mapping of headers is required due to reqwest and axum differ on http crate versions
    let mut headers = HeaderMap::with_capacity(reqwest_response.headers().len());
    headers.extend(reqwest_response.headers().into_iter().map(|(name, value)| {
        let name = HeaderName::from_bytes(name.as_ref()).unwrap();
        let value = HeaderValue::from_bytes(value.as_ref()).unwrap();
        (name, value)
    }));

    response_builder
        .body(Body::from_stream(reqwest_response.bytes_stream()))
        // Same goes for this unwrap
        .unwrap()
}

async fn stream_some_data() -> Body {
    let stream = tokio_stream::iter(0..5)
        .throttle(Duration::from_secs(1))
        .map(|n| n.to_string())
        .map(Ok::<_, Infallible>);
    Body::from_stream(stream)
}
Repository: tokio-rs/axum
File: axum/src/extract/matched_path.rs (Rust)

Code before:
    pub fn as_str(&self) -> &str {
        &self.0
    }
}

#[async_trait]
impl<S> FromRequestParts<S> for MatchedPath
where
    S: Send + Sync,
{
    type Rejection = MatchedPathRejection;

    async fn from_request_parts(parts: &mut Parts, _state: &S) -> Result<Self, Self::Rejection> {
        let matched_path = parts
            .extensions
            .get::<Self>()
            .ok_or(MatchedPathRejection::MatchedPathMissing(MatchedPathMissing))?
            .clone();

        Ok(matched_path)
    }
}

#[async_trait]
impl<S> OptionalFromRequestParts<S> for MatchedPath
where
    S: Send + Sync,
{
    type Rejection = MatchedPathRejection;

    async fn from_request_parts(
        parts: &mut Parts,
        _state: &S,
    ) -> Result<Option<Self>, Self::Rejection> {
        Ok(parts.extensions.get::<Self>().cloned())
    }
}

#[derive(Clone, Debug)]
struct MatchedNestedPath(Arc<str>);

pub(crate) fn set_matched_path_for_request(
    id: RouteId,
    route_id_to_path: &HashMap<RouteId, Arc<str>>,
    extensions: &mut http::Extensions,
) {
    let matched_path = if let Some(matched_path) = route_id_to_path.get(&id) {
        matched_path
    } else {
        #[cfg(debug_assertions)]
        panic!("should always have a matched path for a route id");

Review comment (bug):
I think this should be `Infallible` as missing is the only failure case.

And just to be sure, it seems to me like the only way `MatchedPath` would be missing is if it is accessed inside a fallback handler? If that's so, I think it would be better not to add this impl and have users just use `Result` if they want to guard against using it there.

Code after:
        &self.0
    }
}

impl<S> FromRequestParts<S> for MatchedPath
where
    S: Send + Sync,
{
    type Rejection = MatchedPathRejection;

    async fn from_request_parts(parts: &mut Parts, _state: &S) -> Result<Self, Self::Rejection> {
        let matched_path = parts
            .extensions
            .get::<Self>()
            .ok_or(MatchedPathRejection::MatchedPathMissing(MatchedPathMissing))?
            .clone();

        Ok(matched_path)
    }
}

impl<S> OptionalFromRequestParts<S> for MatchedPath
where
    S: Send + Sync,
{
    type Rejection = Infallible;

    async fn from_request_parts(
        parts: &mut Parts,
        _state: &S,
    ) -> Result<Option<Self>, Self::Rejection> {
        Ok(parts.extensions.get::<Self>().cloned())
    }
}

#[derive(Clone, Debug)]
struct MatchedNestedPath(Arc<str>);

pub(crate) fn set_matched_path_for_request(
    id: RouteId,
    route_id_to_path: &HashMap<RouteId, Arc<str>>,
    extensions: &mut http::Extensions,
) {
    let matched_path = if let Some(matched_path) = route_id_to_path.get(&id) {
        matched_path
    } else {
        #[cfg(debug_assertions)]
        panic!("should always have a matched path for a route id");
        #[cfg(not(debug_assertions))]
        return;
    };
Repository: tokio-rs/axum
File: axum/src/util.rs (Rust)

Code before:
        A { #[pin] inner: A },
        B { #[pin] inner: B },
    }
}

pub(crate) fn try_downcast<T, K>(k: K) -> Result<T, K>
where
    T: 'static,
    K: Send + 'static,
{
    let mut k = Some(k);
    if let Some(k) = <dyn std::any::Any>::downcast_mut::<Option<T>>(&mut k) {
        Ok(k.take().unwrap())
    } else {
        Err(k.unwrap())
    }
}

#[test]
fn test_try_downcast() {
    assert_eq!(try_downcast::<i32, _>(5_u32), Err(5_u32));
    assert_eq!(try_downcast::<i32, _>(5_i32), Ok(5_i32));
}

// `AxumMutex` is a wrapper around `std::sync::Mutex` which, in test mode, tracks the number of
// times its been locked on the current task. That way we can write a test to ensure we don't
// accidentally introduce more locking.
//
// When not in test mode its a literal type alias for `std::sync::Mutex`.
#[cfg(not(test))]
mod mutex {
    #[allow(clippy::disallowed_types)]
    pub(crate) type AxumMutex<T> = std::sync::Mutex<T>;
}

#[cfg(test)]
mod mutex {
    #![allow(clippy::disallowed_types)]

    use std::sync::{
        atomic::{AtomicUsize, Ordering},
        LockResult, Mutex, MutexGuard,
    };

    tokio::task_local! {
        pub(crate) static NUM_LOCKED: AtomicUsize;
    }

    pub(crate) async fn mutex_num_locked<F, Fut>(f: F) -> (usize, Fut::Output)
    where
        F: FnOnce() -> Fut,

Review comment (suggestion):
```suggestion
// times it's been locked on the current task. That way we can write a test to ensure we don't
```

Code after:
        A { #[pin] inner: A },
        B { #[pin] inner: B },
    }
}

pub(crate) fn try_downcast<T, K>(k: K) -> Result<T, K>
where
    T: 'static,
    K: Send + 'static,
{
    let mut k = Some(k);
    if let Some(k) = <dyn std::any::Any>::downcast_mut::<Option<T>>(&mut k) {
        Ok(k.take().unwrap())
    } else {
        Err(k.unwrap())
    }
}

#[test]
fn test_try_downcast() {
    assert_eq!(try_downcast::<i32, _>(5_u32), Err(5_u32));
    assert_eq!(try_downcast::<i32, _>(5_i32), Ok(5_i32));
}

// `AxumMutex` is a wrapper around `std::sync::Mutex` which, in test mode, tracks the number of
// times it's been locked on the current task. That way we can write a test to ensure we don't
// accidentally introduce more locking.
//
// When not in test mode, it is just a type alias for `std::sync::Mutex`.
#[cfg(not(test))]
mod mutex {
    #[allow(clippy::disallowed_types)]
    pub(crate) type AxumMutex<T> = std::sync::Mutex<T>;
}

#[cfg(test)]
mod mutex {
    #![allow(clippy::disallowed_types)]

    use std::sync::{
        atomic::{AtomicUsize, Ordering},
        LockResult, Mutex, MutexGuard,
    };

    tokio::task_local! {
        pub(crate) static NUM_LOCKED: AtomicUsize;
    }

    pub(crate) async fn mutex_num_locked<F, Fut>(f: F) -> (usize, Fut::Output)
    where
        F: FnOnce() -> Fut,
Repository: tokio-rs/axum
File: axum/src/util.rs (Rust)

Code before:
}

pub(crate) fn try_downcast<T, K>(k: K) -> Result<T, K>
where
    T: 'static,
    K: Send + 'static,
{
    let mut k = Some(k);
    if let Some(k) = <dyn std::any::Any>::downcast_mut::<Option<T>>(&mut k) {
        Ok(k.take().unwrap())
    } else {
        Err(k.unwrap())
    }
}

#[test]
fn test_try_downcast() {
    assert_eq!(try_downcast::<i32, _>(5_u32), Err(5_u32));
    assert_eq!(try_downcast::<i32, _>(5_i32), Ok(5_i32));
}

// `AxumMutex` is a wrapper around `std::sync::Mutex` which, in test mode, tracks the number of
// times its been locked on the current task. That way we can write a test to ensure we don't
// accidentally introduce more locking.
//
// When not in test mode its a literal type alias for `std::sync::Mutex`.
#[cfg(not(test))]
mod mutex {
    #[allow(clippy::disallowed_types)]
    pub(crate) type AxumMutex<T> = std::sync::Mutex<T>;
}

#[cfg(test)]
mod mutex {
    #![allow(clippy::disallowed_types)]

    use std::sync::{
        atomic::{AtomicUsize, Ordering},
        LockResult, Mutex, MutexGuard,
    };

    tokio::task_local! {
        pub(crate) static NUM_LOCKED: AtomicUsize;
    }

    pub(crate) async fn mutex_num_locked<F, Fut>(f: F) -> (usize, Fut::Output)
    where
        F: FnOnce() -> Fut,
        Fut: std::future::IntoFuture,
    {
        NUM_LOCKED

Review comment (suggestion):
```suggestion
// When not in test mode, it is just a type alias for `std::sync::Mutex`.
```

Code after:
}

pub(crate) fn try_downcast<T, K>(k: K) -> Result<T, K>
where
    T: 'static,
    K: Send + 'static,
{
    let mut k = Some(k);
    if let Some(k) = <dyn std::any::Any>::downcast_mut::<Option<T>>(&mut k) {
        Ok(k.take().unwrap())
    } else {
        Err(k.unwrap())
    }
}

#[test]
fn test_try_downcast() {
    assert_eq!(try_downcast::<i32, _>(5_u32), Err(5_u32));
    assert_eq!(try_downcast::<i32, _>(5_i32), Ok(5_i32));
}

// `AxumMutex` is a wrapper around `std::sync::Mutex` which, in test mode, tracks the number of
// times it's been locked on the current task. That way we can write a test to ensure we don't
// accidentally introduce more locking.
//
// When not in test mode, it is just a type alias for `std::sync::Mutex`.
#[cfg(not(test))]
mod mutex {
    #[allow(clippy::disallowed_types)]
    pub(crate) type AxumMutex<T> = std::sync::Mutex<T>;
}

#[cfg(test)]
mod mutex {
    #![allow(clippy::disallowed_types)]

    use std::sync::{
        atomic::{AtomicUsize, Ordering},
        LockResult, Mutex, MutexGuard,
    };

    tokio::task_local! {
        pub(crate) static NUM_LOCKED: AtomicUsize;
    }

    pub(crate) async fn mutex_num_locked<F, Fut>(f: F) -> (usize, Fut::Output)
    where
        F: FnOnce() -> Fut,
        Fut: std::future::IntoFuture,
    {
        NUM_LOCKED
Repository: tokio-rs/axum
File: examples/templates-minijinja/src/main.rs (Rust)

Code before:
//! Run with
//!
//! ```not_rust
//! cargo run -p example-templates-minijinja
//! ```
//! Demo for the MiniJinja templating engine.
//! Exposes three pages all sharing the same layout with a minimal nav menu.

use axum::extract::State;
use axum::http::StatusCode;
use axum::{response::Html, routing::get, Router};
use minijinja::{context, Environment};
use std::sync::Arc;

struct AppState<'a> {
    env: Environment<'a>,
}

#[tokio::main]
async fn main() {
    // init template engine and add templates
    let mut env = Environment::new();
    env.add_template("layout", include_str!("../templates/layout.jinja"))
        .unwrap();
    env.add_template("home", include_str!("../templates/home.jinja"))
        .unwrap();
    env.add_template("content", include_str!("../templates/content.jinja"))
        .unwrap();
    env.add_template("about", include_str!("../templates/about.jinja"))
        .unwrap();

    // pass env to handlers via state
    let app_state = Arc::new(AppState { env });

    // define routes
    let app = Router::new()
        .route("/", get(handler_home))
        .route("/content", get(handler_content))
        .route("/about", get(handler_about))
        .with_state(app_state);


Review comment (suggestion):
```suggestion
struct AppState {
    env: Environment<'static>,
```

Since non-static in state is pretty uncommon (might even be completely unsupported, can't remember right now). 

Code after:
//! Run with
//!
//! ```not_rust
//! cargo run -p example-templates-minijinja
//! ```
//! Demo for the MiniJinja templating engine.
//! Exposes three pages all sharing the same layout with a minimal nav menu.

use axum::extract::State;
use axum::http::StatusCode;
use axum::{response::Html, routing::get, Router};
use minijinja::{context, Environment};
use std::sync::Arc;

struct AppState {
    env: Environment<'static>,
}

#[tokio::main]
async fn main() {
    // init template engine and add templates
    let mut env = Environment::new();
    env.add_template("layout", include_str!("../templates/layout.jinja"))
        .unwrap();
    env.add_template("home", include_str!("../templates/home.jinja"))
        .unwrap();
    env.add_template("content", include_str!("../templates/content.jinja"))
        .unwrap();
    env.add_template("about", include_str!("../templates/about.jinja"))
        .unwrap();

    // pass env to handlers via state
    let app_state = Arc::new(AppState { env });

    // define routes
    let app = Router::new()
        .route("/", get(handler_home))
        .route("/content", get(handler_content))
        .route("/about", get(handler_about))
        .with_state(app_state);
Repository: tokio-rs/axum
File: examples/mongodb/src/main.rs (Rust)

Code before:
        .await
        .unwrap();
    tracing::debug!("Listening on {}", listener.local_addr().unwrap());
    axum::serve(listener, app(client)).await.unwrap();
}

// defining routes and state
fn app(client: Client) -> Router {
    let collection: Collection<Member> = client.database("axum-mongo").collection("members");

    Router::new()
        .route("/create", post(create_member))
        .route("/read/:id", get(read_member))
        .route("/update", put(update_member))
        .route("/delete/:id", delete(delete_member))
        .layer(TraceLayer::new_for_http())
        .with_state(collection)
}

// handler to create a new member
async fn create_member(
    State(db): State<Collection<Member>>,
    Json(input): Json<Member>,
) -> impl IntoResponse {
    let result = db.insert_one(input, None).await.unwrap();
    println!("{:?}", result);

    (StatusCode::CREATED, Json(result))
}

// handler to read an existing member
async fn read_member(
    State(db): State<Collection<Member>>,
    Path(id): Path<u32>,
) -> impl IntoResponse {
    let result = db.find_one(doc! { "_id": id }, None).await.unwrap();
    println!("{:?}", result);

    if result.is_none() {
        return (StatusCode::NOT_FOUND, Json(result));
    }

    (StatusCode::OK, Json(result))
}

// handler to update an existing member
async fn update_member(
    State(db): State<Collection<Member>>,
    Json(input): Json<Member>,
) -> impl IntoResponse {
    let result = db

Review comment (suggestion):
Not sure we need all this print. Same goes for the ones below.

```suggestion
```

Code after:

    // run it
    let listener = tokio::net::TcpListener::bind("127.0.0.1:3000")
        .await
        .unwrap();
    tracing::debug!("Listening on {}", listener.local_addr().unwrap());
    axum::serve(listener, app(client)).await.unwrap();
}

// defining routes and state
fn app(client: Client) -> Router {
    let collection: Collection<Member> = client.database("axum-mongo").collection("members");

    Router::new()
        .route("/create", post(create_member))
        .route("/read/:id", get(read_member))
        .route("/update", put(update_member))
        .route("/delete/:id", delete(delete_member))
        .layer(TraceLayer::new_for_http())
        .with_state(collection)
}

// handler to create a new member
async fn create_member(
    State(db): State<Collection<Member>>,
    Json(input): Json<Member>,
) -> Result<Json<InsertOneResult>, (StatusCode, String)> {
    let result = db.insert_one(input, None).await.map_err(internal_error)?;

    Ok(Json(result))
}

// handler to read an existing member
async fn read_member(
    State(db): State<Collection<Member>>,
    Path(id): Path<u32>,
) -> Result<Json<Option<Member>>, (StatusCode, String)> {
    let result = db
        .find_one(doc! { "_id": id }, None)
        .await
        .map_err(internal_error)?;

    Ok(Json(result))
}

// handler to update an existing member
async fn update_member(
    State(db): State<Collection<Member>>,
    Json(input): Json<Member>,
) -> Result<Json<UpdateResult>, (StatusCode, String)> {
    let result = db
Repository: tokio-rs/axum
File: axum-macros/src/lib.rs (Rust)

Code before:
/// impl IntoResponse for MyRejection {
///     fn into_response(self) -> Response {
///         self.0
///     }
/// }
/// ```
///
/// # The whole type at once
///
/// By using `#[from_request(via(...))]` on the container you can extract the whole type at once,
/// instead of each field individually:
///
/// ```
/// use axum_macros::FromRequest;
/// use axum::extract::Extension;
///
/// // This will extracted via `Extension::<State>::from_request`
/// #[derive(Clone, FromRequest)]
/// #[from_request(via(Extension))]
/// struct State {
///     // ...
/// }
///
/// async fn handler(state: State) {}
/// ```
///
/// The rejection will be the "via extractors"'s rejection. For the previous example that would be
/// [`axum::extract::rejection::ExtensionRejection`].
///
/// You can use a different rejection type with `#[from_request(rejection(YourType))]`:
///
/// ```
/// use axum_macros::FromRequest;
/// use axum::{
///     extract::{Extension, rejection::ExtensionRejection},
///     response::{IntoResponse, Response},
///     Json,
///     http::StatusCode,
/// };
/// use serde_json::json;
///
/// // This will extracted via `Extension::<State>::from_request`
/// #[derive(Clone, FromRequest)]
/// #[from_request(
///     via(Extension),
///     // Use your own rejection type
///     rejection(MyRejection),
/// )]
/// struct State {
///     // ...
/// }

Review comment (suggestion):
Otherwise we'll get an empty line in the docs

```suggestion
///     # type Rejection = ();
///     #
///     # async fn from_request_parts(
```

Code after:
/// impl IntoResponse for MyRejection {
///     fn into_response(self) -> Response {
///         self.0
///     }
/// }
/// ```
///
/// ## Concrete state
///
/// If the extraction can be done only for a concrete state, that type can be specified with
/// `#[from_request(state(YourState))]`:
///
/// ```
/// use axum::extract::{FromRequest, FromRequestParts};
///
/// #[derive(Clone)]
/// struct CustomState;
///
/// struct MyInnerType;
///
/// #[axum::async_trait]
/// impl FromRequestParts<CustomState> for MyInnerType {
///     // ...
///     # type Rejection = ();
///
///     # async fn from_request_parts(
///         # _parts: &mut axum::http::request::Parts,
///         # _state: &CustomState
///     # ) -> Result<Self, Self::Rejection> {
///     #    todo!()
///     # }
/// }
///
/// #[derive(FromRequest)]
/// #[from_request(state(CustomState))]
/// struct MyExtractor {
///     custom: MyInnerType,
///     body: String,
/// }
/// ```
///
/// This is not needed for a `State<T>` as the type is inferred in that case.
///
/// ```
/// use axum::extract::{FromRequest, FromRequestParts, State};
///
/// #[derive(Clone)]
/// struct CustomState;
///
/// #[derive(FromRequest)]
/// struct MyExtractor {
Repository: tokio-rs/axum
File: axum-core/src/extract/request_parts.rs (Rust)

Code before:
    while let Some(frame) = body
        .frame()
        .await
        .transpose()
        .map_err(FailedToBufferBody::from_err)?
    {
        let Ok(data) = frame.into_data() else {
            return Ok(());
        };
        bytes.put(data);
    }

    Ok(())
}

#[async_trait]
impl<S> FromRequest<S> for Bytes
where
    S: Send + Sync,
{
    type Rejection = BytesRejection;

    async fn from_request(req: Request, _: &S) -> Result<Self, Self::Rejection> {
        let mut body = req.into_limited_body();

        // If there's only 1 chunk, we can just return Buf::to_bytes()
        let first_chunk = if let Some(frame) = body
            .frame()
            .await
            .transpose()
            .map_err(FailedToBufferBody::from_err)?
        {
            let Ok(first_chunk) = frame.into_data() else {
                return Ok(Bytes::new());
            };
            first_chunk
        } else {
            return Ok(Bytes::new());
        };

        let mut bytes = if let Some(frame) = body
            .frame()
            .await
            .transpose()
            .map_err(FailedToBufferBody::from_err)?
        {
            let Ok(second_chunk) = frame.into_data() else {
                return Ok(first_chunk);
            };

            let cap = first_chunk.remaining()

Review comment (suggestion):
Nit, we're not returning `Buf::to_bytes()` though. Maybe something like

> If there's only 1 chunk, we can just return it directly

?

Feel free to ignore though.

Code after:
        .frame()
        .await
        .transpose()
        .map_err(FailedToBufferBody::from_err)?
    {
        let Ok(data) = frame.into_data() else {
            return Ok(());
        };
        bytes.put(data);
    }

    Ok(())
}

#[async_trait]
impl<S> FromRequest<S> for Bytes
where
    S: Send + Sync,
{
    type Rejection = BytesRejection;

    async fn from_request(req: Request, _: &S) -> Result<Self, Self::Rejection> {
        let bytes = req
            .into_limited_body()
            .collect()
            .await
            .map_err(FailedToBufferBody::from_err)?
            .to_bytes();

        Ok(bytes)
    }
}

#[async_trait]
impl<S> FromRequest<S> for String
where
    S: Send + Sync,
{
    type Rejection = StringRejection;

    async fn from_request(req: Request, state: &S) -> Result<Self, Self::Rejection> {
        let bytes = Bytes::from_request(req, state)
            .await
            .map_err(|err| match err {
                BytesRejection::FailedToBufferBody(inner) => {
                    StringRejection::FailedToBufferBody(inner)
                }
            })?;

        let string = std::str::from_utf8(&bytes)
            .map_err(InvalidUtf8::from_err)?