refactor(rust): read the configuration through serde, and drop the Args layer - #725
wkirschenmann wants to merge 3 commits into
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
e0806aa to
00a6ab6
Compare
00a6ab6 to
75e520f
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75e520fefd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // process, even one naming no option here. | ||
| let options = variables.filter_map(|(name, value)| { | ||
| let name = name.to_string_lossy(); | ||
| let option = name.strip_prefix(ARMONIK_PREFIX)?; |
There was a problem hiding this comment.
On Windows, environment-variable names are case-insensitive, so the previous targeted std::env::var("GrpcClient__Endpoint") lookup accepted spellings such as GRPCCLIENT__ENDPOINT. Enumerating variables and applying Rust's case-sensitive strip_prefix and serde field matching now ignores those variables, causing Client::new() to fail with an unset endpoint or silently use defaults. Normalize the full option name on Windows or query the known keys using the platform environment API.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I will document the current behaviour as intentional
| let options = variables.filter_map(|(name, value)| { | ||
| let name = name.to_string_lossy(); | ||
| let option = name.strip_prefix(ARMONIK_PREFIX)?; | ||
| Some((option.to_owned(), value.to_string_lossy().into_owned())) |
There was a problem hiding this comment.
When a relevant variable such as GrpcClient__CertPem contains a non-Unicode Unix path, to_string_lossy() replaces bytes with U+FFFD rather than preserving or rejecting the value. This can produce a misleading file-not-found error or even load a different Unicode filename that happens to match the rewritten path; the previous reader explicitly returned NotUnicode for this case. After filtering unrelated variables, return a configuration error for non-Unicode values instead of silently changing them.
Useful? React with 👍 / 👎.
…gs layer ClientConfigArgs existed to hold the raw text of every option and hand it to a conversion. HttpConfig now deserialises from that text itself, so the layer goes: one struct, typed fields, public and buildable by hand, and a flat PascalCase vocabulary a deployment already sets. Deserialize only, never Serialize, so no configuration dump can write a secret back out. The flat fields group into thematic units - TlsConfig, TcpConfig, Http2Config - flattened back under a prefix, so grouping them renames no option. The prefix belongs to the embedding rather than to the unit: config.rs declares it next to the field, and a unit carries no name of its own, so the same one can be composed twice under different prefixes. The machinery that makes this work moves into config_utils, which knows nothing about what an option means: the embed_prefixed! macro, and the readers for text, booleans, durations and integers. It names no option, carries no default, and mentions no endpoint, proxy or certificate. The transport's own vocabulary - endpoint, connect timeout, user agent, rate limit - stays in config.rs. That boundary is what would let the module lift into a crate of its own the day a second one needs it. Everything arrives as text because that is what a configuration source spells: an environment variable has no other shape, and a typed source may still write a number or a boolean as itself, so every scalar is read through one reader and interpreted by the option's own. Certificate loading is unchanged: one certificate from the PEM file, as before. Presenting a whole chain is a change of behaviour and belongs to a change of its own. The proxy is set programmatically only. Its field is skipped rather than read, and a document that spells a proxy option is ignored rather than refused, because a type with flattened fields cannot also deny unknown ones. armonik reads GrpcClient__* through serde's MapDeserializer, replacing ClientConfigArgs::from_env with no new dependency. The prefix rule and the lossy decode are split into a function over any set of variables, so they are tested without mutating the environment every other test shares. cargo test -p armonik-transport --all-features: 72 passed, 0 failed. cargo clippy -p armonik-transport -p armonik --all-features --all-targets: clean. cargo check -p armonik-transport --no-default-features: clean.
The proxy is set programmatically here, and flattening the units rules out denying unknown fields, so a document naming a proxy option passes through instead of failing the read. Nothing said so in this crate: the same mechanism was covered only generically, in armonik's environment tests, under a placeholder key. Whoever gives those options meaning has to add the reading rather than assume a guard, which is what this pins. cargo test -p armonik-transport --all-features: 74 passed, 0 failed. cargo clippy -p armonik-transport --all-features --all-targets: clean.
75e520f to
9412088
Compare
A value reached serde through `to_string_lossy`, so a `CertPem` holding a path the OS accepts but Unicode does not became a path with U+FFFD in it: a file-not-found naming something the deployment never wrote, or, if such a file happens to exist, the wrong certificate loaded without a word. The same applies to a password, which would authenticate as a different secret. Values are now taken with `to_str` and refused by name when they are not Unicode. Names keep the lossy decode, and the asymmetry is deliberate: enumerating panics on any non-Unicode variable in the process, even one naming no option here, and a mangled name simply fails to match the prefix, so nothing is lost by it. The prefix stays matched exactly, and now says so. Windows resolves variable names case-insensitively, so `GRPCCLIENT__ENDPOINT` reaches the same variable there and no option here; one documented spelling reads the same on every platform. cargo test -p armonik --all-features --lib client::env: 6 passed, 0 failed. cargo test -p armonik-transport --all-features: 73 passed, 0 failed. cargo clippy -p armonik-transport -p armonik --all-features --all-targets: clean.
|
1 participant
ClientConfigArgsexisted to hold the raw text of every option and hand it to a conversion.HttpConfigdeserialises from that text itself, so the layer goes: one struct, typed fields,buildable by hand, reading the flat
PascalCasevocabulary a deployment already sets. Deserializeonly, never Serialize, so no configuration dump can write a secret back out.
The flat fields group into
TlsConfig,TcpConfigandHttp2Config, flattened back under aprefix, so grouping them renames no option. The prefix belongs to the embedding, not to the
unit:
config.rsdeclares it next to the field, and a unit carries no name of its own, so thesame unit can be composed twice under different prefixes.
src/config_utils/The machinery lives in a module that knows nothing about what an option means:
embed_prefixed!,and the readers for text, booleans, durations and integers. It names no option, carries no
default, and mentions no endpoint, proxy or certificate. The transport's own vocabulary - endpoint,
connect timeout, user agent, rate limit - stays in
config.rs.That boundary is what would let the module lift into a crate of its own, as a directory move
rather than an untangling. Options arrive as text because that is what a source spells: an
environment variable has no other shape, and a typed source may still write a number as itself.
Breaking changes
ClientConfigis renamedHttpConfigandClientConfigArgsis gone.from_config_argshas noreplacement because it has no purpose.
Serializeis removed withEq/Hash. The only consumers were this crate's own tests.Client::new()returnsNewClientError, notConnectionError: the error has to separatereading the configuration from connecting with it.
ReadEnvErrorleavesarmonik-transport'sAPI with no replacement there, because that crate no longer reads an environment.
connectrefuses a default endpoint with`Endpoint` is not set, rather than leaving it towhatever the TCP layer eventually reports. That is the other half of letting an absent
Endpointdeserialise to the default URI instead of failing the read.The proxy is not read from options here
HttpConfig::proxyis set programmatically. Its field is skipped, and a document spelling a proxyoption is ignored rather than refused, because a type with flattened fields cannot also deny
unknown ones. A test pins that, so the PR adding the proxy option surface cannot assume a guard.
armonikGrpcClient__*is read through serde'sMapDeserializer, replacingClientConfigArgs::from_envwith no new dependency. The prefix rule and the lossy decode are split into a function over any
set of variables, so they are tested without mutating the environment every other test shares.
One limit is pinned rather than hidden: a rejected value is reported without the variable it came
from, because a plain
MapDeserializerkeeps no path. Restoring the name is the next PR's job.Tests
Adds 25 (7 removed with the Args layer they exercised): the vocabulary read through each unit's
prefix, empty-means-default, the scalar shapes a typed source may write, the unset-endpoint
rejection, a proxy option passing through ignored, and five in
armonikfor prefix stripping,unknown options, a read failure and a non-Unicode variable name.
cargo test -p armonik-transport --all-features: 72 passed, 0 failed.cargo clippy -p armonik-transport -p armonik --all-features --all-targets: clean.cargo check -p armonik-transport --no-default-features: clean.