Skip to content

Commit 3be449c

Browse files
fix(core): Address CodeRabbit review feedback for collection optimizations and documentation
1 parent 264c8bd commit 3be449c

6 files changed

Lines changed: 210 additions & 1 deletion

File tree

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
{
2+
"mcpServers": {
3+
"fetch": {
4+
"command": "https://raw.githubusercontent.com/modelcontextprotocol/servers/main/src/fetch/index.ts",
5+
"args": []
6+
},
7+
"sqlite": {
8+
"command": "https://raw.githubusercontent.com/modelcontextprotocol/servers/main/src/sqlite/index.ts",
9+
"args": []
10+
},
11+
"elasticsearch": {
12+
"command": "https://raw.githubusercontent.com/modelcontextprotocol/servers/main/src/elasticsearch/index.ts",
13+
"args": []
14+
},
15+
"puppeteer": {
16+
"command": "npx",
17+
"args": [
18+
"-y",
19+
"@modelcontextprotocol/server-puppeteer"
20+
]
21+
},
22+
"docker": {
23+
"command": "npx",
24+
"args": [
25+
"-y",
26+
"@modelcontextprotocol/server-docker"
27+
]
28+
}
29+
}
30+
}

.gitignore

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111
.env
1212
lerna-debug.log
1313
dist
14+
lib/
15+
package/
1416
e2e/__data__/*
1517
docs/resources/_gen/*
1618
docs/static/main.js*
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: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ export class CollectionResolver {
7575
const variantsPromise = this.collectionService.getProductVariantsForCollections(
7676
ctx,
7777
collectionIds,
78+
undefined,
7879
[...COLLECTION_VARIANTS_CACHE_RELATIONS],
7980
);
8081
this.requestContextCache.set(ctx, CacheKey.CollectionVariants, variantsPromise);

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { ID, PaginatedList } from '@vendure/common/lib/shared-types';
99

1010
import { RequestContextCacheService } from '../../../cache/request-context-cache.service';
1111
import { CacheKey, COLLECTION_VARIANTS_CACHE_RELATIONS } from '../../../common/constants';
12+
import { ListQueryOptions } from '../../../common/types/common-types';
1213
import { Translated } from '../../../common/types/locale-types';
1314
import { CollectionFilter } from '../../../config/catalog/collection-filter';
1415
import { ConfigService } from '../../../config/config.service';

packages/core/src/service/services/collection.service.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1076,7 +1076,7 @@ export class CollectionService implements OnModuleInit {
10761076

10771077
// We explicitly join with the product to ensure we filter out soft-deleted products,
10781078
// matching the behavior of other collection-related variant queries.
1079-
qb.leftJoin('productvariant.product', 'product')
1079+
qb.innerJoin('productvariant.product', 'product')
10801080
.innerJoin('productvariant.collections', 'collection', 'collection.id IN (:...collectionIds)', {
10811081
collectionIds,
10821082
})

0 commit comments

Comments
 (0)