sf_core: add typed JSON parameter-binding builder - #1
Closed
zeroshade wants to merge 3 commits into
Closed
Conversation
Add a public sf_core::bindings module that constructs Snowflake
parameter-binding JSON from typed Rust values via a ParamValue enum and
two builders (to_json_single for one row, to_json_arrays for array
binding). The wire-text encodings match the ODBC converters in
odbc/src/conversion byte-for-byte, so a query bound through either path
produces the same server-side value.
Until now sf_core accepted only pre-serialized JSON binding bytes
(BindingType::Json points at raw UTF-8), leaving each caller and test to
hand-write the {"1": {"type", "value"}} payload. The new builder gives
Rust callers a typed, reusable entry point and centralizes the encoding
rules: every value is emitted as a JSON string, TIMESTAMP_LTZ is tagged
TEXT, TIMESTAMP_TZ applies the +1440 offset bias, and a fully-null column
is tagged ANY. Year-month and day-time intervals are accepted as
pre-formatted literals and documented on their enum variants.
Covered by 43 unit tests spanning every logical type, NULL handling,
array binding, string escaping, and the mixed-type and mismatched-length
error paths. cargo build, test, clippy, and fmt are clean for the crate.
Address a code-review finding on the new sf_core::bindings builder. ParamValue::TimestampTz exposes offset_minutes as an arbitrary i32, and encode_tz computed offset_minutes + 1440 unchecked, which panics on overflow in debug builds and wraps into a bogus wire offset in release. Validate offset_minutes against the driver's legal +/-1439-minute range (matching the ODBC converter) before applying the bias, returning the new BindingError::TimestampTzOffsetOutOfRange for anything wider. The range check makes the biased sum provably overflow-safe. Adds regression tests for the i32::MAX rejection and the +/-1439 boundaries.
Address a second code-review finding on sf_core::bindings. encode_wallclock
zero-padded the absolute year and prepended a sign, so year -1 rendered as
"-0001". The ODBC put_year formatter it mirrors counts the sign within the
minimum width of four, producing "-001", so TIMESTAMP_LTZ bind text diverged
for proleptic negative years.
Format the signed year with format!("{:04}", year), which counts the sign in
the width and matches the ODBC output for every year while leaving positive
years unchanged. Adds a negative-year regression test.
Owner
Author
|
Superseded by the upstream PR snowflakedb#1335. |
zeroshade
pushed a commit
that referenced
this pull request
Aug 11, 2026
## What Adds an honest end-to-end proxy test that drives a real **PUT + GET** against a **live Snowflake account** and its backing cloud storage over HTTPS, through a genuine **`mitmdump` (mitmproxy) subprocess** — proxy software the driver team did not write. This is the follow-up to the hermetic CONNECT-tunnel suite (`integration::http::proxy_transfer`, snowflakedb#829). That suite drives transfers through a Rust CONNECT harness we wrote from the same reading of the protocol — so a *shared* misunderstanding stays green — and it targets wiremock, not a real Snowflake backend/cloud. `mitmdump` is an independent implementation, and this runs against a live account + real cloud storage, so it's the genuinely missing proof. Ports the mechanism from `snowflake-connector-python`'s `test/integ/test_proxies.py` (`MitmClient` + `port_detector_addon`). ## How the test stays honest - **Interception proof is structural (CA-trust exclusivity), not a log assertion.** The connection trusts **only** mitmdump's generated CA via `custom_root_store_path`, which *replaces* the built-in roots (reqwest path calls `tls_built_in_root_certs(false)`; the S3 path builds from an empty root store). So a byte-for-byte PUT+GET succeeding over HTTPS is only possible if every leg (login, upload, download) transited mitmdump — a direct hit to real S3/Azure/GCS would present a public-CA leaf this store rejects. Transfer success *itself* is the proof. This is stronger than the legacy Python test, which asserts only `UPLOADED` and proves nothing about routing — and it needs no host-logging addon (the addon is port-detection only). - **Negative control.** A dead loopback proxy port must fail the connection. With valid credentials, a silently bypassed proxy would connect directly and succeed — so `expect_err` is load-bearing. (In UD the proxy is connection-scoped — `proxy_host` is not mutable after connect — so this fails at the login handshake, a strictly stronger statement than "the PUT/GET fails".) - **Mutation self-check.** Reverting the `ProxyConfig`-into-`StageInfo` wiring makes the transfer dial storage directly, which then fails the TLS handshake against the real backend's genuine cert (trust store holds only the mitm CA). So the test genuinely depends on the proxy plumbing. - **Scoped by DI, no global state.** proxy host/port, `use_proxy_env=false`, CA trust, and `crl_check_mode=DISABLED` (UD's equivalent of the legacy `disable_ocsp_checks=True`, for mitmdump's responder-less MITM leaf certs) are all set as **connection options on this test's own client** — never a process-global env var (`HTTP_PROXY`/`REQUESTS_CA_BUNDLE`) or toggle. (code-review-design-discipline #1; flaky-tests `ud-no-environment-variable-side-effects`.) - **Lifecycle safety.** mitmdump spawns on an OS-assigned port with a per-instance `confdir` (CA scoped + auto-cleaned, not `~/.mitmproxy`), readiness is deadline-polled in three stages (CA file → addon-reported port → accepting listener; no bare sleep), stdout/stderr redirect to files (no pipe-buffer deadlock; surfaced on failure), and the process is killed on `Drop`. ## Cloud-agnostic by CI lane Via `with_default_jwt_auth_params`, the one test exercises S3 on `aws`, Azure Blob on `azure`, GCS on `gcp` — no per-cloud branching. Gated `#[ignore]` for the post-merge lane (see CI section). ## S3 implementation verified against (requirement 10) This test is the real-world tie-breaker between the in-flight S3 approaches. It is stacked on **snowflakedb#829** (`SNOW-3850381-proxy-transfer-tests`), whose ancestry includes **snowflakedb#826's reqwest/Smithy S3 adapter** — so it runs against snowflakedb#826, not snowflakedb#842's hand-rolled CONNECT-tunnel connector or the `hyper-http-proxy` alternative. ## CI Runs on every run of the `test` job (PR, push, schedule) — the test itself is ~4s once compiled, and the compile is already paid for by the coverage step that runs on every PR anyway, so there's no real cost to gating it further. `mitmdump --version` right after `pipx install` fails the job outright if the binary lands off-PATH, so this never silently skips and reports green. (Local runs still skip with a visible message if `mitmdump` isn't installed — that's a dev-ergonomics path CI can't reach, since the version gate runs first there.) Selected via the `mitmdump_proxy` substring already present in both test *function* names, not the `proxy_live_mitm` file path — a future file rename can't silently make the filter match nothing. Confirmed passing end-to-end: `mitmproxy 12.2.3` installed, both tests `ok` (`2 passed; 0 failed`). ## Run locally ``` cargo test -p sf_core --test e2e_tests --features protobuf \ put_get::proxy_live_mitm -- --ignored --nocapture ``` 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> GitOrigin-RevId: 02e4baf
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
1 participant
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.
This change adds a public
sf_core::bindingsmodule that constructs Snowflake parameter-binding JSON directly from typed Rust values. It introduces aParamValueenum covering every Snowflake logical type the driver binds, and two builders:to_json_singlefor a single parameter set andto_json_arraysfor array (multi-row) binding. The wire-text encoding for each value is byte-for-byte identical to the existing ODBC converters inodbc/src/conversion, so a statement bound through either path yields the same server-side value.Until now
sf_coreaccepted parameter bindings only as pre-serialized JSON bytes:BindingType::Jsonpoints at raw UTF-8 that the core validates and forwards verbatim. Every consumer and test therefore had to hand-assemble the{"1": {"type", "value"}}payload, duplicating the encoding rules and inviting subtle drift from the ODBC behavior. This module gives Rust callers a single typed entry point and one authoritative place for the encoding rules.The design deliberately mirrors the ODBC converters rather than inventing a new format. A few rules that are easy to get wrong are centralized here: every
valueis emitted as a JSON string (never a bare number or boolean),TIMESTAMP_LTZis taggedTEXTand rendered as a bare wall-clock literal,TIMESTAMP_TZencodes epoch nanoseconds plus a+1440-biased offset, and a column that is entirely NULL is taggedANY.INTERVAL_YEAR_MONTHandINTERVAL_DAY_TIMEare accepted as pre-formatted literal strings — matching the ODBCWriteWireimplementations, which are the identity for those types — and the accepted grammar is documented on the enum variants.The primary risk is divergence from the ODBC encoders. That is mitigated by porting each encoding from the corresponding converter and locking the behavior down with tests built from the same expected values the ODBC path produces.
Testing: 43 unit tests cover every logical type, NULL handling, single and array binding, JSON string escaping, and the mixed-type and mismatched-length error paths, asserting on parsed JSON structure rather than key order.
cargo build,cargo test,cargo clippy -D warnings, andcargo fmt --checkare clean for the crate.