ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(postgres): skip literals and comments when numbering placeholders

#1151
Merged glennmichael123 wants to merge fix/pg-placeholders-skip-literals into main
7 files +224 -45
docs/guide/where.mdmodified+5-0
@@ -239,6 +239,11 @@ const tagged = await Post.query()
239239 .get()
240240```
241241
242A `?` inside a string literal, quoted identifier or comment is not a
243placeholder. Postgres' jsonb `?`, `?|` and `?&` operators can't be told apart
244from placeholders, so use their function forms, `jsonb_exists(tags, ?)`,
245`jsonb_exists_any(tags, ?)` and `jsonb_exists_all(tags, ?)`.
246
242247## Date Conditions
243248
244249```typescript
packages/bun-query-builder/src/client.tsmodified+5-6
@@ -14,7 +14,7 @@ import { bunSql, getOrCreateBunSql, resetConnection } from './db'
1414import { resolvePivot } from './pivot'
1515import { singularizerFor } from './inflect'
1616import type { WhereTerm } from './sql-fragments'
17import { FALSE_PREDICATE, renderInPredicate, renderWhereTerms, scanTopLevelKeywords } from './sql-fragments'
17import { FALSE_PREDICATE, mapPlaceholders, renderInPredicate, renderWhereTerms, scanTopLevelKeywords } from './sql-fragments'
1818
1919export { resetConnection }
2020
@@ -81,12 +81,11 @@ function renderBoundSqlExpression(expression: BoundSqlExpression, startIndex: nu
8181 if (config.dialect !== 'postgres' || parameters.length === 0)
8282 return { text: expression.sql, parameters }
8383
84 // Skips literals and comments: a `?` in `'why?'` is data, and numbering it
85 // shifted every later placeholder onto the wrong parameter.
8486 let parameterIndex = 0
85 const text = expression.sql.replace(/\?/g, (placeholder) => {
86 if (parameterIndex >= parameters.length)
87 return placeholder
88 return getPlaceholder(startIndex + parameterIndex++)
89 })
87 const { text } = mapPlaceholders(expression.sql, () =>
88 parameterIndex < parameters.length ? getPlaceholder(startIndex + parameterIndex++) : undefined)
9089
9190 if (parameterIndex !== parameters.length) {
9291 throw new TypeError(
packages/bun-query-builder/src/orm.tsmodified+10-7
@@ -38,7 +38,7 @@ import { createFakerCompatLayer } from './faker-compat'
3838import { normalizeRelationList } from './relation-utils'
3939import { singularizerFor, toTableName as sharedToTableName } from './inflect'
4040import type { WhereTerm } from './sql-fragments'
41import { renderInPredicate, renderWhereTerms } from './sql-fragments'
41import { countPlaceholders, renderInPredicate, renderWhereTerms, toDialectPlaceholders } from './sql-fragments'
4242
4343/**
4444 * Current timestamp formatted for the active dialect.
@@ -136,7 +136,7 @@ function resolveRawBindings(fragment: string, params: unknown[]): unknown[] {
136136 const only = params[0]
137137 if (Array.isArray(only))
138138 return [...only]
139 if (only === undefined && !fragment.includes('?'))
139 if (only === undefined && countPlaceholders(fragment) === 0)
140140 return []
141141 return params
142142}
@@ -630,9 +630,13 @@ interface OrmExecutor {
630630}
631631
632632/**
633 * Rewrite `?` placeholders to Postgres `$1, $2, …` form. The ORM only ever
634 * emits `?` as a bound-parameter marker (values are always parameterised and
635 * identifiers are validated), so a sequential left-to-right pass is safe.
633 * Rewrite `?` placeholders to Postgres `$1, $2, …` form.
634 *
635 * This was a bare `sql.replace(/\?/g, …)`, on the reasoning that the ORM only
636 * emits `?` as a parameter marker. But `whereRaw` fragments are caller text, so
637 * `whereRaw("name = '?'")` went out as `name = '$1'` and matched nothing, and a
638 * `?` in a comment took a binding meant for a later placeholder. It now skips
639 * literals, quoted identifiers and comments like `toDialectPlaceholders`.
636640 *
637641 * Memoized: query TEXT repeats heavily (values are placeholders), and this
638642 * regex pass runs on every Postgres query the ORM executes. Bounded; cleared
@@ -644,8 +648,7 @@ const PG_PLACEHOLDER_CACHE_MAX = 500
644648function toPostgresPlaceholders(sql: string): string {
645649 const hit = pgPlaceholderCache.get(sql)
646650 if (hit !== undefined) return hit
647 let i = 0
648 const out = sql.replace(/\?/g, () => `$${++i}`)
651 const out = toDialectPlaceholders(sql, 'postgres')
649652 if (pgPlaceholderCache.size >= PG_PLACEHOLDER_CACHE_MAX)
650653 pgPlaceholderCache.clear()
651654 pgPlaceholderCache.set(sql, out)
packages/bun-query-builder/src/sql-fragments.tsmodified+65-31
@@ -109,10 +109,10 @@ export function renderInPredicate(column: string, values: readonly unknown[], ne
109109 * application that developed against SQLite therefore finds out at the point
110110 * its raw queries first meet Postgres, and the error does not name the cause.
111111 *
112 * Only unquoted placeholders are rewritten. A `?` inside a string literal, a
113 * quoted identifier, or a dollar-quoted block is data and is left exactly as
114 * written — the naive global replace is the reason this needs to be a function
115 * with tests rather than one line at a call site.
112 * Only unquoted placeholders are rewritten; see {@link mapPlaceholders} for
113 * what counts as quoted. Postgres' jsonb `?`, `?|` and `?&` operators cannot be
114 * told apart from placeholders, so write them as `jsonb_exists(col, key)`,
115 * `jsonb_exists_any(col, keys)` and `jsonb_exists_all(col, keys)`.
116116 *
117117 * Dialects that already take `?` get the string back unchanged, so this is safe
118118 * to apply unconditionally.
@@ -120,54 +120,88 @@ export function renderInPredicate(column: string, values: readonly unknown[], ne
120120export function toDialectPlaceholders(sql: string, dialect: SupportedDialect): string {
121121 if (dialect !== 'postgres')
122122 return sql
123 return mapPlaceholders(sql, ordinal => `$${ordinal}`).text
124}
123125
126/**
127 * How many `?` placeholders `sql` binds, by the same rules as
128 * {@link toDialectPlaceholders}.
129 */
130export function countPlaceholders(sql: string): number {
131 return mapPlaceholders(sql, () => undefined).count
132}
133
134/**
135 * Walk `sql` and hand each unquoted `?` to `replace`, with its 1-based ordinal.
136 * A string it returns is spliced in; `undefined` keeps the `?`.
137 *
138 * Quoted, and so left alone, is everything that is data or prose rather than a
139 * placeholder:
140 *
141 * - `'…'`, `"…"` and `` `…` `` runs, where doubling escapes the delimiter
142 * - `$$…$$` / `$tag$…$tag$` dollar-quoted strings
143 * - `-- …` line comments and `/* … *\/` block comments, which nest
144 *
145 * The ORM used a bare `sql.replace(/\?/g, …)`, so on Postgres
146 * `whereRaw("name = '?'")` was sent as `name = '$1'` and quietly matched nothing,
147 * and a `?` in a comment consumed a binding. The comment rules are the ones
148 * {@link scanTopLevelKeywords} uses; backslash escapes are not honoured, for the
149 * reason given there.
150 */
151export function mapPlaceholders(
152 sql: string,
153 replace: (ordinal: number) => string | undefined,
154): { text: string, count: number } {
124155 let out = ''
125 let index = 0
156 let count = 0
157 let copied = 0
126158 let position = 0
127159
128160 while (position < sql.length) {
129161 const char = sql[position]!
162 let end = -1
130163
131 // Single-quoted string literal, with '' as the escape.
132 if (char === '\'') {
133 const end = closingQuote(sql, position, '\'')
134 out += sql.slice(position, end)
135 position = end
136 continue
164 if (char === '\'' || char === '"' || char === '`') {
165 end = closingQuote(sql, position, char)
137166 }
138
139 // Double-quoted identifier, with "" as the escape.
140 if (char === '"') {
141 const end = closingQuote(sql, position, '"')
142 out += sql.slice(position, end)
143 position = end
144 continue
167 else if (char === '-' && sql[position + 1] === '-') {
168 const newline = sql.indexOf('\n', position + 2)
169 end = newline === -1 ? sql.length : newline
145170 }
146
147 // Dollar-quoted block: $tag$ … $tag$, where the tag may be empty.
148 if (char === '$') {
171 else if (char === '/' && sql[position + 1] === '*') {
172 let nesting = 1
173 end = position + 2
174 while (end < sql.length && nesting > 0) {
175 if (sql[end] === '/' && sql[end + 1] === '*') { nesting++; end += 2 }
176 else if (sql[end] === '*' && sql[end + 1] === '/') { nesting--; end += 2 }
177 else end++
178 }
179 }
180 else if (char === '$') {
149181 const tag = sql.slice(position).match(/^\$[A-Z_a-z]\w*\$|^\$\$/)?.[0]
150182 if (tag) {
151183 const close = sql.indexOf(tag, position + tag.length)
152 const end = close === -1 ? sql.length : close + tag.length
153 out += sql.slice(position, end)
154 position = end
155 continue
184 end = close === -1 ? sql.length : close + tag.length
156185 }
157186 }
158187
159 if (char === '?') {
160 index++
161 out += `$${index}`
162 position++
188 if (end !== -1) {
189 position = end
163190 continue
164191 }
165192
166 out += char
193 if (char === '?') {
194 count++
195 const replacement = replace(count)
196 if (replacement !== undefined) {
197 out += sql.slice(copied, position) + replacement
198 copied = position + 1
199 }
200 }
167201 position++
168202 }
169203
170 return out
204 return { text: copied === 0 ? sql : out + sql.slice(copied), count }
171205}
172206
173207/** Index just past the closing quote of the literal starting at `start`. */
packages/bun-query-builder/test/orm.pg-placeholder-literals.pg.test.tsadded+97-0
@@ -0,0 +1,97 @@
1/**
2 * `?` inside literals and comments on Postgres.
3 *
4 * The ORM rewrote `?` to `$n` with a bare `sql.replace(/\?/g, …)`. `whereRaw`
5 * fragments are caller text, so on live Postgres:
6 *
7 * - `whereRaw("label = '?'")` went out as `label = '$1'` and matched nothing;
8 * - a `?` in a comment took a number, pushing the real placeholders past the
9 * bindings: `could not determine data type of parameter $2`.
10 *
11 * The select/update builders' `{ sql, parameters }` expressions numbered the
12 * first N `?` the same way, so a literal `'?'` before a placeholder stole its
13 * number and left the placeholder unbound.
14 *
15 * Runs in a subprocess: an earlier configureOrm() in the same process pins the
16 * model layer to sqlite and would mask the network-driver path.
17 */
18
19import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'
20import { tmpdir } from 'node:os'
21import { join, resolve } from 'node:path'
22import process from 'node:process'
23import { describe, expect, it } from 'bun:test'
24import { PG_URL, probePostgres } from './pg'
25
26const pgAvailable = await probePostgres()
27
28describe.skipIf(!pgAvailable)('? inside literals and comments on live Postgres', () => {
29 it('binds only the placeholders, in the ORM and the builders', () => {
30 const srcEntry = resolve(import.meta.dir, '../src/index.ts')
31 const dir = mkdtempSync(join(tmpdir(), 'qb-pgph-'))
32 const scriptPath = join(dir, 'probe.ts')
33
34 writeFileSync(scriptPath, `
35import { SQL } from 'bun'
36import { setConfig, resetConnection, defineModel, clearModelRegistry, createQueryBuilder, buildDatabaseSchema, buildSchemaMeta } from ${JSON.stringify(srcEntry)}
37
38const URL = ${JSON.stringify(PG_URL)}
39const failures = []
40const check = (label, got, want) => {
41 const g = JSON.stringify(got), w = JSON.stringify(want)
42 if (g !== w) failures.push(label + ': got ' + g + ', want ' + w)
43}
44const attempt = async (label, fn) => {
45 try { return await fn() }
46 catch (e) { failures.push(label + ': threw ' + (e && e.message)); return undefined }
47}
48
49const raw = new SQL(URL)
50await raw.unsafe('DROP TABLE IF EXISTS _qb_pgph')
51await raw.unsafe('CREATE TABLE _qb_pgph (id int primary key, label text not null, tags jsonb not null)')
52await raw.unsafe(\`INSERT INTO _qb_pgph VALUES (1, '?', '["bun"]'), (2, 'b', '["node"]'), (3, 'c', '[]')\`)
53
54setConfig({ dialect: 'postgres', database: { url: URL } })
55resetConnection()
56clearModelRegistry()
57const Row = defineModel({ name: 'Pgph', table: '_qb_pgph', primaryKey: 'id',
58 attributes: { label: { type: 'string', fillable: true }, tags: { type: 'json', fillable: true } } })
59const ids = rows => (rows ?? []).map(r => Number(r.id ?? r.get('id'))).sort((a, b) => a - b)
60
61// ORM whereRaw. Matched nothing / threw 42P18 before.
62check('literal ?', ids(await attempt('literal ?', () => Row.query().whereRaw("label = '?'").get())), [1])
63check('comment ?', ids(await attempt('comment ?', () =>
64 Row.query().whereRaw('id = ? /* or ? */ OR label = ?', 1, 'b').get())), [1, 2])
65check('literal ? then where', ids(await attempt('literal ? then where', () =>
66 Row.query().whereRaw("label <> '?'").where('id', 2).get())), [2])
67check('jsonb_exists', ids(await attempt('jsonb_exists', () =>
68 Row.query().whereRaw('jsonb_exists(tags, ?)', 'bun').get())), [1])
69
70// Builder { sql, parameters } expressions.
71const models = { _qb_pgph: { columns: { id: { type: 'integer', isPrimaryKey: true }, label: { type: 'text' }, tags: { type: 'json' } } } }
72const db = createQueryBuilder({ schema: buildDatabaseSchema(models), meta: buildSchemaMeta(models), autoMigration: { enabled: false } })
73await attempt('update with bound expression', () =>
74 db.updateTable('_qb_pgph').set({ label: 'z' }).where({ sql: "label <> '?' AND id = ?", parameters: [3] }).execute())
75check('update landed', (await raw.unsafe('SELECT id, label FROM _qb_pgph ORDER BY id')).map(r => r.label), ['?', 'b', 'z'])
76
77await raw.unsafe('DROP TABLE IF EXISTS _qb_pgph')
78await raw.end()
79
80if (failures.length) {
81 console.error('FAILURES:\\n' + failures.join('\\n'))
82 process.exit(1)
83}
84console.log('OK')
85process.exit(0)
86`)
87
88 const proc = Bun.spawnSync({ cmd: ['bun', scriptPath], stdout: 'pipe', stderr: 'pipe', cwd: process.cwd(), env: { ...process.env } })
89 const dec = new TextDecoder()
90 const out = dec.decode(proc.stdout).trim()
91 const err = dec.decode(proc.stderr).trim()
92 rmSync(dir, { recursive: true, force: true })
93
94 expect(proc.exitCode, `pg placeholder probe failed.\nstdout: ${out}\nstderr: ${err}`).toBe(0)
95 expect(out).toContain('OK')
96 }, 30_000)
97})
packages/bun-query-builder/test/orm.where-raw-bindings.test.tsmodified+6-0
@@ -221,6 +221,12 @@ describe('whereRaw forwarded an undefined bindings argument (#1146)', () => {
221221 expect(User.query().where('id', 1).orWhereRaw('id = 2', undefined).toSql().params).toEqual([1])
222222 })
223223
224 it('ignores a question mark inside a literal or comment when deciding', async () => {
225 const q = forward("name <> '?' /* why? */").where('country', 'UK')
226 expect(q.toSql().params).toEqual(['UK'])
227 expect(ids(await q.get())).toEqual([2])
228 })
229
224230 it('stays a NULL binding when the fragment has one', async () => {
225231 const q = User.query().whereRaw('? IS NULL', undefined).where('country', 'UK')
226232 expect(q.toSql().params).toEqual([undefined, 'UK'])
packages/bun-query-builder/test/placeholders.test.tsmodified+36-1
@@ -10,7 +10,7 @@
1010 * the simple cases and corrupts a query the day somebody stores a question mark.
1111 */
1212import { describe, expect, it } from 'bun:test'
13import { toDialectPlaceholders } from '../src/sql-fragments'
13import { countPlaceholders, mapPlaceholders, toDialectPlaceholders } from '../src/sql-fragments'
1414
1515describe('toDialectPlaceholders', () => {
1616 it('numbers placeholders in order for postgres', () => {
@@ -81,4 +81,39 @@ describe('toDialectPlaceholders', () => {
8181 it('returns an empty string unchanged', () => {
8282 expect(toDialectPlaceholders('', 'postgres')).toBe('')
8383 })
84
85 it('leaves a question mark in a line comment alone', () => {
86 expect(toDialectPlaceholders('SELECT ? -- why?\n, ?', 'postgres'))
87 .toBe('SELECT $1 -- why?\n, $2')
88 })
89
90 it('leaves a question mark in a block comment alone, including nested ones', () => {
91 expect(toDialectPlaceholders('SELECT /* a? /* b? */ c? */ ?', 'postgres'))
92 .toBe('SELECT /* a? /* b? */ c? */ $1')
93 })
94
95 it('leaves a backtick-quoted run alone', () => {
96 expect(toDialectPlaceholders('SELECT `a?b`, ?', 'postgres')).toBe('SELECT `a?b`, $1')
97 })
98
99 it('survives an unterminated comment', () => {
100 expect(toDialectPlaceholders('SELECT ? /* open ?', 'postgres')).toBe('SELECT $1 /* open ?')
101 })
102})
103
104describe('countPlaceholders', () => {
105 it('counts only unquoted placeholders', () => {
106 expect(countPlaceholders('a = ? AND b = ?')).toBe(2)
107 expect(countPlaceholders("label = '?' /* ? */ -- ?")).toBe(0)
108 expect(countPlaceholders('$$ ? $$ "?" ?')).toBe(1)
109 expect(countPlaceholders('')).toBe(0)
110 })
111})
112
113describe('mapPlaceholders', () => {
114 it('replaces only the placeholders the callback answers for', () => {
115 const { text, count } = mapPlaceholders("'?' ? ? ?", ordinal => ordinal === 2 ? '$9' : undefined)
116 expect(text).toBe("'?' ? $9 ?")
117 expect(count).toBe(3)
118 })
84119})