fix(db-d1-sqlite): avoid bound parameter limit when querying relationships and inserting rows (#14099)
Fixes https://discord.com/channels/967097582721572934/1422639568808841329/1425037080051978261 This PR avoids bound parameters usage for `IN` and `NOT_IN` querying on `id` to respect the D1 limit https://developers.cloudflare.com/d1/platform/limits/ And also batches inserts when inserting arrays/blocks/hasMany relationships etc. This is needed because we can't avoid using bound parameters there, but still want to respect the 100 variables limit per query.
This commit is contained in:
@@ -109,6 +109,7 @@ export function sqliteD1Adapter(args: Args): DatabaseAdapterObj<SQLiteD1Adapter>
|
||||
}),
|
||||
idType: sqliteIDType,
|
||||
initializing,
|
||||
limitedBoundParameters: true,
|
||||
localesSuffix: args.localesSuffix || '_locales',
|
||||
logger: args.logger,
|
||||
operators,
|
||||
|
||||
@@ -3,13 +3,14 @@ import type { FlattenedField, Operator, Sort, Where } from 'payload'
|
||||
|
||||
import { and, isNotNull, isNull, ne, notInArray, or, sql } from 'drizzle-orm'
|
||||
import { PgUUID } from 'drizzle-orm/pg-core'
|
||||
import { QueryError } from 'payload'
|
||||
import { APIError, QueryError } from 'payload'
|
||||
import { validOperatorSet } from 'payload/shared'
|
||||
|
||||
import type { DrizzleAdapter, GenericColumn } from '../types.js'
|
||||
import type { BuildQueryJoinAliases } from './buildQuery.js'
|
||||
|
||||
import { getNameFromDrizzleTable } from '../utilities/getNameFromDrizzleTable.js'
|
||||
import { isValidStringID } from '../utilities/isValidStringID.js'
|
||||
import { DistinctSymbol } from '../utilities/rawConstraint.js'
|
||||
import { buildAndOrConditions } from './buildAndOrConditions.js'
|
||||
import { getTableColumnFromPath } from './getTableColumnFromPath.js'
|
||||
@@ -387,10 +388,59 @@ export function parseParams({
|
||||
orConditions.push(isNull(resolvedColumn))
|
||||
resolvedQueryValue = queryValue.filter((v) => v !== null)
|
||||
}
|
||||
|
||||
let constraint = adapter.operators[queryOperator](
|
||||
resolvedColumn,
|
||||
resolvedQueryValue,
|
||||
)
|
||||
|
||||
if (
|
||||
adapter.limitedBoundParameters &&
|
||||
(operator === 'in' || operator === 'not_in') &&
|
||||
relationOrPath === 'id' &&
|
||||
Array.isArray(queryValue)
|
||||
) {
|
||||
let isInvalid = false
|
||||
for (const val of queryValue) {
|
||||
if (typeof val === 'number' || val === null) {
|
||||
continue
|
||||
}
|
||||
if (typeof val === 'string') {
|
||||
if (!isValidStringID(val)) {
|
||||
isInvalid = true
|
||||
break
|
||||
} else {
|
||||
continue
|
||||
}
|
||||
}
|
||||
isInvalid = true
|
||||
break
|
||||
}
|
||||
|
||||
if (isInvalid) {
|
||||
throw new APIError(`Invalid ID value in ${JSON.stringify(queryValue)}`)
|
||||
}
|
||||
|
||||
constraints.push(
|
||||
sql.raw(
|
||||
`${resolvedColumn.name} ${operator === 'in' ? 'IN' : 'NOT IN'} (${queryValue
|
||||
.map((e) => {
|
||||
if (e === null) {
|
||||
return `NULL`
|
||||
}
|
||||
|
||||
if (typeof e === 'number') {
|
||||
return e
|
||||
}
|
||||
|
||||
return `'${e}'`
|
||||
})
|
||||
.join(',')})`,
|
||||
),
|
||||
)
|
||||
break
|
||||
}
|
||||
|
||||
if (orConditions.length) {
|
||||
orConditions.push(constraint)
|
||||
constraint = or(...orConditions)
|
||||
|
||||
@@ -8,6 +8,26 @@ export const insert: Insert = async function (
|
||||
): Promise<Record<string, unknown>[]> {
|
||||
const table = this.tables[tableName]
|
||||
|
||||
// Batch insert if limitedBoundParameters: true
|
||||
if (this.limitedBoundParameters && Array.isArray(values)) {
|
||||
const results: Record<string, unknown>[] = []
|
||||
const colsPerRow = Object.keys(values[0]).length
|
||||
const maxParams = 100
|
||||
const maxRowsPerBatch = Math.max(1, Math.floor(maxParams / colsPerRow))
|
||||
|
||||
for (let i = 0; i < values.length; i += maxRowsPerBatch) {
|
||||
const batch = values.slice(i, i + maxRowsPerBatch)
|
||||
|
||||
const batchResult = onConflictDoUpdate
|
||||
? await db.insert(table).values(batch).onConflictDoUpdate(onConflictDoUpdate).returning()
|
||||
: await db.insert(table).values(batch).returning()
|
||||
|
||||
results.push(...(batchResult as Record<string, unknown>[]))
|
||||
}
|
||||
|
||||
return results
|
||||
}
|
||||
|
||||
const result = onConflictDoUpdate
|
||||
? await db.insert(table).values(values).onConflictDoUpdate(onConflictDoUpdate).returning()
|
||||
: await db.insert(table).values(values).returning()
|
||||
|
||||
@@ -344,8 +344,8 @@ export interface DrizzleAdapter extends BaseDatabaseAdapter {
|
||||
drizzle: LibSQLDatabase | PostgresDB
|
||||
dropDatabase: DropDatabase
|
||||
enums?: never | Record<string, unknown>
|
||||
|
||||
execute: Execute<unknown>
|
||||
|
||||
features: {
|
||||
json?: boolean
|
||||
}
|
||||
@@ -358,6 +358,7 @@ export interface DrizzleAdapter extends BaseDatabaseAdapter {
|
||||
indexes: Set<string>
|
||||
initializing: Promise<void>
|
||||
insert: Insert
|
||||
limitedBoundParameters?: boolean
|
||||
localesSuffix?: string
|
||||
logger: DrizzleConfig['logger']
|
||||
operators: Operators
|
||||
|
||||
14
packages/drizzle/src/utilities/isValidStringID.spec.ts
Normal file
14
packages/drizzle/src/utilities/isValidStringID.spec.ts
Normal file
@@ -0,0 +1,14 @@
|
||||
import { isValidStringID } from './isValidStringID.js'
|
||||
|
||||
describe('isValidStringID', () => {
|
||||
it('should pass', () => {
|
||||
expect(isValidStringID('1')).toBe(true)
|
||||
expect(isValidStringID('a_b_c')).toBe(true)
|
||||
expect(isValidStringID('8cc2df6d-6e07-4da4-be48-5fa747c3b92b')).toBe(true)
|
||||
})
|
||||
|
||||
it('should not pass', () => {
|
||||
expect(isValidStringID('1 2 3')).toBe(false)
|
||||
expect(isValidStringID('1@')).toBe(false)
|
||||
})
|
||||
})
|
||||
3
packages/drizzle/src/utilities/isValidStringID.ts
Normal file
3
packages/drizzle/src/utilities/isValidStringID.ts
Normal file
@@ -0,0 +1,3 @@
|
||||
export function isValidStringID(value: string) {
|
||||
return /^[\w-]+$/.test(value)
|
||||
}
|
||||
99
test/database/sqlite-bound-parameters-limit.int.spec.ts
Normal file
99
test/database/sqlite-bound-parameters-limit.int.spec.ts
Normal file
@@ -0,0 +1,99 @@
|
||||
import type { Payload } from 'payload'
|
||||
|
||||
/* eslint-disable jest/require-top-level-describe */
|
||||
import path from 'path'
|
||||
import { fileURLToPath } from 'url'
|
||||
|
||||
import { initPayloadInt } from '../helpers/initPayloadInt.js'
|
||||
|
||||
const filename = fileURLToPath(import.meta.url)
|
||||
const dirname = path.dirname(filename)
|
||||
|
||||
const describeSqlite = process.env.PAYLOAD_DATABASE?.startsWith('sqlite') ? describe : describe.skip
|
||||
|
||||
let payload: Payload
|
||||
|
||||
describeSqlite('database - sqlite bound parameters limit', () => {
|
||||
beforeAll(async () => {
|
||||
;({ payload } = await initPayloadInt(dirname))
|
||||
})
|
||||
|
||||
afterAll(async () => {
|
||||
await payload.destroy()
|
||||
})
|
||||
|
||||
it('should not use bound parameters for where querying on ID with IN if limitedBoundParameters: true', async () => {
|
||||
const defaultExecute = payload.db.drizzle.$client.execute.bind(payload.db.drizzle.$client)
|
||||
|
||||
// Limit bounds parameters length
|
||||
payload.db.drizzle.$client.execute = async function execute(...args) {
|
||||
const res = await defaultExecute(...args)
|
||||
const [{ args: boundParameters }] = args as [{ args: any[] }]
|
||||
|
||||
// eslint-disable-next-line jest/no-conditional-in-test
|
||||
if (boundParameters.length > 100) {
|
||||
throw new Error('Exceeded limit of bound parameters!')
|
||||
}
|
||||
return res
|
||||
}
|
||||
|
||||
payload.db.limitedBoundParameters = false
|
||||
|
||||
const IN = Array.from({ length: 300 }, (_, i) => i)
|
||||
|
||||
// Should fail here because too the length exceeds the limit
|
||||
await expect(
|
||||
payload.find({
|
||||
collection: 'simple',
|
||||
pagination: false,
|
||||
where: { id: { in: IN } },
|
||||
}),
|
||||
).rejects.toBeTruthy()
|
||||
|
||||
// Should fail here because too the length exceeds the limit
|
||||
await expect(
|
||||
payload.find({
|
||||
collection: 'simple',
|
||||
pagination: false,
|
||||
where: { id: { not_in: IN } },
|
||||
}),
|
||||
).rejects.toBeTruthy()
|
||||
|
||||
payload.db.limitedBoundParameters = true
|
||||
|
||||
// Should not fail because limitedBoundParameters: true
|
||||
await expect(
|
||||
payload.find({
|
||||
collection: 'simple',
|
||||
pagination: false,
|
||||
where: { id: { in: IN } },
|
||||
}),
|
||||
).resolves.toBeTruthy()
|
||||
|
||||
// Should not fail because limitedBoundParameters: true
|
||||
await expect(
|
||||
payload.find({
|
||||
collection: 'simple',
|
||||
pagination: false,
|
||||
where: { id: { not_in: IN } },
|
||||
}),
|
||||
).resolves.toBeTruthy()
|
||||
|
||||
// Verify that "in" still works properly
|
||||
|
||||
const docs = await Promise.all(
|
||||
Array.from({ length: 300 }, () => payload.create({ collection: 'simple', data: {} })),
|
||||
)
|
||||
|
||||
const res = await payload.find({
|
||||
collection: 'simple',
|
||||
pagination: false,
|
||||
where: { id: { in: docs.map((e) => e.id) } },
|
||||
})
|
||||
|
||||
expect(res.totalDocs).toBe(300)
|
||||
for (const docInRes of res.docs) {
|
||||
expect(docs.some((doc) => doc.id === docInRes.id)).toBeTruthy()
|
||||
}
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user