From 55ce1373998358d6425a18705ebe17e9e031519d Mon Sep 17 00:00:00 2001 From: Dmitry Minkovsky Date: Mon, 4 Feb 2019 09:21:01 -0800 Subject: [PATCH] RelayPublishQueue: applied selector store updates are not applied correctly during rebase (#2489) Summary: Fixes an issue with sequential optimistic mutation payloads (not updaters) where a field update from an earlier payload could be overwritten with the *original* value if the same record happened to be updated in a subsequent payload. This happened because the helper `copyFieldsFromRecord` was copying *all* fields from the base version of the record, rather than only updating new fields from the input record. The PR added new test cases here, the fix on top of that is to update copyFieldsFromRecord to only create a backup record but not re-copy fields from that record to the sink. -- original pr -- Added some tests that show expected/unexpected behavior I encountered. Suppose the publish queue has two applied optimistic updates that touch different fields on the same record. If I `RelayEnvironment#commitPayload()`, a backup rebase is executed to commit the payload. During this backup rebase, the second applied optimistic update can wipe the first applied optimistic update. Pull Request resolved: https://github.com/facebook/relay/pull/2489 Reviewed By: kassens Differential Revision: D13914926 Pulled By: josephsavona fbshipit-source-id: 8bbab97a9117af6387e0e60f7b0dbd2454621d94 --- .../mutations/RelayRecordSourceMutator.js | 2 +- .../mutations/RelayRecordSourceProxy.js | 1 - .../RelayRecordSourceMutator-test.js | 1 - .../store/__tests__/RelayPublishQueue-test.js | 109 +++++++++++++++++- 4 files changed, 109 insertions(+), 4 deletions(-) diff --git a/packages/relay-runtime/mutations/RelayRecordSourceMutator.js b/packages/relay-runtime/mutations/RelayRecordSourceMutator.js index 2e81fc72133ab..12bfef3d8cc02 100644 --- a/packages/relay-runtime/mutations/RelayRecordSourceMutator.js +++ b/packages/relay-runtime/mutations/RelayRecordSourceMutator.js @@ -148,7 +148,7 @@ class RelayRecordSourceMutator { } copyFieldsFromRecord(record: Record, sinkID: DataID): void { - this.copyFields(RelayModernRecord.getDataID(record), sinkID); + this._createBackupRecord(sinkID); const sink = this._getSinkRecord(sinkID); RelayModernRecord.copyFields(record, sink); this._setSentinelFieldsInBackupRecord(sinkID, sink); diff --git a/packages/relay-runtime/mutations/RelayRecordSourceProxy.js b/packages/relay-runtime/mutations/RelayRecordSourceProxy.js index 676d1d9a84d07..c8cd9c6b079f0 100644 --- a/packages/relay-runtime/mutations/RelayRecordSourceProxy.js +++ b/packages/relay-runtime/mutations/RelayRecordSourceProxy.js @@ -65,7 +65,6 @@ class RelayRecordSourceProxy implements RecordSourceProxy { this.create(dataID, RelayModernRecord.getType(sourceRecord)); } this.__mutator.copyFieldsFromRecord(sourceRecord, dataID); - delete this._proxies[dataID]; } } else if (status === NONEXISTENT) { this.delete(dataID); diff --git a/packages/relay-runtime/mutations/__tests__/RelayRecordSourceMutator-test.js b/packages/relay-runtime/mutations/__tests__/RelayRecordSourceMutator-test.js index facb9aaef2a2f..b574475d4ef78 100644 --- a/packages/relay-runtime/mutations/__tests__/RelayRecordSourceMutator-test.js +++ b/packages/relay-runtime/mutations/__tests__/RelayRecordSourceMutator-test.js @@ -236,7 +236,6 @@ describe('RelayRecordSourceMutator', () => { sf: { [ID_KEY]: 'sf', [TYPENAME_KEY]: 'Page', - name: 'San Francisco', state: 'California', }, }); diff --git a/packages/relay-runtime/store/__tests__/RelayPublishQueue-test.js b/packages/relay-runtime/store/__tests__/RelayPublishQueue-test.js index 1093369ac17f1..80fe3b9ef0f15 100644 --- a/packages/relay-runtime/store/__tests__/RelayPublishQueue-test.js +++ b/packages/relay-runtime/store/__tests__/RelayPublishQueue-test.js @@ -288,7 +288,7 @@ describe('RelayPublishQueue', () => { expect(sourceData['4'].name).toEqual('ZUCK'); }); - it('applies multiple updaters in subsequent run()s', () => { + it('applies updates in subsequent run()s (payload then updater)', () => { const queue = new RelayPublishQueue(store); queue.applyUpdate({ operation: operationDescriptor, @@ -313,6 +313,113 @@ describe('RelayPublishQueue', () => { expect(sourceData['4'].name).toEqual('ZUCK'); }); + it('applies updates in subsequent run()s (updater then updater)', () => { + const queue = new RelayPublishQueue(store); + const optimisticUpdate = { + storeUpdater: storeProxy => { + const zuck = storeProxy.get('4'); + zuck.setValue('zuck', 'name'); + }, + }; + const optimisticUpdate2 = { + storeUpdater: storeProxy => { + const zuck = storeProxy.get('4'); + zuck.setValue('zuckerberg', 'lastName'); + }, + }; + queue.applyUpdate(optimisticUpdate); + queue.applyUpdate(optimisticUpdate2); + queue.run(); + expect(sourceData).toEqual({ + ...initialData, + 4: { + ...initialData['4'], + name: 'zuck', // from first updater + lastName: 'zuckerberg', // from second updater + }, + }); + }); + + it('applies updates in subsequent run()s (payload then payload)', () => { + const queue = new RelayPublishQueue(store); + // First set `name`. + const nameMutation = generateAndCompile(` + mutation ChangeNameMutation( + $input: ActorNameChangeInput! + ) { + actorNameChange(input: $input) { + actor { + name + } + } + } + `).ChangeNameMutation; + const nameMutationDescriptor = createOperationDescriptor(nameMutation, { + input: {}, + }); + queue.applyUpdate({ + operation: nameMutationDescriptor, + response: { + actorNameChange: { + actor: { + __typename: 'User', + id: '4', + name: 'zuck', + }, + }, + }, + }); + // Next set `lastName`. + const lastNameMutation = generateAndCompile(` + mutation ChangeNameMutation( + $input: ActorNameChangeInput! + ) { + actorNameChange(input: $input) { + actor { + lastName + } + } + } + `).ChangeNameMutation; + const lastNameMutationDescriptor = createOperationDescriptor( + lastNameMutation, + {input: {}}, + ); + queue.applyUpdate({ + operation: lastNameMutationDescriptor, + response: { + actorNameChange: { + actor: { + __typename: 'User', + id: '4', + lastName: 'zuckerberg', + }, + }, + }, + }); + queue.run(); + expect(sourceData).toEqual({ + ...initialData, + [ROOT_ID]: { + ...initialData[ROOT_ID], + 'actorNameChange(input:{})': { + __ref: 'client:root:actorNameChange(input:{})', + }, + }, + 'client:root:actorNameChange(input:{})': { + __id: 'client:root:actorNameChange(input:{})', + __typename: 'ActorNameChangePayload', + actor: {__ref: '4'}, + }, + 4: { + ...initialData['4'], + id: '4', + name: 'zuck', // from first updater + lastName: 'zuckerberg', // from second updater + }, + }); + }); + it('rebases changes when an earlier change is reverted', () => { const queue = new RelayPublishQueue(store); const optimisticUpdate = {