malpercio.dev
An easy-to-host PDS on the ATProtocol, MacOS. Grandma-approved.
fix(MM-69): address PR review — UTF-8 safety, error context, pure core
Critical:
- config_loader: replace std::env::vars() with vars_os() + manual
EZPDS_* filter; non-UTF-8 env var values return ConfigError::Invalid
instead of panicking
- config: replace to_string_lossy() with .to_str().ok_or_else() for
the derived database_url default; non-UTF-8 data_dir is now an
explicit error at config-load time rather than silent corruption
Important:
- config: apply_env_overrides now takes RawConfig by value and returns
Result<RawConfig, ConfigError> — genuinely functional (no mutation)
- config: EZPDS_PORT parse errors now include the source ParseIntError
- config: ConfigError::Io is now a struct variant carrying the file
path, surfacing it without anyhow::Context
- config: add env_override_wins_over_toml_value test (precedence
contract was previously untested)
- config_loader: add loads_minimal_valid_toml_produces_missing_field_error
test (empty file → MissingField through the full I/O path)
- relay: tracing subscriber init uses .try_init() instead of .init();
panic on double-registration is now a structured error via run()
- Cargo.toml: move tempfile to workspace.dependencies per convention
- relay/main.rs: drop redundant (env: EZPDS_CONFIG) from CLI arg doc
+101
-29
+3
Cargo.toml
+3
Cargo.toml
+1
-1
crates/common/Cargo.toml
+1
-1
crates/common/Cargo.toml
+52
-19
crates/common/src/config.rs
+52
-19
crates/common/src/config.rs
···
47
47
48
48
#[derive(Debug, thiserror::Error)]
49
49
pub enum ConfigError {
50
-
#[error("failed to read config file: {0}")]
51
-
Io(#[from] std::io::Error),
50
+
#[error("failed to read config file {path}: {source}")]
51
+
Io {
52
+
path: PathBuf,
53
+
#[source]
54
+
source: std::io::Error,
55
+
},
52
56
#[error("failed to parse config file: {0}")]
53
57
Parse(#[from] toml::de::Error),
54
58
#[error("invalid configuration: missing required field '{field}'")]
···
57
61
Invalid(String),
58
62
}
59
63
60
-
/// Apply `EZPDS_*` environment variable overrides to a `RawConfig`.
64
+
/// Apply `EZPDS_*` environment variable overrides to a [`RawConfig`], returning the updated config.
61
65
///
62
-
/// Receives the environment as a map so this function stays pure (no `std::env` access).
66
+
/// Receives the environment as a map so this function stays isolated from I/O (no `std::env`
67
+
/// access). Takes `raw` by value and returns it so callers can chain calls without mutation.
63
68
pub(crate) fn apply_env_overrides(
64
-
raw: &mut RawConfig,
69
+
mut raw: RawConfig,
65
70
env: &HashMap<String, String>,
66
-
) -> Result<(), ConfigError> {
71
+
) -> Result<RawConfig, ConfigError> {
67
72
if let Some(v) = env.get("EZPDS_BIND_ADDRESS") {
68
73
raw.bind_address = Some(v.clone());
69
74
}
70
75
if let Some(v) = env.get("EZPDS_PORT") {
71
-
raw.port = Some(v.parse::<u16>().map_err(|_| {
72
-
ConfigError::Invalid(format!("EZPDS_PORT is not a valid port number: '{v}'"))
76
+
raw.port = Some(v.parse::<u16>().map_err(|e| {
77
+
ConfigError::Invalid(format!("EZPDS_PORT is not a valid port number: '{v}': {e}"))
73
78
})?);
74
79
}
75
80
if let Some(v) = env.get("EZPDS_DATA_DIR") {
···
81
86
if let Some(v) = env.get("EZPDS_PUBLIC_URL") {
82
87
raw.public_url = Some(v.clone());
83
88
}
84
-
Ok(())
89
+
Ok(raw)
85
90
}
86
91
87
-
/// Validate a `RawConfig` and build a `Config`, applying defaults for optional fields.
92
+
/// Validate a [`RawConfig`] and build a [`Config`], applying defaults for optional fields.
93
+
///
94
+
/// Required fields: `data_dir`, `public_url`.
95
+
/// Defaults: `bind_address = "0.0.0.0"`, `port = 8080`,
96
+
/// `database_url = "{data_dir}/relay.db"` (derived; fails if `data_dir` is non-UTF-8).
88
97
pub(crate) fn validate_and_build(raw: RawConfig) -> Result<Config, ConfigError> {
89
98
let bind_address = raw.bind_address.unwrap_or_else(|| "0.0.0.0".to_string());
90
99
let port = raw.port.unwrap_or(8080);
···
92
101
.data_dir
93
102
.ok_or(ConfigError::MissingField { field: "data_dir" })?
94
103
.into();
95
-
let database_url = raw
96
-
.database_url
97
-
.unwrap_or_else(|| data_dir.join("relay.db").to_string_lossy().into_owned());
104
+
let database_url = match raw.database_url {
105
+
Some(url) => url,
106
+
None => data_dir
107
+
.join("relay.db")
108
+
.to_str()
109
+
.ok_or_else(|| {
110
+
ConfigError::Invalid(
111
+
"data_dir contains non-UTF-8 characters, cannot derive database_url"
112
+
.to_string(),
113
+
)
114
+
})?
115
+
.to_owned(),
116
+
};
98
117
let public_url = raw.public_url.ok_or(ConfigError::MissingField {
99
118
field: "public_url",
100
119
})?;
···
183
202
184
203
#[test]
185
204
fn env_override_port() {
186
-
let mut raw = minimal_raw();
187
205
let env = HashMap::from([("EZPDS_PORT".to_string(), "9090".to_string())]);
188
-
apply_env_overrides(&mut raw, &env).unwrap();
206
+
let raw = apply_env_overrides(minimal_raw(), &env).unwrap();
189
207
let config = validate_and_build(raw).unwrap();
190
208
191
209
assert_eq!(config.port, 9090);
192
210
}
193
211
194
212
#[test]
213
+
fn env_override_wins_over_toml_value() {
214
+
// env always takes precedence over explicit TOML values
215
+
let toml = r#"
216
+
data_dir = "/var/pds"
217
+
port = 3000
218
+
public_url = "https://pds.example.com"
219
+
"#;
220
+
let raw: RawConfig = toml::from_str(toml).unwrap();
221
+
let env = HashMap::from([("EZPDS_PORT".to_string(), "9999".to_string())]);
222
+
let raw = apply_env_overrides(raw, &env).unwrap();
223
+
let config = validate_and_build(raw).unwrap();
224
+
225
+
assert_eq!(config.port, 9999);
226
+
}
227
+
228
+
#[test]
195
229
fn env_override_all_fields() {
196
-
let mut raw = RawConfig::default();
197
230
let env = HashMap::from([
198
231
("EZPDS_BIND_ADDRESS".to_string(), "127.0.0.1".to_string()),
199
232
("EZPDS_PORT".to_string(), "4000".to_string()),
···
207
240
"https://pds.test".to_string(),
208
241
),
209
242
]);
210
-
apply_env_overrides(&mut raw, &env).unwrap();
243
+
let raw = apply_env_overrides(RawConfig::default(), &env).unwrap();
211
244
let config = validate_and_build(raw).unwrap();
212
245
213
246
assert_eq!(config.bind_address, "127.0.0.1");
···
219
252
220
253
#[test]
221
254
fn env_override_invalid_port_returns_error() {
222
-
let mut raw = minimal_raw();
223
255
let env = HashMap::from([("EZPDS_PORT".to_string(), "not_a_port".to_string())]);
224
-
let err = apply_env_overrides(&mut raw, &env).unwrap_err();
256
+
let err = apply_env_overrides(minimal_raw(), &env).unwrap_err();
225
257
226
258
assert!(matches!(err, ConfigError::Invalid(_)));
227
259
assert!(err.to_string().contains("EZPDS_PORT"));
260
+
assert!(err.to_string().contains("not_a_port"));
228
261
}
229
262
230
263
#[test]
+42
-7
crates/common/src/config_loader.rs
+42
-7
crates/common/src/config_loader.rs
···
5
5
6
6
use crate::config::{apply_env_overrides, validate_and_build, Config, ConfigError, RawConfig};
7
7
8
+
/// Collect only `EZPDS_*` env vars from the process environment, rejecting any with non-UTF-8
9
+
/// values rather than panicking (as `std::env::vars()` would on non-UTF-8 data).
10
+
fn collect_ezpds_env() -> Result<HashMap<String, String>, ConfigError> {
11
+
let mut map = HashMap::new();
12
+
for (key_os, val_os) in std::env::vars_os() {
13
+
let key = match key_os.to_str() {
14
+
Some(k) if k.starts_with("EZPDS_") => k.to_owned(),
15
+
_ => continue,
16
+
};
17
+
let val = val_os.into_string().map_err(|_| {
18
+
ConfigError::Invalid(format!(
19
+
"environment variable {key} contains non-UTF-8 data"
20
+
))
21
+
})?;
22
+
map.insert(key, val);
23
+
}
24
+
Ok(map)
25
+
}
26
+
8
27
/// Load [`Config`] from a TOML file with an explicit environment map.
9
28
///
10
-
/// Prefer [`load_config`] for production use. This variant is `pub(crate)` so
11
-
/// tests can pass a controlled environment without leaking real `EZPDS_*` vars.
29
+
/// Prefer [`load_config`] for production use. This variant is `pub(crate)` so tests can pass a
30
+
/// controlled environment without leaking real `EZPDS_*` vars.
12
31
pub(crate) fn load_config_with_env(
13
32
path: &Path,
14
33
env: &HashMap<String, String>,
15
34
) -> Result<Config, ConfigError> {
16
-
let contents = std::fs::read_to_string(path)?;
17
-
let mut raw: RawConfig = toml::from_str(&contents)?;
18
-
apply_env_overrides(&mut raw, env)?;
35
+
let contents = std::fs::read_to_string(path).map_err(|source| ConfigError::Io {
36
+
path: path.to_owned(),
37
+
source,
38
+
})?;
39
+
let raw: RawConfig = toml::from_str(&contents)?;
40
+
let raw = apply_env_overrides(raw, env)?;
19
41
validate_and_build(raw)
20
42
}
21
43
22
44
/// Load [`Config`] from a TOML file, applying `EZPDS_*` environment variable overrides.
23
45
pub fn load_config(path: &Path) -> Result<Config, ConfigError> {
24
-
let env: HashMap<String, String> = std::env::vars().collect();
46
+
let env = collect_ezpds_env()?;
25
47
load_config_with_env(path, &env)
26
48
}
27
49
···
52
74
}
53
75
54
76
#[test]
77
+
fn loads_minimal_valid_toml_produces_missing_field_error() {
78
+
// An empty file is valid TOML but missing required fields.
79
+
let tmp = tempfile::NamedTempFile::new().unwrap();
80
+
81
+
let err = load_config_with_env(tmp.path(), &empty_env()).unwrap_err();
82
+
83
+
assert!(matches!(
84
+
err,
85
+
ConfigError::MissingField { field: "data_dir" }
86
+
));
87
+
}
88
+
89
+
#[test]
55
90
fn env_overrides_applied_from_file() {
56
91
let mut tmp = tempfile::NamedTempFile::new().unwrap();
57
92
writeln!(
···
71
106
fn returns_error_for_missing_file() {
72
107
let result = load_config_with_env(Path::new("/nonexistent/relay.toml"), &empty_env());
73
108
74
-
assert!(matches!(result, Err(ConfigError::Io(_))));
109
+
assert!(matches!(result, Err(ConfigError::Io { .. })));
75
110
}
76
111
77
112
#[test]
+3
-2
crates/relay/src/main.rs
+3
-2
crates/relay/src/main.rs
···
7
7
#[derive(Parser)]
8
8
#[command(name = "relay", about = "ezpds relay server")]
9
9
struct Cli {
10
-
/// Path to relay.toml config file (env: EZPDS_CONFIG)
10
+
/// Path to relay.toml config file
11
11
#[arg(long, env = "EZPDS_CONFIG")]
12
12
config: Option<PathBuf>,
13
13
}
···
22
22
fn run() -> anyhow::Result<()> {
23
23
tracing_subscriber::fmt()
24
24
.with_env_filter(tracing_subscriber::EnvFilter::from_default_env())
25
-
.init();
25
+
.try_init()
26
+
.map_err(|e| anyhow::anyhow!("failed to initialize tracing subscriber: {e}"))?;
26
27
27
28
let cli = Cli::parse();
28
29
let config_path = cli.config.unwrap_or_else(|| PathBuf::from("relay.toml"));