feat(config): add serde Serialize/Deserialize to RTCConfiguration by nightness · Pull Request #81 · webrtc-rs/rtc

nightness · 2026-04-01T16:42:52Z

Summary

Add #[derive(Serialize, Deserialize)] with #[serde(rename_all = "camelCase")] to RTCConfiguration
Add #[serde(default)] to RTCConfiguration and RTCIceServer for partial JSON deserialization (e.g. only providing iceServers)
Skip certificates field during both serialization and deserialization to avoid persisting private-key material
Replace old commented-out TODO test with structured round-trip JSON test using serde_json::Value assertions
Add partial deserialization test to verify defaults for omitted fields
Enables JavaScript/JSON config interoperability with W3C camelCase naming

Note: serde is already a hard dependency used across 44+ files in the crate, so no feature gate is needed.

Test plan

cargo test -p rtc test_configuration_json_round_trip
cargo test -p rtc test_configuration_json_partial_deserialize
Verify camelCase keys (iceServers, iceTransportPolicy, etc.) in serialized output
Verify deserialization from camelCase JSON
Verify partial JSON (only iceServers) deserializes with defaults
Verify certificates with non-empty vec are omitted from serialized JSON
cargo fmt --check passes
cargo clippy -p rtc passes

🤖 Generated with Claude Code

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR adds opt-in Serde support for RTCConfiguration to enable JSON/JavaScript interoperability, and introduces a JSON round-trip test to validate camelCase serialization/deserialization.

Changes:

Derives Serialize/Deserialize for RTCConfiguration with camelCase field naming.
Skips certificates during serialization/deserialization to avoid persisting private-key material.
Replaces an old commented TODO with a working JSON round-trip test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-04T14:15:25Z

 use crate::peer_connection::certificate::RTCCertificate;
 pub use crate::peer_connection::transport::ice::server::RTCIceServer;
 use rcgen::KeyPair;
+use serde::{Deserialize, Serialize};


The PR description says Serde support is “opt-in” behind a serde feature gate, but this diff adds unconditional serde imports/derives and an unconditional serde_json test. If the feature really is meant to be optional, gate the import, derives, and the test with cfg(feature = "serde") / cfg_attr(feature = "serde", derive(...)) (and similarly for the serde attributes) so builds without the feature don’t fail.

Addressed: serde is an unconditional hard dependency used across 44+ files in this crate (statistics, SDP, transport types). Gating it behind a feature would be a much larger refactor outside this PR's scope. The PR description has been updated to clarify this. This comment should be marked outdated.

Copilot · 2026-04-04T14:15:25Z

+    #[test]
+    fn test_configuration_json_round_trip() {


The PR description says Serde support is “opt-in” behind a serde feature gate, but this diff adds unconditional serde imports/derives and an unconditional serde_json test. If the feature really is meant to be optional, gate the import, derives, and the test with cfg(feature = "serde") / cfg_attr(feature = "serde", derive(...)) (and similarly for the serde attributes) so builds without the feature don’t fail.

Same as above — serde is unconditional in this crate. PR description updated to clarify. Should be marked outdated.

Copilot · 2026-04-04T14:15:25Z

+            .with_rtcp_mux_policy(RTCRtcpMuxPolicy::Require)
+            .build();
+
+        let json = serde_json::to_string(&original).expect("serialize");


The PR description says Serde support is “opt-in” behind a serde feature gate, but this diff adds unconditional serde imports/derives and an unconditional serde_json test. If the feature really is meant to be optional, gate the import, derives, and the test with cfg(feature = "serde") / cfg_attr(feature = "serde", derive(...)) (and similarly for the serde attributes) so builds without the feature don’t fail.

Same as above — serde is unconditional in this crate. PR description updated to clarify. Should be marked outdated.

Copilot · 2026-04-04T14:15:26Z

+        assert!(json.contains("iceServers"), "camelCase key expected");
+        assert!(json.contains("stun:stun.l.google.com:19302"));
+
+        let restored: RTCConfiguration = serde_json::from_str(&json).expect("deserialize");


The PR description says Serde support is “opt-in” behind a serde feature gate, but this diff adds unconditional serde imports/derives and an unconditional serde_json test. If the feature really is meant to be optional, gate the import, derives, and the test with cfg(feature = "serde") / cfg_attr(feature = "serde", derive(...)) (and similarly for the serde attributes) so builds without the feature don’t fail.

Same as above — serde is unconditional in this crate. PR description updated to clarify. Should be marked outdated.

codecov · 2026-04-04T14:20:14Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.24%. Comparing base (9feb4a3) to head (537d950).

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
+ Coverage   71.17%   71.24%   +0.06%     
==========================================
  Files         442      442              
  Lines       67330    67330              
==========================================
+ Hits        47922    47966      +44     
+ Misses      19408    19364      -44

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Add `#[serde(default)]` to RTCConfiguration and RTCIceServer so partial JSON (e.g. only `iceServers`) deserializes correctly - Replace brittle `contains()` test assertions with structured serde_json::Value checks on specific keys/values - Add test for partial deserialization to verify default behavior - Fix long lines flagged by cargo fmt Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (2)

rtc/src/peer_connection/transport/ice/server.rs:18

#[serde(default)] on the whole RTCIceServer makes the required urls field optional during deserialization (missing urls becomes an empty Vec). Because RTCIceServer::validate() currently treats an empty urls list as OK, invalid ICE server entries like {} can deserialize and pass validation. Prefer putting #[serde(default)] only on optional fields (username, credential) and/or enforce urls non-empty in validate() (or via a custom deserializer).

#[derive(Default, Debug, Clone, PartialEq, Serialize, Deserialize, Hash)]
#[serde(default)]
pub struct RTCIceServer {
    pub urls: Vec<String>,
    pub username: String,
    pub credential: String,

rtc/src/peer_connection/transport/ice/server.rs:18

W3C RTCIceServer.urls accepts either a single string or a sequence of strings. With urls: Vec<String>, JSON like { "urls": "stun:..." } will fail to deserialize (even though it’s spec-valid / common in JS). If JS/JSON interop is a goal, consider a custom (de)serializer or an untagged enum to accept both string and array forms.

pub struct RTCIceServer {
    pub urls: Vec<String>,
    pub username: String,
    pub credential: String,

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-08T08:07:34Z

+        assert_eq!(restored.bundle_policy, RTCBundlePolicy::MaxBundle);
+        assert_eq!(restored.rtcp_mux_policy, RTCRtcpMuxPolicy::Require);
+        // certificates are skipped — restored config gets empty Vec
+        assert!(restored.certificates.is_empty());
+    }


test_configuration_json_round_trip asserts restored.certificates.is_empty(), but original never sets certificates to a non-empty value, so this doesn’t actually prove that serialization omitted the field (it would round-trip as empty regardless). Consider making original.certificates non-empty and asserting the serialized JSON has no certificates key.

Addressed: test now generates a real RTCCertificate, asserts it's present before serialization, and verifies the JSON has no "certificates" key — proving serde(skip) works. Should be marked outdated.

- Remove duplicated struct-level doc paragraph on RTCConfiguration - Update #[serde(skip)] comment to clarify it affects both serialization AND deserialization (not just serialization) - Strengthen round-trip test: set non-empty certificates on the original config and assert the serialized JSON contains no "certificates" key, proving serde(skip) works as intended Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…lexive_test are disabled Both test files exist but reference turn::auth / turn::server / turn::relay APIs from a different TURN library than rtc-turn. Enabling them requires porting to the rtc-turn API. Replace the misleading //TODO comments with a clear explanation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…xplanations Five test locations had //TODO or /* TODO: */ markers implying they just needed uncommenting. Investigation shows none can be trivially enabled: - sdp/mod.rs: sdp_test.rs imports crate::api::APIBuilder which belongs to the old async API and does not exist in this sans-IO crate. - configuration/mod.rs: test_configuration_json is Go source code, never ported. - configuration/media_engine.rs: media_engine_test.rs does not exist. - rtc-ice/src/candidate/mod.rs: relay/srflx tests use turn::auth/turn::server APIs from a different TURN library than rtc-turn (done in previous commit). Replace all with accurate comments so future contributors know what work is needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- Wrap `RTCConfiguration` in backticks and clarify it refers to the Rust type used in the module (configuration/mod.rs comment) - Format `turn::auth`, `turn::server`, `turn::relay` as separate backtick-delimited identifiers for clarity (candidate/mod.rs comment) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Adds serde round-trip support to RTCConfiguration using W3C camelCase field names (iceServers, iceTransportPolicy, bundlePolicy, rtcpMuxPolicy, peerIdentity, iceCandidatePoolSize). All dependent types already had serde derives (RTCIceServer, RTCBundlePolicy, RTCIceTransportPolicy, RTCRtcpMuxPolicy, RTCSdpSemantics). Certificates are excluded via #[serde(skip)] because they contain private-key material. Use RTCCertificate::serialize_pem() / from_pem() to persist certificates separately. Adds test_configuration_json_round_trip to replace the previously commented-out Go test with a Rust equivalent. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- Add `#[serde(default)]` to RTCConfiguration and RTCIceServer so partial JSON (e.g. only `iceServers`) deserializes correctly - Replace brittle `contains()` test assertions with structured serde_json::Value checks on specific keys/values - Add test for partial deserialization to verify default behavior - Fix long lines flagged by cargo fmt Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

- Remove duplicated struct-level doc paragraph on RTCConfiguration - Update #[serde(skip)] comment to clarify it affects both serialization AND deserialization (not just serialization) - Strengthen round-trip test: set non-empty certificates on the original config and assert the serialized JSON contains no "certificates" key, proving serde(skip) works as intended Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

nightness · 2026-04-10T05:26:13Z

Rebased onto PR #79's branch. Merge #79 first, then this PR applies cleanly. Previous branch structure has been cleaned up to avoid merge conflicts.

rainliu requested a review from Copilot April 4, 2026 14:08

Copilot AI reviewed Apr 4, 2026

View reviewed changes

Copilot started reviewing on behalf of rainliu April 4, 2026 14:28 View session

nightness requested a review from Copilot April 8, 2026 08:01

Copilot started reviewing on behalf of nightness April 8, 2026 08:01 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

nightness and others added 6 commits April 10, 2026 00:16

nightness force-pushed the feat/config-serde branch from a3d6c9a to bc7a59c Compare April 10, 2026 05:23

nightness mentioned this pull request Apr 10, 2026

Summary: 19 PRs ready for review #87

Open

Uh oh!

Conversation

nightness commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

Uh oh!

nightness Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

Uh oh!

nightness Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

Uh oh!

nightness Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

Uh oh!

nightness Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Apr 4, 2026

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

nightness Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

nightness commented Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nightness commented Apr 1, 2026 •

edited

Loading