Splitting the "app_config.rs is a large multi-responsibility module" item out of #1249 into a concrete, actionable refactor.
Problem
Adding a single config option to src/app_config.rs touches 5+ disconnected places, and nothing makes the compiler tell you when you forgot one:
-
A new field on the ~30-field AppConfig struct, usually with its own #[serde(default = "...")].
-
A standalone top-level default_*() free function returning a literal, declared far away from the field it belongs to. There are 18 of them today:
$ grep -c "fn default_" src/app_config.rs
18
e.g. default_max_file_size, default_max_pending_rows, default_oidc_scopes.
-
Sometimes a bespoke deserialize_with helper: deserialize_port, deserialize_socket_addr, deserialize_site_prefix, deserialize_duration_seconds.
-
The env-var name is implicit (resolved by the config crate in load_from_file / env_config), so it never appears in code next to the field.
-
A hand-maintained row in the configuration.md table. Nothing ties the field to its default to its doc row.
Concrete drift this already caused
The doc comment on max_uploaded_file_size disagrees with its own default function:
So the inline Rust comment drifted to claim 10 MiB while the real default and the docs table both say 5 MiB. Exactly the silent drift this structure invites.
Proposed change
- Move the literal defaults into a single
impl Default for AppConfig (or a const-backed table) and use #[serde(default)], eliminating the ~18 default_* free functions and putting each default next to its field.
- Group the flat struct into nested domain sub-structs (database, oidc, https, telemetry, markdown) with
#[serde(flatten)], so related field + default + validation co-locate and the file shrinks.
- Add a doc-coverage test asserting every public config field name appears in
configuration.md, killing drift mechanically (a value-coverage assertion could also catch the 5/10 MiB case).
Risks and mitigations
serde rename/flatten can change the accepted file keys / env var names. Mitigate by keeping field names byte-for-byte identical and adding round-trip tests that load representative existing config files / env layouts and assert the parsed AppConfig is unchanged.
- Land the
impl Default change separately from the sub-struct grouping, so each PR is small and the bisect surface stays tight.
Expected win
One place to edit per option, defaults next to their fields, and doc drift caught by a test instead of by a user.
Refs #1249.
Splitting the "
app_config.rsis a large multi-responsibility module" item out of #1249 into a concrete, actionable refactor.Problem
Adding a single config option to
src/app_config.rstouches 5+ disconnected places, and nothing makes the compiler tell you when you forgot one:A new field on the ~30-field
AppConfigstruct, usually with its own#[serde(default = "...")].A standalone top-level
default_*()free function returning a literal, declared far away from the field it belongs to. There are 18 of them today:e.g.
default_max_file_size,default_max_pending_rows,default_oidc_scopes.Sometimes a bespoke
deserialize_withhelper:deserialize_port,deserialize_socket_addr,deserialize_site_prefix,deserialize_duration_seconds.The env-var name is implicit (resolved by the
configcrate inload_from_file/env_config), so it never appears in code next to the field.A hand-maintained row in the
configuration.mdtable. Nothing ties the field to its default to its doc row.Concrete drift this already caused
The doc comment on
max_uploaded_file_sizedisagrees with its own default function:Maximum size of uploaded files in bytes. The default is 10MiB (10 * 1024 * 1024 bytes)default_max_file_size, L625-L627 actually returns5 * 1024 * 1024(5 MiB)configuration.mdL27 says5242880/ "Defaults to 5 MiB"So the inline Rust comment drifted to claim 10 MiB while the real default and the docs table both say 5 MiB. Exactly the silent drift this structure invites.
Proposed change
impl Default for AppConfig(or a const-backed table) and use#[serde(default)], eliminating the ~18default_*free functions and putting each default next to its field.#[serde(flatten)], so related field + default + validation co-locate and the file shrinks.configuration.md, killing drift mechanically (a value-coverage assertion could also catch the 5/10 MiB case).Risks and mitigations
serderename/flattencan change the accepted file keys / env var names. Mitigate by keeping field names byte-for-byte identical and adding round-trip tests that load representative existing config files / env layouts and assert the parsedAppConfigis unchanged.impl Defaultchange separately from the sub-struct grouping, so each PR is small and the bisect surface stays tight.Expected win
One place to edit per option, defaults next to their fields, and doc drift caught by a test instead of by a user.
Refs #1249.