Skip to content

Commit

Permalink
bug(config): Fix handling of the default value for `ProxyConfig::enab…
Browse files Browse the repository at this point in the history
…led` (#19604)

Because the `enabled` field has a serde field-level default of `true` but a
type-level default of `false`, this struct will not survive
serialize-deserialize pairs intact. This change fixes the field default check
and adds a partial proptest for the back-and-forth conversion of the whole
struct.
  • Loading branch information
bruceg authored Jan 12, 2024
1 parent c2f3259 commit 1705dfd
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 1 deletion.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions lib/vector-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ ndarray-stats = "0.5.1"
noisy_float = "0.2.0"
rand = "0.8.5"
rand_distr = "0.4.3"
serde_yaml = { version = "0.9.30", default-features = false }
tracing-subscriber = { version = "0.3.18", default-features = false, features = ["env-filter", "fmt", "ansi", "registry"] }
vector-common = { path = "../vector-common", default-features = false, features = ["test"] }

Expand Down
40 changes: 39 additions & 1 deletion lib/vector-core/src/config/proxy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ pub struct ProxyConfig {
/// Enables proxying support.
#[serde(
default = "ProxyConfig::default_enabled",
skip_serializing_if = "is_default"
skip_serializing_if = "is_enabled"
)]
pub enabled: bool,

Expand Down Expand Up @@ -105,6 +105,11 @@ impl Default for ProxyConfig {
}
}

#[allow(clippy::trivially_copy_pass_by_ref)] // Calling convention is required by serde
fn is_enabled(e: &bool) -> bool {
e == &true
}

impl ProxyConfig {
fn default_enabled() -> bool {
true
Expand Down Expand Up @@ -204,11 +209,44 @@ mod tests {
header::{AUTHORIZATION, PROXY_AUTHORIZATION},
HeaderName, HeaderValue, Uri,
};
use proptest::prelude::*;

const PROXY_HEADERS: [HeaderName; 2] = [AUTHORIZATION, PROXY_AUTHORIZATION];

use super::*;

impl Arbitrary for ProxyConfig {
type Parameters = ();
type Strategy = BoxedStrategy<Self>;

fn arbitrary_with((): Self::Parameters) -> Self::Strategy {
(
any::<bool>(),
any::<Option<String>>(),
any::<Option<String>>(),
)
.prop_map(|(enabled, http, https)| Self {
enabled,
http,
https,
// TODO: Neither NoProxy nor IpCidr contained with in it supports proptest. Once
// they support proptest, add another any here.
no_proxy: Default::default(),
})
.boxed()
}
}

proptest! {
#[test]
fn encodes_and_decodes_through_yaml(config:ProxyConfig) {
let yaml = serde_yaml::to_string(&config).expect("Could not serialize config");
let reloaded: ProxyConfig = serde_yaml::from_str(&yaml)
.expect("Could not deserialize config");
assert_eq!(config, reloaded);
}
}

#[test]
fn merge_simple() {
let first = ProxyConfig::default();
Expand Down

0 comments on commit 1705dfd

Please sign in to comment.