Skip to content

Commit

Permalink
Auto merge of #14000 - linyihai:fix-env-prefix, r=weihanglo
Browse files Browse the repository at this point in the history
Fix: Skip deserialization of unrelated fields with overlapping name

### What does this PR try to resolve?

Split from #13687 (comment)

This fixes the overlap of environment variable names:

> For example, when env_key is UNSTABLE_GITOXIDE_FETCH
and field_key is UNSTABLE_GIT, the field shouldn't be
added because `unstable.gitoxide.fetch` doesn't
belong to `unstable.git` struct.

### How should we test and review this PR?

Updates of test cases `struct_with_overlapping_inner_struct_and_defaults` can be used to compare changes before and after changes

### Additional information
r? weihanglo and very appreciate your more optimized code
  • Loading branch information
bors committed Jun 1, 2024
2 parents 52ebc6a + d347704 commit 721cd55
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 3 deletions.
10 changes: 9 additions & 1 deletion src/cargo/util/context/de.rs
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,15 @@ impl<'gctx> ConfigMapAccess<'gctx> {
let mut field_key = de.key.clone();
field_key.push(field);
for env_key in de.gctx.env_keys() {
if env_key.starts_with(field_key.as_env_key()) {
let Some(nested_field) = env_key.strip_prefix(field_key.as_env_key()) else {
continue;
};
// This distinguishes fields that share the same prefix.
// For example, when env_key is UNSTABLE_GITOXIDE_FETCH
// and field_key is UNSTABLE_GIT, the field shouldn't be
// added because `unstable.gitoxide.fetch` doesn't
// belong to `unstable.git` struct.
if nested_field.is_empty() || nested_field.starts_with('_') {
fields.insert(KeyKind::Normal(field.to_string()));
}
}
Expand Down
19 changes: 17 additions & 2 deletions tests/testsuite/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1454,7 +1454,6 @@ fn struct_with_overlapping_inner_struct_and_defaults() {
// If, in the future, we can handle this more correctly, feel free to delete
// this case.
#[derive(Deserialize, Default)]
#[serde(default)]
struct PrefixContainer {
inn: bool,
inner: Inner,
Expand All @@ -1466,7 +1465,7 @@ fn struct_with_overlapping_inner_struct_and_defaults() {
.get::<PrefixContainer>("prefixcontainer")
.err()
.unwrap();
assert!(format!("{}", err).contains("missing config key `prefixcontainer.inn`"));
assert!(format!("{}", err).contains("missing field `inn`"));
let gctx = GlobalContextBuilder::new()
.env("CARGO_PREFIXCONTAINER_INNER_VALUE", "12")
.env("CARGO_PREFIXCONTAINER_INN", "true")
Expand All @@ -1475,6 +1474,22 @@ fn struct_with_overlapping_inner_struct_and_defaults() {
assert_eq!(f.inner.value, 12);
assert_eq!(f.inn, true);

// Use default attribute of serde, then we can skip setting the inn field
#[derive(Deserialize, Default)]
#[serde(default)]
struct PrefixContainerFieldDefault {
inn: bool,
inner: Inner,
}
let gctx = GlobalContextBuilder::new()
.env("CARGO_PREFIXCONTAINER_INNER_VALUE", "12")
.build();
let f = gctx
.get::<PrefixContainerFieldDefault>("prefixcontainer")
.unwrap();
assert_eq!(f.inner.value, 12);
assert_eq!(f.inn, false);

// Containing struct where the inner value's field is a prefix of another
//
// This is a limitation of mapping environment variables on to a hierarchy.
Expand Down

0 comments on commit 721cd55

Please sign in to comment.