ronantakizawa/codereview-bench huggingface dataset:
Repository: actix/actix-web
File: src/json.rs (Rust)

Code before:
    /// Change max size of payload. By default max size is 256Kb
    pub fn limit(&mut self, limit: usize) -> &mut Self {
        self.limit = limit;
        self
    }

    /// Set custom error handler
    pub fn error_handler<F>(&mut self, f: F) -> &mut Self
    where
        F: Fn(JsonPayloadError, &HttpRequest<S>) -> Error + 'static,
    {
        self.ehandler = Rc::new(f);
        self
    }

    /// Add allowed content type
    pub fn content_type(&mut self, content_type: ContentTypeFilter) -> &mut Self {
        self.content_types.push(content_type);
        self
    }
}

impl<S> Default for JsonConfig<S> {
    fn default() -> Self {
        let mut content_types = Vec::new();
        content_types.push(ContentTypeFilter::new(None, Some(mime::JSON), None));
        content_types.push(ContentTypeFilter::new(None, None, Some(mime::JSON)));
        JsonConfig {
            limit: 262_144,
            ehandler: Rc::new(|e, _| e.into()),
            content_types,
        }
    }
}

/// Filter type for allowed `Content-Type` header values.
pub struct ContentTypeFilter {
    type_: Option<mime::Name<'static>>,
    subtype: Option<mime::Name<'static>>,
    suffix: Option<mime::Name<'static>>,
}

impl ContentTypeFilter {
    pub fn new(
        type_: Option<mime::Name<'static>>,
        subtype: Option<mime::Name<'static>>,
        suffix: Option<mime::Name<'static>>,
    ) -> Self {
        Self {
            type_,
            subtype,

Review comment (suggestion):
Json should be always default mime type, do not add it by default

Code after:
    pub fn limit(&mut self, limit: usize) -> &mut Self {
        self.limit = limit;
        self
    }

    /// Set custom error handler
    pub fn error_handler<F>(&mut self, f: F) -> &mut Self
    where
        F: Fn(JsonPayloadError, &HttpRequest<S>) -> Error + 'static,
    {
        self.ehandler = Rc::new(f);
        self
    }

    /// Set predicate for allowed content types
    pub fn content_type<F>(&mut self, predicate: F) -> &mut Self
    where
        F: Fn(mime::Mime) -> bool + 'static,
    {
        self.content_type = Some(Box::new(predicate));
        self
    }
}

impl<S> Default for JsonConfig<S> {
    fn default() -> Self {
        JsonConfig {
            limit: 262_144,
            ehandler: Rc::new(|e, _| e.into()),
            content_type: None,
        }
    }
}

/// Request payload json parser that resolves to a deserialized `T` value.
///
/// Returns error:
///
/// * content type is not `application/json`
///   (unless specified in [`JsonConfig`](struct.JsonConfig.html))
/// * content length is greater than 256k
///
/// # Server example
///
/// ```rust
/// # extern crate actix_web;
/// # extern crate futures;
/// # #[macro_use] extern crate serde_derive;
/// use actix_web::{AsyncResponder, Error, HttpMessage, HttpRequest, HttpResponse};
/// use futures::future::Future;
///
Repository: actix/actix-web
File: src/test.rs (Rust)

Code before:
            prefix: 0,
        }
    }

    /// Set HTTP version of this request
    pub fn version(mut self, ver: Version) -> Self {
        self.version = ver;
        self
    }

    /// Set HTTP method of this request
    pub fn method(mut self, meth: Method) -> Self {
        self.method = meth;
        self
    }

    /// Set HTTP Uri of this request
    pub fn uri(mut self, path: &str) -> Self {
        self.uri = Uri::from_str(path).unwrap();
        self
    }

    /// set cookie of this request
    pub fn cookie(mut self, cookie: Cookie<'static>) -> Self {
        let mut should_insert = true;
        let mut cookies = match self.cookies {
            Some(old_cookies) => {
                for old_cookie in &old_cookies {
                    if old_cookie == &cookie {
                        should_insert = false
                    };
                }
                old_cookies
            }
            None => { Vec::<Cookie>::new() }
        };
        if should_insert {
            cookies.push(cookie)
        };
        self.cookies = Some(cookies);
        self
    }

    /// Set a header
    pub fn set<H: Header>(mut self, hdr: H) -> Self {
        if let Ok(value) = hdr.try_into() {
            self.headers.append(H::name(), value);
            return self;
        }
        panic!("Can not set header");
    }

Review comment (suggestion):
I think it would be better to match by mut reference and insert inside `Some(ref mut cookies)` instead of moving out and moving in again


Code after:
            payload: None,
            prefix: 0,
        }
    }

    /// Set HTTP version of this request
    pub fn version(mut self, ver: Version) -> Self {
        self.version = ver;
        self
    }

    /// Set HTTP method of this request
    pub fn method(mut self, meth: Method) -> Self {
        self.method = meth;
        self
    }

    /// Set HTTP Uri of this request
    pub fn uri(mut self, path: &str) -> Self {
        self.uri = Uri::from_str(path).unwrap();
        self
    }

    /// set cookie of this request
    pub fn cookie(mut self, cookie: Cookie<'static>) -> Self {
        if self.cookies.is_none() {
            let mut should_insert = true;
            let old_cookies = self.cookies.as_mut().unwrap();
            for old_cookie in old_cookies.iter() {
                if old_cookie == &cookie {
                    should_insert = false
                };
            };
            if should_insert {
                old_cookies.push(cookie);
            };
        } else {
            self.cookies = Some(vec![cookie]);
        };
        self
    }

    /// Set a header
    pub fn set<H: Header>(mut self, hdr: H) -> Self {
        if let Ok(value) = hdr.try_into() {
            self.headers.append(H::name(), value);
            return self;
        }
        panic!("Can not set header");
    }
Repository: actix/actix-web
File: src/types/path.rs (Rust)

Code before:
/// }
///
/// /// extract `Info` from a path using serde
/// fn index(info: web::Path<Info>) -> Result<String, Error> {
///     Ok(format!("Welcome {}!", info.username))
/// }
///
/// fn main() {
///     let app = App::new().service(
///         web::resource("/{username}/index.html") // <- define path parameters
///              .route(web::get().to(index)) // <- use handler with Path` extractor
///     );
/// }
/// ```
impl<T, P> FromRequest<P> for Path<T>
where
    T: de::DeserializeOwned,
{
    type Error = Error;
    type Future = Result<Self, Error>;

    #[inline]
    fn from_request(req: &HttpRequest, _: &mut Payload<P>) -> Self::Future {
        de::Deserialize::deserialize(PathDeserializer::new(req.match_info()))
            .map(|inner| {
                log::error!("Failed to deserialize during Path extractor");
                Path { inner }
            })
            .map_err(ErrorNotFound)
    }
}

#[cfg(test)]
mod tests {
    use actix_router::ResourceDef;

    use super::*;
    use crate::test::{block_on, TestRequest};

    #[test]
    fn test_extract_path_single() {
        let resource = ResourceDef::new("/{value}/");

        let mut req = TestRequest::with_uri("/32/").to_srv_request();
        resource.match_path(req.match_info_mut());

        let (req, mut pl) = req.into_parts();
        assert_eq!(*Path::<i8>::from_request(&req, &mut pl).unwrap(), 32);
    }

    #[test]

Review comment (bug):
i think this is wrong position and this should be debug

Code after:
/// }
///
/// /// extract `Info` from a path using serde
/// fn index(info: web::Path<Info>) -> Result<String, Error> {
///     Ok(format!("Welcome {}!", info.username))
/// }
///
/// fn main() {
///     let app = App::new().service(
///         web::resource("/{username}/index.html") // <- define path parameters
///              .route(web::get().to(index)) // <- use handler with Path` extractor
///     );
/// }
/// ```
impl<T, P> FromRequest<P> for Path<T>
where
    T: de::DeserializeOwned,
{
    type Error = Error;
    type Future = Result<Self, Error>;

    #[inline]
    fn from_request(req: &HttpRequest, _: &mut Payload<P>) -> Self::Future {
        de::Deserialize::deserialize(PathDeserializer::new(req.match_info()))
            .map(|inner| Path { inner })
            .map_err(ErrorNotFound)
    }
}

#[cfg(test)]
mod tests {
    use actix_router::ResourceDef;

    use super::*;
    use crate::test::{block_on, TestRequest};

    #[test]
    fn test_extract_path_single() {
        let resource = ResourceDef::new("/{value}/");

        let mut req = TestRequest::with_uri("/32/").to_srv_request();
        resource.match_path(req.match_info_mut());

        let (req, mut pl) = req.into_parts();
        assert_eq!(*Path::<i8>::from_request(&req, &mut pl).unwrap(), 32);
    }

    #[test]
    fn test_tuple_extract() {
        let resource = ResourceDef::new("/{key}/{value}/");
Repository: actix/actix-web
File: src/types/path.rs (Rust)

Code before:
///
/// /// extract `Info` from a path using serde
/// fn index(info: web::Path<Info>) -> Result<String, Error> {
///     Ok(format!("Welcome {}!", info.username))
/// }
///
/// fn main() {
///     let app = App::new().service(
///         web::resource("/{username}/index.html") // <- define path parameters
///              .route(web::get().to(index)) // <- use handler with Path` extractor
///     );
/// }
/// ```
impl<T, P> FromRequest<P> for Path<T>
where
    T: de::DeserializeOwned,
{
    type Error = Error;
    type Future = Result<Self, Error>;

    #[inline]
    fn from_request(req: &HttpRequest, _: &mut Payload<P>) -> Self::Future {
        de::Deserialize::deserialize(PathDeserializer::new(req.match_info()))
            .map(|inner| Path { inner })
            .map_err(|| {
                log::debug!("Failed to deserialize during Path extractor");
                ErrorNotFound
            })
    }
}

#[cfg(test)]
mod tests {
    use actix_router::ResourceDef;

    use super::*;
    use crate::test::{block_on, TestRequest};

    #[test]
    fn test_extract_path_single() {
        let resource = ResourceDef::new("/{value}/");

        let mut req = TestRequest::with_uri("/32/").to_srv_request();
        resource.match_path(req.match_info_mut());

        let (req, mut pl) = req.into_parts();
        assert_eq!(*Path::<i8>::from_request(&req, &mut pl).unwrap(), 32);
    }

    #[test]
    fn test_tuple_extract() {

Review comment (suggestion):
one more thing. i think we should include `path` to log

Code after:
///
/// /// extract `Info` from a path using serde
/// fn index(info: web::Path<Info>) -> Result<String, Error> {
///     Ok(format!("Welcome {}!", info.username))
/// }
///
/// fn main() {
///     let app = App::new().service(
///         web::resource("/{username}/index.html") // <- define path parameters
///              .route(web::get().to(index)) // <- use handler with Path` extractor
///     );
/// }
/// ```
impl<T, P> FromRequest<P> for Path<T>
where
    T: de::DeserializeOwned,
{
    type Error = Error;
    type Future = Result<Self, Error>;

    #[inline]
    fn from_request(req: &HttpRequest, _: &mut Payload<P>) -> Self::Future {
        de::Deserialize::deserialize(PathDeserializer::new(req.match_info()))
            .map(|inner| Path { inner })
            .map_err(ErrorNotFound)
    }
}

#[cfg(test)]
mod tests {
    use actix_router::ResourceDef;

    use super::*;
    use crate::test::{block_on, TestRequest};

    #[test]
    fn test_extract_path_single() {
        let resource = ResourceDef::new("/{value}/");

        let mut req = TestRequest::with_uri("/32/").to_srv_request();
        resource.match_path(req.match_info_mut());

        let (req, mut pl) = req.into_parts();
        assert_eq!(*Path::<i8>::from_request(&req, &mut pl).unwrap(), 32);
    }

    #[test]
    fn test_tuple_extract() {
        let resource = ResourceDef::new("/{key}/{value}/");

        let mut req = TestRequest::with_uri("/name/user1/?id=test").to_srv_request();
Repository: actix/actix-web
File: actix-http/src/cookie/builder.rs (Rust)

Code before:
use std::borrow::Cow;

use time::{Duration, Tm};

use super::{Cookie, SameSite};

/// Structure that follows the builder pattern for building `Cookie` structs.
///
/// To construct a cookie:
///
///   1. Call [`Cookie::build`](struct.Cookie.html#method.build) to start building.
///   2. Use any of the builder methods to set fields in the cookie.
///   3. Call [finish](#method.finish) to retrieve the built cookie.
///
/// # Example
///
/// ```rust
/// use actix_http::cookie::Cookie;
/// use time::Duration;
///
/// # fn main() {
/// let cookie: Cookie = Cookie::build("name", "value")
///     .domain("www.rust-lang.org")
///     .path("/")
///     .secure(true)
///     .http_only(true)
///     .max_age(Duration::days(1))
///     .finish();

Review comment (suggestion):
we should drop `time`, there is no reason to keep both.

Code after:
use std::borrow::Cow;

use time::Tm;
use chrono::Duration;

use super::{Cookie, SameSite};

/// Structure that follows the builder pattern for building `Cookie` structs.
///
/// To construct a cookie:
///
///   1. Call [`Cookie::build`](struct.Cookie.html#method.build) to start building.
///   2. Use any of the builder methods to set fields in the cookie.
///   3. Call [finish](#method.finish) to retrieve the built cookie.
///
/// # Example
///
/// ```rust
/// use actix_http::cookie::Cookie;
/// use chrono::Duration;
///
/// # fn main() {
/// let cookie: Cookie = Cookie::build("name", "value")
///     .domain("www.rust-lang.org")
///     .path("/")
///     .secure(true)
///     .http_only(true)
///     .max_age(Duration::days(1))
Repository: actix/actix-web
File: src/service.rs (Rust)

Code before:
}

impl<T> ServiceFactory for ServiceFactoryWrapper<T>
where
    T: HttpServiceFactory,
{
    fn register(&mut self, config: &mut AppService) {
        if let Some(item) = self.factory.take() {
            item.register(config)
        }
    }
}

pub struct ServiceRequest {
    req: HttpRequest,
    payload: Payload,
}

impl ServiceRequest {
    /// Construct service request from parts
    pub(crate) fn from_parts(req: HttpRequest, payload: Payload) -> Self {
        ServiceRequest { req, payload }
    }

    /// Get a mutable reference to parts
    pub fn parts_mut(&mut self) -> (&mut HttpRequest, &mut Payload) {
        (&mut self.req, &mut self.payload)
    }

    /// Get a reference to parts
    pub fn parts(&self) -> (&HttpRequest, &Payload) {
        (&self.req, &self.payload)
    }

    /// Deconstruct request into parts
    pub fn into_parts(self) -> (HttpRequest, Payload) {
        (self.req, self.payload)
    }

    /// Create service response
    #[inline]
    pub fn into_response<B, R: Into<Response<B>>>(self, res: R) -> ServiceResponse<B> {
        ServiceResponse::new(self.req, res.into())
    }

    /// Create service response for error
    #[inline]
    pub fn error_response<B, E: Into<Error>>(self, err: E) -> ServiceResponse<B> {
        let res: Response = err.into().into();
        ServiceResponse::new(self.req, res.into_body())
    }

Review comment (suggestion):
This should not be allowed, otherwise request could be cloned and you would get panic trying to modify request head

Code after:
}

impl<T> ServiceFactory for ServiceFactoryWrapper<T>
where
    T: HttpServiceFactory,
{
    fn register(&mut self, config: &mut AppService) {
        if let Some(item) = self.factory.take() {
            item.register(config)
        }
    }
}

pub struct ServiceRequest {
    req: HttpRequest,
    payload: Payload,
}

impl ServiceRequest {
    /// Construct service request from parts
    pub(crate) fn from_parts(req: HttpRequest, payload: Payload) -> Self {
        ServiceRequest { req, payload }
    }

    /// Deconstruct request into parts
    pub fn into_parts(self) -> (HttpRequest, Payload) {
        (self.req, self.payload)
    }

    /// Create service response
    #[inline]
    pub fn into_response<B, R: Into<Response<B>>>(self, res: R) -> ServiceResponse<B> {
        ServiceResponse::new(self.req, res.into())
    }

    /// Create service response for error
    #[inline]
    pub fn error_response<B, E: Into<Error>>(self, err: E) -> ServiceResponse<B> {
        let res: Response = err.into().into();
        ServiceResponse::new(self.req, res.into_body())
    }

    /// This method returns reference to the request head
    #[inline]
    pub fn head(&self) -> &RequestHead {
        &self.req.head()
    }

    /// This method returns reference to the request head
    #[inline]
    pub fn head_mut(&mut self) -> &mut RequestHead {
Repository: actix/actix-web
File: src/middleware/identity.rs (Rust)

Code before:
    }

    /// This method is used to 'forget' the current identity on subsequent
    /// requests.
    pub fn forget(&self) {
        if let Some(id) = self.0.extensions_mut().get_mut::<IdentityItem>() {
            id.id = None;
            id.changed = true;
        }
    }

    fn get_identity(extensions: &Extensions) -> Option<String> {
        if let Some(id) = extensions.get::<IdentityItem>() {
            id.id.clone()
        } else {
            None
        }
    }
}

struct IdentityItem {
    id: Option<String>,
    changed: bool,
}

/// Helper trait that allows to get Identity
pub trait RequestIdentity {
    fn get_identity(&self) -> Option<String>;
}

impl<T> RequestIdentity for T
where
    T: HttpMessage,
{
    fn get_identity(&self) -> Option<String> {
        Identity::get_identity(&self.extensions())
    }
}

/// Extractor implementation for Identity type.
///
/// ```rust
/// # use actix_web::*;
/// use actix_web::middleware::identity::Identity;
///
/// fn index(id: Identity) -> String {
///     // access request identity
///     if let Some(id) = id.identity() {
///         format!("Welcome! {}", id)
///     } else {
///         "Welcome Anonymous!".to_owned()

Review comment (suggestion):
need more docs. add note that it could be used in middleware but identity policy must be set before any other middleware that needs identity

Code after:
    }

    /// This method is used to 'forget' the current identity on subsequent
    /// requests.
    pub fn forget(&self) {
        if let Some(id) = self.0.extensions_mut().get_mut::<IdentityItem>() {
            id.id = None;
            id.changed = true;
        }
    }

    fn get_identity(extensions: &Extensions) -> Option<String> {
        if let Some(id) = extensions.get::<IdentityItem>() {
            id.id.clone()
        } else {
            None
        }
    }
}

struct IdentityItem {
    id: Option<String>,
    changed: bool,
}

/// Helper trait that allows to get Identity.
/// It could be used in middleware but identity policy must be set before any other middleware that needs identity
pub trait RequestIdentity {
    fn get_identity(&self) -> Option<String>;
}

impl<T> RequestIdentity for T
where
    T: HttpMessage,
{
    fn get_identity(&self) -> Option<String> {
        Identity::get_identity(&self.extensions())
    }
}

/// Extractor implementation for Identity type.
///
/// ```rust
/// # use actix_web::*;
/// use actix_web::middleware::identity::Identity;
///
/// fn index(id: Identity) -> String {
///     // access request identity
///     if let Some(id) = id.identity() {
///         format!("Welcome! {}", id)
///     } else {
Repository: actix/actix-web
File: src/middleware/condition.rs (Rust)

Code before:
use actix_service::{Service, Transform};
use futures::future::{ok, Either, FutureResult, Map};
use futures::{Future, Poll};

/// `Middleware` for conditianally enables another middleware
/// The controled middleware must not change the `Service` interfaces.
/// This means you cannot control such middlewares like `Logger` or `Compress`.
///
/// ## Usage
///
/// ```
/// use actix_web::middleware::{Condition, NormalizePath};
/// use actix_web::App;
///
/// fn main() {
///     let enable_normalize = std::env::var("NORMALIZE_PATH") == Ok("true".into());
///     let app = App::new()
///         .wrap(Condition::new(enable_normalize, NormalizePath));
/// }
/// ```
///
pub struct Condition<T> {
    trans: T,
    enable: bool,
}

impl<T> Condition<T> {

Review comment (suggestion):
```suggestion
//! `Middleware` for conditionally enables another middleware.
use actix_service::{Service, Transform};
```

Code after:
//! `Middleware` for conditionally enables another middleware.
use actix_service::{Service, Transform};
use futures::future::{ok, Either, FutureResult, Map};
use futures::{Future, Poll};

/// `Middleware` for conditionally enables another middleware.
/// The controled middleware must not change the `Service` interfaces.
/// This means you cannot control such middlewares like `Logger` or `Compress`.
///
/// ## Usage
///
/// ```rust
/// use actix_web::middleware::{Condition, NormalizePath};
/// use actix_web::App;
///
/// fn main() {
///     let enable_normalize = std::env::var("NORMALIZE_PATH") == Ok("true".into());
///     let app = App::new()
///         .wrap(Condition::new(enable_normalize, NormalizePath));
/// }
/// ```
pub struct Condition<T> {
    trans: T,
    enable: bool,
}

impl<T> Condition<T> {
Repository: actix/actix-web
File: src/middleware/condition.rs (Rust)

Code before:
use actix_service::{Service, Transform};
use futures::future::{ok, Either, FutureResult, Map};
use futures::{Future, Poll};

/// `Middleware` for conditianally enables another middleware
/// The controled middleware must not change the `Service` interfaces.
/// This means you cannot control such middlewares like `Logger` or `Compress`.
///
/// ## Usage
///
/// ```
/// use actix_web::middleware::{Condition, NormalizePath};
/// use actix_web::App;
///
/// fn main() {
///     let enable_normalize = std::env::var("NORMALIZE_PATH") == Ok("true".into());
///     let app = App::new()
///         .wrap(Condition::new(enable_normalize, NormalizePath));
/// }
/// ```
///
pub struct Condition<T> {
    trans: T,
    enable: bool,
}

impl<T> Condition<T> {
    pub fn new(enable: bool, trans: T) -> Self {
        Self { trans, enable }
    }

Review comment (suggestion):
```suggestion
/// `Middleware` for conditionally enables another middleware.
```

Code after:
//! `Middleware` for conditionally enables another middleware.
use actix_service::{Service, Transform};
use futures::future::{ok, Either, FutureResult, Map};
use futures::{Future, Poll};

/// `Middleware` for conditionally enables another middleware.
/// The controled middleware must not change the `Service` interfaces.
/// This means you cannot control such middlewares like `Logger` or `Compress`.
///
/// ## Usage
///
/// ```rust
/// use actix_web::middleware::{Condition, NormalizePath};
/// use actix_web::App;
///
/// fn main() {
///     let enable_normalize = std::env::var("NORMALIZE_PATH") == Ok("true".into());
///     let app = App::new()
///         .wrap(Condition::new(enable_normalize, NormalizePath));
/// }
/// ```
pub struct Condition<T> {
    trans: T,
    enable: bool,
}

impl<T> Condition<T> {
    pub fn new(enable: bool, trans: T) -> Self {
        Self { trans, enable }
    }