ronantakizawa/codereview-bench huggingface dataset:
Repository: bottlerocket-os/bottlerocket
File: sources/settings-extensions/oci-hooks/src/lib.rs (Rust)

Code before:
type Result<T> = std::result::Result<T, Infallible>;

impl SettingsModel for OciHooksSettingsV1 {
    type PartialKind = Self;
    type ErrorKind = Infallible;

    fn get_version() -> &'static str {
        "v1"
    }

    fn set(_current_value: Option<Self>, _target: Self) -> Result<()> {
        // Set anything that parses as OciHooksSettingsV1.
        Ok(())
    }

    fn generate(
        existing_partial: Option<Self::PartialKind>,
        _dependent_settings: Option<serde_json::Value>,
    ) -> Result<GenerateResult<Self::PartialKind, Self>> {
        Ok(GenerateResult::Complete(
            existing_partial.unwrap_or_default(),
        ))
    }

    fn validate(_value: Self, _validated_settings: Option<serde_json::Value>) -> Result<()> {
        // Validate anything that parses as OciHooksSettingsV1.
        Ok(())
    }
}

#[cfg(test)]
mod test {
    use super::*;

    #[test]
    fn test_generate_oci_hooks() {
        assert_eq!(
            OciHooksSettingsV1::generate(None, None).unwrap(),
            GenerateResult::Complete(OciHooksSettingsV1 {
                log4j_hotpatch_enabled: None,
            })
        )
    }

    #[test]
    fn test_serde_oci_hooks() {
        let test_json = r#"{"log4j-hotpatch-enabled":true}"#;

        let oci_hooks: OciHooksSettingsV1 = serde_json::from_str(test_json).unwrap();
        assert_eq!(
            oci_hooks,

Review comment (suggestion):
I know this has been copied from another settings extension, but I think it could be more clear.

```suggestion
        // OciHooksSettingsV1 is validated during deserialization.
```

Code after:
}

type Result<T> = std::result::Result<T, Infallible>;

impl SettingsModel for OciHooksSettingsV1 {
    type PartialKind = Self;
    type ErrorKind = Infallible;

    fn get_version() -> &'static str {
        "v1"
    }

    fn set(_current_value: Option<Self>, _target: Self) -> Result<()> {
        // Set anything that can be parsed as OciHooksSettingsV1.
        Ok(())
    }

    fn generate(
        existing_partial: Option<Self::PartialKind>,
        _dependent_settings: Option<serde_json::Value>,
    ) -> Result<GenerateResult<Self::PartialKind, Self>> {
        Ok(GenerateResult::Complete(
            existing_partial.unwrap_or_default(),
        ))
    }

    fn validate(_value: Self, _validated_settings: Option<serde_json::Value>) -> Result<()> {
        // OciHooksSettingsV1 is validated during deserialization.
        Ok(())
    }
}

#[cfg(test)]
mod test {
    use super::*;

    #[test]
    fn test_generate_oci_hooks() {
        assert_eq!(
            OciHooksSettingsV1::generate(None, None).unwrap(),
            GenerateResult::Complete(OciHooksSettingsV1 {
                log4j_hotpatch_enabled: None,
            })
        )
    }

    #[test]
    fn test_serde_oci_hooks() {
        let test_json = r#"{"log4j-hotpatch-enabled":true}"#;

        let oci_hooks: OciHooksSettingsV1 = serde_json::from_str(test_json).unwrap();
Repository: bottlerocket-os/bottlerocket
File: sources/api/apiclient/src/main.rs (Rust)

Code before:
    tty: Option<bool>,
}

/// Stores user-supplied arguments for the 'get' subcommand.
#[derive(Debug)]
enum GetArgs {
    Prefixes(Vec<String>),
    Uri(String),
}

/// Stores user-supplied arguments for the 'raw' subcommand.
#[derive(Debug)]
struct RawArgs {
    method: String,
    uri: String,
    data: Option<String>,
}

/// Stores user-supplied arguments for the 'reboot' subcommand.
#[derive(Debug)]
struct RebootArgs {}

/// Stores a vector of user-supplied key-value pairs for the 'set' subcommand.
#[derive(Serialize, Deserialize)]
pub struct SetKeyPairSettings {
    data: Vec<String>,
}
/// Stores user-supplied arguments for the 'set' subcommand.
#[derive(Debug)]
enum SetArgs {
    Simple(Vec<String>),
    Json(serde_json::Value),
}

/// Stores the 'update' subcommand specified by the user.
#[derive(Debug)]
enum UpdateSubcommand {
    Check(UpdateCheckArgs),
    Apply(UpdateApplyArgs),
    Cancel(UpdateCancelArgs),
}

/// The available 'report' subcommands.
#[derive(Debug)]
enum ReportSubcommand {
    Cis(CisReportArgs),
    CisK8s(CisReportArgs),
    Fips(FipsReportArgs),
}

/// Stores common user-supplied arguments for the cis report subcommand.

Review comment (suggestion):
nit: maybe something less generic? (this admittedly isn't much better)
```suggestion
    request_payload: Vec<String>,
```

Code after:
    tty: Option<bool>,
}

/// Stores user-supplied arguments for the 'get' subcommand.
#[derive(Debug)]
enum GetArgs {
    Prefixes(Vec<String>),
    Uri(String),
}

/// Stores user-supplied arguments for the 'raw' subcommand.
#[derive(Debug)]
struct RawArgs {
    method: String,
    uri: String,
    data: Option<String>,
}

/// Stores user-supplied arguments for the 'reboot' subcommand.
#[derive(Debug)]
struct RebootArgs {}

/// Stores a vector of user-supplied key-value pairs for the 'set' subcommand.
#[derive(Serialize, Deserialize)]
pub struct SetKeyPairSettings {
    request_payload: Vec<String>,
}

/// Stores user-supplied arguments for the 'set' subcommand.
#[derive(Debug)]
enum SetArgs {
    Simple(Vec<String>),
    Json(serde_json::Value),
}

/// Stores the 'update' subcommand specified by the user.
#[derive(Debug)]
enum UpdateSubcommand {
    Check(UpdateCheckArgs),
    Apply(UpdateApplyArgs),
    Cancel(UpdateCancelArgs),
}

/// The available 'report' subcommands.
#[derive(Debug)]
enum ReportSubcommand {
    Cis(CisReportArgs),
    CisK8s(CisReportArgs),
    Fips(FipsReportArgs),
}
Repository: bottlerocket-os/bottlerocket
File: sources/api/apiclient/src/set.rs (Rust)

Code before:
    let (_status, _body) = crate::raw_request(&socket_path, &uri, method, Some(settings))
        .await
        .context(error::RequestSnafu { uri, method })?;

    // Commit the transaction and apply it to the system.
    let uri = format!("/tx/commit_and_apply?tx={}", transaction);
    let method = "POST";
    let (_status, _body) = crate::raw_request(&socket_path, &uri, method, None)
        .await
        .context(error::RequestSnafu { uri, method })?;

    Ok(())
}

/// Gets the URI based on what type of data is being passed.
/// If its of type keypair, we send it to /settings/keypair
/// If its of type json, we send it to /settings.
/// See the API model for information about these endpoints.
fn get_uri(settings: &str, transaction: &str) -> String {
    if let Ok(json_value) = serde_json::from_str::<Value>(settings) {
        if let Some(_value) = json_value.get("data") {
            return format!("/settings/keypair?tx={}", transaction);
        }
    }
    format!("/settings?tx={}", transaction)
}

mod error {
    use snafu::Snafu;

    #[derive(Debug, Snafu)]
    #[snafu(visibility(pub(super)))]
    pub enum Error {
        #[snafu(display("Unable to serialize data: {}", source))]
        Serialize { source: serde_json::Error },

        #[snafu(display("Failed {} request to '{}': {}", method, uri, source))]
        Request {
            method: String,
            uri: String,
            #[snafu(source(from(crate::Error, Box::new)))]
            source: Box<crate::Error>,
        },
    }
}
pub use error::Error;
pub type Result<T> = std::result::Result<T, error::Error>;


Review comment (suggestion):
It would be somewhat more idiomatic Rust to have a `SettingsInput` enum (with an associated `String` value and a `Display` impl) and then use a `match` statement here. But that has the potential to touch a lot of other callers if `apiclient::set` is widely used (is it? might not be).

Code after:
    };
    let method = "PATCH";
    let (_status, _body) = crate::raw_request(&socket_path, &uri, method, Some(settings_data))
        .await
        .context(error::RequestSnafu)?;

    // Commit the transaction and apply it to the system.
    let uri = format!("/tx/commit_and_apply?tx={}", transaction);
    let method = "POST";
    let (_status, _body) = crate::raw_request(&socket_path, &uri, method, None)
        .await
        .context(error::RequestSnafu)?;

    Ok(())
}

mod error {
    use snafu::Snafu;

    #[derive(Debug, Snafu)]
    #[snafu(visibility(pub(super)))]
    pub enum Error {
        #[snafu(display("Unable to serialize data: {}", source))]
        Serialize { source: serde_json::Error },

        #[snafu(display("{}", source))]
        Request {
            #[snafu(source(from(crate::Error, Box::new)))]
            source: Box<crate::Error>,
        },
    }
}
pub use error::Error;
pub type Result<T> = std::result::Result<T, error::Error>;
Repository: bottlerocket-os/bottlerocket
File: sources/api/apiclient/src/main.rs (Rust)

Code before:
/// Stores user-supplied arguments for the 'get' subcommand.
#[derive(Debug)]
enum GetArgs {
    Prefixes(Vec<String>),
    Uri(String),
}

/// Stores user-supplied arguments for the 'raw' subcommand.
#[derive(Debug)]
struct RawArgs {
    method: String,
    uri: String,
    data: Option<String>,
}

/// Stores user-supplied arguments for the 'reboot' subcommand.
#[derive(Debug)]
struct RebootArgs {}

/// Stores a vector of user-supplied key-value pairs for the 'set' subcommand.
#[derive(Serialize, Deserialize)]
pub struct SetKeyPairSettings {
    request_payload: Vec<String>,
}
/// Stores user-supplied arguments for the 'set' subcommand.
#[derive(Debug)]
enum SetArgs {
    Simple(Vec<String>),
    Json(serde_json::Value),
}

/// Stores the 'update' subcommand specified by the user.
#[derive(Debug)]
enum UpdateSubcommand {
    Check(UpdateCheckArgs),
    Apply(UpdateApplyArgs),
    Cancel(UpdateCancelArgs),
}

/// The available 'report' subcommands.
#[derive(Debug)]
enum ReportSubcommand {
    Cis(CisReportArgs),
    CisK8s(CisReportArgs),
    Fips(FipsReportArgs),
}

/// Stores common user-supplied arguments for the cis report subcommand.
#[derive(Debug)]
struct CisReportArgs {
    level: Option<i32>,

Review comment (suggestion):
nit

```suggestion
}

/// Stores user-supplied arguments for the 'set' subcommand.
```

Code after:
/// Stores user-supplied arguments for the 'get' subcommand.
#[derive(Debug)]
enum GetArgs {
    Prefixes(Vec<String>),
    Uri(String),
}

/// Stores user-supplied arguments for the 'raw' subcommand.
#[derive(Debug)]
struct RawArgs {
    method: String,
    uri: String,
    data: Option<String>,
}

/// Stores user-supplied arguments for the 'reboot' subcommand.
#[derive(Debug)]
struct RebootArgs {}

/// Stores a vector of user-supplied key-value pairs for the 'set' subcommand.
#[derive(Serialize, Deserialize)]
pub struct SetKeyPairSettings {
    request_payload: Vec<String>,
}

/// Stores user-supplied arguments for the 'set' subcommand.
#[derive(Debug)]
enum SetArgs {
    Simple(Vec<String>),
    Json(serde_json::Value),
}

/// Stores the 'update' subcommand specified by the user.
#[derive(Debug)]
enum UpdateSubcommand {
    Check(UpdateCheckArgs),
    Apply(UpdateApplyArgs),
    Cancel(UpdateCancelArgs),
}

/// The available 'report' subcommands.
#[derive(Debug)]
enum ReportSubcommand {
    Cis(CisReportArgs),
    CisK8s(CisReportArgs),
    Fips(FipsReportArgs),
}

/// Stores common user-supplied arguments for the cis report subcommand.
#[derive(Debug)]
struct CisReportArgs {
Repository: bottlerocket-os/bottlerocket
File: sources/settings-migrations/v1.28.0/kernel-sysctl-hugepages/src/main.rs (Rust)

Code before:
use migration_helpers::common_migrations::{AddMetadataMigration, SettingMetadata};
use migration_helpers::{migrate, Result};
use std::process;

///  We added new settings metadata, `metadata.settings.kernel.sysctl.vm/nr_hugepages.setting-generator`
fn run() -> Result<()> {
    migrate(AddMetadataMigration(&[SettingMetadata {
        metadata: &["setting-generator"],
        setting: "metadata.settings.kernel.sysctl.vm/nr_hugepages",
    }]))
}

// Returning a Result from main makes it print a Debug representation of the error, but with Snafu
// we have nice Display representations of the error, so we wrap "main" (run) and print any error.
// https://github.com/shepmaster/snafu/issues/110
fn main() {
    if let Err(e) = run() {
        eprintln!("{}", e);
        process::exit(1);
    }
}


Review comment (suggestion):
I think you want to provide the setting itself here:
```suggestion
        setting: "settings.kernel.sysctl.vm/nr_hugepages",
```

Code after:
use migration_helpers::common_migrations::{AddMetadataMigration, SettingMetadata};
use migration_helpers::{migrate, Result};
use std::process;

///  We added new settings metadata, `metadata.settings.kernel.sysctl.vm/nr_hugepages.setting-generator`
fn run() -> Result<()> {
    migrate(AddMetadataMigration(&[SettingMetadata {
        metadata: &["setting-generator"],
        setting: "settings.kernel.sysctl.vm/nr_hugepages",
    }]))
}

// Returning a Result from main makes it print a Debug representation of the error, but with Snafu
// we have nice Display representations of the error, so we wrap "main" (run) and print any error.
// https://github.com/shepmaster/snafu/issues/110
fn main() {
    if let Err(e) = run() {
        eprintln!("{}", e);
        process::exit(1);
    }
}
Repository: bottlerocket-os/bottlerocket
File: sources/settings-migrations/v1.40.0/kubelet-device-plugins-cdi-settings/src/main.rs (Rust)

Code before:
                        setting, data
                    );
                    }
                }
            } else {
                println!("Found no '{}' to change on downgrade", setting);
            }
        }
        Ok(input)
    }
}

#[cfg(test)]
mod test_replace_list {
    use super::ReplaceListToStringMigration;
    use crate::{Migration, MigrationData};
    use maplit::hashmap;
    use std::collections::HashMap;

    #[test]
    fn forward_test() {
        let data = MigrationData {
            data: hashmap! {
                "hi".into() => vec!["cdi-cri"].into(),
            },
            metadata: HashMap::new(),
        };
        let result = ReplaceListToStringMigration(&["hi"]).forward(data).unwrap();
        assert_eq!(
            result.data,
            hashmap! {
                "hi".into() => vec!["cdi-cri"].into(),
            }
        );
    }

    #[test]
    fn forward_test_2() {
        let data = MigrationData {
            data: hashmap! {
                "hi".into() => "cdi-cri".into(),
            },
            metadata: HashMap::new(),
        };
        let result = ReplaceListToStringMigration(&["hi"]).forward(data).unwrap();
        assert_eq!(
            result.data,
            hashmap! {
                "hi".into() => "cdi-cri".into(),
            }
        );

Review comment (suggestion):
Can you please write your test cases in a way that you provide the test case in a vec, and you iterate through the cases?

You will need an extra case to handle the empty list.

Code after:
                .fail()?,
            }
        } else {
            println!("Found no '{}' to change on downgrade", setting);
        }

        Ok(input)
    }
}

#[cfg(test)]
mod test_replace_list {
    use super::*;
    use crate::{Migration, MigrationData};
    use maplit::hashmap;
    use serde_json::Value;
    use std::collections::HashMap;

    #[test]
    fn forward_test() {
        let data = MigrationData {
            data: hashmap! {
                DEVICE_LIST_STRATEGY_SETTING.into() => "volume-mounts".into(),
            },
            metadata: HashMap::new(),
        };
        let result = ReplaceDeviceListStrategy.forward(data).unwrap();
        assert_eq!(
            result.data,
            hashmap! {
                DEVICE_LIST_STRATEGY_SETTING.into() => "volume-mounts".into(),
            }
        );
    }

    #[test]
    fn backward_test() {
        let test_cases: Vec<(MigrationData, HashMap<String, Value>)> = vec![
            (
                MigrationData {
                    data: hashmap! {
                        DEVICE_LIST_STRATEGY_SETTING.into() => vec!["cdi-cri"].into(),
                    },
                    metadata: HashMap::new(),
                },
                hashmap! {
                    DEVICE_LIST_STRATEGY_SETTING.into() => "volume-mounts".into(),
                },
            ),
            (
                MigrationData {
Repository: bottlerocket-os/bottlerocket
File: sources/api/migration/migration-helpers/src/error.rs (Rust)

Code before:
        metadata,
        setting,
        data
    ))]
    ReplaceMetadataListContents {
        setting: String,
        metadata: String,
        data: Vec<serde_json::Value>,
    },

    #[snafu(display("Failed to delete file '{}': '{}'", path.display(), source))]
    RemoveFile {
        path: PathBuf,
        source: std::io::Error,
    },

    #[snafu(display("Failed to create async runtime: {}", source))]
    CreateTokioRuntime { source: std::io::Error },

    #[snafu(display(
        "Error in deserializing response value to SettingsGenerator: {}",
        source
    ))]
    DeserializeSettingsGenerator { source: serde_json::Error },

    #[snafu(display("setting data '{}' must be either a string or a list", data))]
    InvalidSettingType { data: String },
}

/// Result alias containing our Error type.
pub type Result<T> = std::result::Result<T, Error>;


Review comment (suggestion):
Nit: 

```suggestion
    #[snafu(display("Setting data '{}' must be either a string or a list", data))]
```

Code after:
        metadata,
        setting,
        data
    ))]
    ReplaceMetadataListContents {
        setting: String,
        metadata: String,
        data: Vec<serde_json::Value>,
    },

    #[snafu(display("Failed to delete file '{}': '{}'", path.display(), source))]
    RemoveFile {
        path: PathBuf,
        source: std::io::Error,
    },

    #[snafu(display("Failed to create async runtime: {}", source))]
    CreateTokioRuntime { source: std::io::Error },

    #[snafu(display(
        "Error in deserializing response value to SettingsGenerator: {}",
        source
    ))]
    DeserializeSettingsGenerator { source: serde_json::Error },

    #[snafu(display("Setting data '{}' must be either a string or a list", data))]
    InvalidSettingType { data: String },
}

/// Result alias containing our Error type.
pub type Result<T> = std::result::Result<T, Error>;
Repository: bottlerocket-os/bottlerocket
File: sources/settings-migrations/v1.49.0/kubernetes-reserved-pid-settings/src/main.rs (Rust)

Code before:
use migration_helpers::common_migrations::AddPrefixesMigration;
use migration_helpers::{migrate, Result};
use std::process;

// We added new kubernetes settings to reserve pids for kubernetes and system components.
fn run() -> Result<()> {
    migrate(AddPrefixesMigration(vec![
        "settings.kubernetes.kube-reserved.pid",
        "settings.kubernetes.system-reserved.pid",
    ]))
}

// Returning a Result from main makes it print a Debug representation of the error, but with Snafu
// we have nice Display representations of the error, so we wrap "main" (run) and print any error.
// https://github.com/shepmaster/snafu/issues/110
fn main() {
    if let Err(e) = run() {
        eprintln!("{e}");
        process::exit(1);
    }
}


Review comment (suggestion):
Can we add context (maybe to the comment) why we are using `AddPrefixesMigration` vs the usual `AddSettingsMigration` for a setting. 

I know it was called out here:
https://github.com/bottlerocket-os/bottlerocket-settings-sdk/pull/98#pullrequestreview-3181401321

But I am curious on the behavior. 

Code after:
use migration_helpers::common_migrations::AddSettingsMigration;
use migration_helpers::{migrate, Result};
use std::process;

// We added new kubernetes settings to reserve pids for kubernetes and system components.
fn run() -> Result<()> {
    migrate(AddSettingsMigration(&[
        "settings.kubernetes.kube-reserved.pid",
        "settings.kubernetes.system-reserved.pid",
    ]))
}

// Returning a Result from main makes it print a Debug representation of the error, but with Snafu
// we have nice Display representations of the error, so we wrap "main" (run) and print any error.
// https://github.com/shepmaster/snafu/issues/110
fn main() {
    if let Err(e) = run() {
        eprintln!("{e}");
        process::exit(1);
    }
}
Repository: caddyserver/caddy
File: modules/caddyhttp/reverseproxy/fastcgi/caddyfile.go (Go)

Code before:
				for i := 0; i < len(extensions); i++ {
					dispenser.Delete()
				}
				if len(extensions) == 0 {
					return nil, dispenser.ArgErr()
				}

			case "env":
				args := dispenser.RemainingArgs()
				dispenser.Delete()
				for i := 0; i < len(args); i++ {
					dispenser.Delete()
				}
				if len(args) != 2 {
					return nil, dispenser.ArgErr()
				}
				if fcgiTransport.EnvVars == nil {
					fcgiTransport.EnvVars = make(map[string]string)
				}
				fcgiTransport.EnvVars[args[0]] = args[1]

			case "index":
				args := dispenser.RemainingArgs()
				dispenser.Delete()
				for i := 0; i < len(args); i++ {
					dispenser.Delete()
				}
				if len(args) != 1 {
					return nil, dispenser.ArgErr()
				}
				indexFile = args[0]
			}
		}
	}

	// reset the dispenser after we're done so that the reverse_proxy
	// unmarshaler can read it from the start
	dispenser.Reset()

	// set up a route list that we'll append to
	routes := caddyhttp.RouteList{}

	// set the list of allowed path segments on which to split
	fcgiTransport.SplitPath = extensions

	// if the index is turned off, we skip the redirect and try_files
	if indexFile != "off" {
		// route to redirect to canonical path if index PHP file
		redirMatcherSet := caddy.ModuleMap{
			"file": h.JSON(fileserver.MatchFile{
				TryFiles: []string{"{http.request.uri.path}/" + indexFile},

Review comment (suggestion):
Check this out:

```suggestion
				for range args {
					dispenser.Delete()
```

Code after:
				for range extensions {
					dispenser.Delete()
				}
				if len(extensions) == 0 {
					return nil, dispenser.ArgErr()
				}

			case "env":
				args := dispenser.RemainingArgs()
				dispenser.Delete()
				for range args {
					dispenser.Delete()
				}
				if len(args) != 2 {
					return nil, dispenser.ArgErr()
				}
				if fcgiTransport.EnvVars == nil {
					fcgiTransport.EnvVars = make(map[string]string)
				}
				fcgiTransport.EnvVars[args[0]] = args[1]

			case "index":
				args := dispenser.RemainingArgs()
				dispenser.Delete()
				for range args {
					dispenser.Delete()
				}
				if len(args) != 1 {
					return nil, dispenser.ArgErr()
				}
				indexFile = args[0]
			}
		}
	}

	// reset the dispenser after we're done so that the reverse_proxy
	// unmarshaler can read it from the start
	dispenser.Reset()

	// set up a route list that we'll append to
	routes := caddyhttp.RouteList{}

	// set the list of allowed path segments on which to split
	fcgiTransport.SplitPath = extensions

	// if the index is turned off, we skip the redirect and try_files
	if indexFile != "off" {
		// route to redirect to canonical path if index PHP file
		redirMatcherSet := caddy.ModuleMap{
			"file": h.JSON(fileserver.MatchFile{
				TryFiles: []string{"{http.request.uri.path}/" + indexFile},