Skip to content

fix: bind query parameters server-side instead of splicing them into SQL - #313

Open
lukekim wants to merge 5 commits into
trunkfrom
fix/server-side-parameter-binding
Open

fix: bind query parameters server-side instead of splicing them into SQL#313
lukekim wants to merge 5 commits into
trunkfrom
fix/server-side-parameter-binding

Conversation

@lukekim

@lukekim lukekim commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

What

Parameterized queries on the Node gRPC path now run as Flight SQL prepared statements — CreatePreparedStatementDoPut (bind a typed Arrow record batch) → GetFlightInfoDoGetClosePreparedStatement — instead of substituting values into the SQL text client-side.

The HTTP/browser path already bound server-side and is unchanged. No public API change: .sql(query, { parameters }) works exactly as before, for both positional and named parameters.

Why

The client-side substitution was justified by this comment in src/grpc/client.node.ts:

Flight SQL prepared statements are not currently supported by the Spice server.

That is not true. The runtime implements CreatePreparedStatement in flight/actions.rs, spice-rs binds through it today, and I confirmed it directly against a live spice run — the server returns a handle and a typed parameter schema.

The consequence of splicing was that type information was discarded: every value was rendered to a SQL literal and re-parsed by the server. Escaping was applied, so this was not trivially injectable, but the semantics differed from the other five SDKs, which all bind server-side.

Notably, this PR does not so much add machinery as connect it: src/adbc/client.ts already contained a complete set of prepared-statement encoders and decoders, with no caller anywhere in the package. What was missing was the IPC-to-Flight framing — an Arrow IPC stream frames each message as [continuation][length][metadata][body], while Flight carries the metadata and the body as separate fields. Sending the undivided stream makes the server fail with Error decoding root message.

Two wire details worth recording

Both established empirically against a live runtime, and both are the kind of thing the next person will otherwise rediscover the hard way:

  • Positional placeholders bind through columns named $1, $2, …
  • Named placeholders bind through the bare name — column nm for placeholder $nm. Binding $nm fails with No value found for placeholder with name $nm, even though the parameter schema the server reports back from CreatePreparedStatement spells the field $nm.

The asymmetry is surprising enough that it's called out in a comment on buildNamedParameterColumns and pinned by a test.

Also worth flagging: GetFlightInfo accepts an unbound or wrongly-bound statement without complaint — the placeholder error only surfaces at DoGet. Any future work here should verify through DoGet, not GetFlightInfo.

Test changes

test/parameter-substitution.test.ts is removed. It defined a local ParameterSubstitutionHelper class that "mirrors the logic in GrpcFlightClient" and tested that copy, never importing from the SDK — so it would have gone on passing against substitution code that no longer exists. Its ~130 assertions are why the total test count drops.

Replaced by test/prepared-statement-binding.test.ts (18 tests) covering the column-naming rules for both placeholder styles, IPC stream framing and round-trip reassembly, and that values carrying SQL syntax stay data.

Verification

Run against a live spice run (Spice v2.1.1) with a local CSV dataset:

case result
plain query, no parameters
positional int / string / two params
named parameters ({ nm: ... } and { $nm: ... })
float parameter
streaming callback with parameters ✅ (3 rows streamed, 3 final)
"CANADA' OR '1'='1" as a value ✅ returns [] — bound as data
  • npm run build
  • npm run typecheck — clean
  • npm test — 19 failures, byte-identical to the failure set on unmodified trunk (all pre-existing, in nested-date-conversion, response-* and related suites)
  • npm run test:browser — 61 passed, 0 failed
  • Integration verified manually against a live runtime, as tabled above

Two notes for the reviewer:

  • npm run lint reports 653 errors against 641 on trunk. The 12 added are no-unsafe-assignment / no-explicit-any in the new code, matching the pattern already used throughout src/adbc/client.ts. CI does not run npm run lint, and tightening the file's typing felt like a separate change.
  • The README links to docs/PARAMETERIZED_QUERIES.md, which does not exist in the repo. Pre-existing and left alone here.

The Node gRPC path substituted parameter values into the SQL text client-side,
justified by a comment claiming the runtime does not support Flight SQL
prepared statements. That is not true: the runtime implements
CreatePreparedStatement, and spice-rs binds through it today.

Parameterized queries now run as prepared statements — CreatePreparedStatement,
DoPut to bind a typed Arrow record batch, GetFlightInfo, DoGet, then
ClosePreparedStatement once the results are consumed. Values never enter the
query string and keep their Arrow types end to end. The HTTP/browser path
already bound server-side and is unchanged.

This wires up the encoders that were already in src/adbc/client.ts but had no
caller, and adds the IPC-to-Flight message framing DoPut requires: an Arrow IPC
stream frames each message as [continuation][length][metadata][body], while
Flight carries metadata and body as separate fields.

Two wire details worth recording, both established against a live runtime:
positional placeholders bind through columns named `$1`, `$2`, while named
placeholders bind through the bare name (`nm`, not `$nm`) even though the
parameter schema the server reports spells the field with the leading `$`.

Removes test/parameter-substitution.test.ts, which exercised its own private
copy of the substitution logic rather than the SDK's, and so would have kept
passing against code that no longer exists.
@vercel

vercel Bot commented Jul 27, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
spice-js Ready Ready Preview Jul 29, 2026 11:14pm

Request Review

@lukekim
lukekim requested review from a team, Copilot and sgrebnov July 27, 2026 18:09
@lukekim lukekim self-assigned this Jul 27, 2026
@lukekim lukekim added the enhancement New feature or request label Jul 27, 2026
@lukekim lukekim added this to the v3.1.0 milestone Jul 27, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR switches the Node.js gRPC (Arrow Flight SQL) execution path from client-side SQL parameter substitution to server-side binding via Flight SQL prepared statements, aligning the Node transport with the existing HTTP/browser behavior and preserving parameter typing over the wire.

Changes:

  • Implement Flight SQL prepared statement flow on the Node gRPC path (CreatePreparedStatement → DoPut bind → GetFlightInfo → DoGet → ClosePreparedStatement).
  • Add Arrow IPC stream splitting logic to correctly frame DoPut messages as Flight dataHeader/dataBody.
  • Replace the old parameter-substitution unit test suite with focused tests for binding column naming and IPC framing; update README wording/examples accordingly.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/grpc/client.node.ts Implements prepared-statement execution and DoPut binding, plus IPC stream splitting for Flight framing.
src/adbc/client.ts Adds named-parameter column building and IPC serialization for server-side binding.
test/prepared-statement-binding.test.ts Adds unit coverage for placeholder→column naming rules and IPC framing/splitting behavior.
test/parameter-substitution.test.ts Removes tests for the now-deleted client-side substitution logic.
README.md Updates documentation to describe server-side binding on the Flight SQL transport and adds a named-parameter example.

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

Comment thread src/grpc/client.node.ts
test/scripts/spicepod.yaml declared `version: v1beta1`, which current spiced
rejects outright:

  Failed to start Spice runtime: Unable to load spicepod .../test/scripts:
  Unsupported Spicepod version in .../test/scripts: 'v1beta1'
  Supported versions are: v1, v2

With no spicepod loaded the runtime never serves, so every Local Runtime test
fails on ECONNREFUSED to 127.0.0.1:8090. This affects every open PR, not any
one change.

Verified locally: with `version: v1` the runtime loads both datasets and
starts serving.
v2 is what `spice init` emits, so the fixture now matches what the CLI
generates rather than trailing it.

Verified locally: spiced loads the fixture unchanged under v2 — both datasets
dispatch and the runtime serves. No schema migration was needed.
Two problems, both meaning an explicitly typed parameter was bound as whatever
Arrow inferred from the JavaScript value:

- The public Param class is { value, type: ArrowTypeId, options }, while the
  column builder expected { value, dataType }. A Param therefore looked like an
  untyped wrapper and its type was dropped. normalizeParam now translates
  between the two, including the PascalCase-to-lowercase type names.
- serializeParametersToIPC computed a `types` map and then discarded it,
  because tableFromArrays infers from the values. Explicit types are now built
  with vectorFromArray so they reach the wire.

Param.int32(42) binds as Int32 rather than Int64, Param.float32(1.5) as Float32
rather than Float64. Types needing options that were not supplied (time32,
decimal128, fixed_size_binary) fall back to inference instead of guessing.

Verified against a live runtime: plain, Param.int32/int64/string, named Params,
and multiple typed parameters all return correct rows. 8 new tests.
@claudespice

Copy link
Copy Markdown

This PR is approved and its only red checks are stale — a maintainer re-run should clear it.

All six Cloud Tests legs come from one run (30498855850, 2026-07-29T23:15Z, never re-run). Every failure is in the same suite and has the same cause:

● cloud › Vercel Endpoint (via SDK) › simple query with constants
  HTTP query failed with status 500: {"success":false,"error":"undefined undefined: undefined"}

That is a server-side outage on the Vercel endpoint, not a defect here — this PR changes server-side parameter binding and never touches that endpoint. Evidence the endpoint has since recovered: the same six legs passed on run 30584139765 (2026-07-30, PR #316). #315 is red for the identical reason from an even older run.

One diagnostic note for the suite itself: the error handling works case passed in every failing leg, because a blanket 500 also satisfies an "it errors" assertion. That test can't distinguish an outage from healthy error handling.

gh run rerun returns 403 for this account, so the re-run needs someone with admin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants