Skip to content

Commit 9a309af

Browse files
fix(db-mongodb): avoid unnecessary aggregation when no joins are selected (#17785)
Backport of #17745 to 3.x. ### What? `find`, `findOne` and `queryDrafts` in `@payloadcms/db-mongodb` run a MongoDB aggregation even when the join pipeline they just built is empty — no `$lookup` stages in it at all. The effect is that any collection with a join field stops using `Model.paginate` / `Model.findOne` on reads whose `select` doesn't include the join, and goes through `Model.aggregate` instead for no benefit. This PR routes those queries back to the non-aggregation path. ### Why? `buildJoinAggregation` returned `PipelineStage[] | undefined`. It returns `undefined` from its two early exits (join aggregations disabled; no joins configured, or `joins === false`), but when it runs to completion with every configured join skipped, it returns an empty array. Joins get skipped on two entirely normal inputs, and both guards exist in both the polymorphic and the regular join loop: - a `select` that omits the join field — `projection && !projection[projectionPath]` - per-join `joins: { <name>: false }` `[]` is truthy, so `if (aggregate)` at all three call sites took the aggregation branch anyway. ### How? `buildJoinAggregation` now always returns an array (`Promise<PipelineStage[]>`), and the three call sites gate on `aggregate.length > 0`, symmetric with the `sortAggregation.length > 0` check sitting next to them. Fixing it at the type level rather than with `aggregate?.length` at each call site is deliberate: nothing ever consumed the `undefined`/`[]` distinction — all three post-query `resolveJoins` branches read `this.useJoinAggregations` directly — and on a non-nullable array `if (aggregate)` is visibly always-true, so the original mistake can't be rewritten. The function isn't exported from the package, so the signature change isn't a public API break.
1 parent 6e5f386 commit 9a309af

6 files changed

Lines changed: 163 additions & 12 deletions

File tree

‎packages/db-mongodb/src/find.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ export const find: Find = async function find(
154154
query,
155155
})
156156

157-
if (aggregate || sortAggregation.length > 0) {
157+
if (aggregate.length > 0 || sortAggregation.length > 0) {
158158
result = await aggregatePaginate({
159159
adapter: this,
160160
collation: paginationOptions.collation,

‎packages/db-mongodb/src/findOne.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ export const findOne: FindOne = async function findOne(
5151
}
5252

5353
let doc
54-
if (aggregate) {
54+
if (aggregate.length > 0) {
5555
const { docs } = await aggregatePaginate({
5656
adapter: this,
5757
joinAggregation: aggregate,

‎packages/db-mongodb/src/queryDrafts.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ export const queryDrafts: QueryDrafts = async function queryDrafts(
145145
versions: true,
146146
})
147147

148-
if (aggregate || sortAggregation.length > 0) {
148+
if (aggregate.length > 0 || sortAggregation.length > 0) {
149149
result = await aggregatePaginate({
150150
adapter: this,
151151
collation: paginationOptions.collation,

‎packages/db-mongodb/src/utilities/buildJoinAggregation.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,16 +43,16 @@ export const buildJoinAggregation = async ({
4343
locale,
4444
projection,
4545
versions,
46-
}: BuildJoinAggregationArgs): Promise<PipelineStage[] | undefined> => {
46+
}: BuildJoinAggregationArgs): Promise<PipelineStage[]> => {
4747
if (!adapter.useJoinAggregations) {
48-
return
48+
return []
4949
}
5050
if (
5151
(Object.keys(collectionConfig.joins).length === 0 &&
5252
collectionConfig.polymorphicJoins.length == 0) ||
5353
joins === false
5454
) {
55-
return
55+
return []
5656
}
5757

5858
const joinConfig = adapter.payload.collections[collection]?.config?.joins

‎test/joins/buildJoinAggregation.int.spec.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ describe(
122122

123123
expect(aggregation).toBeInstanceOf(Array)
124124

125-
const lookups = getLookups(aggregation!)
125+
const lookups = getLookups(aggregation)
126126

127127
expect(lookups).toHaveLength(2)
128128

@@ -155,7 +155,7 @@ describe(
155155

156156
expect(aggregation).toBeInstanceOf(Array)
157157

158-
const lookups = getLookups(aggregation!)
158+
const lookups = getLookups(aggregation)
159159
expect(lookups).toHaveLength(1)
160160

161161
expect(lookups[0]!.as).toBe('posts.docs')
@@ -182,7 +182,7 @@ describe(
182182

183183
expect(aggregation).toBeInstanceOf(Array)
184184

185-
const lookups = getLookups(aggregation!)
185+
const lookups = getLookups(aggregation)
186186
expect(lookups).toHaveLength(1)
187187

188188
expect(lookups[0]!.as).toBe('postsMany.docs')
@@ -224,7 +224,7 @@ describe(
224224

225225
expect(aggregation).toBeInstanceOf(Array)
226226

227-
const lookups = getLookups(aggregation!)
227+
const lookups = getLookups(aggregation)
228228

229229
expect(lookups).toHaveLength(2)
230230

@@ -266,7 +266,7 @@ describe(
266266

267267
expect(aggregation).toBeInstanceOf(Array)
268268

269-
const lookups = getLookups(aggregation!)
269+
const lookups = getLookups(aggregation)
270270
expect(lookups).toHaveLength(1)
271271

272272
expect(lookups[0]!.as).toBe('version.posts.docs')
@@ -302,7 +302,7 @@ describe(
302302

303303
expect(aggregation).toBeInstanceOf(Array)
304304

305-
const lookups = getLookups(aggregation!)
305+
const lookups = getLookups(aggregation)
306306
expect(lookups).toHaveLength(1)
307307

308308
expect(lookups[0]!.as).toBe('version.postsMany.docs')

‎test/joins/queryPath.int.spec.ts‎

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
// @ts-ignore
2+
import { type MongooseAdapter } from '@payloadcms/db-mongodb'
3+
import { buildConfig, getPayload } from 'payload'
4+
import { afterEach, expect, it, vi } from 'vitest'
5+
6+
import { describe } from '../__helpers/int/vitest.js'
7+
8+
describe(
9+
'mongodb read path selection',
10+
{ db: (adapter) => adapter === 'mongodb' || adapter === 'mongodb-atlas' },
11+
() => {
12+
const createdIDs: (number | string)[] = []
13+
14+
const getPayloadInstance = async () =>
15+
await getPayload({
16+
key: '_joinsQueryPath',
17+
config: await buildConfig({
18+
secret: '______',
19+
db: await import('../databaseAdapter.js').then((mod) => mod.databaseAdapter),
20+
collections: [
21+
{
22+
slug: 'categories',
23+
fields: [
24+
{ name: 'title', type: 'text' },
25+
{
26+
name: 'posts',
27+
type: 'join',
28+
collection: 'posts',
29+
on: 'category',
30+
},
31+
],
32+
versions: false,
33+
},
34+
{
35+
slug: 'posts',
36+
fields: [
37+
{
38+
name: 'category',
39+
type: 'relationship',
40+
relationTo: 'categories',
41+
},
42+
],
43+
versions: false,
44+
},
45+
],
46+
}),
47+
})
48+
49+
const seedCategory = async () => {
50+
const payload = await getPayloadInstance()
51+
52+
const category = await payload.create({
53+
collection: 'categories',
54+
// @ts-expect-error not generated
55+
data: { title: 'a' },
56+
})
57+
createdIDs.push(category.id)
58+
59+
return {
60+
adapter: payload.db as unknown as MongooseAdapter,
61+
category,
62+
payload,
63+
}
64+
}
65+
66+
afterEach(async () => {
67+
vi.restoreAllMocks()
68+
69+
const payload = await getPayloadInstance()
70+
71+
for (const id of createdIDs) {
72+
await payload.delete({ collection: 'categories', id })
73+
}
74+
75+
createdIDs.length = 0
76+
})
77+
78+
it('should use Model.paginate when a select excludes every join field', async () => {
79+
const { adapter, payload } = await seedCategory()
80+
const Model = adapter.collections.categories!
81+
82+
const aggregateSpy = vi.spyOn(Model, 'aggregate')
83+
const paginateSpy = vi.spyOn(Model, 'paginate')
84+
85+
await payload.find({
86+
collection: 'categories',
87+
limit: 20,
88+
// @ts-expect-error not generated
89+
select: { title: true },
90+
where: { title: { equals: 'a' } },
91+
})
92+
93+
expect(aggregateSpy).not.toHaveBeenCalled()
94+
expect(paginateSpy).toHaveBeenCalledTimes(1)
95+
})
96+
97+
it('should use Model.paginate when every join is disabled individually', async () => {
98+
const { adapter, payload } = await seedCategory()
99+
const Model = adapter.collections.categories!
100+
101+
const aggregateSpy = vi.spyOn(Model, 'aggregate')
102+
const paginateSpy = vi.spyOn(Model, 'paginate')
103+
104+
await payload.find({
105+
collection: 'categories',
106+
// @ts-expect-error not generated
107+
joins: { posts: false },
108+
limit: 20,
109+
where: { title: { equals: 'a' } },
110+
})
111+
112+
expect(aggregateSpy).not.toHaveBeenCalled()
113+
expect(paginateSpy).toHaveBeenCalledTimes(1)
114+
})
115+
116+
it('should use Model.findOne for findByID when a select excludes every join field', async () => {
117+
const { adapter, category, payload } = await seedCategory()
118+
const Model = adapter.collections.categories!
119+
120+
const aggregateSpy = vi.spyOn(Model, 'aggregate')
121+
const findOneSpy = vi.spyOn(Model, 'findOne')
122+
123+
await payload.findByID({
124+
collection: 'categories',
125+
id: category.id,
126+
// @ts-expect-error not generated
127+
select: { title: true },
128+
})
129+
130+
expect(aggregateSpy).not.toHaveBeenCalled()
131+
expect(findOneSpy).toHaveBeenCalledTimes(1)
132+
})
133+
134+
it('should still use Model.aggregate when a join field is actually selected', async () => {
135+
const { adapter, payload } = await seedCategory()
136+
const Model = adapter.collections.categories!
137+
138+
const aggregateSpy = vi.spyOn(Model, 'aggregate')
139+
const paginateSpy = vi.spyOn(Model, 'paginate')
140+
141+
await payload.find({
142+
collection: 'categories',
143+
limit: 20,
144+
where: { title: { equals: 'a' } },
145+
})
146+
147+
expect(aggregateSpy).toHaveBeenCalledTimes(1)
148+
expect(paginateSpy).not.toHaveBeenCalled()
149+
})
150+
},
151+
)

0 commit comments

Comments
 (0)