ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(client): honour the hooks a builder was constructed with

#1074
Merged glennmichael123 wants to merge fix/builder-hooks into main
2 files +176 -19
packages/bun-query-builder/src/client.tsmodified+53-19
@@ -4,7 +4,7 @@
44import type { SchemaMeta } from './meta'
55import type { ResolvedPivot } from './pivot'
66import type { DatabaseSchema } from './schema'
7import type { QueryBuilderOptions } from './types'
7import type { QueryBuilderOptions, QueryHooks } from './types'
88import { config, getPlaceholder, getPlaceholders, isMysqlLike, setConfig } from './config'
99import type { DriverConnection } from './db'
1010import { bunSql, getOrCreateBunSql, resetConnection } from './db'
@@ -2532,6 +2532,21 @@ interface InternalState {
25322532 meta?: SchemaMeta
25332533 schema?: any
25342534 txDefaults?: TransactionOptions
2535 /**
2536 * Lifecycle hooks for this builder specifically.
2537 *
2538 * `createQueryBuilder({ schema, meta, hooks })` is the form the docs have
2539 * shown for a long time, and it did not exist: `hooks` was not a member of
2540 * this interface, so the call was a `TS2353` excess-property error. The
2541 * obvious workaround made it worse rather than better — `as any` compiled,
2542 * and then the hooks never fired, because every hook site read the
2543 * process-wide `config.hooks` and nothing ever looked at builder state. A
2544 * builder that appeared configured silently ignored every hook it was given.
2545 *
2546 * These merge OVER `config.hooks` per key (see `activeHooks`), so a global
2547 * logger or tracer keeps firing for builders that only override one hook.
2548 */
2549 hooks?: QueryHooks
25352550 /**
25362551 * Set on the builder handed to a `transaction()` callback. A nested
25372552 * `transaction()` must open a SAVEPOINT (`tx.savepoint(...)`) rather than a
@@ -2766,6 +2781,25 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
27662781 const meta = state?.meta
27672782 const schema = state?.schema
27682783
2784 /**
2785 * The hooks this builder should fire.
2786 *
2787 * Read through this rather than reaching for `config.hooks` directly, for the
2788 * same reason statements go through `_sql`: a builder can be handed its own,
2789 * and ignoring them makes a configured builder silently do nothing.
2790 *
2791 * Resolved per call, not captured once, because `setConfig({ hooks })` may
2792 * land after the builder is constructed — the usual
2793 * `export const db = createQueryBuilder(...)` at module scope followed by
2794 * configuration at boot. Merged per key so a global logger still fires for a
2795 * builder that overrides only `beforeCreate`.
2796 */
2797 function activeHooks(): QueryHooks | undefined {
2798 if (!state?.hooks)
2799 return config.hooks
2800 return { ...config.hooks, ...state.hooks }
2801 }
2802
27692803 // `whereCreatedAt` → `created_at` resolution for the dynamic-where Proxy,
27702804 // cached per (table, prop): the schema is fixed for this builder, and the
27712805 // resolution (regex strip + snake-case + column scan) otherwise re-runs on
@@ -2894,7 +2928,7 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
28942928 }
28952929
28962930 function runWithHooks<T = any>(q: any, kind: 'select' | 'insert' | 'update' | 'delete' | 'raw', opts?: { signal?: AbortSignal, timeoutMs?: number }): Promise<T> {
2897 const hooks = config.hooks
2931 const hooks = activeHooks()
28982932 const slowMs = hooks?.slowQueryThresholdMs
28992933 const slowEnabled = slowMs != null && slowMs >= 0
29002934 const hasSlowQuery = Boolean(hooks?.onSlowQuery || slowEnabled)
@@ -5605,7 +5639,7 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
56055639 return text
56065640 },
56075641 async get() {
5608 const hooks = config.hooks
5642 const hooks = activeHooks()
56095643 const hasQueryHooks = hooks && (hooks.onQueryStart || hooks.onQueryEnd || hooks.onQueryError || hooks.startSpan || hasSlowQueryHook(hooks))
56105644
56115645 // Ultra-fast path: skip unsafe() entirely, use _prepareStatement for direct stmt access
@@ -5683,7 +5717,7 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
56835717 },
56845718 async first() {
56855719 // Ultra-fast path: skip overhead, prepare statement directly from text
5686 const fHooks = config.hooks
5720 const fHooks = activeHooks()
56875721 const fHasQueryHooks = fHooks && (fHooks.onQueryStart || fHooks.onQueryEnd || fHooks.onQueryError || fHooks.startSpan || hasSlowQueryHook(fHooks))
56885722 if (!config.softDeletes?.enabled && !useCache && !timeoutMs && !abortSignal && !fHasQueryHooks) {
56895723 const prepareFn = _sql._prepareStatement
@@ -5778,7 +5812,7 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
57785812 }
57795813
57805814 // Ultra-fast path
5781 const cHooks = config.hooks
5815 const cHooks = activeHooks()
57825816 const cHasHooks = cHooks && (cHooks.onQueryStart || cHooks.onQueryEnd || cHooks.onQueryError || cHooks.startSpan || hasSlowQueryHook(cHooks))
57835817 if (!config.softDeletes?.enabled && !useCache && !timeoutMs && !abortSignal && !cHasHooks) {
57845818 const prepareFn = _sql._prepareStatement
@@ -5804,7 +5838,7 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
58045838 : `SELECT AVG(${column}) as a FROM ${table}`
58055839
58065840 // Ultra-fast path
5807 const aHooks = config.hooks
5841 const aHooks = activeHooks()
58085842 const aHasHooks = aHooks && (aHooks.onQueryStart || aHooks.onQueryEnd || aHooks.onQueryError || aHooks.startSpan || hasSlowQueryHook(aHooks))
58095843 if (!config.softDeletes?.enabled && !useCache && !timeoutMs && !abortSignal && !aHasHooks) {
58105844 const prepareFn = _sql._prepareStatement
@@ -6496,7 +6530,7 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
64966530 },
64976531 execute() {
64986532 // Ultra-fast path: use _prepareStatement to skip unsafe() and runWithHooks overhead
6499 const hooks = config.hooks
6533 const hooks = activeHooks()
65006534 const hasHooks = hooks && (hooks.onQueryStart || hooks.onQueryEnd || hooks.onQueryError || hooks.startSpan || hooks.beforeCreate || hooks.afterCreate || hasSlowQueryHook(hooks))
65016535 if (!hasHooks) {
65026536 const prepareFn = _sql._prepareStatement
@@ -6914,7 +6948,7 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
69146948 },
69156949 async execute() {
69166950 try {
6917 await config.hooks?.beforeDelete?.({ table: String(table), where: whereCondition })
6951 await activeHooks()?.beforeDelete?.({ table: String(table), where: whereCondition })
69186952 }
69196953 catch (err) {
69206954 throw err
@@ -6923,7 +6957,7 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
69236957 const result = mutationCount(await runWithHooks(ensureDelBuilt(), 'delete'))
69246958
69256959 try {
6926 await config.hooks?.afterDelete?.({ table: String(table), where: whereCondition, result })
6960 await activeHooks()?.afterDelete?.({ table: String(table), where: whereCondition, result })
69276961 }
69286962 catch {}
69296963
@@ -7049,7 +7083,7 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
70497083 },
70507084 async reserve() {
70517085 const reserved = await (bunSql as any).reserve()
7052 const qb = createQueryBuilder<DB>({ sql: reserved, meta, schema }) as any
7086 const qb = createQueryBuilder<DB>({ sql: reserved, meta, schema, hooks: state?.hooks }) as any
70537087 qb.release = () => reserved.release()
70547088 return qb
70557089 },
@@ -7117,7 +7151,7 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
71177151 opts.logger?.({ type: 'start', attempt })
71187152 const start = Date.now()
71197153 return await (txConn as any)[txMethod](async (tx: any) => {
7120 const qb = createQueryBuilder<DB>({ sql: tx, meta, schema, inTransaction: true })
7154 const qb = createQueryBuilder<DB>({ sql: tx, meta, schema, hooks: state?.hooks, inTransaction: true })
71217155
71227156 // Transaction isolation + read-only mode — dialect-specific
71237157 // SQL, with a clear "not supported" path for SQLite. The
@@ -7226,14 +7260,14 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
72267260 return await s.savepoint(async (sp: any) => {
72277261 // Carry the flag: a savepoint body is still inside a transaction, so a
72287262 // nested savepoint()/transaction() from here must nest, not begin.
7229 const qb = createQueryBuilder<DB>({ sql: sp, meta, schema, inTransaction: true })
7263 const qb = createQueryBuilder<DB>({ sql: sp, meta, schema, hooks: state?.hooks, inTransaction: true })
72307264 return await fn(qb)
72317265 })
72327266 },
72337267 async beginDistributed(name, fn) {
72347268 const txConn: any = state?.sql ?? bunSql
72357269 const res = await (txConn as any).beginDistributed(name, async (tx: any) => {
7236 const qb = createQueryBuilder<DB>({ sql: tx, meta, schema })
7270 const qb = createQueryBuilder<DB>({ sql: tx, meta, schema, hooks: state?.hooks })
72377271 return await fn(qb)
72387272 })
72397273 return res as any
@@ -7382,13 +7416,13 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
73827416 async rawQuery(query: string) {
73837417 const start = Date.now()
73847418 try {
7385 config.hooks?.onQueryStart?.({ sql: query, kind: 'raw' })
7419 activeHooks()?.onQueryStart?.({ sql: query, kind: 'raw' })
73867420 const res = await (_sql as any).unsafe(query)
7387 config.hooks?.onQueryEnd?.({ sql: query, durationMs: Date.now() - start, kind: 'raw' })
7421 activeHooks()?.onQueryEnd?.({ sql: query, durationMs: Date.now() - start, kind: 'raw' })
73887422 return res
73897423 }
73907424 catch (err) {
7391 config.hooks?.onQueryError?.({ sql: query, error: err, durationMs: Date.now() - start, kind: 'raw' })
7425 activeHooks()?.onQueryError?.({ sql: query, error: err, durationMs: Date.now() - start, kind: 'raw' })
73927426 throw err
73937427 }
73947428 },
@@ -7397,7 +7431,7 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
73977431
73987432 // beforeCreate hook
73997433 try {
7400 await config.hooks?.beforeCreate?.({ table: String(table), data: values })
7434 await activeHooks()?.beforeCreate?.({ table: String(table), data: values })
74017435 }
74027436 catch (err) {
74037437 throw err
@@ -7424,7 +7458,7 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
74247458
74257459 // afterCreate hook
74267460 try {
7427 await config.hooks?.afterCreate?.({ table: String(table), data: values, result: row })
7461 await activeHooks()?.afterCreate?.({ table: String(table), data: values, result: row })
74287462 }
74297463 catch {}
74307464
@@ -7448,7 +7482,7 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
74487482
74497483 // afterCreate hook
74507484 try {
7451 await config.hooks?.afterCreate?.({ table: String(table), data: values, result: row })
7485 await activeHooks()?.afterCreate?.({ table: String(table), data: values, result: row })
74527486 }
74537487 catch {}
74547488
packages/bun-query-builder/test/client.builder-hooks.test.tsadded+123-0
@@ -0,0 +1,123 @@
1import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
2import { buildDatabaseSchema, config, createQueryBuilder, setConfig } from '../src'
3import { resetConnection } from '../src/db'
4
5/**
6 * `createQueryBuilder({ hooks })` — per-builder lifecycle hooks.
7 *
8 * The form has been in the docs and the README for a long time and did not
9 * exist: `hooks` was not a member of the builder's internal state, so the call
10 * was a `TS2353` excess-property error. What made it worth a test rather than a
11 * type tweak is the workaround. `as any` compiled, and then the hooks never
12 * fired — every hook site read the process-wide `config.hooks` and nothing
13 * looked at builder state — so a builder that appeared configured silently
14 * ignored every hook it had been handed. A silent no-op is worse than the
15 * compile error in front of it.
16 */
17
18const schema = buildDatabaseSchema({
19 User: {
20 name: 'User',
21 table: 'users',
22 primaryKey: 'id',
23 attributes: { id: { validation: { rule: {} } }, name: { validation: { rule: {} } } },
24 },
25} as any)
26
27let saved: { dialect: any, database: any, hooks: any }
28
29beforeEach(async () => {
30 saved = { dialect: config.dialect, database: { ...config.database }, hooks: config.hooks }
31 setConfig({ dialect: 'sqlite', database: { database: ':memory:' }, hooks: {} })
32 resetConnection()
33 await createQueryBuilder<typeof schema>({ schema })
34 .unsafe('CREATE TABLE IF NOT EXISTS users (id INTEGER PRIMARY KEY, name TEXT)')
35})
36
37afterEach(() => {
38 setConfig({ dialect: saved.dialect, database: saved.database, hooks: saved.hooks } as any)
39 resetConnection()
40})
41
42describe('createQueryBuilder({ hooks })', () => {
43 it('fires the hooks it was given', async () => {
44 const seen: string[] = []
45 // No cast: this is the compile error the issue was about.
46 const db = createQueryBuilder<typeof schema>({
47 schema,
48 hooks: { onQueryStart: e => void seen.push(String(e.kind)) },
49 })
50
51 await db.selectFrom('users').execute()
52
53 expect(seen).toEqual(['select'])
54 })
55
56 it('leaves a builder without hooks on the global ones', async () => {
57 const seen: string[] = []
58 setConfig({ hooks: { onQueryStart: () => void seen.push('global') } })
59
60 await createQueryBuilder<typeof schema>({ schema }).selectFrom('users').execute()
61
62 expect(seen).toEqual(['global'])
63 })
64
65 it('merges per key, so a global hook still fires when a builder overrides a different one', async () => {
66 // The reason this merges rather than replaces: an app-wide logger or tracer
67 // should not go quiet because one builder wanted a `beforeCreate`.
68 const globals: string[] = []
69 const locals: string[] = []
70 setConfig({ hooks: { onQueryStart: () => void globals.push('global') } })
71
72 const db = createQueryBuilder<typeof schema>({
73 schema,
74 hooks: { onQueryEnd: () => void locals.push('local') },
75 })
76 await db.selectFrom('users').execute()
77
78 expect(globals).toEqual(['global'])
79 expect(locals).toEqual(['local'])
80 })
81
82 it('lets a builder hook override the global one of the same name', async () => {
83 const order: string[] = []
84 setConfig({ hooks: { onQueryStart: () => void order.push('global') } })
85
86 const db = createQueryBuilder<typeof schema>({
87 schema,
88 hooks: { onQueryStart: () => void order.push('builder') },
89 })
90 await db.selectFrom('users').execute()
91
92 expect(order).toEqual(['builder'])
93 })
94
95 it('passes its hooks down to the transaction callback builder', async () => {
96 // The tx callback gets a NEW builder around the transaction's connection;
97 // without threading, it silently lost the parent's hooks.
98 const seen: string[] = []
99 const db = createQueryBuilder<typeof schema>({
100 schema,
101 hooks: { onQueryStart: () => void seen.push('tx') },
102 })
103
104 await db.transaction(async (tx) => {
105 await tx.selectFrom('users').execute()
106 })
107
108 expect(seen.length).toBeGreaterThan(0)
109 })
110
111 it('sees hooks configured after the builder was constructed', async () => {
112 // `export const db = createQueryBuilder(...)` at module scope, then
113 // `setConfig({ hooks })` at boot, is the ordinary shape of an app. Capturing
114 // hooks once at construction would silently miss all of them.
115 const seen: string[] = []
116 const db = createQueryBuilder<typeof schema>({ schema })
117
118 setConfig({ hooks: { onQueryStart: () => void seen.push('late') } })
119 await db.selectFrom('users').execute()
120
121 expect(seen).toEqual(['late'])
122 })
123})