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, }, }, ], }, }) ```
This commit is contained in:
@@ -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),
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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<any>
|
||||
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 })
|
||||
}
|
||||
}
|
||||
|
||||
@@ -9,6 +9,7 @@ import { parseParams } from './parseParams.js'
|
||||
|
||||
export type BuildQueryJoinAliases = {
|
||||
condition: SQL
|
||||
queryPath?: string
|
||||
table: GenericTable | PgTableWithColumns<any>
|
||||
type?: 'innerJoin' | 'leftJoin' | 'rightJoin'
|
||||
}[]
|
||||
|
||||
@@ -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,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
|
||||
|
||||
Reference in New Issue
Block a user