fix: keep a backreference for previously merged identifiers (#9183)

* fix: keep a backreference for previously merged identifiers

* update tests

Author: runspired

.vscode/settings.json

@@ -1,7 +1,43 @@
 {
+  "eslint.format.enable": true,
+  "eslint.workingDirectories": [{ "mode": "auto" }, "packages/*", "tests/*"],
+  "eslint.options": { "reportUnusedDisableDirectives": "error" },
+  "editor.codeActionsOnSave": {
+    "source.fixAll.eslint": "explicit"
+  },
   "files.associations": {
     "turbo.json": "jsonc"
   },
-  "eslint.workingDirectories": [{ "mode": "auto" }, "packages/*", "tests/*"],
-  "editor.formatOnSave": true
+  "editor.defaultFormatter": "esbenp.prettier-vscode",
+  "editor.formatOnSave": true,
+  "[javascript]": {
+    "editor.defaultFormatter": "esbenp.prettier-vscode",
+    "editor.formatOnSave": true
+  },
+  "[typescript]": {
+    "editor.defaultFormatter": "esbenp.prettier-vscode",
+    "editor.formatOnSave": true
+  },
+  "[handlebars]": {
+    "editor.defaultFormatter": "esbenp.prettier-vscode",
+    "editor.formatOnSave": true
+  },
+  "[json]": {
+    "editor.defaultFormatter": "esbenp.prettier-vscode",
+    "editor.formatOnSave": true
+  },
+  "[glimmer-js]": {
+    "editor.defaultFormatter": "esbenp.prettier-vscode",
+    "editor.formatOnSave": true
+  },
+  "[glimmer-ts]": {
+    "editor.defaultFormatter": "esbenp.prettier-vscode",
+    "editor.formatOnSave": true
+  },
+  "[markdown]": {
+    "editor.detectIndentation": false,
+    "editor.insertSpaces": false,
+    "editor.tabSize": 4
+  },
+  "typescript.preferences.importModuleSpecifier": "project-relative"
 }

packages/store/src/-private/caches/identifier-cache.ts

@@ -84,6 +84,7 @@ type StableCache = {
   resources: IdentifierMap;
   documents: Map<string, StableDocumentIdentifier>;
   resourcesByType: TypeMap;
+  polymorphicLidBackMap: Map<string, string[]>;
 };
 
 export type KeyInfoMethod = (resource: unknown, known: StableRecordIdentifier | null) => KeyInfo;
@@ -251,6 +252,7 @@ export class IdentifierCache {
       resources: new Map<string, StableRecordIdentifier>(),
       resourcesByType: Object.create(null) as TypeMap,
       documents: new Map<string, StableDocumentIdentifier>(),
+      polymorphicLidBackMap: new Map<string, string[]>(),
     };
   }
 
@@ -560,16 +562,33 @@ export class IdentifierCache {
     const kept = this._merge(identifier, existingIdentifier, data);
     const abandoned = kept === identifier ? existingIdentifier : identifier;
 
+    // get any backreferences before forgetting this identifier, as it will be removed from the cache
+    // and we will no longer be able to find them
+    const abandonedBackReferences = this._cache.polymorphicLidBackMap.get(abandoned.lid);
+    // delete the backreferences for the abandoned identifier so that forgetRecordIdentifier
+    // does not try to remove them.
+    if (abandonedBackReferences) this._cache.polymorphicLidBackMap.delete(abandoned.lid);
+
     // cleanup the identifier we no longer need
     this.forgetRecordIdentifier(abandoned);
 
-    // ensure a secondary cache entry for this id for the identifier we do keep
-    // keyOptions.id.set(newId, kept);
+    // ensure a secondary cache entry for the original lid for the abandoned identifier
+    this._cache.resources.set(abandoned.lid, kept);
+
+    // backReferences let us know which other identifiers are pointing at this identifier
+    // so we can delete them later if we forget this identifier
+    const keptBackReferences = this._cache.polymorphicLidBackMap.get(kept.lid) ?? [];
+    keptBackReferences.push(abandoned.lid);
 
-    // ensure a secondary cache entry for this id for the abandoned identifier's type we do keep
-    // let baseKeyOptions = getTypeIndex(this._cache.resourcesByType, existingIdentifier.type);
-    // baseKeyOptions.id.set(newId, kept);
+    // update the backreferences from the abandoned identifier to be for the kept identifier
+    if (abandonedBackReferences) {
+      abandonedBackReferences.forEach((lid) => {
+        keptBackReferences.push(lid);
+        this._cache.resources.set(lid, kept);
+      });
+    }
 
+    this._cache.polymorphicLidBackMap.set(kept.lid, keptBackReferences);
     return kept;
   }
 
@@ -596,6 +615,14 @@ export class IdentifierCache {
     this._cache.resources.delete(identifier.lid);
     typeSet.lid.delete(identifier.lid);
 
+    const backReferences = this._cache.polymorphicLidBackMap.get(identifier.lid);
+    if (backReferences) {
+      backReferences.forEach((lid) => {
+        this._cache.resources.delete(lid);
+      });
+      this._cache.polymorphicLidBackMap.delete(identifier.lid);
+    }
+
     if (DEBUG) {
       identifier[DEBUG_STALE_CACHE_OWNER] = identifier[CACHE_OWNER];
     }

tests/main/tests/integration/identifiers/configuration-test.ts

@@ -382,6 +382,10 @@ module('Integration | Identifiers - configuration', function (hooks) {
       if (bucket !== 'record') {
         throw new Error('Test cannot generate an lid for a non-record');
       }
+      if (typeof resource === 'object' && resource !== null && 'lid' in resource && typeof resource.lid === 'string') {
+        generateLidCalls++;
+        return resource.lid;
+      }
       if (typeof resource !== 'object' || resource === null || !('type' in resource)) {
         throw new Error('Test cannot generate an lid for a non-object');
       }
@@ -393,12 +397,12 @@ module('Integration | Identifiers - configuration', function (hooks) {
       return `${resource.type}:${resource.id}`;
     });
     let forgetMethodCalls = 0;
-    // eslint-disable-next-line prefer-const
+
     let expectedIdentifier;
 
     let testMethod = (identifier) => {
       forgetMethodCalls++;
-      assert.strictEqual(expectedIdentifier, identifier, `We forgot the expected identifier ${expectedIdentifier}`);
+      assert.strictEqual(identifier, expectedIdentifier, `We forgot the expected identifier ${expectedIdentifier}`);
     };
 
     setIdentifierForgetMethod((identifier) => {
@@ -434,7 +438,11 @@ module('Integration | Identifiers - configuration', function (hooks) {
     const finalUserByIdIdentifier = recordIdentifierFor(userById);
 
     assert.strictEqual(generateLidCalls, 2, 'We generated no new lids when we looked up the originals');
-    assert.strictEqual(store.identifierCache._cache.resources.size, 1, 'We now have only 1 identifier in the cache');
+    assert.strictEqual(
+      store.identifierCache._cache.resources.size,
+      2,
+      'We keep a back reference identifier in the cache'
+    );
     assert.strictEqual(forgetMethodCalls, 1, 'We abandoned an identifier');
 
     assert.notStrictEqual(
@@ -453,6 +461,36 @@ module('Integration | Identifiers - configuration', function (hooks) {
       'We are using the identifier by id for the result of findRecord with username'
     );
 
+    const recordA = store.peekRecord({ lid: finalUserByUsernameIdentifier.lid });
+    const recordB = store.peekRecord({ lid: finalUserByIdIdentifier.lid });
+    const recordC = store.peekRecord({ lid: originalUserByUsernameIdentifier.lid });
+    const recordD = store.peekRecord('user', '@runspired');
+    const recordE = store.peekRecord('user', '1');
+
+    assert.strictEqual(recordA, recordB, 'We have a single record for both identifiers');
+    assert.strictEqual(recordA, recordC, 'We have a single record for both identifiers');
+    assert.strictEqual(recordA, recordD, 'We have a single record for both identifiers');
+    assert.strictEqual(recordA, recordE, 'We have a single record for both identifiers');
+
+    const regeneratedIdentifier = store.identifierCache.getOrCreateRecordIdentifier({
+      lid: finalUserByUsernameIdentifier.lid,
+    });
+    const regeneratedIdentifier2 = store.identifierCache.getOrCreateRecordIdentifier({
+      id: '@runspired',
+      type: 'user',
+    });
+    assert.strictEqual(regeneratedIdentifier, finalUserByUsernameIdentifier, 'We regenerate the same identifier');
+    assert.strictEqual(regeneratedIdentifier2, finalUserByUsernameIdentifier, 'We regenerate the same identifier');
+
+    expectedIdentifier = finalUserByIdIdentifier;
+    store.unloadRecord(recordA);
+    await settled();
+    assert.strictEqual(
+      store.identifierCache._cache.resources.size,
+      0,
+      'We have no identifiers or backreferences in the cache'
+    );
+
     // end test before store teardown
     testMethod = () => {};
   });

tests/main/tests/integration/identifiers/scenarios-test.ts

@@ -468,12 +468,14 @@ module('Integration | Identifiers - scenarios', function (hooks) {
 
       // ensure we truly are in a good state internally
       const lidCache = store.identifierCache._cache.resources;
-      const lids = [...lidCache.values()];
-      assert.strictEqual(
-        lidCache.size,
-        1,
-        `We only have the lid '${identifierByUsername.lid}' in ['${lids.join("', '")}']`
+      assert.strictEqual(lidCache.size, 2, `We should have both lids in the cache still since one is a backreference`);
+      assert.deepEqual(
+        [...lidCache.keys()],
+        ['remote:user:1:9001', 'remote:user:@runspired:9000'],
+        'We have the expected keys'
       );
+      const lids = [...lidCache.values()];
+      assert.arrayStrictEquals(lids, [identifierById, identifierById], 'We have the expected values');
 
       // ensure we still use secondary caching for @runspired post-merging of the identifiers
       const recordByUsernameAgain = (await store.findRecord('user', '@runspired')) as User;

tests/main/tests/integration/records/polymorphic-find-record-test.ts

@@ -0,0 +1,53 @@
+import { settled } from '@ember/test-helpers';
+
+import { module, test } from 'qunit';
+
+import type Store from 'ember-data/store';
+import { setupTest } from 'ember-qunit';
+
+import Model, { attr } from '@ember-data/model';
+import { recordIdentifierFor } from '@ember-data/store';
+
+class Person extends Model {
+  @attr declare name: string;
+}
+class Employee extends Person {}
+
+module('integration/records/polymorphic-find-record - Polymorphic findRecord', function (hooks) {
+  setupTest(hooks);
+
+  test('when findRecord with abstract type returns concrete type', async function (assert) {
+    this.owner.register('model:person', Person);
+    this.owner.register('model:employee', Employee);
+
+    const store = this.owner.lookup('service:store') as Store;
+    const adapter = store.adapterFor('application');
+
+    adapter.findRecord = () => {
+      return Promise.resolve({
+        data: {
+          id: '1',
+          type: 'employee',
+          attributes: {
+            name: 'Rey Skybarker',
+          },
+        },
+      });
+    };
+
+    const person = (await store.findRecord('person', '1')) as Employee;
+    assert.ok(person instanceof Employee, 'record is an instance of Employee');
+    assert.strictEqual(person.name, 'Rey Skybarker', 'name is correct');
+    assert.strictEqual(recordIdentifierFor(person).type, 'employee', 'identifier has the concrete type');
+
+    const employee = store.peekRecord('employee', '1');
+    const person2 = store.peekRecord('person', '1');
+    assert.strictEqual(employee, person, 'peekRecord returns the same instance for concrete type');
+    assert.strictEqual(person2, person, 'peekRecord returns the same instance for abstract type');
+    assert.strictEqual(store.identifierCache._cache.resources.size, 2, 'identifier cache contains backreferences');
+
+    person.unloadRecord();
+    await settled();
+    assert.strictEqual(store.identifierCache._cache.resources.size, 0, 'identifier cache is empty');
+  });
+});