From 46dfe1da8cb28aaf365bfacef321887ee054b295 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Thu, 13 Jul 2023 02:23:02 -0700 Subject: [PATCH] fix: catch errors during didCommit in DEBUG (#8708) --- packages/graph/src/-private/graph/graph.ts | 12 + packages/model/src/-private/many-array.ts | 4 + .../-private/managers/record-array-manager.ts | 41 +- .../record-arrays/identifier-array.ts | 10 +- packages/store/src/-private/store-service.ts | 37 +- .../integration/record-array-manager-test.js | 10 +- .../records/new-record-unload-test.js | 358 ++++++++++++++++++ .../tests/integration/records/save-test.js | 24 ++ .../tests/integration/records/unload-test.js | 30 ++ 9 files changed, 498 insertions(+), 28 deletions(-) create mode 100644 tests/main/tests/integration/records/new-record-unload-test.js diff --git a/packages/graph/src/-private/graph/graph.ts b/packages/graph/src/-private/graph/graph.ts index 4572a3adb1a..1b05756b4ed 100644 --- a/packages/graph/src/-private/graph/graph.ts +++ b/packages/graph/src/-private/graph/graph.ts @@ -199,6 +199,10 @@ export class Graph { isReleasable(identifier: StableRecordIdentifier): boolean { const relationships = this.identifiers.get(identifier); if (!relationships) { + if (LOG_GRAPH) { + // eslint-disable-next-line no-console + console.log(`graph: RELEASABLE ${String(identifier)}`); + } return true; } const keys = Object.keys(relationships); @@ -211,9 +215,17 @@ export class Graph { } assert(`Expected a relationship`, relationship); if (relationship.definition.inverseIsAsync) { + if (LOG_GRAPH) { + // eslint-disable-next-line no-console + console.log(`graph: <> RELEASABLE ${String(identifier)}`); + } return false; } } + if (LOG_GRAPH) { + // eslint-disable-next-line no-console + console.log(`graph: RELEASABLE ${String(identifier)}`); + } return true; } diff --git a/packages/model/src/-private/many-array.ts b/packages/model/src/-private/many-array.ts index 8f08b7f518d..a7bed55b947 100644 --- a/packages/model/src/-private/many-array.ts +++ b/packages/model/src/-private/many-array.ts @@ -343,6 +343,10 @@ export default class RelatedCollection extends RecordArray { return record; } + + destroy() { + super.destroy(false); + } } RelatedCollection.prototype.isAsync = false; RelatedCollection.prototype.isPolymorphic = false; diff --git a/packages/store/src/-private/managers/record-array-manager.ts b/packages/store/src/-private/managers/record-array-manager.ts index ed9e5bacc1c..70cca01b5d0 100644 --- a/packages/store/src/-private/managers/record-array-manager.ts +++ b/packages/store/src/-private/managers/record-array-manager.ts @@ -79,6 +79,7 @@ class RecordArrayManager { declare store: Store; declare isDestroying: boolean; declare isDestroyed: boolean; + declare _set: Map>; declare _live: Map; declare _managed: Set; declare _pending: Map; @@ -86,6 +87,7 @@ class RecordArrayManager { declare _staged: Map; declare _subscription: UnsubscribeToken; declare _keyedArrays: Map; + declare _visibilitySet: Map; constructor(options: { store: Store }) { this.store = options.store; @@ -97,13 +99,17 @@ class RecordArrayManager { this._staged = new Map(); this._keyedArrays = new Map(); this._identifiers = new Map(); + this._set = new Map(); + this._visibilitySet = new Map(); this._subscription = this.store.notifications.subscribe( 'resource', (identifier: StableRecordIdentifier, type: CacheOperation) => { if (type === 'added') { + this._visibilitySet.set(identifier, true); this.identifierAdded(identifier); } else if (type === 'removed') { + this._visibilitySet.set(identifier, false); this.identifierRemoved(identifier); } else if (type === 'state') { this.identifierChanged(identifier); @@ -119,7 +125,7 @@ class RecordArrayManager { return; } - sync(array, pending); + sync(array, pending, this._set.get(array)!); this._pending.delete(array); } @@ -154,6 +160,7 @@ class RecordArrayManager { manager: this, }); this._live.set(type, array); + this._set.set(array, new Set(identifiers)); } return array; @@ -178,6 +185,7 @@ class RecordArrayManager { }; let array = new Collection(options); this._managed.add(array); + this._set.set(array, new Set(options.identifiers || [])); if (config.identifiers) { associate(this._identifiers, array, config.identifiers); } @@ -261,6 +269,7 @@ class RecordArrayManager { const old = source.slice(); source.length = 0; fastPush(source, identifiers); + this._set.set(array, new Set(identifiers)); notifyArray(array); array.meta = payload.meta || null; @@ -306,6 +315,12 @@ class RecordArrayManager { identifierChanged(identifier: StableRecordIdentifier): void { let newState = this.store._instanceCache.recordIsLoaded(identifier, true); + // if the change matches the most recent direct added/removed + // state, then we can ignore it + if (this._visibilitySet.get(identifier) === newState) { + return; + } + if (newState) { this.identifierAdded(identifier); } else { @@ -313,16 +328,19 @@ class RecordArrayManager { } } - clear() { - this._live.forEach((array) => array.destroy()); - this._managed.forEach((array) => array.destroy()); + clear(isClear = true) { + this._live.forEach((array) => array.destroy(isClear)); + this._managed.forEach((array) => array.destroy(isClear)); this._managed.clear(); this._identifiers.clear(); + this._pending.clear(); + this._set.forEach((set) => set.clear()); + this._visibilitySet.clear(); } destroy() { this.isDestroying = true; - this.clear(); + this.clear(false); this._live.clear(); this.isDestroyed = true; // eslint-disable-next-line @typescript-eslint/no-unsafe-argument @@ -367,19 +385,24 @@ export function disassociateIdentifier( } } -function sync(array: IdentifierArray, changes: Map) { +function sync( + array: IdentifierArray, + changes: Map, + arraySet: Set +) { let state = array[SOURCE]; const adds: StableRecordIdentifier[] = []; const removes: StableRecordIdentifier[] = []; changes.forEach((value, key) => { if (value === 'add') { // likely we want to keep a Set along-side - if (state.includes(key)) { + if (arraySet.has(key)) { return; } adds.push(key); + arraySet.add(key); } else { - if (state.includes(key)) { + if (arraySet.has(key)) { removes.push(key); } } @@ -387,6 +410,7 @@ function sync(array: IdentifierArray, changes: Map void) { assert(`EmberData should never encounter a nested run`, !this._cbs); const _cbs: { coalesce?: () => void; sync?: () => void; notify?: () => void } = (this._cbs = {}); - cb(); - if (_cbs.coalesce) { - _cbs.coalesce(); - } - if (_cbs.sync) { - _cbs.sync(); - } - if (_cbs.notify) { - _cbs.notify(); + if (DEBUG) { + try { + cb(); + if (_cbs.coalesce) { + _cbs.coalesce(); + } + if (_cbs.sync) { + _cbs.sync(); + } + if (_cbs.notify) { + _cbs.notify(); + } + } finally { + this._cbs = null; + } + } else { + cb(); + if (_cbs.coalesce) { + _cbs.coalesce(); + } + if (_cbs.sync) { + _cbs.sync(); + } + if (_cbs.notify) { + _cbs.notify(); + } + this._cbs = null; } - this._cbs = null; } _join(cb: () => void): void { if (this._cbs) { diff --git a/tests/main/tests/integration/record-array-manager-test.js b/tests/main/tests/integration/record-array-manager-test.js index 29535dd08b2..1d13fb62840 100644 --- a/tests/main/tests/integration/record-array-manager-test.js +++ b/tests/main/tests/integration/record-array-manager-test.js @@ -79,26 +79,26 @@ module('integration/record_array_manager', function (hooks) { let adapterPopulated = manager.createArray({ type: 'person', query }); let identifier = recordIdentifierFor(person); - assert.false(all.isDestroyed, 'initial: no calls to all.willDestroy'); - assert.false(adapterPopulated.isDestroyed, 'initial: no calls to adapterPopulated.willDestroy'); + assert.false(all.isDestroyed, 'initial: LiveArray is not destroyed'); + assert.false(adapterPopulated.isDestroyed, 'initial: Collection is not destroyed'); assert.strictEqual( manager._identifiers.get(identifier)?.size, undefined, - 'initial: expected the person to be a member of 0 AdapterPopulatedRecordArrays' + 'initial: expected the person to be a member of 0 Collections' ); assert.true(manager._live.has('person'), 'initial: we have a live array for person'); manager.destroy(); await settled(); - assert.true(all.isDestroyed, 'all.willDestroy called once'); + assert.true(all.isDestroyed, 'LiveArray is destroyed'); assert.false(manager._live.has('person'), 'no longer have a live array for person'); assert.strictEqual( manager._identifiers.get(identifier)?.size, undefined, 'expected the person to be a member of no recordArrays' ); - assert.true(adapterPopulated.isDestroyed, 'adapterPopulated.willDestroy called once'); + assert.true(adapterPopulated.isDestroyed, 'Collection is destroyed'); }); test('#GH-4041 store#query AdapterPopulatedRecordArrays are removed from their managers instead of retained when #destroy is called', async function (assert) { diff --git a/tests/main/tests/integration/records/new-record-unload-test.js b/tests/main/tests/integration/records/new-record-unload-test.js new file mode 100644 index 00000000000..f5a9f23548b --- /dev/null +++ b/tests/main/tests/integration/records/new-record-unload-test.js @@ -0,0 +1,358 @@ +import { settled } from '@ember/test-helpers'; + +import { module, test } from 'qunit'; + +import { setupTest } from 'ember-qunit'; + +import Model, { attr, hasMany } from '@ember-data/model'; +import testInDebug from '@ember-data/unpublished-test-infra/test-support/test-in-debug'; + +class Person extends Model { + @attr name; + @hasMany('person', { async: true, inverse: 'friends' }) friends; +} + +module('Integration | Records | New Record Unload', function (hooks) { + setupTest(hooks); + + hooks.beforeEach(function () { + this.owner.register('model:person', Person); + }); + + test('Rolling Back Attributes on a New Record unloads that record safely', async function (assert) { + const store = this.owner.lookup('service:store'); + const Matt = store.push({ + data: { + type: 'person', + id: '1', + attributes: { + name: 'Matthew Seidel', + }, + relationships: { + friends: { + data: [], + }, + }, + }, + }); + let Pat = store.createRecord('person', { name: 'Patrick Wachter' }); + const friends = Matt.hasMany('friends').value(); + friends.push(Pat); + let people = store.peekAll('person'); + + assert.strictEqual(friends.length, 1, 'Matt has friends'); + assert.strictEqual(people.length, 2, 'precond - two people records in the store'); + assert.true(Pat.hasDirtyAttributes, 'precond - record has dirty attributes'); + assert.true(Pat.isNew, 'precond - record is new'); + + Pat.rollbackAttributes(); + + assert.false(Pat.isDestroyed, 'record is not yet destroyed'); + assert.true(Pat.isDestroying, 'record is destroying'); + assert.strictEqual(friends.length, 0, 'Matt has no friends'); + assert.strictEqual(people.length, 1, 'precond - one person left in the store'); + + await settled(); + + assert.true(Pat.isDestroyed, 'record is destroyed'); + assert.true(Pat.isDestroying, 'record is destroying'); + assert.strictEqual(friends.length, 0, 'Matt has no friends'); + assert.strictEqual(people.length, 1, 'precond - one person left in the store'); + }); + + test('Unload on a New Record unloads that record safely', async function (assert) { + const store = this.owner.lookup('service:store'); + const Matt = store.push({ + data: { + type: 'person', + id: '1', + attributes: { + name: 'Matthew Seidel', + }, + relationships: { + friends: { + data: [], + }, + }, + }, + }); + let Pat = store.createRecord('person', { name: 'Patrick Wachter' }); + const friends = Matt.hasMany('friends').value(); + friends.push(Pat); + let people = store.peekAll('person'); + + assert.strictEqual(friends.length, 1, 'Matt has friends'); + assert.strictEqual(people.length, 2, 'precond - two people records in the store'); + assert.true(Pat.hasDirtyAttributes, 'precond - record has dirty attributes'); + assert.true(Pat.isNew, 'precond - record is new'); + + Pat.unloadRecord(); + + assert.false(Pat.isDestroyed, 'record is not yet destroyed'); + assert.true(Pat.isDestroying, 'record is destroying'); + assert.strictEqual(friends.length, 0, 'Matt has no friends'); + assert.strictEqual(people.length, 1, 'precond - one person left in the store'); + + await settled(); + + assert.true(Pat.isDestroyed, 'record is destroyed'); + assert.true(Pat.isDestroying, 'record is destroying'); + assert.strictEqual(friends.length, 0, 'Matt has no friends'); + assert.strictEqual(people.length, 1, 'precond - one person left in the store'); + }); + + testInDebug('Unload after a save that failed for missing data is safe', async function (assert) { + const store = this.owner.lookup('service:store'); + const adapter = store.adapterFor('application'); + adapter.createRecord = () => Promise.resolve({ data: null }); + const Matt = store.push({ + data: { + type: 'person', + id: '1', + attributes: { + name: 'Matthew Seidel', + }, + relationships: { + friends: { + data: [], + }, + }, + }, + }); + let Pat = store.createRecord('person', { name: 'Patrick Wachter' }); + const friends = Matt.hasMany('friends').value(); + friends.push(Pat); + let people = store.peekAll('person'); + + assert.strictEqual(friends.length, 1, 'Matt has friends'); + assert.strictEqual(people.length, 2, 'precond - two people records in the store'); + assert.true(Pat.hasDirtyAttributes, 'precond - record has dirty attributes'); + assert.true(Pat.isNew, 'precond - record is new'); + + try { + await Pat.save(); + assert.ok(false, 'save failed'); + } catch (e) { + assert.ok(true, 'save failed'); + } + + Pat.unloadRecord(); + + assert.false(Pat.isDestroyed, 'after unload (sync): record is not yet destroyed'); + assert.true(Pat.isDestroying, 'after unload (sync): record is destroying'); + assert.strictEqual(friends.length, 0, 'after unload (sync): Matt has no friends'); + assert.strictEqual(people.length, 1, 'after unload (sync): one person left in the store'); + + await settled(); + + assert.true(Pat.isDestroyed, 'final: record is destroyed'); + assert.true(Pat.isDestroying, 'final: record is destroying'); + assert.strictEqual(friends.length, 0, 'final: Matt has no friends'); + assert.strictEqual(people.length, 1, 'final: one person left in the store'); + }); + + testInDebug('Unload after a save that failed for missing id is safe', async function (assert) { + const store = this.owner.lookup('service:store'); + const adapter = store.adapterFor('application'); + adapter.createRecord = () => Promise.resolve({ data: { type: 'person' } }); + const Matt = store.push({ + data: { + type: 'person', + id: '1', + attributes: { + name: 'Matthew Seidel', + }, + relationships: { + friends: { + data: [], + }, + }, + }, + }); + let Pat = store.createRecord('person', { name: 'Patrick Wachter' }); + const friends = Matt.hasMany('friends').value(); + friends.push(Pat); + let people = store.peekAll('person'); + + assert.strictEqual(friends.length, 1, 'Matt has friends'); + assert.strictEqual(people.length, 2, 'precond - two people records in the store'); + assert.true(Pat.hasDirtyAttributes, 'precond - record has dirty attributes'); + assert.true(Pat.isNew, 'precond - record is new'); + + try { + await Pat.save(); + assert.ok(false, 'save failed'); + } catch (e) { + assert.ok(true, 'save failed'); + } + + Pat.unloadRecord(); + + assert.false(Pat.isDestroyed, 'after unload (sync): record is not yet destroyed'); + assert.true(Pat.isDestroying, 'after unload (sync): record is destroying'); + assert.strictEqual(friends.length, 0, 'after unload (sync): Matt has no friends'); + assert.strictEqual(people.length, 1, 'after unload (sync): one person left in the store'); + + await settled(); + + assert.true(Pat.isDestroyed, 'final: record is destroyed'); + assert.true(Pat.isDestroying, 'final: record is destroying'); + assert.strictEqual(friends.length, 0, 'final: Matt has no friends'); + assert.strictEqual(people.length, 1, 'final: one person left in the store'); + }); + + test('Unload after a failed save is safe', async function (assert) { + const store = this.owner.lookup('service:store'); + const adapter = store.adapterFor('application'); + adapter.createRecord = () => Promise.reject(new Error('Invalid Data')); + const Matt = store.push({ + data: { + type: 'person', + id: '1', + attributes: { + name: 'Matthew Seidel', + }, + relationships: { + friends: { + data: [], + }, + }, + }, + }); + let Pat = store.createRecord('person', { name: 'Patrick Wachter' }); + const friends = Matt.hasMany('friends').value(); + friends.push(Pat); + let people = store.peekAll('person'); + + assert.strictEqual(friends.length, 1, 'Matt has friends'); + assert.strictEqual(people.length, 2, 'precond - two people records in the store'); + assert.true(Pat.hasDirtyAttributes, 'precond - record has dirty attributes'); + assert.true(Pat.isNew, 'precond - record is new'); + + try { + await Pat.save(); + assert.ok(false, 'save failed'); + } catch (e) { + assert.ok(true, 'save failed'); + } + + Pat.unloadRecord(); + + assert.false(Pat.isDestroyed, 'after unload (sync): record is not yet destroyed'); + assert.true(Pat.isDestroying, 'after unload (sync): record is destroying'); + assert.strictEqual(friends.length, 0, 'after unload (sync): Matt has no friends'); + assert.strictEqual(people.length, 1, 'after unload (sync): one person left in the store'); + + await settled(); + + assert.true(Pat.isDestroyed, 'final: record is destroyed'); + assert.true(Pat.isDestroying, 'final: record is destroying'); + assert.strictEqual(friends.length, 0, 'final: Matt has no friends'); + assert.strictEqual(people.length, 1, 'final: one person left in the store'); + }); + + test('Unload after a save is safe', async function (assert) { + const store = this.owner.lookup('service:store'); + const adapter = store.adapterFor('application'); + adapter.createRecord = () => Promise.resolve({ data: { type: 'person', id: '2' } }); + const Matt = store.push({ + data: { + type: 'person', + id: '1', + attributes: { + name: 'Matthew Seidel', + }, + relationships: { + friends: { + data: [], + }, + }, + }, + }); + let Pat = store.createRecord('person', { name: 'Patrick Wachter' }); + const friends = Matt.hasMany('friends').value(); + friends.push(Pat); + let people = store.peekAll('person'); + + assert.strictEqual(friends.length, 1, 'Matt has friends'); + assert.strictEqual(people.length, 2, 'precond - two people records in the store'); + assert.true(Pat.hasDirtyAttributes, 'precond - record has dirty attributes'); + assert.true(Pat.isNew, 'precond - record is new'); + + try { + await Pat.save(); + assert.ok(true, 'save succeeded'); + } catch (e) { + assert.ok(false, 'save succeeded'); + } + + assert.strictEqual(friends.length, 1, 'after save: Matt has friends'); + assert.strictEqual(people.length, 2, 'after save: two people records in the store'); + assert.false(Pat.hasDirtyAttributes, 'after save: record is clean'); + assert.false(Pat.isNew, 'after save: record is not new'); + + Pat.unloadRecord(); + + assert.false(Pat.isDestroyed, 'after unload (sync): record is not yet destroyed'); + assert.true(Pat.isDestroying, 'after unload (sync): record is destroying'); + assert.strictEqual(friends.length, 0, 'after unload (sync): Matt has no friends'); + assert.strictEqual(people.length, 1, 'after unload (sync): one person left in the store'); + + await settled(); + + assert.true(Pat.isDestroyed, 'final: record is destroyed'); + assert.true(Pat.isDestroying, 'final: record is destroying'); + assert.strictEqual(friends.length, 0, 'final: Matt has no friends'); + assert.strictEqual(people.length, 1, 'final: one person left in the store'); + }); + + test('Unload after a save is safe (no access after save before unload)', async function (assert) { + const store = this.owner.lookup('service:store'); + const adapter = store.adapterFor('application'); + adapter.createRecord = () => Promise.resolve({ data: { type: 'person', id: '2' } }); + const Matt = store.push({ + data: { + type: 'person', + id: '1', + attributes: { + name: 'Matthew Seidel', + }, + relationships: { + friends: { + data: [], + }, + }, + }, + }); + let Pat = store.createRecord('person', { name: 'Patrick Wachter' }); + const friends = Matt.hasMany('friends').value(); + friends.push(Pat); + let people = store.peekAll('person'); + + assert.strictEqual(friends.length, 1, 'Matt has friends'); + assert.strictEqual(people.length, 2, 'precond - two people records in the store'); + assert.true(Pat.hasDirtyAttributes, 'precond - record has dirty attributes'); + assert.true(Pat.isNew, 'precond - record is new'); + + try { + await Pat.save(); + assert.ok(true, 'save succeeded'); + } catch (e) { + assert.ok(false, 'save succeeded'); + } + + Pat.unloadRecord(); + + assert.false(Pat.isDestroyed, 'after unload (sync): record is not yet destroyed'); + assert.true(Pat.isDestroying, 'after unload (sync): record is destroying'); + assert.strictEqual(friends.length, 0, 'after unload (sync): Matt has no friends'); + assert.strictEqual(people.length, 1, 'after unload (sync): one person left in the store'); + + await settled(); + + assert.true(Pat.isDestroyed, 'final: record is destroyed'); + assert.true(Pat.isDestroying, 'final: record is destroying'); + assert.strictEqual(friends.length, 0, 'final: Matt has no friends'); + assert.strictEqual(people.length, 1, 'final: one person left in the store'); + }); +}); diff --git a/tests/main/tests/integration/records/save-test.js b/tests/main/tests/integration/records/save-test.js index a03bbea78ac..8a42c989f84 100644 --- a/tests/main/tests/integration/records/save-test.js +++ b/tests/main/tests/integration/records/save-test.js @@ -220,6 +220,30 @@ module('integration/records/save - Save Record', function (hooks) { } }); + test('Will not unload record if it fails to save on create', async function (assert) { + const store = this.owner.lookup('service:store'); + const adapter = store.adapterFor('application'); + const post = store.createRecord('post', { title: 'toto' }); + const posts = store.peekAll('post'); + + assert.strictEqual(posts.length, 1, 'precond - store has one post'); + + adapter.createRecord = async function () { + const error = new InvalidError([{ title: 'not valid' }]); + return Promise.reject(error); + }; + + try { + await post.save(); + assert.ok(false, 'we should error'); + } catch { + assert.ok(true, 'save operation was rejected'); + } + + assert.false(post.isDestroyed, 'post is not destroyed'); + assert.strictEqual(posts.length, 1, 'store still has the post'); + }); + test('Will error when saving after unloading record via the store', async function (assert) { assert.expect(1); diff --git a/tests/main/tests/integration/records/unload-test.js b/tests/main/tests/integration/records/unload-test.js index 1fb394ab14f..34de2339e33 100644 --- a/tests/main/tests/integration/records/unload-test.js +++ b/tests/main/tests/integration/records/unload-test.js @@ -688,6 +688,36 @@ module('integration/unload - Unloading Records', function (hooks) { }; } + test('unloadAll() does not destroy the record array', function (assert) { + store.push({ + data: { + type: 'person', + id: '1', + attributes: { + name: 'Could be Anybody', + }, + }, + }); + const all = store.peekAll('person'); + assert.strictEqual(all.length, 1, 'precond - record array has one item'); + store.unloadAll(); + assert.strictEqual(all.length, 0, 'after unloadAll: record array has no items'); + assert.false(all.isDestroyed, 'after unloadAll: record array is not destroyed'); + store.push({ + data: { + type: 'person', + id: '1', + attributes: { + name: 'Could be Anybody', + }, + }, + }); + const all2 = store.peekAll('person'); + assert.strictEqual(all2.length, 1, 'after next push: record array has one item'); + // eslint-disable-next-line qunit/no-ok-equality + assert.true(all === all2, 'after next push: record array is the same'); + }); + test('unloadAll(type) does not leave stranded internalModels in relationships (rediscover via store.push)', async function (assert) { assert.expect(13);