Skip to content

refactor(sql_builder): consolidate slot SQL preparation into dedicated functions - #109

Merged
isdaniel merged 1 commit into
mainfrom
refactor/collapse-deserializer-policy-slot-glue-test-seam
Jul 26, 2026
Merged

refactor(sql_builder): consolidate slot SQL preparation into dedicated functions#109
isdaniel merged 1 commit into
mainfrom
refactor/collapse-deserializer-policy-slot-glue-test-seam

Conversation

@isdaniel

Copy link
Copy Markdown
Owner
  • Deserializer error policy (deserializer.rs): collapse the two ~220-line
    serde impls behind a compile-time ErrorPolicy trait (Strict/Lenient) plus one
    shared coercion impl. Coercion rules and error messages are now single-sourced.
    Strict's zero-copy visit_borrowed_* + #[inline] path is preserved via
    monomorphization. The two Deserializer impls stay separate (a single impl is
    impossible under serde's 'de borrow model) — they differ only in the final
    borrowed-vs-owned visit.
  • Slot glue (sql_builder.rs, both backends): add
    prepare_{create,alter,read}_slot to pair the version-check with the SQL builder
    in one place; both backends now call them instead of re-pairing. The real seam
  • Frame-injection test seam (libpq.rs, stream.rs): add libpq's matching
    null_for_testing_with_frames; 8 shared-pump tests now run on both backends,
    closing the libpq COPY-ingress coverage hole. ..._terminates_after_transport_stop
    stays rustls-only (it drives PQputCopyEnd, which segfaults on libpq's
    null-pointer conn; that path is backend-specific and integration-tested).

@isdaniel

Copy link
Copy Markdown
Owner Author

@copilot review

@codspeed-hq

codspeed-hq Bot commented Jul 26, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 50 untouched benchmarks


Comparing refactor/collapse-deserializer-policy-slot-glue-test-seam (235752d) with main (df60b02)

Open in CodSpeed

@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.02632% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.96%. Comparing base (df60b02) to head (235752d).

Files with missing lines Patch % Lines
src/deserializer.rs 98.52% 4 Missing ⚠️
src/sql_builder.rs 91.30% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #109      +/-   ##
==========================================
+ Coverage   94.76%   94.96%   +0.20%     
==========================================
  Files          27       27              
  Lines       20678    20805     +127     
==========================================
+ Hits        19596    19758     +162     
+ Misses       1082     1047      -35     

☔ View full report in Codecov by Harness.
📢 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.

Copilot AI commented Jul 26, 2026

Copy link
Copy Markdown

@copilot review

Reviewed 5805412; no actionable correctness or security findings. Local fmt, both backend unit-test suites, and Clippy pass. The Fuzz check failure is a nightly rustc ICE while compiling Tokio, not a project-code failure.

@isdaniel
isdaniel force-pushed the refactor/collapse-deserializer-policy-slot-glue-test-seam branch 2 times, most recently from 5a94caa to dfd7037 Compare July 26, 2026 14:36
@isdaniel

Copy link
Copy Markdown
Owner Author

@copilot review

Copilot AI commented Jul 26, 2026

Copy link
Copy Markdown

@copilot review

Second review pass done. Verified in more depth this time:

  • Lenient numeric semantics preserved: lenient now routes through parse_int_signed/parse_int_unsigned instead of FromStr. Checked the edge case — parse_int_signed accumulates negatives via checked_sub, so i64::MIN still parses (covered at deserializer.rs:2974). Invalid-UTF-8 integer input still yields the invalid UTF-8 for {type} message, since numeric_parse_msg validates on the cold path.
  • Lenient bytes/byte_buf/unit/enum are behavior-identical to the pre-refactor impls (same messages, same fallbacks, single error push).
  • Slot glue: prepare_* keeps check-before-build ordering in both backends.
  • libpq test seam is inside a #[cfg(test)] impl, so it isn't dead code in release builds.

One note: lenient-mode message wording changed for two cases — integer parse failures no longer append the FromStr cause, and invalid bool now reports the (expected t/f/...) form via lossy_token. That's the intended single-sourcing with strict, and no test asserts the old text.

Fixed two comment defects introduced when the commit was amended/reflowed: a duplicated sentence fragment in stream.rs and a garbled sentence (missing word) plus the dropped doc line on prepare_create_slot in sql_builder.rsdd7f4c2. fmt, clippy, and both backend suites (1246 libpq / 1240 rustls) pass.

Fuzz CI remains red from a nightly rustc ICE while compiling Tokio, unrelated to this branch.

@copilot review

Correction: the comment fixes landed in 8cbab4a, not the hash cited above.

…d functions

refactor(deserializer): implement strict and lenient error handling policies
refactor(connection): streamline replication slot SQL commands in PgReplicationConnection and NativeConnection
test(stream): remove rustls-tls feature checks from tests for consistency
@isdaniel
isdaniel force-pushed the refactor/collapse-deserializer-policy-slot-glue-test-seam branch from 8cbab4a to 235752d Compare July 26, 2026 14:44
@isdaniel

Copy link
Copy Markdown
Owner Author

@copilot review

Copilot AI commented Jul 26, 2026

Copy link
Copy Markdown

@copilot review

Reviewed 235752d; no actionable correctness or security findings. Local fmt, both backend unit-test suites, and Clippy pass. CI and Benchmarks pass, and the pinned-nightly Fuzz workflow now passes all three targets; Code Quality is still running.

@isdaniel
isdaniel merged commit 99274d5 into main Jul 26, 2026
26 checks passed
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.

2 participants