From 98fec353687f269e2de473dda013d0b194bbb455 Mon Sep 17 00:00:00 2001 From: Sasha <64744993+r1tsuu@users.noreply.github.com> Date: Tue, 11 Feb 2025 01:16:18 +0200 Subject: [PATCH] fix(db-postgres): incorrect pagination results when querying hasMany relationships multiple times (#11096) Fixes https://github.com/payloadcms/payload/issues/10810 This was caused by using `COUNT(*)` aggregation instead of `COUNT(DISTINCT table.id)`. However, we want to use `COUNT(*)` because `COUNT(DISTINCT table.id)` is slow on large tables. Now we fallback to `COUNT(DISTINCT table.id)` only when `COUNT(*)` cannot work properly. Example of a query that leads to incorrect `totalDocs`: ```ts const res = await payload.find({ collection: 'directors', limit: 10, where: { or: [ { movies: { equals: movie2.id, }, }, { movies: { equals: movie1.id, }, }, { movies: { equals: movie1.id, }, }, ], }, }) ``` --- packages/db-sqlite/src/countDistinct.ts | 17 +++++-- .../drizzle/src/postgres/countDistinct.ts | 16 +++++-- packages/drizzle/src/queries/addJoinTable.ts | 8 ++-- packages/drizzle/src/queries/buildQuery.ts | 1 + .../src/queries/getTableColumnFromPath.ts | 13 +++-- test/relationships/int.spec.ts | 47 +++++++++++++++++++ 6 files changed, 89 insertions(+), 13 deletions(-) diff --git a/packages/db-sqlite/src/countDistinct.ts b/packages/db-sqlite/src/countDistinct.ts index 14b26d9acc..e57441d0ad 100644 --- a/packages/db-sqlite/src/countDistinct.ts +++ b/packages/db-sqlite/src/countDistinct.ts @@ -1,7 +1,7 @@ import type { ChainedMethods } from '@payloadcms/drizzle/types' import { chainMethods } from '@payloadcms/drizzle' -import { count } from 'drizzle-orm' +import { count, sql } from 'drizzle-orm' import type { CountDistinct, SQLiteAdapter } from './types.js' @@ -11,18 +11,27 @@ export const countDistinct: CountDistinct = async function countDistinct( ) { const chainedMethods: ChainedMethods = [] - joins.forEach(({ condition, table }) => { + // COUNT(DISTINCT id) is slow on large tables, so we only use DISTINCT if we have to + const visitedPaths = new Set([]) + let useDistinct = false + joins.forEach(({ condition, queryPath, table }) => { + if (!useDistinct && queryPath) { + if (visitedPaths.has(queryPath)) { + useDistinct = true + } else { + visitedPaths.add(queryPath) + } + } chainedMethods.push({ args: [table, condition], method: 'leftJoin', }) }) - const countResult = await chainMethods({ methods: chainedMethods, query: db .select({ - count: count(), + count: useDistinct ? sql`COUNT(DISTINCT ${this.tables[tableName].id})` : count(), }) .from(this.tables[tableName]) .where(where), diff --git a/packages/drizzle/src/postgres/countDistinct.ts b/packages/drizzle/src/postgres/countDistinct.ts index bea19daaf4..789142f3b0 100644 --- a/packages/drizzle/src/postgres/countDistinct.ts +++ b/packages/drizzle/src/postgres/countDistinct.ts @@ -1,4 +1,4 @@ -import { count } from 'drizzle-orm' +import { count, sql } from 'drizzle-orm' import type { ChainedMethods, TransactionPg } from '../types.js' import type { BasePostgresAdapter, CountDistinct } from './types.js' @@ -11,7 +11,17 @@ export const countDistinct: CountDistinct = async function countDistinct( ) { const chainedMethods: ChainedMethods = [] - joins.forEach(({ condition, table }) => { + // COUNT(DISTINCT id) is slow on large tables, so we only use DISTINCT if we have to + const visitedPaths = new Set([]) + let useDistinct = false + joins.forEach(({ condition, queryPath, table }) => { + if (!useDistinct && queryPath) { + if (visitedPaths.has(queryPath)) { + useDistinct = true + } else { + visitedPaths.add(queryPath) + } + } chainedMethods.push({ args: [table, condition], method: 'leftJoin', @@ -22,7 +32,7 @@ export const countDistinct: CountDistinct = async function countDistinct( methods: chainedMethods, query: (db as TransactionPg) .select({ - count: count(), + count: useDistinct ? sql`COUNT(DISTINCT ${this.tables[tableName].id})` : count(), }) .from(this.tables[tableName]) .where(where), diff --git a/packages/drizzle/src/queries/addJoinTable.ts b/packages/drizzle/src/queries/addJoinTable.ts index 3d9564d4c8..d876d2b45f 100644 --- a/packages/drizzle/src/queries/addJoinTable.ts +++ b/packages/drizzle/src/queries/addJoinTable.ts @@ -1,5 +1,5 @@ -import type { SQL } from 'drizzle-orm' -import type { PgTableWithColumns } from 'drizzle-orm/pg-core' +import { type SQL } from 'drizzle-orm' +import { type PgTableWithColumns } from 'drizzle-orm/pg-core' import type { GenericTable } from '../types.js' import type { BuildQueryJoinAliases } from './buildQuery.js' @@ -10,16 +10,18 @@ export const addJoinTable = ({ type, condition, joins, + queryPath, table, }: { condition: SQL joins: BuildQueryJoinAliases + queryPath?: string table: GenericTable | PgTableWithColumns type?: 'innerJoin' | 'leftJoin' | 'rightJoin' }) => { const name = getNameFromDrizzleTable(table) if (!joins.some((eachJoin) => getNameFromDrizzleTable(eachJoin.table) === name)) { - joins.push({ type, condition, table }) + joins.push({ type, condition, queryPath, table }) } } diff --git a/packages/drizzle/src/queries/buildQuery.ts b/packages/drizzle/src/queries/buildQuery.ts index 7777355a36..95dc2a75ab 100644 --- a/packages/drizzle/src/queries/buildQuery.ts +++ b/packages/drizzle/src/queries/buildQuery.ts @@ -9,6 +9,7 @@ import { parseParams } from './parseParams.js' export type BuildQueryJoinAliases = { condition: SQL + queryPath?: string table: GenericTable | PgTableWithColumns type?: 'innerJoin' | 'leftJoin' | 'rightJoin' }[] diff --git a/packages/drizzle/src/queries/getTableColumnFromPath.ts b/packages/drizzle/src/queries/getTableColumnFromPath.ts index edcede204a..bbd72fe56a 100644 --- a/packages/drizzle/src/queries/getTableColumnFromPath.ts +++ b/packages/drizzle/src/queries/getTableColumnFromPath.ts @@ -363,7 +363,10 @@ export const getTableColumnFromPath = ({ const { newAliasTable: aliasRelationshipTable, newAliasTableName: aliasRelationshipTableName, - } = getTableAlias({ adapter, tableName: relationTableName }) + } = getTableAlias({ + adapter, + tableName: relationTableName, + }) if (selectLocale && field.localized && adapter.payload.config.localization) { selectFields._locale = aliasRelationshipTable.locale @@ -380,17 +383,21 @@ export const getTableColumnFromPath = ({ conditions.push(eq(aliasRelationshipTable.locale, locale)) } - joins.push({ + addJoinTable({ condition: and(...conditions), + joins, + queryPath: `${constraintPath}.${field.name}`, table: aliasRelationshipTable, }) } else { // Join in the relationships table - joins.push({ + addJoinTable({ condition: and( eq((aliasTable || adapter.tables[rootTableName]).id, aliasRelationshipTable.parent), like(aliasRelationshipTable.path, `${constraintPath}${field.name}`), ), + joins, + queryPath: `${constraintPath}.${field.name}`, table: aliasRelationshipTable, }) } diff --git a/test/relationships/int.spec.ts b/test/relationships/int.spec.ts index 9fbb1740e6..d229c0532c 100644 --- a/test/relationships/int.spec.ts +++ b/test/relationships/int.spec.ts @@ -412,6 +412,51 @@ describe('Relationships', () => { expect(customIdNumberRelation).toMatchObject({ id: generatedCustomIdNumber }) }) + it('should retrieve totalDocs correctly with hasMany,', async () => { + const movie1 = await payload.create({ + collection: 'movies', + data: {}, + }) + const movie2 = await payload.create({ + collection: 'movies', + data: {}, + }) + + await payload.create({ + collection: 'directors', + data: { + name: 'Quentin Tarantino', + movies: [movie2.id, movie1.id], + }, + }) + + const res = await payload.find({ + collection: 'directors', + limit: 10, + where: { + or: [ + { + movies: { + equals: movie2.id, + }, + }, + { + movies: { + equals: movie1.id, + }, + }, + { + movies: { + equals: movie1.id, + }, + }, + ], + }, + }) + + expect(res.totalDocs).toBe(1) + }) + it('should query using "contains" by hasMany relationship field', async () => { const movie1 = await payload.create({ collection: 'movies', @@ -498,6 +543,7 @@ describe('Relationships', () => { }, }) + // eslint-disable-next-line jest/no-standalone-expect expect(query1.totalDocs).toStrictEqual(1) }) @@ -1424,6 +1470,7 @@ describe('Relationships', () => { }) .then((res) => res.json()) + // eslint-disable-next-line jest/no-standalone-expect expect(queryOne.docs).toHaveLength(1) })