Skip to content

Commit ba2ac94

Browse files
RubenDarioGuerreroNeiraRubenDarioGuerreroNeira
committed
fix(core): Optimize collection variants N+1 and fix stability issues
1 parent b6133a4 commit ba2ac94

14 files changed

Lines changed: 232 additions & 37 deletions

File tree

‎packages/core/src/api/middleware/auth-guard.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { Reflector } from '@nestjs/core';
33
import { Permission } from '@vendure/common/lib/generated-types';
44
import { Request, Response } from 'express';
55
import { GraphQLResolveInfo } from 'graphql';
6-
import ms, { type StringValue } from 'ms';
6+
import ms from 'ms';
77

88
import { ForbiddenError } from '../../common/error/errors';
99
import { API_KEY_AUTH_STRATEGY_NAME } from '../../config';
@@ -212,7 +212,7 @@ export class AuthGuard implements CanActivate {
212212
const lastUsedThreshold = new Date(
213213
Date.now() -
214214
(typeof strategy.lastUsedAtUpdateInterval === 'string'
215-
? ms(strategy.lastUsedAtUpdateInterval as StringValue)
215+
? ms(strategy.lastUsedAtUpdateInterval)
216216
: strategy.lastUsedAtUpdateInterval),
217217
);
218218
if (!apiKey.lastUsedAt || apiKey.lastUsedAt < lastUsedThreshold) {

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

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import { PaginatedList } from '@vendure/common/lib/shared-types';
1818
import { GraphQLResolveInfo } from 'graphql';
1919

2020
import { RequestContextCacheService } from '../../../cache/request-context-cache.service';
21-
import { CacheKey } from '../../../common/constants';
21+
import { CacheKey, COLLECTION_VARIANTS_CACHE_RELATIONS } from '../../../common/constants';
2222
import { UserInputError } from '../../../common/error/errors';
2323
import { Translated } from '../../../common/types/locale-types';
2424
import { CollectionFilter } from '../../../config/catalog/collection-filter';
@@ -66,11 +66,19 @@ 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, 'productVariants')) {
75+
const variantsPromise = this.collectionService.getProductVariantsForCollections(
76+
ctx,
77+
collectionIds,
78+
[...COLLECTION_VARIANTS_CACHE_RELATIONS],
79+
);
80+
this.requestContextCache.set(ctx, CacheKey.CollectionVariants, variantsPromise);
81+
}
7482
return collections;
7583
}
7684

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

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,10 @@ import {
88
import { ID, PaginatedList } from '@vendure/common/lib/shared-types';
99

1010
import { RequestContextCacheService } from '../../../cache/request-context-cache.service';
11-
import { CacheKey } from '../../../common/constants';
12-
import { ListQueryOptions } from '../../../common/types/common-types';
11+
import { CacheKey, COLLECTION_VARIANTS_CACHE_RELATIONS } from '../../../common/constants';
1312
import { Translated } from '../../../common/types/locale-types';
1413
import { CollectionFilter } from '../../../config/catalog/collection-filter';
14+
import { ConfigService } from '../../../config/config.service';
1515
import { Asset, Collection, Product, ProductVariant } from '../../../entity';
1616
import { LocaleStringHydrator } from '../../../service/helpers/locale-string-hydrator/locale-string-hydrator';
1717
import { AssetService } from '../../../service/services/asset.service';
@@ -33,6 +33,7 @@ export class CollectionEntityResolver {
3333
private localeStringHydrator: LocaleStringHydrator,
3434
private configurableOperationCodec: ConfigurableOperationCodec,
3535
private requestContextCache: RequestContextCacheService,
36+
private configService: ConfigService,
3637
) {}
3738

3839
@ResolveField()
@@ -63,6 +64,40 @@ export class CollectionEntityResolver {
6364
@Api() apiType: ApiType,
6465
@Relations({ entity: ProductVariant, omit: ['assets'] }) relations: RelationPaths<ProductVariant>,
6566
): Promise<PaginatedList<Translated<ProductVariant>>> {
67+
const isDefaultOptions = !args.options || Object.keys(args.options).length === 0;
68+
if (isDefaultOptions && apiType === 'admin') {
69+
const cachedVariantsPromise = this.requestContextCache.get<
70+
Promise<Map<string, ProductVariant[]>>
71+
>(ctx, CacheKey.CollectionVariants);
72+
if (cachedVariantsPromise) {
73+
const variantsMap = await cachedVariantsPromise;
74+
const variants = variantsMap.get(String(collection.id));
75+
if (variants) {
76+
// Check if the requested relations are compatible with the cached data.
77+
// The cache was populated with default relations defined by COLLECTION_VARIANTS_CACHE_RELATIONS.
78+
// We can use the cache ONLY if the requested relations are a subset of or equal to the default relations.
79+
const isCacheCompatible = relations.every(rel =>
80+
(COLLECTION_VARIANTS_CACHE_RELATIONS as readonly string[]).includes(rel),
81+
);
82+
83+
if (isCacheCompatible) {
84+
// Cache is compatible, use it.
85+
const { adminListQueryLimit } = this.configService.apiOptions;
86+
const skip = args.options?.skip ?? 0;
87+
const take = args.options?.take ?? adminListQueryLimit;
88+
const items = await this.productVariantService.applyPricesAndTranslateVariants(
89+
ctx,
90+
variants.slice(skip, skip + take),
91+
);
92+
return {
93+
items,
94+
totalItems: variants.length,
95+
};
96+
}
97+
}
98+
}
99+
}
100+
66101
let options: ListQueryOptions<Product> = args.options;
67102
if (apiType === 'shop') {
68103
options = {

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,4 +85,10 @@ 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
};
90+
91+
/**
92+
* The default relations used when pre-caching product variants for collections.
93+
*/
94+
export const COLLECTION_VARIANTS_CACHE_RELATIONS = ['taxCategory'] as const;

‎packages/core/src/config/auth/default-verification-token-strategy.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import ms, { type StringValue } from 'ms';
1+
import ms from 'ms';
22

33
import { RequestContext } from '../../api/common/request-context';
44
import { Injector } from '../../common';
@@ -43,7 +43,7 @@ export class DefaultVerificationTokenStrategy implements VerificationTokenStrate
4343
const { verificationTokenDuration } = this.configService.authOptions;
4444
const verificationTokenDurationInMs =
4545
typeof verificationTokenDuration === 'string'
46-
? ms(verificationTokenDuration as StringValue)
46+
? ms(verificationTokenDuration)
4747
: verificationTokenDuration;
4848

4949
const [generatedOn] = token.split('_');

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

Lines changed: 15 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';
@@ -7,6 +7,7 @@ import { InjectableStrategy } from '../common/types/injectable-strategy';
77

88
import { resetConfig } from './config-helpers';
99
import { ConfigService } from './config.service';
10+
import { Logger } from './logger/vendure-logger';
1011

1112
@Module({
1213
providers: [ConfigService],
@@ -15,7 +16,7 @@ import { ConfigService } from './config.service';
1516
export class ConfigModule implements OnApplicationBootstrap, OnApplicationShutdown {
1617
constructor(
1718
private configService: ConfigService,
18-
private moduleRef: ModuleRef,
19+
@Optional() private moduleRef: ModuleRef | undefined,
1920
) {}
2021

2122
async onApplicationBootstrap() {
@@ -37,6 +38,12 @@ export class ConfigModule implements OnApplicationBootstrap, OnApplicationShutdo
3738
}
3839

3940
private async initInjectableStrategies() {
41+
if (!this.moduleRef) {
42+
Logger.warn(
43+
'ConfigModule: moduleRef missing — skipping initialization of injectable strategies.',
44+
);
45+
return;
46+
}
4047
const injector = new Injector(this.moduleRef);
4148
for (const strategy of this.getInjectableStrategies()) {
4249
if (typeof strategy.init === 'function') {
@@ -54,6 +61,12 @@ export class ConfigModule implements OnApplicationBootstrap, OnApplicationShutdo
5461
}
5562

5663
private async initConfigurableOperations() {
64+
if (!this.moduleRef) {
65+
Logger.warn(
66+
'ConfigModule: moduleRef missing — skipping initialization of configurable operations.',
67+
);
68+
return;
69+
}
5770
const injector = new Injector(this.moduleRef);
5871
for (const operation of this.getConfigurableOperations()) {
5972
await operation.init(injector);

‎packages/core/src/config/order/order-by-code-access-strategy.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import ms, { type StringValue } from 'ms';
1+
import ms from 'ms';
22

33
import { RequestContext } from '../../api/common/request-context';
44
import { InjectableStrategy } from '../../common/types/injectable-strategy';
@@ -68,7 +68,7 @@ export class DefaultOrderByCodeAccessStrategy implements OrderByCodeAccessStrate
6868
// For guest Customers, allow access to the Order for the following
6969
// time period
7070
const anonymousAccessPermitted = () => {
71-
const anonymousAccessLimit = ms(this.anonymousAccessDuration as StringValue);
71+
const anonymousAccessLimit = ms(this.anonymousAccessDuration);
7272
const orderPlaced = order.orderPlacedAt ? +order.orderPlacedAt : 0;
7373
const now = Date.now();
7474
return now - orderPlaced < anonymousAccessLimit;

‎packages/core/src/plugin/default-scheduler-plugin/default-scheduler-strategy.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { UpdateScheduledTaskInput } from '@vendure/common/lib/generated-types';
22
import { Cron } from 'croner';
3-
import ms, { type StringValue } from 'ms';
3+
import ms from 'ms';
44

55
import { Injector } from '../../common';
66
import { assertFound } from '../../common/utils';
@@ -85,7 +85,7 @@ export class DefaultSchedulerStrategy implements SchedulerStrategy {
8585
try {
8686
this.runningTasks.push(task);
8787
const timeout = task.options.timeout ?? (this.pluginOptions.defaultTimeout as number);
88-
const timeoutMs = typeof timeout === 'number' ? timeout : ms(timeout as StringValue);
88+
const timeoutMs = typeof timeout === 'number' ? timeout : ms(timeout);
8989

9090
let timeoutTimer: NodeJS.Timeout | undefined;
9191
const timeoutPromise = new Promise((_, reject) => {

‎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+
throw new Error(
406+
`Could not build EXISTS subquery for custom property "${condition.isExistsCondition.customPropertyKey}". This filter condition cannot be applied.`,
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)