Skip to content

docs(serde): document and test supported serde patterns for IPv4 / IPv6 / FixedString#427

Open
gregvirgin-ls wants to merge 1 commit into
ClickHouse:mainfrom
gregvirgin-ls:fix-ipv4-fixedstring-serde-row
Open

docs(serde): document and test supported serde patterns for IPv4 / IPv6 / FixedString#427
gregvirgin-ls wants to merge 1 commit into
ClickHouse:mainfrom
gregvirgin-ls:fix-ipv4-fixedstring-serde-row

Conversation

@gregvirgin-ls
Copy link
Copy Markdown
Contributor

Summary

This is not a bug fix — the wire format is already correct. What was missing was:

  • An explicit rustdoc statement of the required-annotation contract for Ipv4Addr (clickhouse::serde::ipv4 is required because the default Ipv4Addr serde produces 4 network-order bytes whereas the IPv4 wire format is little-endian u32).
  • A symmetric helper for Ipv6Addr (clickhouse::serde::ipv6) — pass-through, but available so mixed-family row definitions read consistently.
  • Byte-level unit tests pinning the round-trip wire format for all three shapes.
  • A live integration test that exercises the documented annotations against a real ClickHouse server with schema validation enabled.

What changed

  1. src/serde.rs: rustdoc on clickhouse::serde::ipv4 made explicit; new clickhouse::serde::ipv6 pass-through helper.
  2. src/rowbinary/tests.rs: byte-level round-trip tests under mod ip_and_fixed_string covering IPv4, IPv6 (default and helper), and FixedString(4).
  3. tests/it/ip.rs: new ipv4_ipv6_fixedstring_round_trip test creating a table with all three column types, inserting via #[derive(Row, Serialize)], fetching with validation enabled, and asserting both Rust-side equality and CH-side toString(...) equivalence.

What is NOT in this PR

  • A PaddedString<N> (or similar) newtype that would let users write String and have it auto-pad-or-truncate to FixedString(N). Out of scope; if maintainers want it, follow-up.
  • Any change to existing Ipv4Addr / Ipv6Addr serde behaviour. The contract is unchanged; it's now documented.

Behaviour change

None observable to existing code. The new clickhouse::serde::ipv6 helper is opt-in. Existing users who rely on the default Ipv6Addr serde keep working.

Test matrix

cargo fmt -- --check
cargo clippy --all-targets --no-default-features
cargo clippy --all-targets --all-features
cargo clippy --features rustls-tls
cargo test --workspace --no-default-features
cargo test --workspace
cargo test --workspace --all-features
cargo rustdoc -p clickhouse --all-features -- -D warnings --cfg docsrs

The live IT test (tests/it/ip.rs::ipv4_ipv6_fixedstring_round_trip) requires a running ClickHouse instance, per the existing convention for tests/it/.

Downstream motivation

A non-trivial fraction of clickhouse-rs consumers we've looked at use String columns to hold IPs because the round-trip via Ipv4Addr / Ipv6Addr was opaque enough to be distrusted in production. Putting the contract in rustdoc plus pinning it with a live IT test resolves the discoverability question.

…/FixedString

Three changes that retire client-side workarounds for the spec items in
upstream-clickhouse-rs.md §1.4. None of these is a "fix" in the
demonstrated-bug sense — the wire format is already correct; what was
missing is documentation of the required-annotation contract and tests
that pin it byte-for-byte.

- Add `clickhouse::serde::ipv6` as a pass-through helper for IPv6
  columns, mirroring the existing `clickhouse::serde::ipv4` helper.
  The default `Ipv6Addr` serde already produces network-order bytes
  that match the RowBinary wire format for `IPv6`, so the helper is
  an explicit no-op pass-through. Two reasons to ship it:
    1. Symmetry with the `ipv4` annotation makes mixed-family row
       definitions read consistently.
    2. Future-proofs against any wire-format change.

- Expand the `clickhouse::serde::ipv4` rustdoc to make the
  required-annotation contract explicit. The default `Ipv4Addr`
  serde produces 4 network-order bytes; the wire format is a
  little-endian u32; the bytes do not match. Schema validation
  rejects the mismatched row, which is the right behaviour but the
  message did not point at the fix.

- Add unit-level round-trip tests in `src/rowbinary/tests.rs`
  covering IPv4 (via helper), IPv6 (via default *and* helper), and
  FixedString(4). Pin the wire format byte-for-byte so future
  regressions are caught without needing a live ClickHouse server.

- Add a live integration test `tests/it/ip.rs::ipv4_ipv6_fixedstring_
  round_trip` that creates a single-table schema with all three
  types, inserts via `#[derive(Row, Serialize)]` with the documented
  annotations, fetches with validation enabled, and asserts both
  Rust-side equality and CH-side `toString(...)` equivalence. Pins
  the contract end-to-end against a real server so a Rust-side serde
  regression *or* a CH-side decoding regression surfaces here.

Variable-length-into-FixedString helpers (e.g. a `PaddedString<N>`
newtype) are intentionally out of scope here and left as a follow-up.

Motivated by downstream ClickFlow code that uses `String` columns to
hold IPs because the IPv4/IPv6 round-trip was opaque enough to be
distrusted in production. With the docstring + the live IT test, the
recommended pattern is discoverable from `cargo doc` and verified
against a real CH instance.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant