fix: clear relationships properly when unloading new records (#8791)

Windvis · runspired · web-flow · commit edf2a0b28301 · 2023-08-27T13:14:08.000-07:00
* Add a failing test case

Unloading a new record that is related to another record does not remove it from the relationship.

* update test

* fix: dont orphan new records in implicit relationships

* fixes

---------

Co-authored-by: Chris Thoburn &lt;runspired@gmail.com&gt;
diff --git a/packages/graph/src/-private/graph.ts b/packages/graph/src/-private/graph.ts
@@ -234,7 +234,7 @@ export class Graph {
         continue;
       }
       assert(`Expected a relationship`, relationship);
-      if (relationship.definition.inverseIsAsync) {
+      if (relationship.definition.inverseIsAsync && !isNew(identifier)) {
         if (LOG_GRAPH) {
           // eslint-disable-next-line no-console
           console.log(`graph: <<NOT>> RELEASABLE ${String(identifier)}`);
diff --git a/packages/json-api/src/-private/cache.ts b/packages/json-api/src/-private/cache.ts
@@ -868,7 +868,16 @@ export default class JSONAPICache implements Cache {
     const removeFromRecordArray = !this.isDeletionCommitted(identifier);
     let removed = false;
     const cached = this.__peek(identifier, false);
-    peekGraph(storeWrapper)?.unload(identifier);
+
+    if (cached.isNew) {
+      peekGraph(storeWrapper)?.push({
+        op: 'deleteRecord',
+        record: identifier,
+        isNew: true,
+      });
+    } else {
+      peekGraph(storeWrapper)?.unload(identifier);
+    }
 
     // effectively clearing these is ensuring that
     // we report as `isEmpty` during teardown.
@@ -1025,11 +1034,7 @@ export default class JSONAPICache implements Cache {
     }
 
     if (cached.isNew) {
-      this.__graph.push({
-        op: 'deleteRecord',
-        record: identifier,
-        isNew: true,
-      });
+      // > Note: Graph removal handled by unloadRecord
       cached.isDeleted = true;
       cached.isNew = false;
     }
@@ -1083,14 +1088,7 @@ export default class JSONAPICache implements Cache {
   setIsDeleted(identifier: StableRecordIdentifier, isDeleted: boolean): void {
     const cached = this.__peek(identifier, false);
     cached.isDeleted = isDeleted;
-    if (cached.isNew) {
-      // TODO can we delete this since we will do this in unload?
-      this.__graph.push({
-        op: 'deleteRecord',
-        record: identifier,
-        isNew: true,
-      });
-    }
+    // > Note: Graph removal for isNew handled by unloadRecord
     this.__storeWrapper.notifyChange(identifier, 'state');
   }
 
diff --git a/tests/graph/ember-cli-build.js b/tests/graph/ember-cli-build.js
@@ -8,6 +8,9 @@ module.exports = function (defaults) {
   let app = new EmberApp(defaults, {
     emberData: {
       compatWith,
+      debug: {
+        // LOG_GRAPH: true,
+      },
     },
     babel: {
       // this ensures that the same build-time code stripping that is done
diff --git a/tests/graph/tests/integration/graph/edge-removal/abstract-edge-removal-test.ts b/tests/graph/tests/integration/graph/edge-removal/abstract-edge-removal-test.ts
@@ -175,8 +175,8 @@ module('Integration | Graph | Edge Removal', function (hooks) {
 
         // we remove if the record was new or if the relationship was sync (client side delete semantics)
         let removed = config.useCreate || !config.async;
-        // we clear sync non-implicit relationships (client side delete semantics)
-        let cleared = !config.async && !config.inverseNull;
+        // we clear new records, or sync non-implicit relationships (client side delete semantics)
+        let cleared = config.useCreate || (!config.async && !config.inverseNull);
 
         await testFinalState(this, testState, config, { removed, cleared, implicitCleared: true }, assert);
       });
diff --git a/tests/main/tests/index.html b/tests/main/tests/index.html
@@ -36,10 +36,12 @@
 			// the reporter is configured to ignore them. This results
 			// in extremely high memory and cpu utilization and eventually
 			// will crash the console.
-			const ignoredLogTypes = new Set(['log', 'warn', 'info', 'group']);
+			const ignoredLogTypes = new Set(['log', 'warn', 'info', 'group', 'debug', 'groupEnd', 'error', 'trace', 'dir']);
 			// restore original functionality
 			ignoredLogTypes.forEach((method) => {
-				console[method] = Testem.console[method];
+        if (Testem.console[method]) {
+          console[method] = Testem.console[method];
+        }
 			});
 
 			// enable debugging memory over time
diff --git a/tests/main/tests/integration/records/new-record-unload-test.js b/tests/main/tests/integration/records/new-record-unload-test.js
@@ -4,12 +4,13 @@ import { module, test } from 'qunit';
 
 import { setupTest } from 'ember-qunit';
 
-import Model, { attr, hasMany } from '@ember-data/model';
+import Model, { attr, belongsTo, 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;
+  @belongsTo('person', { async: true, inverse: null }) bestFriend;
 }
 
 module('Integration | Records | New Record Unload', function (hooks) {
@@ -60,6 +61,77 @@ module('Integration | Records | New Record Unload', function (hooks) {
     assert.strictEqual(people.length, 1, 'precond - one person left in the store');
   });
 
+  test('Rolling Back Attributes on multiple New (related via async self-reflexive HasMany) Records unloads them safely', async function (assert) {
+    const store = this.owner.lookup('service:store');
+    const Pat = store.createRecord('person', { name: 'Patrick Wachter' });
+    const Matt = store.createRecord('person', { name: 'Matthew Seidel', friends: [Pat] });
+    const friends = Matt.hasMany('friends').value();
+    const 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(Matt.hasDirtyAttributes, 'precond - record has dirty attributes');
+    assert.true(Matt.isNew, 'precond - record is new');
+    assert.true(Pat.hasDirtyAttributes, 'precond - record has dirty attributes');
+    assert.true(Pat.isNew, 'precond - record is new');
+
+    Pat.rollbackAttributes();
+
+    assert.false(Pat.isDestroyed, 'Pat record is not yet destroyed');
+    assert.true(Pat.isDestroying, 'Pat record is destroying');
+    assert.strictEqual(friends.length, 0, 'Matt has no friends');
+    assert.strictEqual(people.length, 1, 'precond - one person left in the store');
+
+    Matt.rollbackAttributes();
+    assert.false(Matt.isDestroyed, 'Matt record is not yet destroyed');
+    assert.true(Matt.isDestroying, 'Matt record is destroying');
+    assert.strictEqual(people.length, 0, 'precond - no people left in the store');
+
+    await settled();
+
+    assert.true(Pat.isDestroyed, 'Pat record is destroyed');
+    assert.true(Pat.isDestroying, 'Pat record is destroying');
+    assert.true(Matt.isDestroyed, 'Matt record is destroyed');
+    assert.true(Matt.isDestroying, 'Matt record is destroying');
+    assert.strictEqual(people.length, 0, 'precond - no people left in the store');
+  });
+
+  test('Rolling Back Attributes on multiple New (related via async belongsTo with no inverse) Records unloads them safely', async function (assert) {
+    const store = this.owner.lookup('service:store');
+    const Pat = store.createRecord('person', { name: 'Patrick Wachter' });
+    const Matt = store.createRecord('person', { name: 'Matthew Seidel', bestFriend: Pat });
+    let bestFriend = Matt.belongsTo('bestFriend').value();
+    const people = store.peekAll('person');
+
+    assert.strictEqual(people.length, 2, 'precond - two people records in the store');
+    assert.strictEqual(bestFriend, Pat, 'Matt has a best friend');
+    assert.true(Matt.hasDirtyAttributes, 'precond - record has dirty attributes');
+    assert.true(Matt.isNew, 'precond - record is new');
+    assert.true(Pat.hasDirtyAttributes, 'precond - record has dirty attributes');
+    assert.true(Pat.isNew, 'precond - record is new');
+
+    Pat.rollbackAttributes();
+
+    bestFriend = Matt.belongsTo('bestFriend').value();
+    assert.strictEqual(bestFriend, null, 'Matt has no best friend');
+    assert.false(Pat.isDestroyed, 'Pat record is not yet destroyed');
+    assert.true(Pat.isDestroying, 'Pat record is destroying');
+    assert.strictEqual(people.length, 1, 'precond - one person left in the store');
+
+    Matt.rollbackAttributes();
+    assert.false(Matt.isDestroyed, 'Matt record is not yet destroyed');
+    assert.true(Matt.isDestroying, 'Matt record is destroying');
+    assert.strictEqual(people.length, 0, 'precond - no people left in the store');
+
+    await settled();
+
+    assert.true(Pat.isDestroyed, 'Pat record is destroyed');
+    assert.true(Pat.isDestroying, 'Pat record is destroying');
+    assert.true(Matt.isDestroyed, 'Matt record is destroyed');
+    assert.true(Matt.isDestroying, 'Matt record is destroying');
+    assert.strictEqual(people.length, 0, 'precond - no people 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({

Original file line number	Diff line number	Diff line change
`@@ -234,7 +234,7 @@ export class Graph {`
`234`	`234`	`continue;`
`235`	`235`	`}`
`236`	`236`	assert(`Expected a relationship`, relationship);
`237`		`- if (relationship.definition.inverseIsAsync) {`
	`237`	`+ if (relationship.definition.inverseIsAsync && !isNew(identifier)) {`
`238`	`238`	`if (LOG_GRAPH) {`
`239`	`239`	`// eslint-disable-next-line no-console`
`240`	`240`	console.log(`graph: <<NOT>> RELEASABLE ${String(identifier)}`);