fix(config): accept single string or array for TOML list fields#1377
Merged
fix(config): accept single string or array for TOML list fields#1377
Conversation
Several TLS/auth config fields are typed as Vec but were documented in the example TOML as plain strings, causing "invalid type: string, expected a sequence" at startup. Apply serde_with::OneOrMany so a bare string and a TOML array both deserialize correctly. Affected fields: server.tls.cert, server.tls.key, server.tls.generate, server.tls.root, client tls.root, web.https.root, auth.tls.root, auth.domains. Closes #1376
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughConfiguration structs in four areas were updated to accept either a single string or a TOML array for certain fields by applying 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rs/moq-relay/src/auth.rs (1)
173-183: ⚡ Quick winConsider adding TOML round-trip tests for the new single-string deserialization paths.
AuthTls.rootandAuthConfig.domainsboth gain single-string TOML acceptance, but unlikeServerTlsConfiginserver.rs, there are no tests verifying this behaviour. A string-vs-array test analogous totest_tls_string_or_arrayinserver.rswould confirm theOneOrManywiring is correct for both fields.✅ Suggested tests
#[test] fn test_toml_auth_tls_root_string_or_array() { let single: AuthTls = toml::from_str(r#"root = "ca.pem""#).unwrap(); assert_eq!(single.root, vec![PathBuf::from("ca.pem")]); let array: AuthTls = toml::from_str(r#"root = ["ca1.pem", "ca2.pem"]"#).unwrap(); assert_eq!(array.root, vec![PathBuf::from("ca1.pem"), PathBuf::from("ca2.pem")]); } #[test] fn test_toml_auth_domains_string_or_array() { let single: AuthConfig = toml::from_str(r#"domains = "cdn.moq.dev""#).unwrap(); assert_eq!(single.domains, vec!["cdn.moq.dev"]); let array: AuthConfig = toml::from_str(r#"domains = ["cdn.moq.dev", "api.moq.dev"]"#).unwrap(); assert_eq!(array.domains, vec!["cdn.moq.dev", "api.moq.dev"]); }Also applies to: 229-309
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-relay/src/auth.rs` around lines 173 - 183, Add TOML round-trip unit tests that verify single-string and array deserialization for AuthTls.root and AuthConfig.domains: create tests similar to server.rs::test_tls_string_or_array that call toml::from_str with a single string and with an array and assert the resulting AuthTls.root (Vec<PathBuf>) and AuthConfig.domains (Vec<String/str>) contain the expected elements; place tests near the existing config tests so they run with cargo test and ensure serde_as OneOrMany behavior is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-native/src/server.rs`:
- Around line 714-736: The test test_tls_string_or_array currently only covers
cert and key as bare strings; update the single-string fixture and assertions to
also include generate and root (both annotated with #[serde_as(as =
"OneOrMany")] on ServerTlsConfig) so their single-string deserialization is
exercised: add generate = "localhost" and root = "ca.pem" to the single TOML,
then assert config.generate == vec!["localhost".to_string()] and config.root ==
vec![PathBuf::from("ca.pem")], keeping the existing cert and key checks.
---
Nitpick comments:
In `@rs/moq-relay/src/auth.rs`:
- Around line 173-183: Add TOML round-trip unit tests that verify single-string
and array deserialization for AuthTls.root and AuthConfig.domains: create tests
similar to server.rs::test_tls_string_or_array that call toml::from_str with a
single string and with an array and assert the resulting AuthTls.root
(Vec<PathBuf>) and AuthConfig.domains (Vec<String/str>) contain the expected
elements; place tests near the existing config tests so they run with cargo test
and ensure serde_as OneOrMany behavior is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3a83ab7-73e0-4ba4-a3c0-94a3f73f1e91
📒 Files selected for processing (4)
rs/moq-native/src/client.rsrs/moq-native/src/server.rsrs/moq-relay/src/auth.rsrs/moq-relay/src/web.rs
Comment on lines
+714
to
+736
| fn test_tls_string_or_array() { | ||
| // Single string should deserialize into a Vec with one entry. | ||
| let single = r#" | ||
| cert = "cert.pem" | ||
| key = "key.pem" | ||
| "#; | ||
| let config: ServerTlsConfig = toml::from_str(single).unwrap(); | ||
| assert_eq!(config.cert, vec![PathBuf::from("cert.pem")]); | ||
| assert_eq!(config.key, vec![PathBuf::from("key.pem")]); | ||
|
|
||
| // TOML arrays should still work. | ||
| let array = r#" | ||
| cert = ["a.pem", "b.pem"] | ||
| key = ["a.key", "b.key"] | ||
| generate = ["localhost"] | ||
| root = ["ca.pem"] | ||
| "#; | ||
| let config: ServerTlsConfig = toml::from_str(array).unwrap(); | ||
| assert_eq!(config.cert, vec![PathBuf::from("a.pem"), PathBuf::from("b.pem")]); | ||
| assert_eq!(config.key, vec![PathBuf::from("a.key"), PathBuf::from("b.key")]); | ||
| assert_eq!(config.generate, vec!["localhost".to_string()]); | ||
| assert_eq!(config.root, vec![PathBuf::from("ca.pem")]); | ||
| } |
Contributor
There was a problem hiding this comment.
generate and root single-string cases are not covered by the test.
The single fixture only sets cert and key; generate and root are never exercised as bare strings, though both have #[serde_as(as = "OneOrMany")] applied. The array path tests all four fields, but the single-string path should too.
🛠️ Proposed fix
let single = r#"
cert = "cert.pem"
key = "key.pem"
+ generate = "localhost"
+ root = "ca.pem"
"#;
let config: ServerTlsConfig = toml::from_str(single).unwrap();
assert_eq!(config.cert, vec![PathBuf::from("cert.pem")]);
assert_eq!(config.key, vec![PathBuf::from("key.pem")]);
+ assert_eq!(config.generate, vec!["localhost".to_string()]);
+ assert_eq!(config.root, vec![PathBuf::from("ca.pem")]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rs/moq-native/src/server.rs` around lines 714 - 736, The test
test_tls_string_or_array currently only covers cert and key as bare strings;
update the single-string fixture and assertions to also include generate and
root (both annotated with #[serde_as(as = "OneOrMany")] on ServerTlsConfig) so
their single-string deserialization is exercised: add generate = "localhost" and
root = "ca.pem" to the single TOML, then assert config.generate ==
vec!["localhost".to_string()] and config.root == vec![PathBuf::from("ca.pem")],
keeping the existing cert and key checks.
This matches the example TOML convention where cert/key/etc. are written as plain strings, and round-trips a single-element Vec back to a string rather than a one-element array.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1376.
Several TLS/auth config fields in the relay are typed as
Vec<_>but were documented in the example TOMLs (e.g.demo/relay/prod.toml) as plain strings. Loading those configs fails with:Apply
serde_with::OneOrMany<_, PreferMany>(already used elsewhere in the codebase, e.g.ClusterConfig.connect,PublicDetailed.subscribe/publish) so a bare string and a TOML array both deserialize into aVec.Affected fields
server.tls.cert,server.tls.key,server.tls.generate,server.tls.root—moq_native::ServerTlsConfigtls.root—moq_native::ClientTlsweb.https.root—moq_relay::HttpsConfigauth.tls.root—moq_relay::AuthTlsauth.domains—moq_relay::AuthConfigCLI behavior is unchanged (still
value_delimiter = ','/ repeated flags). Existing array-form TOML configs continue to work.Test plan
cargo check -p moq-native -p moq-relay --all-targetscargo clippy -p moq-native -p moq-relay --all-targets -- -D warningscargo fmt --all --checkcargo test -p moq-native --lib server::tests(newtest_tls_string_or_arraycovers both single-string and array forms)cargo test -p moq-relay --lib(75 tests pass)Generated by Claude Code