ReviewOS
Reload

also looking at this

stacks/bun-query-builder

feat(paginate): add a spelling that cannot be transposed

#1130
Merged glennmichael123 wants to merge fix/1092-paginate-options into main
3 files +208 -5

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

packages/bun-query-builder/src/client.tsmodified+35-2
@@ -622,6 +622,28 @@ export type WhereExpression<TableColumns> =
622622 | { [K in keyof TableColumns & string]: [key: K, op: WhereOperator, value: WhereValue<TableColumns[K]>] }[keyof TableColumns & string]
623623 | WhereRaw
624624
625/**
626 * # `PaginateOptions`
627 *
628 * The unambiguous spelling of a paginate() call.
629 *
630 * `db.selectFrom(t).paginate(perPage, page)` and `Model.query().paginate(page,
631 * perPage)` take the same two numbers in opposite orders, and both parameters
632 * are `number` — so transposing them is not a type error, it just returns a
633 * different page. Passing this object instead says which is which.
634 *
635 * Both APIs accept it and both keep their positional forms, so this adds a
636 * safe spelling without moving anyone's existing call. See #1092.
637 */
638export interface PaginateOptions {
639 /** Rows per page. Defaults to each API's existing default. */
640 perPage?: number
641 /** 1-based page number. Defaults to 1. */
642 page?: number
643 /** Run the count and the page query inside this transaction — see #1051. */
644 tx?: { unsafe: (sql: string, params?: any[]) => any }
645}
646
625647export type QueryResult = unknown
626648
627649/**
@@ -1930,7 +1952,7 @@ export interface BaseSelectQueryBuilder<
19301952 * const res2 = await db.selectFrom('users').where({ active: true }).paginate(25)
19311953 * ```
19321954 */
1933 paginate: (perPage: number, page?: number, opts?: { tx?: { unsafe: (sql: string, params?: unknown[]) => unknown } }) => Promise<{ data: SelectedRow<DB, TTable, TSelected>[], meta: { perPage: number, page: number, total: number, lastPage: number } }>
1955 paginate: ((perPage: number, page?: number, opts?: { tx?: { unsafe: (sql: string, params?: unknown[]) => unknown } }) => Promise<{ data: SelectedRow<DB, TTable, TSelected>[], meta: { perPage: number, page: number, total: number, lastPage: number } }>) & ((options: PaginateOptions) => Promise<{ data: SelectedRow<DB, TTable, TSelected>[], meta: { perPage: number, page: number, total: number, lastPage: number } }>)
19341956 /**
19351957 * # `simplePaginate`
19361958 *
@@ -5823,7 +5845,18 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
58235845 const e = await (this as any).exists()
58245846 return !e
58255847 },
5826 async paginate(perPage: number, page = 1, opts: { tx?: { unsafe: (sql: string, params?: any[]) => any } } = {}) {
5848 async paginate(perPage: number | PaginateOptions, page = 1, opts: { tx?: { unsafe: (sql: string, params?: any[]) => any } } = {}) {
5849 // Options form: `paginate({ perPage, page })`. The two paginate APIs in
5850 // this library take the same two numbers in opposite orders — this one
5851 // is (perPage, page), the ORM's is (page, perPage) — and both arguments
5852 // are numbers, so getting it backwards is silent: you simply receive a
5853 // different page. Naming them cannot be got wrong. See #1092.
5854 if (typeof perPage === 'object' && perPage !== null) {
5855 const options = perPage
5856 page = options.page ?? 1
5857 opts = options.tx ? { tx: options.tx } : {}
5858 perPage = options.perPage ?? 10
5859 }
58275860 if (!Number.isFinite(perPage) || perPage <= 0 || !Number.isInteger(perPage))
58285861 throw new TypeError(`[query-builder] paginate(perPage): expected positive integer, got ${perPage}`)
58295862 if (!Number.isFinite(page) || page < 1 || !Number.isInteger(page))
packages/bun-query-builder/src/orm.tsmodified+20-3
@@ -26,6 +26,7 @@
2626
2727import { Database, type SQLQueryBindings } from 'bun:sqlite'
2828import type { FactoryFaker } from './faker-compat'
29import type { PaginateOptions } from './client'
2930import type { SupportedDialect } from './types'
3031import type { RelationCardinality } from './type-inference'
3132import { isNumericPlanType, normalizeAttributeType, sqliteAffinityFor } from './column-types'
@@ -3116,7 +3117,7 @@ class ModelQueryBuilder<
31163117 }
31173118 }
31183119
3119 async paginate(page = 1, perPage = 15): Promise<{
3120 async paginate(page: number | PaginateOptions = 1, perPage = 15): Promise<{
31203121 data: ModelInstance<TDef, TSelected>[]
31213122 total: number
31223123 page: number
@@ -3126,7 +3127,17 @@ class ModelQueryBuilder<
31263127 isEmpty: boolean
31273128 from: number | null
31283129 to: number | null
3130 meta: { perPage: number, page: number, total: number, lastPage: number }
31293131 }> {
3132 // Options form: `paginate({ page, perPage })`. This API is (page, perPage)
3133 // and the query builder's is (perPage, page); both arguments are numbers,
3134 // so transposing them returns a different page rather than raising
3135 // anything. Naming them cannot be got wrong. See #1092.
3136 if (typeof page === 'object' && page !== null) {
3137 const options = page
3138 perPage = options.perPage ?? 15
3139 page = options.page ?? 1
3140 }
31303141 const total = await this.count()
31313142 const lastPage = Math.ceil(total / perPage)
31323143 this._limit = perPage
@@ -3142,6 +3153,12 @@ class ModelQueryBuilder<
31423153 isEmpty: data.length === 0,
31433154 from: data.length > 0 ? (page - 1) * perPage + 1 : null,
31443155 to: data.length > 0 ? (page - 1) * perPage + data.length : null,
3156 // Mirrors the query builder's result, which nests under `meta`. The flat
3157 // fields above stay exactly as they were, so this is additive: code
3158 // written against either shape now works against both, and a helper that
3159 // reads `result.meta.total` stops depending on which API produced it.
3160 // See #1092.
3161 meta: { perPage, page, total, lastPage },
31453162 }
31463163 }
31473164
@@ -3319,7 +3336,7 @@ export type ModelStatic<TDef extends ModelDefinition> = StaticWhereOverloads<TDe
33193336 count: () => Promise<number>
33203337 exists: () => Promise<boolean>
33213338 doesntExist: () => Promise<boolean>
3322 paginate: (page?: number, perPage?: number) => Promise<{
3339 paginate: (page?: number | PaginateOptions, perPage?: number) => Promise<{
33233340 data: ModelRecord<TDef>[]
33243341 total: number
33253342 page: number
@@ -3457,7 +3474,7 @@ function createModelInternal<const TDef extends ModelDefinition>(definition: TDe
34573474 count: () => new ModelQueryBuilder<TDef>(definition).count(),
34583475 exists: () => new ModelQueryBuilder<TDef>(definition).exists(),
34593476 doesntExist: () => new ModelQueryBuilder<TDef>(definition).doesntExist(),
3460 paginate: (page?: number, perPage?: number) => new ModelQueryBuilder<TDef>(definition).paginate(page, perPage),
3477 paginate: (page?: number | PaginateOptions, perPage?: number) => new ModelQueryBuilder<TDef>(definition).paginate(page as any, perPage),
34613478
34623479 whereBetween<K extends Cols>(column: K, range: [min: K extends keyof Attrs ? Attrs[K] : unknown, max: K extends keyof Attrs ? Attrs[K] : unknown]) {
34633480 return new ModelQueryBuilder<TDef>(definition).whereBetween(column, range as any)
packages/bun-query-builder/test/paginate-options.test.tsadded+153-0
@@ -0,0 +1,153 @@
1/**
2 * paginate() gains a spelling that cannot be transposed.
3 * stacksjs/bun-query-builder#1092.
4 *
5 * The two paginate APIs take the same two numbers in opposite orders:
6 *
7 * db.selectFrom(t).paginate(perPage, page)
8 * Model.query().paginate(page, perPage)
9 *
10 * Both parameters are `number`, so getting it backwards is not a type error —
11 * you simply receive a different page, with no warning. `forPage(page, perPage)`
12 * sits ~60 lines from the builder's `paginate(perPage, page)` and takes the
13 * opposite order again, which is presumably where this came from.
14 *
15 * Resolved additively, deliberately: realigning either side would silently
16 * change which rows every existing caller gets. So both APIs now also accept
17 * `{ page, perPage }`, both keep their positional forms unchanged, and the
18 * ORM's result gains a `meta` object mirroring the builder's while keeping its
19 * flat fields. Nobody's call moves; there is now a way to write one that
20 * cannot be got wrong, and a shape that reads the same from either API.
21 *
22 * The positional divergence itself is left for a deliberate major.
23 */
24
25import { mkdtempSync, rmSync } from 'node:fs'
26import { tmpdir } from 'node:os'
27import { join } from 'node:path'
28import { Database } from 'bun:sqlite'
29import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
30import { buildDatabaseSchema, buildSchemaMeta, createQueryBuilder } from '../src'
31import { clearModelRegistry, configureOrm, createModel, createTableFromModel } from '../src'
32import { config, setConfig } from '../src/config'
33import { resetConnection } from '../src/db'
34
35const MODELS = {
36 t: { columns: { id: { type: 'integer', isPrimaryKey: true }, name: { type: 'string' } } },
37} as any
38
39let dir: string
40let dbPath: string
41let snapshot: { dialect: string, database: Record<string, unknown> }
42
43function qb(): any {
44 return createQueryBuilder({
45 schema: buildDatabaseSchema(MODELS),
46 meta: buildSchemaMeta(MODELS),
47 autoMigration: { enabled: false } as any,
48 })
49}
50
51beforeEach(() => {
52 snapshot = { dialect: config.dialect, database: { ...config.database } }
53 dir = mkdtempSync(join(tmpdir(), 'qb-1092-'))
54 dbPath = join(dir, 'page.sqlite')
55 const seed = new Database(dbPath, { create: true })
56 seed.run('CREATE TABLE t (id INTEGER PRIMARY KEY, name TEXT)')
57 for (let i = 1; i <= 20; i++) seed.run(`INSERT INTO t (id,name) VALUES (${i},'n${i}')`)
58 seed.close()
59 setConfig({ dialect: 'sqlite', database: { database: dbPath } } as any)
60 resetConnection()
61})
62
63afterEach(() => {
64 config.dialect = snapshot.dialect as any
65 for (const k of Object.keys(config.database)) delete (config.database as any)[k]
66 Object.assign(config.database, snapshot.database)
67 resetConnection()
68 rmSync(dir, { recursive: true, force: true })
69})
70
71describe('the builder accepts the options form (#1092)', () => {
72 it('{ perPage, page } means the same as the positional call', async () => {
73 const byOptions = await qb().selectFrom('t').selectAll().paginate({ perPage: 5, page: 2 })
74 const byPosition = await qb().selectFrom('t').selectAll().paginate(5, 2)
75
76 expect(byOptions.data.map((r: any) => r.id)).toEqual([6, 7, 8, 9, 10])
77 expect(byOptions.data).toEqual(byPosition.data)
78 expect(byOptions.meta).toEqual({ perPage: 5, page: 2, total: 20, lastPage: 4 })
79 })
80
81 it('the positional form is untouched', async () => {
82 const res = await qb().selectFrom('t').selectAll().paginate(5, 2)
83 expect(res.data.map((r: any) => r.id)).toEqual([6, 7, 8, 9, 10])
84 })
85
86 it('page defaults to 1', async () => {
87 const res = await qb().selectFrom('t').selectAll().paginate({ perPage: 3 })
88 expect(res.data.map((r: any) => r.id)).toEqual([1, 2, 3])
89 expect(res.meta.page).toBe(1)
90 })
91
92 it('the existing validation still applies through the options form', async () => {
93 await expect(qb().selectFrom('t').selectAll().paginate({ perPage: 0 }))
94 .rejects.toThrow(/expected positive integer/)
95 await expect(qb().selectFrom('t').selectAll().paginate({ perPage: 5, page: 0 }))
96 .rejects.toThrow(/expected integer >= 1/)
97 })
98})
99
100describe('the ORM accepts the options form and reports meta (#1092)', () => {
101 let Post: any
102
103 beforeEach(async () => {
104 clearModelRegistry()
105 const db = new Database(join(dir, 'orm.sqlite'), { create: true })
106 configureOrm({ database: db })
107 Post = createModel({
108 name: 'Pgpost',
109 table: 'pg_posts',
110 primaryKey: 'id',
111 autoIncrement: true,
112 attributes: { title: { type: 'string', fillable: true } },
113 } as any)
114 await createTableFromModel(Post.getDefinition())
115 for (let i = 1; i <= 20; i++) await Post.create({ title: `t${i}` })
116 })
117
118 afterEach(() => clearModelRegistry())
119
120 it('{ perPage, page } selects the same page the builder would', async () => {
121 const res = await Post.query().paginate({ perPage: 5, page: 2 })
122
123 // The positional ORM call for this page is paginate(2, 5) — the transposed
124 // order that made this worth fixing.
125 expect(res.data.map((r: any) => r.id)).toEqual([6, 7, 8, 9, 10])
126 expect(res.page).toBe(2)
127 expect(res.perPage).toBe(5)
128 })
129
130 it('the positional form keeps its own order', async () => {
131 const res = await Post.query().paginate(2, 5)
132 expect(res.data.map((r: any) => r.id)).toEqual([6, 7, 8, 9, 10])
133 })
134
135 it('the flat fields are unchanged', async () => {
136 const res = await Post.query().paginate({ perPage: 5, page: 2 })
137
138 expect(res.total).toBe(20)
139 expect(res.lastPage).toBe(4)
140 expect(res.hasMorePages).toBe(true)
141 expect(res.isEmpty).toBe(false)
142 expect(res.from).toBe(6)
143 expect(res.to).toBe(10)
144 })
145
146 it('and meta now reads the same as the builder\'s', async () => {
147 const orm = await Post.query().paginate({ perPage: 5, page: 2 })
148 const builder = await qb().selectFrom('t').selectAll().paginate({ perPage: 5, page: 2 })
149
150 // The point of the addition: one accessor works against either API.
151 expect(orm.meta).toEqual(builder.meta)
152 })
153})