Skip to content

Commit 0396c48

Browse files
RubenDarioGuerreroNeiraRubenDarioGuerreroNeira
committed
fix(core): Optimize collection variants N+1 and persist catalog filters
1 parent b6133a4 commit 0396c48

9 files changed

Lines changed: 347 additions & 13 deletions

File tree

‎e2e-common/benchmarks/benchmark-list-query.e2e-spec.ts‎

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
1+
/* eslint-disable no-console */
2+
import { ID, FacetValue, VendureConfig } from '@vendure/core';
3+
import { createTestEnvironment } from '@vendure/testing';
4+
import { gql } from 'graphql-tag';
5+
import path from 'path';
6+
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
7+
8+
import { initialData } from '../e2e-initial-data';
9+
import { testConfig } from '../test-config';
10+
11+
describe('ListQueryBuilder Optimization Benchmark', () => {
12+
let capturedQueries: string[] = [];
13+
const baseConfig = testConfig();
14+
15+
const benchmarkConfig: VendureConfig = {
16+
...baseConfig,
17+
customFields: {
18+
Product: [
19+
{
20+
name: 'testManyToMany',
21+
type: 'relation',
22+
entity: FacetValue,
23+
graphQLType: 'FacetValue',
24+
list: true,
25+
},
26+
{
27+
name: 'testManyToOne',
28+
type: 'relation',
29+
entity: FacetValue,
30+
graphQLType: 'FacetValue',
31+
list: false,
32+
},
33+
],
34+
},
35+
dbConnectionOptions: {
36+
...baseConfig.dbConnectionOptions,
37+
logging: ['query'],
38+
logger: {
39+
logQuery(query: string) {
40+
if (
41+
query.includes('SELECT') &&
42+
(query.includes('"product"') || query.includes('`product`'))
43+
) {
44+
capturedQueries.push(query);
45+
}
46+
},
47+
logQueryError: (error: string) => console.error(error),
48+
logQuerySlow: (time: number, query: string) => console.warn(query, time),
49+
logSchemaBuild: () => {
50+
/* no-op */
51+
},
52+
logMigration: () => {
53+
/* no-op */
54+
},
55+
log: () => {
56+
/* no-op */
57+
},
58+
} as any,
59+
},
60+
};
61+
62+
const { server, adminClient } = createTestEnvironment(benchmarkConfig);
63+
64+
beforeAll(async () => {
65+
await server.init({
66+
initialData,
67+
productsCsvPath: path.join(__dirname, '../../packages/core/e2e/fixtures/e2e-products-minimal.csv'),
68+
customerCount: 1,
69+
});
70+
await adminClient.asSuperAdmin();
71+
}, 240000);
72+
73+
afterAll(async () => {
74+
await server.destroy();
75+
});
76+
77+
it('uses multiple EXISTS for ManyToMany custom field relation AND filter', async () => {
78+
const GET_PRODUCTS = gql`
79+
query GetProducts($options: ProductListOptions) {
80+
products(options: $options) {
81+
items {
82+
id
83+
}
84+
totalItems
85+
}
86+
}
87+
`;
88+
89+
capturedQueries = [];
90+
91+
await adminClient.query(GET_PRODUCTS, {
92+
options: {
93+
filter: {
94+
_and: [
95+
{ testManyToManyId: { eq: '1' } },
96+
{ testManyToManyId: { eq: '2' } }
97+
],
98+
},
99+
},
100+
});
101+
102+
const lastQuery = capturedQueries.find(q => q.includes('WHERE') && q.includes('testManyToMany'));
103+
expect(lastQuery, 'Should have a query with WHERE and testManyToMany').toBeDefined();
104+
if (lastQuery) {
105+
const existsCount = (lastQuery.match(/EXISTS/g) || []).length;
106+
expect(existsCount).toBe(2);
107+
// Verify no JOIN was added for the filter
108+
expect(lastQuery).not.toContain('LEFT JOIN');
109+
}
110+
});
111+
112+
it('uses EXISTS for ManyToOne custom field relation when filtering (optimized)', async () => {
113+
const GET_PRODUCTS = gql`
114+
query GetProducts($options: ProductListOptions) {
115+
products(options: $options) {
116+
items {
117+
id
118+
}
119+
totalItems
120+
}
121+
}
122+
`;
123+
124+
capturedQueries = [];
125+
126+
await adminClient.query(GET_PRODUCTS, {
127+
options: {
128+
filter: {
129+
testManyToOneId: { eq: '1' },
130+
},
131+
},
132+
});
133+
134+
const lastQuery = capturedQueries.find(q => q.includes('WHERE') && q.includes('testManyToOne'));
135+
expect(lastQuery, 'Should have a query with WHERE and testManyToOne').toBeDefined();
136+
if (lastQuery) {
137+
const existsCount = (lastQuery.match(/EXISTS/g) || []).length;
138+
expect(existsCount).toBe(1);
139+
// Verify no JOIN was added for the ManyToOne filter (optimization)
140+
expect(lastQuery).not.toContain('LEFT JOIN');
141+
}
142+
});
143+
144+
it('uses JOIN for ManyToOne custom field relation when sorting', async () => {
145+
const GET_PRODUCTS = gql`
146+
query GetProducts($options: ProductListOptions) {
147+
products(options: $options) {
148+
items {
149+
id
150+
}
151+
}
152+
}
153+
`;
154+
155+
capturedQueries = [];
156+
157+
await adminClient.query(GET_PRODUCTS, {
158+
options: {
159+
sort: {
160+
testManyToOneId: 'ASC',
161+
},
162+
},
163+
});
164+
165+
const lastQuery = capturedQueries.find(q => q.includes('testManyToOne'));
166+
expect(lastQuery, 'Should have a query with testManyToOne').toBeDefined();
167+
if (lastQuery) {
168+
// Verify JOIN was added for sorting
169+
expect(lastQuery).toContain('LEFT JOIN');
170+
// EXISTS is not used for sorting
171+
const existsCount = (lastQuery.match(/EXISTS/g) || []).length;
172+
expect(existsCount).toBe(0);
173+
}
174+
});
175+
});

‎packages/core/src/api/resolvers/admin/collection.resolver.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,11 +66,18 @@ export class CollectionResolver {
6666
const collections = await this.collectionService.findAll(ctx, args.options || undefined, relations);
6767
// Cache the variant counts query promise if productVariantCount is requested,
6868
// allowing the DB query to start before the field resolvers are called
69+
const collectionIds = collections.items.map(c => c.id);
6970
if (isFieldInSelection(info, 'productVariantCount')) {
70-
const collectionIds = collections.items.map(c => c.id);
7171
const countsPromise = this.collectionService.getProductVariantCounts(ctx, collectionIds);
7272
this.requestContextCache.set(ctx, CacheKey.CollectionVariantCounts, countsPromise);
7373
}
74+
if (isFieldInSelection(info, 'variants')) {
75+
const variantsPromise = this.collectionService.getProductVariantsForCollections(
76+
ctx,
77+
collectionIds,
78+
);
79+
this.requestContextCache.set(ctx, CacheKey.CollectionVariants, variantsPromise);
80+
}
7481
return collections;
7582
}
7683

‎packages/core/src/api/resolvers/entity/collection-entity.resolver.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,27 @@ export class CollectionEntityResolver {
6363
@Api() apiType: ApiType,
6464
@Relations({ entity: ProductVariant, omit: ['assets'] }) relations: RelationPaths<ProductVariant>,
6565
): Promise<PaginatedList<Translated<ProductVariant>>> {
66+
const isDefaultOptions = !args.options || Object.keys(args.options).length === 0;
67+
if (isDefaultOptions && apiType === 'admin') {
68+
const cachedVariantsPromise = this.requestContextCache.get<
69+
Promise<Map<string, ProductVariant[]>>
70+
>(ctx, CacheKey.CollectionVariants);
71+
if (cachedVariantsPromise) {
72+
const variantsMap = await cachedVariantsPromise;
73+
const variants = variantsMap.get(String(collection.id));
74+
if (variants) {
75+
const items = await this.productVariantService.applyPricesAndTranslateVariants(
76+
ctx,
77+
variants,
78+
);
79+
return {
80+
items,
81+
totalItems: items.length,
82+
};
83+
}
84+
}
85+
}
86+
6687
let options: ListQueryOptions<Product> = args.options;
6788
if (apiType === 'shop') {
6889
options = {

‎packages/core/src/common/constants.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,4 +85,5 @@ export const CacheKey = {
8585
ActiveTaxZone: (channelId: ID) => `ActiveTaxZone:${channelId}`,
8686
ActiveTaxZone_PPA: (channelId: ID) => `ActiveTaxZone_PPA:${channelId}`,
8787
CollectionVariantCounts: 'CollectionService.getProductVariantCounts',
88+
CollectionVariants: 'CollectionService.getProductVariantsForCollections',
8889
};

‎packages/core/src/config/config.module.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { Module, OnApplicationBootstrap, OnApplicationShutdown } from '@nestjs/common';
1+
import { Module, OnApplicationBootstrap, OnApplicationShutdown, Optional } from '@nestjs/common';
22
import { ModuleRef } from '@nestjs/core';
33

44
import { ConfigurableOperationDef } from '../common/configurable-operation';
@@ -15,7 +15,7 @@ import { ConfigService } from './config.service';
1515
export class ConfigModule implements OnApplicationBootstrap, OnApplicationShutdown {
1616
constructor(
1717
private configService: ConfigService,
18-
private moduleRef: ModuleRef,
18+
@Optional() private moduleRef: ModuleRef,
1919
) {}
2020

2121
async onApplicationBootstrap() {
@@ -37,6 +37,9 @@ export class ConfigModule implements OnApplicationBootstrap, OnApplicationShutdo
3737
}
3838

3939
private async initInjectableStrategies() {
40+
if (!this.moduleRef) {
41+
return;
42+
}
4043
const injector = new Injector(this.moduleRef);
4144
for (const strategy of this.getInjectableStrategies()) {
4245
if (typeof strategy.init === 'function') {
@@ -54,6 +57,9 @@ export class ConfigModule implements OnApplicationBootstrap, OnApplicationShutdo
5457
}
5558

5659
private async initConfigurableOperations() {
60+
if (!this.moduleRef) {
61+
return;
62+
}
5763
const injector = new Injector(this.moduleRef);
5864
for (const operation of this.getConfigurableOperations()) {
5965
await operation.init(injector);

‎packages/core/src/service/helpers/list-query-builder/list-query-builder.ts‎

Lines changed: 58 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -401,8 +401,12 @@ export class ListQueryBuilder implements OnApplicationBootstrap {
401401
} else {
402402
qb.orWhere(existsClause.clause, existsClause.parameters);
403403
}
404-
return;
404+
} else {
405+
Logger.warn(
406+
`Could not build EXISTS subquery for custom property "${condition.isExistsCondition.customPropertyKey}". Skipping filter condition.`,
407+
);
405408
}
409+
return;
406410
}
407411

408412
// Standard WHERE clause handling
@@ -480,7 +484,10 @@ export class ListQueryBuilder implements OnApplicationBootstrap {
480484
// Helper to escape identifiers for the current database driver (handles PostgreSQL quoting)
481485
const escapeId = (name: string) => mainQb.connection.driver.escape(name);
482486
const escapeTablePath = (path: string) =>
483-
path.split('.').map(segment => mainQb.connection.driver.escape(segment)).join('.');
487+
path
488+
.split('.')
489+
.map(segment => mainQb.connection.driver.escape(segment))
490+
.join('.');
484491

485492
let existsQuery: string;
486493

@@ -553,6 +560,29 @@ export class ListQueryBuilder implements OnApplicationBootstrap {
553560
SELECT 1 FROM ${escapeTablePath(inverseTableName)} ${escapeId(relatedAlias)}
554561
WHERE ${escapeId(relatedAlias)}.${escapeId(foreignKeyColumn)} = ${escapeId(mainQb.alias)}.${escapeId('id')} AND ${whereCondition}
555562
)`;
563+
} else if (relation.isManyToOne) {
564+
// ManyToOne: The foreign key is on the main entity table
565+
const relatedAlias = aliasBase;
566+
const joinColumns = relation.joinColumns;
567+
if (!joinColumns || joinColumns.length === 0) {
568+
return null;
569+
}
570+
const foreignKeyColumn = joinColumns[0].databaseName;
571+
572+
const whereCondition = this.buildWhereConditionClause(
573+
relatedAlias,
574+
columnName,
575+
comparisonOperator,
576+
newParamKey,
577+
escapeId,
578+
);
579+
580+
// EXISTS (SELECT 1 FROM related_table rt
581+
// WHERE rt.id = main_entity.foreignKey AND rt.columnName = :paramValue)
582+
existsQuery = `EXISTS (
583+
SELECT 1 FROM ${escapeTablePath(inverseTableName)} ${escapeId(relatedAlias)}
584+
WHERE ${escapeId(relatedAlias)}.${escapeId('id')} = ${escapeId(mainQb.alias)}.${escapeId(foreignKeyColumn)} AND ${whereCondition}
585+
)`;
556586
} else {
557587
// Not a *-to-Many relation, shouldn't happen but fall back gracefully
558588
return null;
@@ -672,7 +702,18 @@ export class ListQueryBuilder implements OnApplicationBootstrap {
672702
// to join the associated relations.
673703
continue;
674704
}
675-
const relationPath = path.split('.').slice(0, -1);
705+
const parts = path.split('.');
706+
const relationPath = parts.slice(0, -1);
707+
708+
// Optimization: If the custom property is a ManyToOne relation and is NOT being used for sorting,
709+
// we can skip the JOIN and let the filter be handled by an EXISTS subquery.
710+
if (relationPath.length === 1) {
711+
const relationMetadata = metadata.findRelationWithPropertyPath(relationPath[0]);
712+
if (relationMetadata?.isManyToOne && !(options.sort as any)?.[property]) {
713+
continue;
714+
}
715+
}
716+
676717
let targetMetadata = metadata;
677718
const reconstructedPath = [];
678719
for (const relationPathPart of relationPath) {
@@ -689,7 +730,7 @@ export class ListQueryBuilder implements OnApplicationBootstrap {
689730
}
690731

691732
private customPropertyIsBeingUsed(property: string, options: ListQueryOptions<any>): boolean {
692-
return !!(options.sort?.[property] || this.isPropertyUsedInFilter(property, options.filter));
733+
return !!((options.sort as any)?.[property] || this.isPropertyUsedInFilter(property, options.filter));
693734
}
694735

695736
private isPropertyUsedInFilter(
@@ -720,6 +761,19 @@ export class ListQueryBuilder implements OnApplicationBootstrap {
720761
continue;
721762
}
722763
let parts = customPropertyMap[property].split('.');
764+
765+
// Optimization: If the custom property is a ManyToOne relation and is NOT being used for sorting,
766+
// we can skip the JOIN and let the filter be handled by an EXISTS subquery.
767+
// This avoids performance issues when many custom fields are present.
768+
if (parts.length === 2) {
769+
const relationMetadata = qb.expressionMap.mainAlias?.metadata.findRelationWithPropertyPath(
770+
parts[0],
771+
);
772+
if (relationMetadata?.isManyToOne && !(options.sort as any)?.[property]) {
773+
continue;
774+
}
775+
}
776+
723777
const normalizedRelationPath: string[] = [];
724778
let entityMetadata = qb.expressionMap.mainAlias?.metadata;
725779
let entityAlias = qb.alias;

‎packages/core/src/service/helpers/list-query-builder/parse-filter-params.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,10 @@ function getToManyRelationCustomProperties<T extends VendureEntity>(
214214
const relationName = pathParts[0];
215215
const relationMetadata = metadata.findRelationWithPropertyPath(relationName);
216216

217-
if (relationMetadata && (relationMetadata.isOneToMany || relationMetadata.isManyToMany)) {
217+
if (
218+
relationMetadata &&
219+
(relationMetadata.isOneToMany || relationMetadata.isManyToMany || relationMetadata.isManyToOne)
220+
) {
218221
toManyProperties.add(property);
219222
}
220223
}

0 commit comments

Comments
 (0)