ronantakizawa/codereview-bench huggingface dataset:
Repository: launchbadge/sqlx
File: sqlx-postgres/src/column.rs (Rust)

Code before:
use crate::ext::ustr::UStr;
use crate::{PgTypeInfo, Postgres};

pub(crate) use sqlx_core::column::{Column, ColumnIndex};

#[derive(Debug, Clone)]
#[cfg_attr(feature = "offline", derive(serde::Serialize, serde::Deserialize))]
pub struct PgColumn {
    pub(crate) ordinal: usize,
    pub(crate) name: UStr,
    pub(crate) type_info: PgTypeInfo,
    #[cfg_attr(feature = "offline", serde(skip))]
    pub(crate) relation_id: Option<crate::types::Oid>,
    #[cfg_attr(feature = "offline", serde(skip))]
    pub(crate) relation_attribute_no: Option<i16>,
}

impl PgColumn {
    pub fn relation_id(&self) -> Option<u32> {
        self.relation_id.map(|oid| oid.0)
    }

    pub fn relation_attribute_no(&self) -> Option<i16> {
        self.relation_attribute_no
    }
}

impl Column for PgColumn {
    type Database = Postgres;

    fn ordinal(&self) -> usize {
        self.ordinal
    }

    fn name(&self) -> &str {
        &self.name
    }

    fn type_info(&self) -> &PgTypeInfo {
        &self.type_info
    }
}


Review comment (suggestion):
```suggestion
    /// Returns the OID of the table this column is from, if applicable.
    ///
    /// This will be `None` if the column is the result of an expression.
    ///
    /// Corresponds to column `attrelid` of the `pg_catalog.pg_attribute` table:
    /// <https://www.postgresql.org/docs/current/catalog-pg-attribute.html>
    pub fn relation_id(&self) -> Option<Oid> {
```

Code after:
use crate::ext::ustr::UStr;
use crate::{PgTypeInfo, Postgres};

pub(crate) use sqlx_core::column::{Column, ColumnIndex};

#[derive(Debug, Clone)]
#[cfg_attr(feature = "offline", derive(serde::Serialize, serde::Deserialize))]
pub struct PgColumn {
    pub(crate) ordinal: usize,
    pub(crate) name: UStr,
    pub(crate) type_info: PgTypeInfo,
    #[cfg_attr(feature = "offline", serde(skip))]
    pub(crate) relation_id: Option<crate::types::Oid>,
    #[cfg_attr(feature = "offline", serde(skip))]
    pub(crate) relation_attribute_no: Option<i16>,
}

impl PgColumn {
    /// Returns the OID of the table this column is from, if applicable.
    ///
    /// This will be `None` if the column is the result of an expression.
    ///
    /// Corresponds to column `attrelid` of the `pg_catalog.pg_attribute` table:
    /// <https://www.postgresql.org/docs/current/catalog-pg-attribute.html>
    pub fn relation_id(&self) -> Option<crate::types::Oid> {
        self.relation_id
    }

    /// Returns the 1-based index of this column in its parent table, if applicable.
    ///
    /// This will be `None` if the column is the result of an expression.
    ///
    /// Corresponds to column `attnum` of the `pg_catalog.pg_attribute` table:
    /// <https://www.postgresql.org/docs/current/catalog-pg-attribute.html>
    pub fn relation_attribute_no(&self) -> Option<i16> {
        self.relation_attribute_no
    }
}

impl Column for PgColumn {
    type Database = Postgres;

    fn ordinal(&self) -> usize {
        self.ordinal
Repository: launchbadge/sqlx
File: sqlx-postgres/src/message/row_description.rs (Rust)

Code before:
            ));
        }

        let cnt = buf.get_u16();
        let mut fields = Vec::with_capacity(cnt as usize);

        for _ in 0..cnt {
            let name = buf.get_str_nul()?.to_owned();

            if buf.len() < 18 {
                return Err(err_protocol!(
                    "expected at least 18 bytes after field name {name:?}, got {}",
                    buf.len()
                ));
            }

            let relation_id = Oid(buf.get_u32());
            let relation_attribute_no = buf.get_i16();
            let data_type_id = Oid(buf.get_u32());
            let data_type_size = buf.get_i16();
            let type_modifier = buf.get_i32();
            let format = buf.get_i16();

            fields.push(Field {
                name,
                relation_id: if relation_id.0 == 0 {
                    None
                } else {
                    Some(relation_id)
                },
                relation_attribute_no: if relation_attribute_no == 0 {
                    None
                } else {
                    Some(relation_attribute_no)
                },
                data_type_id,
                data_type_size,
                type_modifier,
                format,
            })
        }

        Ok(Self { fields })
    }
}

// TODO: Unit Test RowDescription
// TODO: Benchmark RowDescription


Review comment (suggestion):
```suggestion
                relation_id: if relation_id == 0 {
```

Code after:
            ));
        }

        let cnt = buf.get_u16();
        let mut fields = Vec::with_capacity(cnt as usize);

        for _ in 0..cnt {
            let name = buf.get_str_nul()?.to_owned();

            if buf.len() < 18 {
                return Err(err_protocol!(
                    "expected at least 18 bytes after field name {name:?}, got {}",
                    buf.len()
                ));
            }

            let relation_id = buf.get_u32();
            let relation_attribute_no = buf.get_i16();
            let data_type_id = Oid(buf.get_u32());
            let data_type_size = buf.get_i16();
            let type_modifier = buf.get_i32();
            let format = buf.get_i16();

            fields.push(Field {
                name,
                relation_id: if relation_id == 0 {
                    None
                } else {
                    Some(Oid(relation_id))
                },
                relation_attribute_no: if relation_attribute_no == 0 {
                    None
                } else {
                    Some(relation_attribute_no)
                },
                data_type_id,
                data_type_size,
                type_modifier,
                format,
            })
        }

        Ok(Self { fields })
    }
}

// TODO: Unit Test RowDescription
// TODO: Benchmark RowDescription
Repository: launchbadge/sqlx
File: sqlx-postgres/src/message/row_description.rs (Rust)

Code before:
        let cnt = buf.get_u16();
        let mut fields = Vec::with_capacity(cnt as usize);

        for _ in 0..cnt {
            let name = buf.get_str_nul()?.to_owned();

            if buf.len() < 18 {
                return Err(err_protocol!(
                    "expected at least 18 bytes after field name {name:?}, got {}",
                    buf.len()
                ));
            }

            let relation_id = Oid(buf.get_u32());
            let relation_attribute_no = buf.get_i16();
            let data_type_id = Oid(buf.get_u32());
            let data_type_size = buf.get_i16();
            let type_modifier = buf.get_i32();
            let format = buf.get_i16();

            fields.push(Field {
                name,
                relation_id: if relation_id.0 == 0 {
                    None
                } else {
                    Some(relation_id)
                },
                relation_attribute_no: if relation_attribute_no == 0 {
                    None
                } else {
                    Some(relation_attribute_no)
                },
                data_type_id,
                data_type_size,
                type_modifier,
                format,
            })
        }

        Ok(Self { fields })
    }
}

// TODO: Unit Test RowDescription
// TODO: Benchmark RowDescription


Review comment (suggestion):
```suggestion
                    Some(Oid(relation_id))
```

Code after:
        let cnt = buf.get_u16();
        let mut fields = Vec::with_capacity(cnt as usize);

        for _ in 0..cnt {
            let name = buf.get_str_nul()?.to_owned();

            if buf.len() < 18 {
                return Err(err_protocol!(
                    "expected at least 18 bytes after field name {name:?}, got {}",
                    buf.len()
                ));
            }

            let relation_id = buf.get_u32();
            let relation_attribute_no = buf.get_i16();
            let data_type_id = Oid(buf.get_u32());
            let data_type_size = buf.get_i16();
            let type_modifier = buf.get_i32();
            let format = buf.get_i16();

            fields.push(Field {
                name,
                relation_id: if relation_id == 0 {
                    None
                } else {
                    Some(Oid(relation_id))
                },
                relation_attribute_no: if relation_attribute_no == 0 {
                    None
                } else {
                    Some(relation_attribute_no)
                },
                data_type_id,
                data_type_size,
                type_modifier,
                format,
            })
        }

        Ok(Self { fields })
    }
}

// TODO: Unit Test RowDescription
// TODO: Benchmark RowDescription
Repository: launchbadge/sqlx
File: sqlx-postgres/src/column.rs (Rust)

Code before:
    pub(crate) name: UStr,
    pub(crate) type_info: PgTypeInfo,
    #[cfg_attr(feature = "offline", serde(skip))]
    pub(crate) relation_id: Option<crate::types::Oid>,
    #[cfg_attr(feature = "offline", serde(skip))]
    pub(crate) relation_attribute_no: Option<i16>,
}

impl PgColumn {
    pub fn relation_id(&self) -> Option<u32> {
        self.relation_id.map(|oid| oid.0)
    }

    pub fn relation_attribute_no(&self) -> Option<i16> {
        self.relation_attribute_no
    }
}

impl Column for PgColumn {
    type Database = Postgres;

    fn ordinal(&self) -> usize {
        self.ordinal
    }

    fn name(&self) -> &str {
        &self.name
    }

    fn type_info(&self) -> &PgTypeInfo {
        &self.type_info
    }
}


Review comment (suggestion):
```suggestion
    /// Returns the 1-based index of this column in its parent table, if applicable.
    ///
    /// This will be `None` if the column is the result of an expression.
    ///
    /// Corresponds to column `attnum` of the `pg_catalog.pg_attribute` table:
    /// <https://www.postgresql.org/docs/current/catalog-pg-attribute.html>
    pub fn relation_attribute_no(&self) -> Option<i16> {
```

Code after:
    pub(crate) name: UStr,
    pub(crate) type_info: PgTypeInfo,
    #[cfg_attr(feature = "offline", serde(skip))]
    pub(crate) relation_id: Option<crate::types::Oid>,
    #[cfg_attr(feature = "offline", serde(skip))]
    pub(crate) relation_attribute_no: Option<i16>,
}

impl PgColumn {
    /// Returns the OID of the table this column is from, if applicable.
    ///
    /// This will be `None` if the column is the result of an expression.
    ///
    /// Corresponds to column `attrelid` of the `pg_catalog.pg_attribute` table:
    /// <https://www.postgresql.org/docs/current/catalog-pg-attribute.html>
    pub fn relation_id(&self) -> Option<crate::types::Oid> {
        self.relation_id
    }

    /// Returns the 1-based index of this column in its parent table, if applicable.
    ///
    /// This will be `None` if the column is the result of an expression.
    ///
    /// Corresponds to column `attnum` of the `pg_catalog.pg_attribute` table:
    /// <https://www.postgresql.org/docs/current/catalog-pg-attribute.html>
    pub fn relation_attribute_no(&self) -> Option<i16> {
        self.relation_attribute_no
    }
}

impl Column for PgColumn {
    type Database = Postgres;

    fn ordinal(&self) -> usize {
        self.ordinal
    }

    fn name(&self) -> &str {
        &self.name
    }

    fn type_info(&self) -> &PgTypeInfo {
        &self.type_info
    }
}
Repository: launchbadge/sqlx
File: sqlx-postgres/src/options/pgpass.rs (Rust)

Code before:
    }

    #[cfg(not(target_os = "windows"))]
    let default_file = home::home_dir().map(|path| path.join(".pgpass"));
    #[cfg(target_os = "windows")]
    let default_file = {
        use etcetera::BaseStrategy;

        etcetera::base_strategy::Windows::new()
            .ok()
            .map(|basedirs| basedirs.data_dir().join("postgres").join("pgpass.conf"))
    };
    load_password_from_file(default_file?, host, port, username, database)
}

/// try to extract a password from a pgpass file
fn load_password_from_file(
    path: PathBuf,
    host: &str,
    port: u16,
    username: &str,
    database: Option<&str>,
) -> Option<String> {
    let file = File::open(&path)
        .map_err(|e| {
            tracing::debug!(
                path = %path.display(),
                "Failed to open `.pgpass` file: {e:?}",
            );
        })
        .ok()?;

    #[cfg(target_os = "linux")]
    {
        use std::os::unix::fs::PermissionsExt;

        // check file permissions on linux

        let metadata = file.metadata().ok()?;
        let permissions = metadata.permissions();
        let mode = permissions.mode();
        if mode & 0o77 != 0 {
            tracing::warn!(
                path = %path.display(),
                permissions = format!("{mode:o}"),
                "Ignoring path. Permissions are not strict enough",
            );
            return None;
        }
    }


Review comment (bug):
This should only be downgraded to `DEBUG` if the error kind is `NotFound`. If the file exists but we were unable to read it, the user likely wants to know about that.

Code after:
    }

    #[cfg(not(target_os = "windows"))]
    let default_file = home::home_dir().map(|path| path.join(".pgpass"));
    #[cfg(target_os = "windows")]
    let default_file = {
        use etcetera::BaseStrategy;

        etcetera::base_strategy::Windows::new()
            .ok()
            .map(|basedirs| basedirs.data_dir().join("postgres").join("pgpass.conf"))
    };
    load_password_from_file(default_file?, host, port, username, database)
}

/// try to extract a password from a pgpass file
fn load_password_from_file(
    path: PathBuf,
    host: &str,
    port: u16,
    username: &str,
    database: Option<&str>,
) -> Option<String> {
    let file = File::open(&path)
        .map_err(|e| {
            match e.kind() {
                std::io::ErrorKind::NotFound => {
                    tracing::debug!(
                        path = %path.display(),
                        "`.pgpass` file not found",
                    );
                }
                _ => {
                    tracing::warn!(
                        path = %path.display(),
                        "Failed to open `.pgpass` file: {e:?}",
                    );
                }
            };
        })
        .ok()?;

    #[cfg(target_os = "linux")]
    {
        use std::os::unix::fs::PermissionsExt;

        // check file permissions on linux

        let metadata = file.metadata().ok()?;
        let permissions = metadata.permissions();
        let mode = permissions.mode();
Repository: launchbadge/sqlx
File: sqlx-mysql/src/any.rs (Rust)

Code before:
use crate::protocol::text::ColumnType;
use crate::{
    MySql, MySqlColumn, MySqlConnectOptions, MySqlConnection, MySqlQueryResult, MySqlRow,
    MySqlTransactionManager, MySqlTypeInfo,
};
use either::Either;
use futures_core::future::BoxFuture;
use futures_core::stream::BoxStream;
use futures_util::{stream, StreamExt, TryFutureExt, TryStreamExt};
pub use sqlx_core::any::{
    Any, AnyArguments, AnyColumn, AnyConnectOptions, AnyConnectionBackend, AnyQueryResult, AnyRow,
    AnyStatement, AnyTypeInfo, AnyTypeInfoKind,
};
use sqlx_core::connection::Connection;
use sqlx_core::database::Database;
use sqlx_core::describe::Describe;
use sqlx_core::executor::Executor;
use sqlx_core::transaction::TransactionManager;
use std::future;

sqlx_core::declare_driver_with_optional_migrate!(DRIVER = MySql);

impl AnyConnectionBackend for MySqlConnection {
    fn name(&self) -> &str {
        <MySql as Database>::NAME
    }

    fn close(self: Box<Self>) -> BoxFuture<'static, sqlx_core::Result<()>> {
        Connection::close(*self)
    }

    fn close_hard(self: Box<Self>) -> BoxFuture<'static, sqlx_core::Result<()>> {
        Connection::close_hard(*self)
    }


Review comment (bug):
`pub use` here causes IDEs to suggest the wrong imports: https://github.com/launchbadge/sqlx/pull/3254#issuecomment-2144043823

The fix there was to mark `sqlx_postgres::any` as `#[doc(hidden)]` but in hindsight I think we should have just re-scoped the exports.

`pub(crate) use sqlx_core::any::*;` should be fine. 

Alternatively, just import directly from `sqlx_core::any` in the other files. The blanket re-export was only a monkey patch to get the code compiling after [extracting the driver crates from sqlx-core](https://github.com/launchbadge/sqlx/pull/2039).



Code after:
use crate::protocol::text::ColumnType;
use crate::{
    MySql, MySqlColumn, MySqlConnectOptions, MySqlConnection, MySqlQueryResult, MySqlRow,
    MySqlTransactionManager, MySqlTypeInfo,
};
use either::Either;
use futures_core::future::BoxFuture;
use futures_core::stream::BoxStream;
use futures_util::{stream, StreamExt, TryFutureExt, TryStreamExt};
use sqlx_core::any::{
    Any, AnyArguments, AnyColumn, AnyConnectOptions, AnyConnectionBackend, AnyQueryResult, AnyRow,
    AnyStatement, AnyTypeInfo, AnyTypeInfoKind,
};
use sqlx_core::connection::Connection;
use sqlx_core::database::Database;
use sqlx_core::describe::Describe;
use sqlx_core::executor::Executor;
use sqlx_core::transaction::TransactionManager;
use std::future;

sqlx_core::declare_driver_with_optional_migrate!(DRIVER = MySql);

impl AnyConnectionBackend for MySqlConnection {
    fn name(&self) -> &str {
        <MySql as Database>::NAME
    }

    fn close(self: Box<Self>) -> BoxFuture<'static, sqlx_core::Result<()>> {
        Connection::close(*self)
    }

    fn close_hard(self: Box<Self>) -> BoxFuture<'static, sqlx_core::Result<()>> {
        Connection::close_hard(*self)
    }
Repository: launchbadge/sqlx
File: sqlx-mysql/src/query_result.rs (Rust)

Code before:
impl MySqlQueryResult {
    pub fn last_insert_id(&self) -> u64 {
        self.last_insert_id
    }

    pub fn rows_affected(&self) -> u64 {
        self.rows_affected
    }
}

impl Extend<MySqlQueryResult> for MySqlQueryResult {
    fn extend<T: IntoIterator<Item = MySqlQueryResult>>(&mut self, iter: T) {
        for elem in iter {
            self.rows_affected += elem.rows_affected;
            self.last_insert_id = elem.last_insert_id;
        }
    }
}

#[cfg(feature = "any")]
/// This conversion lose the last insert id data.
impl From<MySqlQueryResult> for crate::any::AnyQueryResult {
    fn from(done: MySqlQueryResult) -> Self {
        crate::any::AnyQueryResult {
            rows_affected: done.rows_affected(),
            last_insert_id: None,
        }
    }
}


Review comment (suggestion):
Instead of throwing away `last_insert_id`, use `.try_into().ok()` to convert to `i64` if not out-of-bounds.

Code after:
impl MySqlQueryResult {
    pub fn last_insert_id(&self) -> u64 {
        self.last_insert_id
    }

    pub fn rows_affected(&self) -> u64 {
        self.rows_affected
    }
}

impl Extend<MySqlQueryResult> for MySqlQueryResult {
    fn extend<T: IntoIterator<Item = MySqlQueryResult>>(&mut self, iter: T) {
        for elem in iter {
            self.rows_affected += elem.rows_affected;
            self.last_insert_id = elem.last_insert_id;
        }
    }
}
#[cfg(feature = "any")]
/// This conversion attempts to save last_insert_id by converting to i64.
impl From<MySqlQueryResult> for sqlx_core::any::AnyQueryResult {
    fn from(done: MySqlQueryResult) -> Self {
        sqlx_core::any::AnyQueryResult {
            rows_affected: done.rows_affected(),
            last_insert_id: done.last_insert_id().try_into().ok(),
        }
    }
}
Repository: launchbadge/sqlx
File: sqlx-sqlite/src/query_result.rs (Rust)

Code before:

impl SqliteQueryResult {
    pub fn rows_affected(&self) -> u64 {
        self.changes
    }

    pub fn last_insert_rowid(&self) -> i64 {
        self.last_insert_rowid
    }
}

impl Extend<SqliteQueryResult> for SqliteQueryResult {
    fn extend<T: IntoIterator<Item = SqliteQueryResult>>(&mut self, iter: T) {
        for elem in iter {
            self.changes += elem.changes;
            self.last_insert_rowid = elem.last_insert_rowid;
        }
    }
}

#[cfg(feature = "any")]
impl From<SqliteQueryResult> for crate::any::AnyQueryResult {
    fn from(done: SqliteQueryResult) -> Self {
        crate::any::AnyQueryResult {
            rows_affected: done.rows_affected(),
            last_insert_id: Some(done.last_insert_rowid()),
        }
    }
}


Review comment (suggestion):
`last_insert_rowid()` is 0 if no qualifying `INSERT`s have occurred on the database connection: https://www.sqlite.org/c3ref/last_insert_rowid.html

I'd map 0 to `None` here.

We could have surfaced this better as something like `Option<NonZeroI64>` but hindsight is 20/20.

Code after:

impl SqliteQueryResult {
    pub fn rows_affected(&self) -> u64 {
        self.changes
    }

    pub fn last_insert_rowid(&self) -> i64 {
        self.last_insert_rowid
    }
}

impl Extend<SqliteQueryResult> for SqliteQueryResult {
    fn extend<T: IntoIterator<Item = SqliteQueryResult>>(&mut self, iter: T) {
        for elem in iter {
            self.changes += elem.changes;
            self.last_insert_rowid = elem.last_insert_rowid;
        }
    }
}

#[cfg(feature = "any")]
impl From<SqliteQueryResult> for sqlx_core::any::AnyQueryResult {
    fn from(done: SqliteQueryResult) -> Self {
        let last_insert_id = match done.last_insert_rowid() {
            0 => None,
            n => Some(n),
        };
        sqlx_core::any::AnyQueryResult {
            rows_affected: done.rows_affected(),
            last_insert_id,
        }
    }
}