fix: Implement nested mode for environment variable override#20
fix: Implement nested mode for environment variable override#20
Conversation
Update author email from itsparser@gmail.com to 0xvasanth@gmail.com and repository URLs to reflect the correct GitHub organization.
Fixes #18 - Environment variables can now properly override nested configuration file values at any depth using Deep merge strategy. Changes: - Added nested field and nested() method to Environment struct - Implemented insert_nested() helper for nested structure creation - Modified collect_with_flat_keys() to support nested mode - Added comprehensive tests and working example All 37 tests pass. Backward compatible (nested=false by default).
There was a problem hiding this comment.
Pull Request Overview
This PR implements a fix for Issue #18, which allows environment variables to properly override nested configuration file values. The solution adds a nested mode to the Environment struct that splits flat environment variable keys (using the separator) into nested JSON structures during the merge process.
Key changes:
- Added
nestedflag to theEnvironmentstruct with a builder methodnested(bool) - Implemented
insert_nestedhelper function to recursively create nested map structures - Modified key processing logic in
collect_with_flat_keysto handle nested mode differently (preserving case before splitting, then lowercasing each part)
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/environment.rs | Added nested mode support with flag, builder method, and nested key-to-structure conversion logic |
| tests/issue_18_nested_env_test.rs | Comprehensive test suite covering basic, deep, multiple, and backward compatibility scenarios |
| examples/issue_18_verification.rs | Detailed verification example demonstrating the nested override functionality |
| Cargo.toml | Updated author email and repository URLs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/environment.rs
Outdated
| let nested_map = map | ||
| .entry(key.clone()) | ||
| .or_insert_with(|| Value::Object(Map::new())); | ||
|
|
||
| // If the entry exists but is not an object, replace it with an object | ||
| if let Value::Object(ref mut nested) = nested_map { | ||
| Self::insert_nested(nested, &parts[1..], value); | ||
| } else { | ||
| // Replace non-object with a new object containing the nested value | ||
| let mut new_map = Map::new(); | ||
| Self::insert_nested(&mut new_map, &parts[1..], value); | ||
| map.insert(key, Value::Object(new_map)); |
There was a problem hiding this comment.
The key is cloned on line 287 but then cloned again via entry(key.clone()) on line 289. Store the result of map.entry(key) directly without the intermediate clone, as entry takes ownership and the first clone at line 287 becomes unnecessary.
| let nested_map = map | |
| .entry(key.clone()) | |
| .or_insert_with(|| Value::Object(Map::new())); | |
| // If the entry exists but is not an object, replace it with an object | |
| if let Value::Object(ref mut nested) = nested_map { | |
| Self::insert_nested(nested, &parts[1..], value); | |
| } else { | |
| // Replace non-object with a new object containing the nested value | |
| let mut new_map = Map::new(); | |
| Self::insert_nested(&mut new_map, &parts[1..], value); | |
| map.insert(key, Value::Object(new_map)); | |
| let entry = map.entry(key); | |
| match entry { | |
| serde_json::map::Entry::Occupied(mut occ) => { | |
| if let Value::Object(ref mut nested) = occ.get_mut() { | |
| Self::insert_nested(nested, &parts[1..], value); | |
| } else { | |
| // Replace non-object with a new object containing the nested value | |
| let mut new_map = Map::new(); | |
| Self::insert_nested(&mut new_map, &parts[1..], value); | |
| *occ.get_mut() = Value::Object(new_map); | |
| } | |
| } | |
| serde_json::map::Entry::Vacant(vac) => { | |
| let mut new_map = Map::new(); | |
| Self::insert_nested(&mut new_map, &parts[1..], value); | |
| vac.insert(Value::Object(new_map)); | |
| } |
| let lowercase_parts: Vec<String> = | ||
| parts.iter().map(|p| p.to_lowercase()).collect(); | ||
| let lowercase_parts_refs: Vec<&str> = | ||
| lowercase_parts.iter().map(|s| s.as_str()).collect(); |
There was a problem hiding this comment.
Creating an intermediate Vec<String> allocation and then collecting references to another vector is inefficient. Consider collecting lowercase parts directly into a Vec<&str> using a temporary allocation strategy, or refactor insert_nested to accept an iterator instead of a slice.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tests/issue_18_nested_env_test.rs:1
- [nitpick] The comment is too verbose and could be more concise. Consider simplifying to: 'Environment variable APP_HTTP_PORT is interpreted as flat key http_port, not nested http.port'
/// Test for Issue #18: Environment variables should override nested config file values
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let key_for_map = if self.nested { | ||
| trimmed.to_string() | ||
| } else { | ||
| trimmed.to_lowercase() | ||
| }; |
There was a problem hiding this comment.
The logic for handling case conversion is duplicated in two places (lines 365-369 and lines 395-399). Consider extracting this into a helper method like fn normalize_key(&self, key: &str) -> String to reduce duplication and improve maintainability.
| if parts.len() == 1 { | ||
| // Single part, insert directly (lowercase it) | ||
| result.insert(key.to_lowercase(), value); | ||
| } else { | ||
| // Multiple parts, create nested structure | ||
| // Lowercase each part individually | ||
| let lowercase_parts: Vec<String> = | ||
| parts.iter().map(|p| p.to_lowercase()).collect(); | ||
| let lowercase_parts_refs: Vec<&str> = | ||
| lowercase_parts.iter().map(|s| s.as_str()).collect(); | ||
| Self::insert_nested(&mut result, &lowercase_parts_refs, value); | ||
| } |
There was a problem hiding this comment.
The code creates two intermediate vectors (lowercase_parts and lowercase_parts_refs) for every multi-part nested key. Consider modifying insert_nested() to accept an iterator or to lowercase keys inline, avoiding these allocations and improving performance for configurations with many nested environment variables.
Apply Copilot's review suggestion to use Entry API pattern instead of or_insert_with, eliminating an unnecessary clone of the key. Changes: - Use match on map.entry(key) instead of entry(key.clone()) - Handle Occupied and Vacant cases explicitly - Reduces allocations in recursive nested insertion All tests pass.
Summary
Fixes #18 - Environment variables now properly override nested config file values at any depth when using the Deep merge strategy.
Problem
Environment variables were not overriding nested configuration file values despite the documented priority order. For example:
http.port: 3000APP_HTTP_PORT=9000The root cause was that environment variables created flat keys (
http_port) while config files had nested structures ({"http": {"port": 3000}}), so keys didn't match during merge.Solution
Implemented a nested mode for the
Environmentstruct that converts flat environment variable keys into nested JSON structures:Changes
nested: boolfield toEnvironmentstruct.nested(bool)method for opt-in nested key handlinginsert_nested()helper to recursively build nested structurescollect_with_flat_keys()to split keys on separator and create nested paths whennested=trueHow It Works
With
nested=true:APP_HTTP_PORT=9000→ splits to["HTTP", "PORT"]→ lowercases to["http", "port"]→ creates{"http": {"port": 9000}}Testing
tests/issue_18_nested_env_test.rswith 4 tests:database.pool.maxsize)examples/issue_18_verification.rsdemonstrating multi-level configBackward Compatibility
nested=false) preserved - no breaking changes.nested(true)for new functionalityUsage
```rust
let config: AppConfig = ConfigBuilder::new()
.with_merge_strategy(MergeStrategy::Deep)
.with_file("config.yaml")?
.with_env_custom(Environment::new().with_prefix("APP").nested(true))
.build()?;
```
Now
APP_HTTP_PORT=9000properly overrideshttp.port: 3000from the config file!Limitation
Field names with underscores (e.g.,
max_connections) will be split into multiple parts when using underscore as separator. Workarounds:maxsize).separator("::")env_namemappings