fix(core): Fix race condition when moving Collections

Race condition caused by async job queue interaction overwriting the position of the Collection.

packages/core/e2e/collection.e2e-spec.ts

@@ -10,7 +10,7 @@ import gql from 'graphql-tag';
 import path from 'path';
 
 import { initialData } from '../../../e2e-common/e2e-initial-data';
-import { TEST_SETUP_TIMEOUT_MS, testConfig } from '../../../e2e-common/test-config';
+import { testConfig, TEST_SETUP_TIMEOUT_MS } from '../../../e2e-common/test-config';
 import { pick } from '../../common/lib/pick';
 
 import { COLLECTION_FRAGMENT, FACET_VALUE_FRAGMENT } from './graphql/fragments';
@@ -231,7 +231,7 @@ describe('Collection resolver', () => {
                 },
             });
 
-            expect(updateCollection.assets.map((a) => a.id)).toEqual([assets[3].id, assets[0].id]);
+            expect(updateCollection.assets.map(a => a.id)).toEqual([assets[3].id, assets[0].id]);
         });
 
         it('removes all assets', async () => {
@@ -349,7 +349,7 @@ describe('Collection resolver', () => {
             expect(result.moveCollection.parent!.id).toBe(electronicsCollection.id);
 
             const positions = await getChildrenOf(electronicsCollection.id);
-            expect(positions.map((i) => i.id)).toEqual([pearCollection.id, computersCollection.id]);
+            expect(positions.map(i => i.id)).toEqual([pearCollection.id, computersCollection.id]);
         });
 
         it('re-evaluates Collection contents on move', async () => {
@@ -359,7 +359,7 @@ describe('Collection resolver', () => {
                 GetCollectionProducts.Query,
                 GetCollectionProducts.Variables
             >(GET_COLLECTION_PRODUCT_VARIANTS, { id: pearCollection.id });
-            expect(result.collection!.productVariants.items.map((i) => i.name)).toEqual([
+            expect(result.collection!.productVariants.items.map(i => i.name)).toEqual([
                 'Laptop 13 inch 8GB',
                 'Laptop 15 inch 8GB',
                 'Laptop 13 inch 16GB',
@@ -378,7 +378,7 @@ describe('Collection resolver', () => {
             });
 
             const afterResult = await getChildrenOf(electronicsCollection.id);
-            expect(afterResult.map((i) => i.id)).toEqual([computersCollection.id, pearCollection.id]);
+            expect(afterResult.map(i => i.id)).toEqual([computersCollection.id, pearCollection.id]);
         });
 
         it('alters the position in the current parent 2', async () => {
@@ -391,7 +391,7 @@ describe('Collection resolver', () => {
             });
 
             const afterResult = await getChildrenOf(electronicsCollection.id);
-            expect(afterResult.map((i) => i.id)).toEqual([pearCollection.id, computersCollection.id]);
+            expect(afterResult.map(i => i.id)).toEqual([pearCollection.id, computersCollection.id]);
         });
 
         it('corrects an out-of-bounds negative index value', async () => {
@@ -404,7 +404,7 @@ describe('Collection resolver', () => {
             });
 
             const afterResult = await getChildrenOf(electronicsCollection.id);
-            expect(afterResult.map((i) => i.id)).toEqual([pearCollection.id, computersCollection.id]);
+            expect(afterResult.map(i => i.id)).toEqual([pearCollection.id, computersCollection.id]);
         });
 
         it('corrects an out-of-bounds positive index value', async () => {
@@ -417,7 +417,7 @@ describe('Collection resolver', () => {
             });
 
             const afterResult = await getChildrenOf(electronicsCollection.id);
-            expect(afterResult.map((i) => i.id)).toEqual([computersCollection.id, pearCollection.id]);
+            expect(afterResult.map(i => i.id)).toEqual([computersCollection.id, pearCollection.id]);
         });
 
         it(
@@ -452,7 +452,7 @@ describe('Collection resolver', () => {
 
         async function getChildrenOf(parentId: string): Promise<Array<{ name: string; id: string }>> {
             const result = await adminClient.query<GetCollections.Query>(GET_COLLECTIONS);
-            return result.collections.items.filter((i) => i.parent!.id === parentId);
+            return result.collections.items.filter(i => i.parent!.id === parentId);
         }
     });
 
@@ -622,7 +622,7 @@ describe('Collection resolver', () => {
                 >(GET_COLLECTION_PRODUCT_VARIANTS, {
                     id: electronicsCollection.id,
                 });
-                expect(result.collection!.productVariants.items.map((i) => i.name)).toEqual([
+                expect(result.collection!.productVariants.items.map(i => i.name)).toEqual([
                     'Laptop 13 inch 8GB',
                     'Laptop 15 inch 8GB',
                     'Laptop 13 inch 16GB',
@@ -654,7 +654,7 @@ describe('Collection resolver', () => {
                 >(GET_COLLECTION_PRODUCT_VARIANTS, {
                     id: computersCollection.id,
                 });
-                expect(result.collection!.productVariants.items.map((i) => i.name)).toEqual([
+                expect(result.collection!.productVariants.items.map(i => i.name)).toEqual([
                     'Laptop 13 inch 8GB',
                     'Laptop 15 inch 8GB',
                     'Laptop 13 inch 16GB',
@@ -714,7 +714,7 @@ describe('Collection resolver', () => {
                     },
                 );
 
-                expect(collection!.productVariants.items.map((i) => i.name)).toEqual(['Instant Camera']);
+                expect(collection!.productVariants.items.map(i => i.name)).toEqual(['Instant Camera']);
             });
 
             it('photo OR pear', async () => {
@@ -756,7 +756,7 @@ describe('Collection resolver', () => {
                     },
                 );
 
-                expect(collection!.productVariants.items.map((i) => i.name)).toEqual([
+                expect(collection!.productVariants.items.map(i => i.name)).toEqual([
                     'Laptop 13 inch 8GB',
                     'Laptop 15 inch 8GB',
                     'Laptop 13 inch 16GB',
@@ -811,7 +811,7 @@ describe('Collection resolver', () => {
                     },
                 );
 
-                expect(collection!.productVariants.items.map((i) => i.name)).toEqual([
+                expect(collection!.productVariants.items.map(i => i.name)).toEqual([
                     'Laptop 13 inch 8GB',
                     'Laptop 15 inch 8GB',
                     'Laptop 13 inch 16GB',
@@ -867,7 +867,7 @@ describe('Collection resolver', () => {
                 >(GET_COLLECTION_PRODUCT_VARIANTS, {
                     id: collection.id,
                 });
-                expect(result.collection!.productVariants.items.map((i) => i.name)).toEqual([
+                expect(result.collection!.productVariants.items.map(i => i.name)).toEqual([
                     'Instant Camera',
                     'Camera Lens',
                     'SLR Camera',
@@ -883,7 +883,7 @@ describe('Collection resolver', () => {
                 >(GET_COLLECTION_PRODUCT_VARIANTS, {
                     id: collection.id,
                 });
-                expect(result.collection!.productVariants.items.map((i) => i.name)).toEqual(['Camera Lens']);
+                expect(result.collection!.productVariants.items.map(i => i.name)).toEqual(['Camera Lens']);
             });
 
             it('endsWith operator', async () => {
@@ -895,7 +895,7 @@ describe('Collection resolver', () => {
                 >(GET_COLLECTION_PRODUCT_VARIANTS, {
                     id: collection.id,
                 });
-                expect(result.collection!.productVariants.items.map((i) => i.name)).toEqual([
+                expect(result.collection!.productVariants.items.map(i => i.name)).toEqual([
                     'Instant Camera',
                     'SLR Camera',
                 ]);
@@ -910,7 +910,7 @@ describe('Collection resolver', () => {
                 >(GET_COLLECTION_PRODUCT_VARIANTS, {
                     id: collection.id,
                 });
-                expect(result.collection!.productVariants.items.map((i) => i.name)).toEqual([
+                expect(result.collection!.productVariants.items.map(i => i.name)).toEqual([
                     'Laptop 13 inch 8GB',
                     'Laptop 15 inch 8GB',
                     'Laptop 13 inch 16GB',
@@ -973,7 +973,7 @@ describe('Collection resolver', () => {
                     GetCollectionProducts.Query,
                     GetCollectionProducts.Variables
                 >(GET_COLLECTION_PRODUCT_VARIANTS, { id: pearCollection.id });
-                expect(result.collection!.productVariants.items.map((i) => i.name)).toEqual([
+                expect(result.collection!.productVariants.items.map(i => i.name)).toEqual([
                     'Laptop 13 inch 8GB',
                     'Laptop 15 inch 8GB',
                     'Laptop 13 inch 16GB',
@@ -986,8 +986,8 @@ describe('Collection resolver', () => {
 
             it('updates contents when ProductVariant is updated', async () => {
                 const gamingPc240GB = products
-                    .find((p) => p.name === 'Gaming PC')!
-                    .variants.find((v) => v.name.includes('240GB'))!;
+                    .find(p => p.name === 'Gaming PC')!
+                    .variants.find(v => v.name.includes('240GB'))!;
                 await adminClient.query<UpdateProductVariants.Mutation, UpdateProductVariants.Variables>(
                     UPDATE_PRODUCT_VARIANTS,
                     {
@@ -1006,7 +1006,7 @@ describe('Collection resolver', () => {
                     GetCollectionProducts.Query,
                     GetCollectionProducts.Variables
                 >(GET_COLLECTION_PRODUCT_VARIANTS, { id: pearCollection.id });
-                expect(result.collection!.productVariants.items.map((i) => i.name)).toEqual([
+                expect(result.collection!.productVariants.items.map(i => i.name)).toEqual([
                     'Laptop 13 inch 8GB',
                     'Laptop 15 inch 8GB',
                     'Laptop 13 inch 16GB',
@@ -1020,8 +1020,8 @@ describe('Collection resolver', () => {
 
             it('correctly filters when ProductVariant and Product both have matching FacetValue', async () => {
                 const gamingPc240GB = products
-                    .find((p) => p.name === 'Gaming PC')!
-                    .variants.find((v) => v.name.includes('240GB'))!;
+                    .find(p => p.name === 'Gaming PC')!
+                    .variants.find(v => v.name.includes('240GB'))!;
                 await adminClient.query<UpdateProductVariants.Mutation, UpdateProductVariants.Variables>(
                     UPDATE_PRODUCT_VARIANTS,
                     {
@@ -1040,7 +1040,7 @@ describe('Collection resolver', () => {
                     GetCollectionProducts.Query,
                     GetCollectionProducts.Variables
                 >(GET_COLLECTION_PRODUCT_VARIANTS, { id: pearCollection.id });
-                expect(result.collection!.productVariants.items.map((i) => i.name)).toEqual([
+                expect(result.collection!.productVariants.items.map(i => i.name)).toEqual([
                     'Laptop 13 inch 8GB',
                     'Laptop 15 inch 8GB',
                     'Laptop 13 inch 16GB',
@@ -1090,7 +1090,7 @@ describe('Collection resolver', () => {
                 GetCollectionProducts.Query,
                 GetCollectionProducts.Variables
             >(GET_COLLECTION_PRODUCT_VARIANTS, { id: pearElectronics.id });
-            expect(result.collection!.productVariants.items.map((i) => i.name)).toEqual([
+            expect(result.collection!.productVariants.items.map(i => i.name)).toEqual([
                 'Laptop 13 inch 8GB',
                 'Laptop 15 inch 8GB',
                 'Laptop 13 inch 16GB',
@@ -1132,7 +1132,7 @@ describe('Collection resolver', () => {
         >(GET_COLLECTION_PRODUCT_VARIANTS, {
             id: pearCollection.id,
         });
-        expect(collection!.productVariants.items.map((i) => i.name)).toEqual([
+        expect(collection!.productVariants.items.map(i => i.name)).toEqual([
             'Laptop 13 inch 8GB',
             'Laptop 15 inch 8GB',
             'Laptop 13 inch 16GB',
@@ -1143,7 +1143,7 @@ describe('Collection resolver', () => {
     });
 
     function getFacetValueId(code: string): string {
-        const match = facetValues.find((fv) => fv.code === code);
+        const match = facetValues.find(fv => fv.code === code);
         if (!match) {
             throw new Error(`Could not find a FacetValue with the code "${code}"`);
         }

packages/core/e2e/graphql/generated-e2e-admin-types.ts

@@ -3572,15 +3572,6 @@ export type CreateAssetsMutation = { __typename?: 'Mutation' } & {
     >;
 };
 
-export type DeleteAssetMutationVariables = {
-    id: Scalars['ID'];
-    force?: Maybe<Scalars['Boolean']>;
-};
-
-export type DeleteAssetMutation = { __typename?: 'Mutation' } & {
-    deleteAsset: { __typename?: 'DeletionResponse' } & Pick<DeletionResponse, 'result' | 'message'>;
-};
-
 export type CanCreateCustomerMutationVariables = {
     input: CreateCustomerInput;
 };
@@ -3685,7 +3676,9 @@ export type GetFacetValuesQuery = { __typename?: 'Query' } & {
     };
 };
 
-export type GetCollectionsQueryVariables = {};
+export type GetCollectionsQueryVariables = {
+    options?: Maybe<CollectionListOptions>;
+};
 
 export type GetCollectionsQuery = { __typename?: 'Query' } & {
     collections: { __typename?: 'CollectionList' } & {
@@ -4701,6 +4694,15 @@ export type UpdateAssetMutation = { __typename?: 'Mutation' } & {
     } & AssetFragment;
 };
 
+export type DeleteAssetMutationVariables = {
+    id: Scalars['ID'];
+    force?: Maybe<Scalars['Boolean']>;
+};
+
+export type DeleteAssetMutation = { __typename?: 'Mutation' } & {
+    deleteAsset: { __typename?: 'DeletionResponse' } & Pick<DeletionResponse, 'result' | 'message'>;
+};
+
 export type UpdateOptionGroupMutationVariables = {
     input: UpdateProductOptionGroupInput;
 };
@@ -5474,12 +5476,6 @@ export namespace CreateAssets {
     export type FocalPoint = NonNullable<NonNullable<CreateAssetsMutation['createAssets'][0]>['focalPoint']>;
 }
 
-export namespace DeleteAsset {
-    export type Variables = DeleteAssetMutationVariables;
-    export type Mutation = DeleteAssetMutation;
-    export type DeleteAsset = DeleteAssetMutation['deleteAsset'];
-}
-
 export namespace CanCreateCustomer {
     export type Variables = CanCreateCustomerMutationVariables;
     export type Mutation = CanCreateCustomerMutation;
@@ -6223,6 +6219,12 @@ export namespace UpdateAsset {
     export type FocalPoint = NonNullable<UpdateAssetMutation['updateAsset']['focalPoint']>;
 }
 
+export namespace DeleteAsset {
+    export type Variables = DeleteAssetMutationVariables;
+    export type Mutation = DeleteAssetMutation;
+    export type DeleteAsset = DeleteAssetMutation['deleteAsset'];
+}
+
 export namespace UpdateOptionGroup {
     export type Variables = UpdateOptionGroupMutationVariables;
     export type Mutation = UpdateOptionGroupMutation;

packages/core/src/service/controllers/collection.controller.ts

@@ -2,6 +2,7 @@ import { Controller } from '@nestjs/common';
 import { MessagePattern } from '@nestjs/microservices';
 import { InjectConnection } from '@nestjs/typeorm';
 import { ConfigurableOperation } from '@vendure/common/lib/generated-types';
+import { pick } from '@vendure/common/lib/pick';
 import { ID } from '@vendure/common/lib/shared-types';
 import { Observable } from 'rxjs';
 import { Connection } from 'typeorm';
@@ -32,7 +33,7 @@ export class CollectionController {
     applyCollectionFilters({
         collectionIds,
     }: ApplyCollectionFiltersMessage['data']): Observable<ApplyCollectionFiltersMessage['response']> {
-        return asyncObservable(async (observer) => {
+        return asyncObservable(async observer => {
             Logger.verbose(`Processing ${collectionIds.length} Collections`);
             const timeStart = Date.now();
             const collections = await this.connection.getRepository(Collection).findByIds(collectionIds, {
@@ -59,7 +60,7 @@ export class CollectionController {
     private async applyCollectionFiltersInternal(collection: Collection): Promise<ID[]> {
         const ancestorFilters = await this.collectionService
             .getAncestors(collection.id)
-            .then((ancestors) =>
+            .then(ancestors =>
                 ancestors.reduce(
                     (filters, c) => [...filters, ...(c.filters || [])],
                     [] as ConfigurableOperation[],
@@ -70,11 +71,18 @@ export class CollectionController {
             ...ancestorFilters,
             ...(collection.filters || []),
         ]);
-        const postIds = collection.productVariants.map((v) => v.id);
+        const postIds = collection.productVariants.map(v => v.id);
         try {
             await this.connection
                 .getRepository(Collection)
-                .save(collection, { chunk: Math.ceil(collection.productVariants.length / 500) });
+                // Only update the exact changed properties, to avoid VERY hard-to-debug
+                // non-deterministic race conditions e.g. when the "position" is changed
+                // by moving a Collection and then this save operation clobbers it back
+                // to the old value.
+                .save(pick(collection, ['id', 'productVariants']), {
+                    chunk: Math.ceil(collection.productVariants.length / 500),
+                    reload: false,
+                });
         } catch (e) {
             Logger.error(e);
         }
@@ -82,8 +90,8 @@ export class CollectionController {
         const preIdsSet = new Set(preIds);
         const postIdsSet = new Set(postIds);
         const difference = [
-            ...preIds.filter((id) => !postIdsSet.has(id)),
-            ...postIds.filter((id) => !preIdsSet.has(id)),
+            ...preIds.filter(id => !postIdsSet.has(id)),
+            ...postIds.filter(id => !preIdsSet.has(id)),
         ];
         return difference;
     }
@@ -95,8 +103,8 @@ export class CollectionController {
         if (filters.length === 0) {
             return [];
         }
-        const facetFilters = filters.filter((f) => f.code === facetValueCollectionFilter.code);
-        const variantNameFilters = filters.filter((f) => f.code === variantNameCollectionFilter.code);
+        const facetFilters = filters.filter(f => f.code === facetValueCollectionFilter.code);
+        const variantNameFilters = filters.filter(f => f.code === variantNameCollectionFilter.code);
         let qb = this.connection.getRepository(ProductVariant).createQueryBuilder('productVariant');
 
         // Apply any facetValue-based filters