From 9069bd3fac150f5fcb45b719caa70247da72ff78 Mon Sep 17 00:00:00 2001 From: Sasha <64744993+r1tsuu@users.noreply.github.com> Date: Thu, 24 Oct 2024 21:47:58 +0300 Subject: [PATCH] fix(db-postgres): sort by localized fields (#8839) ### What? Fixes https://github.com/payloadcms/payload/issues/5152 issue related to sorting by a localized field with SQLite / Postgres database adapters. ### Why? It was an incorrect behaviour. ### How? Modifies the `getTableColumnFromPath` file to have correct join conditions. Previously if you had this structure in the _locales table _locale title parent en A 1 es B 1 we sorted by everything that's here, but we need to sort only by the passed locale. Additionally fixes a typescript error in `dev.ts` that I added here https://github.com/payloadcms/payload/pull/8834 Also, removes the condition with `joins.length` in `countDistinct`. It was there as for this issue https://github.com/payloadcms/payload/issues/4889 because sorting by a localized property caused duplication. This can simnifically improve performance for `.find` with nested querying/sorting on large data sets, because `count(*)` is faster than `count(DISTINCT id)` --- packages/db-sqlite/src/countDistinct.ts | 8 +- .../drizzle/src/postgres/countDistinct.ts | 8 +- .../src/queries/getTableColumnFromPath.ts | 151 ++++++++---------- test/dev.ts | 1 + test/localization/int.spec.ts | 117 +++++++++++++- 5 files changed, 192 insertions(+), 93 deletions(-) diff --git a/packages/db-sqlite/src/countDistinct.ts b/packages/db-sqlite/src/countDistinct.ts index 84a08b7795..14b26d9acc 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, sql } from 'drizzle-orm' +import { count } from 'drizzle-orm' import type { CountDistinct, SQLiteAdapter } from './types.js' @@ -22,11 +22,7 @@ export const countDistinct: CountDistinct = async function countDistinct( methods: chainedMethods, query: db .select({ - count: - joins.length > 0 - ? sql`count - (DISTINCT ${this.tables[tableName].id})`.mapWith(Number) - : count(), + count: count(), }) .from(this.tables[tableName]) .where(where), diff --git a/packages/drizzle/src/postgres/countDistinct.ts b/packages/drizzle/src/postgres/countDistinct.ts index 302820ee3a..bea19daaf4 100644 --- a/packages/drizzle/src/postgres/countDistinct.ts +++ b/packages/drizzle/src/postgres/countDistinct.ts @@ -1,4 +1,4 @@ -import { count, sql } from 'drizzle-orm' +import { count } from 'drizzle-orm' import type { ChainedMethods, TransactionPg } from '../types.js' import type { BasePostgresAdapter, CountDistinct } from './types.js' @@ -22,11 +22,7 @@ export const countDistinct: CountDistinct = async function countDistinct( methods: chainedMethods, query: (db as TransactionPg) .select({ - count: - joins.length > 0 - ? sql`count - (DISTINCT ${this.tables[tableName].id})`.mapWith(Number) - : count(), + count: count(), }) .from(this.tables[tableName]) .where(where), diff --git a/packages/drizzle/src/queries/getTableColumnFromPath.ts b/packages/drizzle/src/queries/getTableColumnFromPath.ts index c2102d9380..6ec9103d35 100644 --- a/packages/drizzle/src/queries/getTableColumnFromPath.ts +++ b/packages/drizzle/src/queries/getTableColumnFromPath.ts @@ -186,18 +186,17 @@ export const getTableColumnFromPath = ({ if (locale && field.localized && adapter.payload.config.localization) { newTableName = `${tableName}${adapter.localesSuffix}` + let condition = eq(adapter.tables[tableName].id, adapter.tables[newTableName]._parentID) + + if (locale !== 'all') { + condition = and(condition, eq(adapter.tables[newTableName]._locale, locale)) + } + addJoinTable({ - condition: eq(adapter.tables[tableName].id, adapter.tables[newTableName]._parentID), + condition, joins, table: adapter.tables[newTableName], }) - if (locale !== 'all') { - constraints.push({ - columnName: '_locale', - table: adapter.tables[newTableName], - value: locale, - }) - } } return getTableColumnFromPath({ adapter, @@ -225,21 +224,20 @@ export const getTableColumnFromPath = ({ ) if (locale && field.localized && adapter.payload.config.localization) { + const conditions = [ + eq(adapter.tables[tableName].id, adapter.tables[newTableName].parent), + eq(adapter.tables[newTableName]._locale, locale), + ] + + if (locale !== 'all') { + conditions.push(eq(adapter.tables[newTableName]._locale, locale)) + } + addJoinTable({ - condition: and( - eq(adapter.tables[tableName].id, adapter.tables[newTableName].parent), - eq(adapter.tables[newTableName]._locale, locale), - ), + condition: and(...conditions), joins, table: adapter.tables[newTableName], }) - if (locale !== 'all') { - constraints.push({ - columnName: '_locale', - table: adapter.tables[newTableName], - value: locale, - }) - } } else { addJoinTable({ condition: eq(adapter.tables[tableName].id, adapter.tables[newTableName].parent), @@ -274,18 +272,16 @@ export const getTableColumnFromPath = ({ ] if (locale && field.localized && adapter.payload.config.localization) { + const conditions = [...joinConstraints] + + if (locale !== 'all') { + conditions.push(eq(adapter.tables[newTableName]._locale, locale)) + } addJoinTable({ - condition: and(...joinConstraints, eq(adapter.tables[newTableName]._locale, locale)), + condition: and(...conditions), joins, table: adapter.tables[newTableName], }) - if (locale !== 'all') { - constraints.push({ - columnName: 'locale', - table: adapter.tables[newTableName], - value: locale, - }) - } } else { addJoinTable({ condition: and(...joinConstraints), @@ -313,21 +309,16 @@ export const getTableColumnFromPath = ({ constraintPath = `${constraintPath}${field.name}.%.` if (locale && field.localized && adapter.payload.config.localization) { + const conditions = [eq(arrayParentTable.id, adapter.tables[newTableName]._parentID)] + + if (locale !== 'all') { + conditions.push(eq(adapter.tables[newTableName]._locale, locale)) + } addJoinTable({ - condition: and( - eq(arrayParentTable.id, adapter.tables[newTableName]._parentID), - eq(adapter.tables[newTableName]._locale, locale), - ), + condition: and(...conditions), joins, table: adapter.tables[newTableName], }) - if (locale !== 'all') { - constraints.push({ - columnName: '_locale', - table: adapter.tables[newTableName], - value: locale, - }) - } } else { addJoinTable({ condition: eq(arrayParentTable.id, adapter.tables[newTableName]._parentID), @@ -417,23 +408,21 @@ export const getTableColumnFromPath = ({ constraints = constraints.concat(blockConstraints) selectFields = { ...selectFields, ...blockSelectFields } if (field.localized && adapter.payload.config.localization) { - joins.push({ - condition: and( - eq( - (aliasTable || adapter.tables[tableName]).id, - adapter.tables[newTableName]._parentID, - ), - eq(adapter.tables[newTableName]._locale, locale), + const conditions = [ + eq( + (aliasTable || adapter.tables[tableName]).id, + adapter.tables[newTableName]._parentID, ), + ] + + if (locale !== 'all') { + conditions.push(eq(adapter.tables[newTableName]._locale, locale)) + } + + joins.push({ + condition: and(...conditions), table: adapter.tables[newTableName], }) - if (locale) { - constraints.push({ - columnName: '_locale', - table: adapter.tables[newTableName], - value: locale, - }) - } } else { joins.push({ condition: eq( @@ -471,21 +460,18 @@ export const getTableColumnFromPath = ({ // Join in the relationships table if (locale && field.localized && adapter.payload.config.localization) { + const conditions = [ + eq((aliasTable || adapter.tables[rootTableName]).id, aliasRelationshipTable.parent), + like(aliasRelationshipTable.path, `${constraintPath}${field.name}`), + ] + + if (locale !== 'all') { + conditions.push(eq(aliasRelationshipTable.locale, locale)) + } joins.push({ - condition: and( - eq((aliasTable || adapter.tables[rootTableName]).id, aliasRelationshipTable.parent), - eq(aliasRelationshipTable.locale, locale), - like(aliasRelationshipTable.path, `${constraintPath}${field.name}`), - ), + condition: and(...conditions), table: aliasRelationshipTable, }) - if (locale !== 'all') { - constraints.push({ - columnName: 'locale', - table: aliasRelationshipTable, - value: locale, - }) - } } else { // Join in the relationships table joins.push({ @@ -660,15 +646,22 @@ export const getTableColumnFromPath = ({ tableName: `${rootTableName}${adapter.localesSuffix}`, }) - joins.push({ - condition: and( - eq(aliasLocaleTable._parentID, adapter.tables[rootTableName].id), - eq(aliasLocaleTable._locale, locale), - ), - table: aliasLocaleTable, + const condtions = [eq(aliasLocaleTable._parentID, adapter.tables[rootTableName].id)] + + if (locale !== 'all') { + condtions.push(eq(aliasLocaleTable._locale, locale)) + } + + const localesTable = adapter.tables[`${rootTableName}${adapter.localesSuffix}`] + + addJoinTable({ + condition: and(...condtions), + joins, + table: localesTable, }) + joins.push({ - condition: eq(aliasLocaleTable[columnName], newAliasTable.id), + condition: eq(localesTable[columnName], newAliasTable.id), table: newAliasTable, }) } else { @@ -716,21 +709,19 @@ export const getTableColumnFromPath = ({ newTable = adapter.tables[newTableName] + let condition = eq(parentTable.id, newTable._parentID) + + if (locale !== 'all') { + condition = and(condition, eq(newTable._locale, locale)) + } + addJoinTable({ - condition: eq(parentTable.id, newTable._parentID), + condition, joins, table: newTable, }) aliasTable = undefined - - if (locale !== 'all') { - constraints.push({ - columnName: '_locale', - table: newTable, - value: locale, - }) - } } const targetTable = aliasTable || newTable diff --git a/test/dev.ts b/test/dev.ts index b84f5b3209..e221b212f9 100644 --- a/test/dev.ts +++ b/test/dev.ts @@ -53,6 +53,7 @@ if (args.o) { const port = process.env.PORT ? Number(process.env.PORT) : 3000 +// @ts-expect-error the same as in test/helpers/initPayloadE2E.ts const app = nextImport({ dev: true, hostname: 'localhost', diff --git a/test/localization/int.spec.ts b/test/localization/int.spec.ts index 4598952664..3fa5437de3 100644 --- a/test/localization/int.spec.ts +++ b/test/localization/int.spec.ts @@ -3,7 +3,7 @@ import { type Payload, type Where } from 'payload' import { fileURLToPath } from 'url' import type { NextRESTClient } from '../helpers/NextRESTClient.js' -import type { LocalizedPost, WithLocalizedRelationship } from './payload-types.js' +import type { LocalizedPost, LocalizedSort, WithLocalizedRelationship } from './payload-types.js' import { idToString } from '../helpers/idToString.js' import { initPayloadInt } from '../helpers/initPayloadInt.js' @@ -369,6 +369,7 @@ describe('Localization', () => { describe('Localized Sort Count', () => { const expectedTotalDocs = 5 + const posts: LocalizedSort[] = [] beforeAll(async () => { for (let i = 1; i <= expectedTotalDocs; i++) { const post = await payload.create({ @@ -380,6 +381,8 @@ describe('Localization', () => { locale: englishLocale, }) + posts.push(post) + await payload.update({ id: post.id, collection: localizedSortSlug, @@ -419,6 +422,118 @@ describe('Localization', () => { expect(sortByTitleQuery.totalDocs).toEqual(expectedTotalDocs) expect(sortByDateQuery.totalDocs).toEqual(expectedTotalDocs) }) + + it('should return correct order when sorted by localized fields', async () => { + const { docs: docsAsc } = await payload.find({ collection: localizedSortSlug, sort: 'title' }) + docsAsc.forEach((doc, i) => { + expect(posts[i].id).toBe(doc.id) + }) + + const { docs: docsDesc } = await payload.find({ + collection: localizedSortSlug, + sort: '-title', + }) + docsDesc.forEach((doc, i) => { + expect(posts.at(posts.length - i - 1).id).toBe(doc.id) + }) + + // Test with words + const randomWords = [ + 'sunset', + 'whisper', + 'lighthouse', + 'harmony', + 'crystal', + 'thunder', + 'meadow', + 'voyage', + 'echo', + 'quicksand', + ] + + const randomWordsSpanish = [ + 'atardecer', + 'susurro', + 'faro', + 'armonía', + 'cristal', + 'trueno', + 'pradera', + 'viaje', + 'eco', + 'arenas movedizas', + ] + + expect(randomWords).toHaveLength(randomWordsSpanish.length) + + const randomWordsPosts: (number | string)[] = [] + + for (let i = 0; i < randomWords.length; i++) { + const en = randomWords[i] + const post = await payload.create({ collection: 'localized-sort', data: { title: en } }) + const es = randomWordsSpanish[i] + await payload.update({ + collection: 'localized-sort', + data: { title: es }, + id: post.id, + locale: 'es', + }) + + randomWordsPosts.push(post.id) + } + + const ascSortedWordsEn = randomWords.toSorted((a, b) => a.localeCompare(b)) + const descSortedWordsEn = randomWords.toSorted((a, b) => b.localeCompare(a)) + + const q = { id: { in: randomWordsPosts } } + + const { docs: randomWordsEnAsc } = await payload.find({ + collection: localizedSortSlug, + sort: 'title', + where: q, + }) + randomWordsEnAsc.forEach((doc, i) => { + expect(ascSortedWordsEn[i]).toBe(doc.title) + }) + + const { docs: randomWordsEnDesc } = await payload.find({ + collection: localizedSortSlug, + sort: '-title', + where: q, + }) + + randomWordsEnDesc.forEach((doc, i) => { + expect(descSortedWordsEn[i]).toBe(doc.title) + }) + + // Test sorting for Spanish locale + const ascSortedWordsEs = randomWordsSpanish.toSorted((a, b) => a.localeCompare(b)) + const descSortedWordsEs = randomWordsSpanish.toSorted((a, b) => b.localeCompare(a)) + + // Fetch sorted words in Spanish (ascending) + const { docs: randomWordsEsAsc } = await payload.find({ + collection: localizedSortSlug, + sort: 'title', + where: q, + locale: 'es', + }) + + randomWordsEsAsc.forEach((doc, i) => { + expect(ascSortedWordsEs[i]).toBe(doc.title) + }) + + // Fetch sorted words in Spanish (descending) + const { docs: randomWordsEsDesc } = await payload.find({ + collection: localizedSortSlug, + sort: '-title', + where: q, + locale: 'es', + }) + + randomWordsEsDesc.forEach((doc, i) => { + expect(descSortedWordsEs[i]).toBe(doc.title) + }) + }) }) describe('Localized Relationship', () => {