(graphcache) - Fix edge cases in operation ordering (#638)

* Fix edge cases in operation ordering

- Ensure that reserveLayer can be called repeatedly
- Apply optimistic updates later in the chain
- Add last-chance reserveLayer to every query/subscription

* Reproduce a failing edge case from master in cacheExchange.test.ts

* Add changeset

* Reproduce failing subscription updates from master in cacheExchange.test.ts

* Add idempotency tests to data.test.ts

.changeset/happy-keys-type.md

@@ -0,0 +1,5 @@
+---
+'@urql/exchange-graphcache': patch
+---
+
+Fix critical ordering bug in commutative queries and mutations. Subscriptions and queries would ad-hoc be receiving an empty optimistic layer accidentally. This leads to subscription results potentially being cleared, queries from being erased on a second write, and layers from sticking around on every second write or indefinitely. This affects versions `> 2.2.2` so please upgrade!

exchanges/graphcache/src/cacheExchange.test.ts

@@ -1206,7 +1206,9 @@ describe('commutativity', () => {
     expect(reexec).toHaveBeenCalledTimes(1);
     expect(data).toHaveProperty('node.name', 'optimistic');
 
-    nextOp(queryOpB);
+    // NOTE: We purposefully skip the following:
+    // nextOp(queryOpB);
+
     nextRes({
       operation: queryOpB,
       data: {
@@ -1502,4 +1504,125 @@ describe('commutativity', () => {
 
     expect(data).toHaveProperty('node.name', 'subscription');
   });
+
+  it('allows subscription results to be commutative above mutations', () => {
+    let data: any;
+    const client = createClient({ url: 'http://0.0.0.0' });
+    const { source: ops$, next: nextOp } = makeSubject<Operation>();
+    const { source: res$, next: nextRes } = makeSubject<OperationResult>();
+
+    jest.spyOn(client, 'reexecuteOperation').mockImplementation(nextOp);
+
+    const query = gql`
+      {
+        node {
+          id
+          name
+        }
+      }
+    `;
+
+    const subscription = gql`
+      subscription {
+        node {
+          id
+          name
+        }
+      }
+    `;
+
+    const mutation = gql`
+      mutation {
+        node {
+          id
+          name
+        }
+      }
+    `;
+
+    const forward = (ops$: Source<Operation>): Source<OperationResult> =>
+      merge([
+        pipe(
+          ops$,
+          filter(() => false)
+        ) as any,
+        res$,
+      ]);
+
+    pipe(
+      cacheExchange()({ forward, client })(ops$),
+      tap(result => {
+        if (result.operation.operationName === 'query') {
+          data = result.data;
+        }
+      }),
+      publish
+    );
+
+    const queryOpA = client.createRequestOperation('query', { key: 1, query });
+
+    const subscriptionOp = client.createRequestOperation('subscription', {
+      key: 2,
+      query: subscription,
+    });
+
+    const mutationOp = client.createRequestOperation('mutation', {
+      key: 3,
+      query: mutation,
+    });
+
+    nextOp(queryOpA);
+    // Force commutative layers to be created:
+    nextOp(client.createRequestOperation('query', { key: 2, query }));
+    nextOp(subscriptionOp);
+
+    nextRes({
+      operation: queryOpA,
+      data: {
+        __typename: 'Query',
+        node: {
+          __typename: 'Node',
+          id: 'node',
+          name: 'query a',
+        },
+      },
+    });
+
+    nextOp(mutationOp);
+
+    nextRes({
+      operation: mutationOp,
+      data: {
+        node: {
+          __typename: 'Node',
+          id: 'node',
+          name: 'mutation',
+        },
+      },
+    });
+
+    nextRes({
+      operation: subscriptionOp,
+      data: {
+        node: {
+          __typename: 'Node',
+          id: 'node',
+          name: 'subscription a',
+        },
+      },
+    });
+
+    nextRes({
+      operation: subscriptionOp,
+      data: {
+        node: {
+          __typename: 'Node',
+          id: 'node',
+          name: 'subscription b',
+        },
+      },
+    });
+
+    expect(data).toHaveProperty('node.name', 'subscription b');
+  });
 });

exchanges/graphcache/src/cacheExchange.ts

@@ -15,7 +15,6 @@ import {
   merge,
   pipe,
   share,
-  tap,
   fromPromise,
   fromArray,
   buffer,
@@ -140,6 +139,23 @@ export const cacheExchange = (opts?: CacheExchangeOpts): Exchange => ({
       reserveLayer(store.data, operation.key);
     } else if (operation.operationName === 'teardown') {
       noopDataState(store.data, operation.key);
+    } else if (operation.operationName === 'mutation') {
+      // This executes an optimistic update for mutations and registers it if necessary
+      if (operation.context.requestPolicy !== 'network-only') {
+        const { dependencies } = writeOptimistic(
+          store,
+          operation,
+          operation.key
+        );
+        if (dependencies.size !== 0) {
+          optimisticKeysToDependencies.set(operation.key, dependencies);
+          const pendingOperations = new Set<number>();
+          collectPendingOperations(pendingOperations, dependencies);
+          executePendingOperations(operation, pendingOperations);
+        }
+      } else {
+        reserveLayer(store.data, operation.key);
+      }
     }
 
     return {
@@ -154,25 +170,6 @@ export const cacheExchange = (opts?: CacheExchangeOpts): Exchange => ({
     };
   };
 
-  // This executes an optimistic update for mutations and registers it if necessary
-  const optimisticUpdate = (operation: Operation) => {
-    const { key } = operation;
-    if (
-      operation.operationName === 'mutation' &&
-      operation.context.requestPolicy !== 'network-only'
-    ) {
-      const { dependencies } = writeOptimistic(store, operation, key);
-      if (dependencies.size !== 0) {
-        optimisticKeysToDependencies.set(key, dependencies);
-        const pendingOperations = new Set<number>();
-        collectPendingOperations(pendingOperations, dependencies);
-        executePendingOperations(operation, pendingOperations);
-      }
-    } else {
-      noopDataState(store.data, key, true);
-    }
-  };
-
   // This updates the known dependencies for the passed operation
   const updateDependencies = (op: Operation, dependencies: Set<string>) => {
     dependencies.forEach(dep => {
@@ -227,8 +224,7 @@ export const cacheExchange = (opts?: CacheExchangeOpts): Exchange => ({
         optimisticKeysToDependencies.get(key)
       );
       optimisticKeysToDependencies.delete(key);
-    } else if (operation.operationName === 'subscription') {
-      // If we're writing a subscription, we ad-hoc reserve a layer
+    } else {
       reserveLayer(store.data, operation.key);
     }
 
@@ -275,16 +271,7 @@ export const cacheExchange = (opts?: CacheExchangeOpts): Exchange => ({
         )
       : (empty as Source<Operation>);
 
-    const inputOps$ = pipe(
-      concat([bufferedOps$, sharedOps$]),
-      // Returns the given operation with added __typename fields on its query
-      map(op => ({
-        ...op,
-        query: formatDocument(op.query),
-      })),
-      tap(optimisticUpdate),
-      share
-    );
+    const inputOps$ = pipe(concat([bufferedOps$, sharedOps$]), share);
 
     // Filter by operations that are cacheable and attempt to query them from the cache
     const cacheOps$ = pipe(

exchanges/graphcache/src/store/data.test.ts

@@ -423,4 +423,21 @@ describe('commutative changes', () => {
       },
     ]);
   });
+
+  it('allows reserveLayer to be called repeatedly', () => {
+    InMemoryData.reserveLayer(data, 1);
+    InMemoryData.reserveLayer(data, 1);
+    expect(data.optimisticOrder).toEqual([1]);
+    expect([...data.commutativeKeys]).toEqual([1]);
+  });
+
+  it('allows reserveLayer to be called after registering an optimistc layer', () => {
+    InMemoryData.noopDataState(data, 1, true);
+    expect(data.optimisticOrder).toEqual([1]);
+    expect(data.commutativeKeys.size).toBe(0);
+
+    InMemoryData.reserveLayer(data, 1);
+    expect(data.optimisticOrder).toEqual([1]);
+    expect([...data.commutativeKeys]).toEqual([1]);
+  });
 });

exchanges/graphcache/src/store/data.ts

@@ -473,12 +473,13 @@ export const writeLink = (
 
 /** Reserves an optimistic layer and preorders it */
 export const reserveLayer = (data: InMemoryData, layerKey: number) => {
-  if (!data.commutativeKeys.has(layerKey)) {
+  if (data.optimisticOrder.indexOf(layerKey) === -1) {
     // The new layer needs to be reserved in front of all other commutative
     // keys but after all non-commutative keys (which are added by `forceUpdate`)
     data.optimisticOrder.unshift(layerKey);
-    data.commutativeKeys.add(layerKey);
   }
+
+  data.commutativeKeys.add(layerKey);
 };
 
 /** Creates an optimistic layer of links and records */