Skip to content

feat(wacore-binary): zero-copy Serialize for NodeRef type family - #539

Merged
jlucaso1 merged 2 commits into
mainfrom
feat/serde-serialize-ref-types
Apr 14, 2026
Merged

feat(wacore-binary): zero-copy Serialize for NodeRef type family#539
jlucaso1 merged 2 commits into
mainfrom
feat/serde-serialize-ref-types

Conversation

@jlucaso1

@jlucaso1 jlucaso1 commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Add serde::Serialize impls (behind #[cfg(feature = "serde")]) for all borrowed/ref types: NodeStr, JidRef, ValueRef, NodeContentRef, NodeRef, and OwnedNodeRef
  • Zero allocation during serialization — everything borrows from the yoke buffer via Deref/AsRef
  • Output is byte-identical to the owned type serialization, enabling downstream code to drop to_owned_node() clones when serializing OwnedNodeRef

Motivation

OwnedNodeRef exists to avoid cloning decoded nodes, but serialization previously required to_owned_node() which defeats the purpose. With these impls, consumers like whatsapp-rust-bridge can serialize directly from the yoke buffer:

// Before: allocates cloned Node
let val = serde_json::to_value(&node.to_owned_node())?;

// After: zero-copy from yoke buffer
let val = serde_json::to_value(node.get())?;

Test plan

  • cargo clippy --all --tests — clean
  • cargo test -p wacore-binary --features serde -- serde_tests — 5 parity tests pass
    • node_ref_serializes_same_as_node — attrs with String + Jid values, String content
    • nested_nodes_serialize_same — recursive Nodes content
    • bytes_content_serializes_same — Bytes content
    • value_ref_matches_node_value — both String and Jid variants
    • owned_node_ref_serializes_same_as_owned — full round-trip through marshal/unmarshal

Summary by CodeRabbit

  • New Features

    • Optional Serde-based JSON serialization for core data structures (JID, node/value/content types, owned and borrowed node representations).
  • Tests

    • Added Serde unit tests to verify JSON equivalence between owned and borrowed representations, including nested nodes and byte/string variants.

…amily

Add `serde::Serialize` implementations (behind `#[cfg(feature = "serde")]`)
for all borrowed/ref types so that `OwnedNodeRef` can be serialized directly
from the yoke buffer without cloning via `to_owned_node()`.

Types covered: NodeStr, JidRef, ValueRef, NodeContentRef, NodeRef, OwnedNodeRef.

All impls produce byte-identical output to their owned counterparts,
verified by parity tests. Zero allocation during serialization.
@coderabbitai

coderabbitai Bot commented Apr 14, 2026

Copy link
Copy Markdown

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

Adds serde::Serialize implementations for several borrowed reference types in wacore-binary (JidRef, NodeStr, ValueRef, NodeContentRef, NodeRef, OwnedNodeRef), adds serde JSON dev-dependency, and introduces serde-based unit tests validating serialization.

Changes

Cohort / File(s) Summary
Dependency Addition
wacore/binary/Cargo.toml
Added serde_json (workspace = true, features = ["std"]) to [dev-dependencies] for serialization tests.
JID Serialization
wacore/binary/src/jid.rs
Added #[cfg(feature = "serde")] impl serde::Serialize for JidRef<'_> that serializes as a struct "Jid" with fields user, server, agent, device, integrator.
Node Types Serialization & Tests
wacore/binary/src/node.rs
Added #[cfg(feature = "serde")] Serialize impls for NodeStr<'_>, ValueRef<'_>, NodeContentRef<'_>, NodeRef<'_>, and OwnedNodeRef, plus serde unit tests asserting JSON equivalence between owned and borrowed representations and validating variant/struct framing.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

api-design

Poem

🐇 I nibble bytes and names so neat,

I tuck your refs in JSON sheets,
From Jid to Node, I hum and hop,
Serialize! — then stop to plop. ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 47.62% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main change: adding zero-copy Serialize implementations for the NodeRef type family in wacore-binary.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/serde-serialize-ref-types

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@jlucaso1

Copy link
Copy Markdown
Collaborator Author

@claude review

@claude

claude Bot commented Apr 14, 2026

Copy link
Copy Markdown

Claude finished @jlucaso1's task in 2m 15s —— View job


Code Review: Zero-copy Serialize for NodeRef type family

  • Read changed files and gather context
  • Review Cargo.toml changes
  • Review jid.rs serialization impl
  • Review node.rs serialization impls and tests
  • Post review feedback

Overall: Looks good. Clean, well-tested PR. The impls correctly mirror the derive behavior of the owned types, and the test suite verifies JSON parity. A few observations below.


Correctness

The manual Serialize impls carefully reproduce the serde-derive output of their owned counterparts:

  • Struct names match: "Jid", "Node" — same as the derive on Jid/Node
  • Enum names + variant indices match: "NodeValue" (0=String, 1=Jid), "NodeContent" (0=Bytes, 1=String, 2=Nodes) — all match the derive ordering on NodeValue/NodeContent
  • OwnedNodeRef correctly delegates to self.get().serialize(serializer) — elegant

The zero-copy claim holds: NodeStr deref-borrows through to &str, JidRef borrows NodeStr::user, and Cow<'a, [u8]> in NodeContentRef::Bytes borrows via as_ref(). No allocations during serialization.

Nit: Impl placement in jid.rs

The Serialize impl for JidRef (lines 375-387) is placed before the JidRef struct definition (line 389). This compiles fine but reads oddly — you encounter the impl before the type. Consider moving it after the struct definition to match the typical convention used elsewhere in the file (e.g., Jid's derive is on the struct itself).

Observation: Attrs parity is JSON-specific

Node.attrs is Attrs(Vec<...>) — a newtype struct. Its derive emits serialize_newtype_struct("Attrs", &self.0). In contrast, NodeRef.attrs is AttrsRef<'a> = bare Vec<(NodeStr<'a>, ValueRef<'a>)>, so it serializes as a plain sequence.

For JSON (serde_json) this is invisible — serialize_newtype_struct is transparent. The tests confirm byte-identical JSON output. But for non-self-describing formats (bincode, postcard, etc.) the structural difference could surface. This is fine for the stated use case (JSON bridge), but worth documenting if you ever extend serialization to binary formats.

Tests

Solid coverage:

  • node_ref_serializes_same_as_node — attrs with String + Jid values, String content
  • nested_nodes_serialize_same — recursive Nodes content
  • bytes_content_serializes_same — Bytes content
  • value_ref_matches_node_value — both String and Jid variants
  • owned_node_ref_serializes_same_as_owned — full marshal/unmarshal round-trip

Fictitious phone numbers (5550199999) used throughout — good.

Cargo.toml

serde_json added to [dev-dependencies] — correct, since the serde feature only enables dep:serde + compact_str/serde, not serde_json. The test module needs serde_json directly.

Summary

Clean PR with correct impls and thorough parity tests. The only actionable suggestion is the minor impl-before-struct ordering in jid.rs.


@github-actions

github-actions Bot commented Apr 14, 2026

Copy link
Copy Markdown

Benchmark Results

59 unchanged benchmark(s)
Benchmark Current Baseline Change
reporting_token_benchmark::content_extraction_group::bench_content_extraction simple:setup_simple_message() 3,882 3,882 +0.0%
reporting_token_benchmark::content_extraction_group::bench_content_extraction extended:setup_extended_message() 11,867 11,867 +0.0%
reporting_token_benchmark::key_derivation_group::bench_key_derivation 43,401 43,401 +0.0%
reporting_token_benchmark::token_calculation_group::bench_token_calculation 19,365 19,365 +0.0%
reporting_token_benchmark::full_generation_group::bench_full_token_generation simple:setup_full_gen_simple() 68,820 68,820 +0.0%
reporting_token_benchmark::full_generation_group::bench_full_token_generation extended:setup_full_gen_extended() 76,800 76,800 +0.0%
reporting_token_benchmark::message_encoding_group::bench_message_encoding simple:setup_simple_message() 2,217 2,217 +0.0%
reporting_token_benchmark::message_encoding_group::bench_message_encoding extended:setup_extended_message() 5,951 5,951 +0.0%
send_receive_benchmark::dm_send::bench_dm_send text:setup_dm_send() 174,791 174,364 +0.2%
send_receive_benchmark::dm_recv::bench_dm_recv text:setup_dm_recv() 191,637 191,785 -0.1%
send_receive_benchmark::group_send::bench_group_send group_10:setup_group_send_10() 886,177 886,226 -0.0%
send_receive_benchmark::group_send::bench_group_send group_50:setup_group_send_50() 977,582 977,539 +0.0%
send_receive_benchmark::group_send::bench_group_send group_256:setup_group_send_256() 1,462,724 1,462,762 -0.0%
send_receive_benchmark::group_send_skdm::bench_group_send_skdm skdm_10:setup_group_skdm_10() 2,636,654 2,636,761 -0.0%
send_receive_benchmark::group_send_skdm::bench_group_send_skdm skdm_50:setup_group_skdm_50() 9,619,788 9,619,907 -0.0%
send_receive_benchmark::group_send_skdm::bench_group_send_skdm skdm_256:setup_group_skdm_256() 45,638,805 45,638,388 +0.0%
send_receive_benchmark::group_recv::bench_group_recv text:setup_group_recv() 12,573,618 12,614,471 -0.3%
binary_benchmark::marshal_group::bench_marshal_allocating 95,742 95,742 +0.0%
binary_benchmark::marshal_group::bench_marshal_auto_allocating 95,775 95,775 +0.0%
binary_benchmark::marshal_group::bench_marshal_exact_allocating 114,155 114,155 +0.0%
binary_benchmark::marshal_group::bench_marshal_reusing_buffer 102,854 102,854 +0.0%
binary_benchmark::marshal_group::bench_marshal_reusing_buffer_vec_writer 95,842 95,842 +0.0%
binary_benchmark::marshal_group::bench_marshal_long_string 15,748 15,748 +0.0%
binary_benchmark::marshal_group::bench_marshal_auto_long_string 15,792 15,792 +0.0%
binary_benchmark::marshal_group::bench_marshal_exact_long_string 17,581 17,581 +0.0%
binary_benchmark::marshal_group::bench_marshal_huge_bytes_allocating 533,115 533,115 +0.0%
binary_benchmark::marshal_group::bench_marshal_auto_huge_bytes_allocating 532,681 532,681 +0.0%
binary_benchmark::marshal_group::bench_marshal_exact_huge_bytes_allocating 534,042 534,042 +0.0%
binary_benchmark::marshal_group::bench_marshal_many_children_allocating 13,423,541 13,423,541 +0.0%
binary_benchmark::marshal_group::bench_marshal_auto_many_children_allocating 13,367,806 13,367,806 +0.0%
binary_benchmark::marshal_group::bench_marshal_exact_many_children_allocating 26,668,743 26,668,743 +0.0%
binary_benchmark::unmarshal_group::bench_unmarshal small:setup_small_marshaled() 2,498 2,498 +0.0%
binary_benchmark::unmarshal_group::bench_unmarshal large:setup_large_marshaled() 38,500 38,500 +0.0%
binary_benchmark::unpack_group::bench_unpack_uncompressed 785 785 +0.0%
binary_benchmark::unpack_group::bench_unpack_compressed 556,214 556,214 +0.0%
binary_benchmark::attr_parser_group::bench_attr_parser attr_lookup:setup_attr_marshaled() 5,024 5,024 +0.0%
binary_benchmark::roundtrip_group::bench_roundtrip small:setup_small_marshaled() 7,483 7,483 +0.0%
binary_benchmark::roundtrip_group::bench_roundtrip large:setup_large_marshaled() 90,792 90,792 +0.0%
binary_benchmark::roundtrip_group::bench_roundtrip_auto small:setup_small_marshaled() 7,510 7,510 +0.0%
binary_benchmark::roundtrip_group::bench_roundtrip_auto large:setup_large_marshaled() 90,828 90,828 +0.0%
binary_benchmark::roundtrip_group::bench_roundtrip_exact small:setup_small_marshaled() 8,838 8,838 +0.0%
binary_benchmark::roundtrip_group::bench_roundtrip_exact large:setup_large_marshaled() 104,662 104,662 +0.0%
binary_benchmark::child_iteration_group::bench_get_children_by_tag 477,570 477,570 +0.0%
binary_benchmark::jid_optimization_group::bench_jid_to_owned_access jid_access:setup_jid_heavy_marshaled() 13,468 13,468 +0.0%
libsignal_benchmark::dm_group::bench_dm_session_establishment setup:setup_dm_users() 17,414,515 17,293,807 +0.7%
libsignal_benchmark::dm_group::bench_dm_encrypt_first_message first_msg:setup_dm_session() 157,773 157,773 +0.0%
libsignal_benchmark::dm_group::bench_dm_decrypt_first_message decrypt_prekey:setup_dm_with_first_message() 5,511,012 5,511,012 +0.0%
libsignal_benchmark::dm_group::bench_dm_encrypt_subsequent_message subsequent:setup_established_dm_session() 158,602 158,602 +0.0%
libsignal_benchmark::group_messaging_group::bench_group_create_distribution_message create:setup_group_sender() 298,353 298,353 +0.0%
libsignal_benchmark::group_messaging_group::bench_group_encrypt_message encrypt:setup_group_with_distribution() 709,311 709,311 +0.0%
libsignal_benchmark::group_messaging_group::bench_group_decrypt_message decrypt:setup_group_with_encrypted_message() 12,653,785 12,584,905 +0.5%
libsignal_benchmark::conversation_group::bench_full_dm_conversation full:setup_conversation_data() 27,553,401 27,526,352 +0.1%
libsignal_benchmark::signature_group::bench_signature_creation sign:setup_keypair_with_message() 3,467,011 3,467,011 +0.0%
libsignal_benchmark::signature_group::bench_signature_verification verify:setup_keypair_with_message() 125,855,633 123,866,173 +1.6%
libsignal_benchmark::signature_group::bench_key_generation keygen 2,830,452 2,830,452 +0.0%
libsignal_benchmark::session_optimization_group::bench_decrypt_with_previous_session previous_session:setup_with_archived_sessions() 46,003 46,003 +0.0%
libsignal_benchmark::session_optimization_group::bench_out_of_order_decryption out_of_order:setup_out_of_order_messages() 5,089,072 5,089,072 +0.0%
libsignal_benchmark::session_optimization_group::bench_promote_matching_session promote:setup_promote_matching_session() 316,826 316,826 +0.0%
libsignal_benchmark::session_optimization_group::bench_message_key_eviction eviction:setup_message_key_eviction() 14,247,117 14,247,117 +0.0%
No significant changes detected.

- Wrap AttrsRef in newtype-struct serializer to match Attrs derive output
  for non-self-describing formats (bincode, postcard, etc.)
- Move JidRef Serialize impl after the struct definition
@jlucaso1
jlucaso1 merged commit ce620a2 into main Apr 14, 2026
7 of 8 checks passed
@jlucaso1
jlucaso1 deleted the feat/serde-serialize-ref-types branch April 14, 2026 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant