ReviewOS

also looking at this

stacks/bun-query-builder

fix(client): whereJsonContains binds the document, not its JSON text (#1091)

#1100
Merged glennmichael123 wants to merge fix/1091-json-contains-double-encode into main
3 files +172 -3
10 days ago

Closes #1091.

whereJsonContains pushed JSON.stringify(json), but Bun's driver already JSON-encodes a value bound to a jsonb parameter. The document was encoded twice, so Postgres received the jsonb string "[\"bun\"]" where the array ["bun"] was meant. A jsonb string never @>-contains anything, so the predicate was vacuously false and the query returned zero rows for every input, with no error.

Verified against live Postgres 17

Same script through the query builder, before and after:

operandcolumnbeforeafter
['bun']tags[] ✗[1] ✓
['bun','sql']tags[] ✗[1] ✓
{ published: true }meta[] ✗[1] ✓
{ published: true, tier: 'pro' }meta[] ✗[1] ✓
'bun'tags[] ✗[1] ✓
1tags[] ✗[1] ✓
truetags[] ✗[1] ✓
non-matches (4 cases)—[] ✓[] ✓
jsonb_contains mode (3 cases)—[] ✗✓

9 failed before, 0 after.

On scalars

Binding the value directly fixes objects, arrays and strings. Numbers and booleans are bound as int4/bool rather than jsonb, and @> has no operator for those — a naive fix turns them from silently wrong into operator does not exist: jsonb @> integer. They are lifted with to_jsonb() instead, so whereJsonContains('tags', 1) now actually works.

I checked the alternatives on the live server rather than guessing: $1::jsonb with the stringified form fails on every shape (the driver encodes the string again), and to_jsonb($1) fails on documents (could not determine polymorphic type). Only the split above handles all seven shapes.

Scope

MySQL is untouched — JSON_CONTAINS takes its candidate as JSON text, so the stringified form is correct there. SQLite binds scalars through json_each and never saw this. #1091 explicitly advised against changing all three together.

On the tests

The emitted SQL was correct throughout (tags @> $1); only the bound parameter was wrong, and toSQL() does not expose parameters. No SQL-text assertion can observe this bug — which is why the existing #1026 suite asserted the text and stayed green for its entire lifetime. So the real guard here executes against a live Postgres, gated on availability and run in a subprocess (config.dialect is global and other files pin it to sqlite).

Worth noting: every negative case passed while the bug was live, because the answer was always zero rows. Only containment that genuinely holds can fail, so the new cases assert rows come back.

Both new tests were confirmed to fail without the src change.

Checks

  • CI: lint, typecheck, test, publish-commit — all green
  • Locally: bun test — 1955 pass, 4 skip, 1 fail; typecheck and pickier clean

The one local failure is CLI > version prints package version (expects 0.2.41, gets 0.1.37), and it is unrelated to this change — a stale bin/qbx. That binary is gitignored and compiled at build time; mine dates from June, when the package was at 0.1.37. CI rebuilds it, which is why the test passes there. Nothing to fix in the repo.

The live-Postgres guard was confirmed to actually execute on the runner rather than skip:

(pass) whereJsonContains against live Postgres (#1091) > matches rows for
       documents and scalars, in both containment modes [153.61ms]

🤖 Generated with Claude Code

3 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.