ronantakizawa/codereview-bench huggingface dataset:
Repository: launchbadge/sqlx
File: sqlx-core/src/types/json.rs (Rust)

Code before:
//! Json for json and jsonb fields
//! 
//! Will attempt to cast to type passed in as the generic.
//! ```
//! #[derive(FromRow)]
//! struct Author {
//!   name: String,
//!   books: types::Json<Book>
//! }
//! ```
//! 
//! Can also be used to turn the json/jsonb into a hashmap
//! ```
//! #[derive(FromRow)]
//! struct FooStruct {
//!   id: String,
//!   properties: types::Json<HashMap<String, BarStruct >>
//! }
//! ```
//! 

use std::ops::{Deref, DerefMut};

use serde::{Deserialize, Serialize};
pub use serde_json::value::RawValue as JsonRawValue;
pub use serde_json::Value as JsonValue;

use crate::database::{Database, HasArguments, HasValueRef};
use crate::decode::Decode;
use crate::encode::{Encode, IsNull};
use crate::error::BoxDynError;
use crate::types::Type;

#[derive(
    Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Default, Serialize, Deserialize,
)]
#[serde(transparent)]
pub struct Json<T: ?Sized>(pub T);

impl<T> Deref for Json<T> {
    type Target = T;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

Review comment (suggestion):
These examples won't be rendered as the `json` module isn't public. You should move these to the `Json` type itself.

Code after:
use std::ops::{Deref, DerefMut};

use serde::{Deserialize, Serialize};
pub use serde_json::value::RawValue as JsonRawValue;
pub use serde_json::Value as JsonValue;

use crate::database::{Database, HasArguments, HasValueRef};
use crate::decode::Decode;
use crate::encode::{Encode, IsNull};
use crate::error::BoxDynError;
use crate::types::Type;

/// Json for json and jsonb fields
///
/// Will attempt to cast to type passed in as the generic.
///
/// ```toml
/// [dependencies]
/// serde_json = { version = "1.0", features = ["raw_value"] }
///
/// ```
///
/// # Example
///
/// ```
/// # use serde::Deserialize;
/// #[derive(Deserialize)]
/// struct Book {
///   name: String
/// }
///
/// #[derive(sqlx::FromRow)]
/// struct Author {
///   name: String,
///   books: sqlx::types::Json<Book>
/// }
/// ```
///
/// Can also be used to turn the json/jsonb into a hashmap
/// ```
/// use std::collections::HashMap;
/// use serde::Deserialize;
///
/// #[derive(Deserialize)]
/// struct Book {
///   name: String
Repository: launchbadge/sqlx
File: sqlx-core/src/sqlite/options/mod.rs (Rust)

Code before:
    /// Set the maximum number of commands to buffer for the worker thread before backpressure is
    /// applied.
    ///
    /// Given that most commands sent to the worker thread involve waiting for a result,
    /// the command channel is unlikely to fill up unless a lot queries are executed in a short
    /// period but cancelled before their full resultsets are returned.
    pub fn command_buffer_size(mut self, size: usize) -> Self {
        self.command_channel_size = size;
        self
    }

    /// Set the maximum number of rows to buffer back to the calling task when a query is executed.
    ///
    /// If the calling task cannot keep up, backpressure will be applied to the worker thread
    /// in order to limit CPU and memory usage.
    pub fn row_buffer_size(mut self, size: usize) -> Self {
        self.row_channel_size = size;
        self
    }

    /// Sets the [`vfs`](https://www.sqlite.org/vfs.html) parameter of the database connection.
    ///
    /// The default value is empty, and sqlite will use the default VFS object dependeing on the
    /// operating system.
    pub fn vfs(mut self, vfs_name: impl AsRef<str>) -> Self {
        self.vfs = Some(Cow::Owned(vfs_name.as_ref().to_string()));
        self
    }
}


Review comment (suggestion):
`impl Into<Cow<'static, str>>` would avoid copies when the user passes `&'static str` or `String`

Code after:
    /// Set the maximum number of commands to buffer for the worker thread before backpressure is
    /// applied.
    ///
    /// Given that most commands sent to the worker thread involve waiting for a result,
    /// the command channel is unlikely to fill up unless a lot queries are executed in a short
    /// period but cancelled before their full resultsets are returned.
    pub fn command_buffer_size(mut self, size: usize) -> Self {
        self.command_channel_size = size;
        self
    }

    /// Set the maximum number of rows to buffer back to the calling task when a query is executed.
    ///
    /// If the calling task cannot keep up, backpressure will be applied to the worker thread
    /// in order to limit CPU and memory usage.
    pub fn row_buffer_size(mut self, size: usize) -> Self {
        self.row_channel_size = size;
        self
    }

    /// Sets the [`vfs`](https://www.sqlite.org/vfs.html) parameter of the database connection.
    ///
    /// The default value is empty, and sqlite will use the default VFS object dependeing on the
    /// operating system.
    pub fn vfs(mut self, vfs_name: impl Into<Cow<'static, str>>) -> Self {
        self.vfs = Some(vfs_name.into());
        self
    }
}
Repository: launchbadge/sqlx
File: sqlx-macros/src/database/mssql.rs (Rust)

Code before:
use sqlx_core as sqlx;

impl_database_ext! {
    sqlx::mssql::Mssql {
        bool,
        u8,
        i8,
        i16,
        i32,
        i64,
        f32,
        f64,
        String,
    },
    ParamChecking::Weak,
    feature-types: _info => None,
    row = sqlx::mssql::MssqlRow,
    name = "MSSQL"
}


Review comment (suggestion):
```suggestion
```

Listing this over `i8` is also a breaking change as it will cause any existing invocation of the query macros to switch from `i8` to `u8`. Since they're both considered compatible with the same types, listing it _after_ `i8` is pointless. We can fix the macros in the next major revision.


Code after:
use sqlx_core as sqlx;

impl_database_ext! {
    sqlx::mssql::Mssql {
        bool,
        i8,
        i16,
        i32,
        i64,
        f32,
        f64,
        String,
    },
    ParamChecking::Weak,
    feature-types: _info => None,
    row = sqlx::mssql::MssqlRow,
    name = "MSSQL"
}
Repository: launchbadge/sqlx
File: tests/sqlite/sqlite.rs (Rust)

Code before:
    let write = sqlx_core::rt::spawn({
        let mut conn = pool.acquire().await.unwrap();

        async move {
            for i in 0u32..n {
                sqlx::query("INSERT INTO kv (k, v) VALUES (?, ?)")
                    .bind(i)
                    .bind(i * i)
                    .execute(&mut *conn)
                    .await
                    .unwrap();
            }
        }
    });

    read.await;
    write.await;
}

#[sqlx_macros::test]
async fn test_query_with_progress_handler() -> anyhow::Result<()> {
    let mut conn = new::<Sqlite>().await?;
    conn.lock_handle()
        .await?
        .set_progress_handler(1, || false)
        .await;

    match sqlx::query("SELECT 'hello' AS title")
        .fetch_all(&mut conn)
        .await
    {
        Err(sqlx::Error::Database(err)) => assert_eq!(err.message(), String::from("interrupted")),
        _ => panic!("expected an interrupt"),
    }

    Ok(())
}


Review comment (bug):
**Please** include some sort of captured state in this closure so it tests correct allocation/deallocation of the callback, e.g.:

```suggestion
    // Using this string as a canary to ensure the callback doesn't get called with the wrong data pointer.
    let string = format!("Hello from the progress handler!");

    conn.lock_handle()
        .await?
        .set_progress_handler(1, move || { 
            println!("{}", string);
            false 
        });
```

Code after:

    let write = sqlx_core::rt::spawn({
        let mut conn = pool.acquire().await.unwrap();

        async move {
            for i in 0u32..n {
                sqlx::query("INSERT INTO kv (k, v) VALUES (?, ?)")
                    .bind(i)
                    .bind(i * i)
                    .execute(&mut *conn)
                    .await
                    .unwrap();
            }
        }
    });

    read.await;
    write.await;
}

#[sqlx_macros::test]
async fn test_query_with_progress_handler() -> anyhow::Result<()> {
    let mut conn = new::<Sqlite>().await?;

    // Using this string as a canary to ensure the callback doesn't get called with the wrong data pointer.
    let state = format!("test");
    conn.lock_handle().await?.set_progress_handler(1, move || {
        assert_eq!(state, "test");
        false
    });

    match sqlx::query("SELECT 'hello' AS title")
        .fetch_all(&mut conn)
        .await
    {
        Err(sqlx::Error::Database(err)) => assert_eq!(err.message(), String::from("interrupted")),
        _ => panic!("expected an interrupt"),
    }

    Ok(())
}

#[sqlx_macros::test]
async fn test_multiple_set_progress_handler_calls_drop_old_handler() -> anyhow::Result<()> {
    let ref_counted_object = Arc::new(0);
    assert_eq!(1, Arc::strong_count(&ref_counted_object));

    {
        let mut conn = new::<Sqlite>().await?;

        let o = ref_counted_object.clone();
Repository: launchbadge/sqlx
File: sqlx-core/src/from_row.rs (Rust)

Code before:
///     user_name: String,
///     street: String,
///     city: String,
/// }
///
/// #[derive(sqlx::FromRow)]
/// struct User {
///     name: String,
///     #[sqlx(skip)]
///     addresses: Vec<Address>,
/// }
/// ```
///
/// Given 2 querys
///
/// ```sql
/// SELECT name FROM users;
/// ```
///
/// and
///
/// ```sql
/// SELECT user_name, street, city addresses;
/// ```
///
/// the addresses can be assigned to the empty `addresses` field of each `User`.
///
/// ## Manual implementation
///
/// You can also implement the [`FromRow`] trait by hand. This can be useful if you
/// have a struct with a field that needs manual decoding:
///
///
/// ```rust,ignore
/// use sqlx::{FromRow, sqlite::SqliteRow, sqlx::Row};
/// struct MyCustomType {
///     custom: String,
/// }
///
/// struct Foo {
///     bar: MyCustomType,
/// }
///
/// impl FromRow<'_, SqliteRow> for Foo {
///     fn from_row(row: &SqliteRow) -> sqlx::Result<Self> {
///         Ok(Self {
///             bar: MyCustomType {
///                 custom: row.try_get("custom")?
///             }
///         })
///     }

Review comment (suggestion):
```suggestion
/// Then when querying into `User`, only `name` needs to be set:
///
/// ```rust,ignore
/// let user: User = sqlx::query_as("SELECT name FROM users")
///    .fetch_one(&mut some_connection)
///    .await?;
///
/// `Default` for `Vec<Address>` is an empty vector.
/// assert!(user.addresses.is_empty());
/// ```
```

Code after:
///     user_name: String,
///     street: String,
///     city: String,
/// }
///
/// #[derive(sqlx::FromRow)]
/// struct User {
///     name: String,
///     #[sqlx(skip)]
///     addresses: Vec<Address>,
/// }
/// ```
///
/// Then when querying into `User`, only `name` needs to be set:
///
/// ```rust,ignore
/// let user: User = sqlx::query_as("SELECT name FROM users")
///    .fetch_one(&mut some_connection)
///    .await?;
///
/// `Default` for `Vec<Address>` is an empty vector.
/// assert!(user.addresses.is_empty());
/// ```
///
/// ## Manual implementation
///
/// You can also implement the [`FromRow`] trait by hand. This can be useful if you
/// have a struct with a field that needs manual decoding:
///
///
/// ```rust,ignore
/// use sqlx::{FromRow, sqlite::SqliteRow, sqlx::Row};
/// struct MyCustomType {
///     custom: String,
/// }
///
/// struct Foo {
///     bar: MyCustomType,
/// }
///
/// impl FromRow<'_, SqliteRow> for Foo {
///     fn from_row(row: &SqliteRow) -> sqlx::Result<Self> {
///         Ok(Self {
///             bar: MyCustomType {
///                 custom: row.try_get("custom")?
///             }
///         })
///     }
/// }
/// ```
///
Repository: launchbadge/sqlx
File: sqlx-mysql/src/options/mod.rs (Rust)

Code before:

    /// Sets the collation for the connection.
    ///
    /// The default collation is derived from the `charset`. Normally, you should only have to set
    /// the `charset`.
    pub fn collation(mut self, collation: &str) -> Self {
        self.collation = Some(collation.to_owned());
        self
    }

    /// Sets the flag that enables or disables the `PIPES_AS_CONCAT` connection setting
    ///
    /// The default value is set to true, but some MySql databases such as PlanetScale
    /// error out with this connection setting so it needs to be set false in such
    /// cases.
    pub fn pipes_as_concat(mut self, flag_val: bool) -> Self {
        self.pipes_as_concat = flag_val;
        self
    }

    /// Enables mysql_clear_password plugin support.
    ///
    /// Security Note:
    /// Sending passwords as cleartext may be a security problem in some
    /// configurations. Please consider using TLS or encrypted tunnels for
    /// server connection.
    pub fn enable_cleartext_plugin(mut self, flag_val: bool) -> Self {
        self.enable_cleartext_plugin = flag_val;
        self
    }
}


Review comment (security):
I feel that this severely understates the security risk here.

It should at least recommend `.ssl_mode(MySqlSslMode::Required)`, or if the user is following the AWS guide then they should upgrade that to `VerifyCa` or `VerifyIdentity`.

Code after:

    /// Sets the collation for the connection.
    ///
    /// The default collation is derived from the `charset`. Normally, you should only have to set
    /// the `charset`.
    pub fn collation(mut self, collation: &str) -> Self {
        self.collation = Some(collation.to_owned());
        self
    }

    /// Sets the flag that enables or disables the `PIPES_AS_CONCAT` connection setting
    ///
    /// The default value is set to true, but some MySql databases such as PlanetScale
    /// error out with this connection setting so it needs to be set false in such
    /// cases.
    pub fn pipes_as_concat(mut self, flag_val: bool) -> Self {
        self.pipes_as_concat = flag_val;
        self
    }

    /// Enables mysql_clear_password plugin support.
    ///
    /// Security Note:
    /// Sending passwords as cleartext may be a security problem in some
    /// configurations. Without additional defensive configuration like
    /// ssl-mode=VERIFY_IDENTITY, an attacker can compromise a router
    /// and trick the application into divulging its credentials.
    ///
    /// It is strongly recommended to set `.ssl_mode` to `Required`,
    /// `VerifyCa`, or `VerifyIdentity` when enabling cleartext plugin.
    pub fn enable_cleartext_plugin(mut self, flag_val: bool) -> Self {
        self.enable_cleartext_plugin = flag_val;
        self
    }
}
Repository: launchbadge/sqlx
File: tests/postgres/test-attr.rs (Rust)

Code before:
        sqlx::query_scalar(r#"SELECT username FROM "user" ORDER BY username"#)
            .fetch_all(&pool)
            .await?;

    assert_eq!(usernames, ["alice", "bob"]);

    let post_exists: bool = sqlx::query_scalar("SELECT exists(SELECT 1 FROM post)")
        .fetch_one(&pool)
        .await?;

    assert!(!post_exists);

    let comment_exists: bool = sqlx::query_scalar("SELECT exists(SELECT 1 FROM comment)")
        .fetch_one(&pool)
        .await?;

    assert!(!comment_exists);

    Ok(())
}

// This should apply migrations and then `../fixtures-postgres/users.sql` and `../fixtures-postgres/posts.sql`
#[sqlx::test(
    migrations = "tests/postgres/migrations",
    fixtures_path("../fixtures-postgres/users", "../fixtures-postgres/posts")
)]
async fn it_gets_posts(pool: PgPool) -> sqlx::Result<()> {
    let post_contents: Vec<String> =
        sqlx::query_scalar("SELECT content FROM post ORDER BY created_at")
            .fetch_all(&pool)
            .await?;

    assert_eq!(
        post_contents,
        [
            "This new computer is lightning-fast!",
            "@alice is a haxxor :("
        ]
    );

    let comment_exists: bool = sqlx::query_scalar("SELECT exists(SELECT 1 FROM comment)")
        .fetch_one(&pool)
        .await?;

    assert!(!comment_exists);

    Ok(())
}

// Try `migrator`
#[sqlx::test(migrator = "MIGRATOR", fixtures("users", "posts", "comments"))]

Review comment (suggestion):
I would really prefer if you use a new test instead of co-opting an existing one. Even just making a copy of this one would be fine.

Code after:
        sqlx::query_scalar(r#"SELECT username FROM "user" ORDER BY username"#)
            .fetch_all(&pool)
            .await?;

    assert_eq!(usernames, ["alice", "bob"]);

    let post_exists: bool = sqlx::query_scalar("SELECT exists(SELECT 1 FROM post)")
        .fetch_one(&pool)
        .await?;

    assert!(!post_exists);

    let comment_exists: bool = sqlx::query_scalar("SELECT exists(SELECT 1 FROM comment)")
        .fetch_one(&pool)
        .await?;

    assert!(!comment_exists);

    Ok(())
}

// This should apply migrations and then fixtures `fixtures/users.sql` and `fixtures/posts.sql`
#[sqlx::test(migrations = "tests/postgres/migrations", fixtures("users", "posts"))]
async fn it_gets_posts(pool: PgPool) -> sqlx::Result<()> {
    let post_contents: Vec<String> =
        sqlx::query_scalar("SELECT content FROM post ORDER BY created_at")
            .fetch_all(&pool)
            .await?;

    assert_eq!(
        post_contents,
        [
            "This new computer is lightning-fast!",
            "@alice is a haxxor :("
        ]
    );

    let comment_exists: bool = sqlx::query_scalar("SELECT exists(SELECT 1 FROM comment)")
        .fetch_one(&pool)
        .await?;

    assert!(!comment_exists);

    Ok(())
}

// This should apply migrations and then `../fixtures/postgres/users.sql` and `../fixtures/postgres/posts.sql`
#[sqlx::test(
    migrations = "tests/postgres/migrations",
    fixtures("../fixtures/postgres/users.sql", "../fixtures/postgres/posts.sql")
)]
Repository: launchbadge/sqlx
File: sqlx-sqlite/src/types/chrono.rs (Rust)

Code before:
use crate::value::ValueRef;
use crate::{
    decode::Decode,
    encode::{Encode, IsNull},
    error::BoxDynError,
    type_info::DataType,
    types::Type,
    Sqlite, SqliteArgumentValue, SqliteTypeInfo, SqliteValueRef,
};
use bitflags::_core::fmt::Display;
use chrono::FixedOffset;
use chrono::{
    DateTime, Local, NaiveDate, NaiveDateTime, NaiveTime, Offset, SecondsFormat, TimeZone, Utc,
};

impl<Tz: TimeZone> Type<Sqlite> for DateTime<Tz> {
    fn type_info() -> SqliteTypeInfo {
        SqliteTypeInfo(DataType::Datetime)
    }

    fn compatible(ty: &SqliteTypeInfo) -> bool {
        <NaiveDateTime as Type<Sqlite>>::compatible(ty)
    }
}

impl Type<Sqlite> for NaiveDateTime {
    fn type_info() -> SqliteTypeInfo {
        SqliteTypeInfo(DataType::Datetime)
    }

    fn compatible(ty: &SqliteTypeInfo) -> bool {
        matches!(
            ty.0,
            DataType::Datetime | DataType::Text | DataType::Int64 | DataType::Int | DataType::Float
        )

Review comment (suggestion):
Nice catch. For a long while, IntelliJ Rust had a bad habit of using this over the `std` path when auto-importing.

Code after:
use std::fmt::Display;

use crate::value::ValueRef;
use crate::{
    decode::Decode,
    encode::{Encode, IsNull},
    error::BoxDynError,
    type_info::DataType,
    types::Type,
    Sqlite, SqliteArgumentValue, SqliteTypeInfo, SqliteValueRef,
};
use chrono::FixedOffset;
use chrono::{
    DateTime, Local, NaiveDate, NaiveDateTime, NaiveTime, Offset, SecondsFormat, TimeZone, Utc,
};

impl<Tz: TimeZone> Type<Sqlite> for DateTime<Tz> {
    fn type_info() -> SqliteTypeInfo {
        SqliteTypeInfo(DataType::Datetime)
    }

    fn compatible(ty: &SqliteTypeInfo) -> bool {
        <NaiveDateTime as Type<Sqlite>>::compatible(ty)
    }
}

impl Type<Sqlite> for NaiveDateTime {
    fn type_info() -> SqliteTypeInfo {
        SqliteTypeInfo(DataType::Datetime)
    }

    fn compatible(ty: &SqliteTypeInfo) -> bool {
        matches!(
            ty.0,
            DataType::Datetime | DataType::Text | DataType::Int64 | DataType::Int | DataType::Float
Repository: launchbadge/sqlx
File: examples/postgres/chat/src/main.rs (Rust)

Code before:
    let mut stdout = io::stdout();
    execute!(stdout, EnterAlternateScreen, EnableMouseCapture)?;
    let backend = CrosstermBackend::new(stdout);
    let mut terminal = Terminal::new(backend)?;

    // create app and run it
    let app = ChatApp::new(pool);
    let res = app.run(&mut terminal, listener).await;

    // restore terminal
    disable_raw_mode()?;
    execute!(
        terminal.backend_mut(),
        LeaveAlternateScreen,
        DisableMouseCapture,
    )?;
    terminal.show_cursor()?;

    if let Err(err) = res {
        println!("{:?}", err)
    }

    Ok(())
}

async fn notify(pool: &PgPool, s: String) -> Result<(), sqlx::Error> {
    sqlx::query(
        r#"
SELECT pg_notify(chan, payload)
FROM (VALUES ('chan0', $1)) v(chan, payload)
"#,
    )
    .bind(s)
    .execute(pool)
    .await?;

    Ok(())
}


Review comment (suggestion):
```suggestion
async fn notify(pool: &PgPool, s: &str) -> Result<(), sqlx::Error> {
```

Code after:
    enable_raw_mode()?;
    let mut stdout = io::stdout();
    execute!(stdout, EnterAlternateScreen, EnableMouseCapture)?;
    let backend = CrosstermBackend::new(stdout);
    let mut terminal = Terminal::new(backend)?;

    // create app and run it
    let app = ChatApp::new(pool);
    let res = app.run(&mut terminal, listener).await;

    // restore terminal
    disable_raw_mode()?;
    execute!(
        terminal.backend_mut(),
        LeaveAlternateScreen,
        DisableMouseCapture,
    )?;
    terminal.show_cursor()?;

    if let Err(err) = res {
        println!("{:?}", err)
    }

    Ok(())
}

async fn notify(pool: &PgPool, s: &str) -> Result<(), sqlx::Error> {
    sqlx::query(
        r#"
SELECT pg_notify(chan, payload)
FROM (VALUES ('chan0', $1)) v(chan, payload)
"#,
    )
    .bind(s)
    .execute(pool)
    .await?;

    Ok(())
}