fix: backport various fixes to 5.3 (#9225)

* fix: JSONAPISerializer should not reify empty records (#8934)

* fix: JSONAPISerializer should not reify empty records

* fix lint

* fix: url configuration should respect / for host and error more meaningfully when invalid (#9164)

* fix: url configuration should respect / for host and error more meaningfully when invalid

* fix prettier

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

* fix: keep a backreference for previously merged identifiers

* update tests

Author: runspired

.vscode/settings.json

@@ -0,0 +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"
+  },
+  "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/legacy-compat/src/legacy-network-handler/snapshot.ts

@@ -148,8 +148,13 @@ export default class Snapshot implements Snapshot {
    @type {Model}
    @public
    */
-  get record(): RecordInstance {
-    return this._store._instanceCache.getRecord(this.identifier);
+  get record(): RecordInstance | null {
+    const record = this._store.peekRecord(this.identifier);
+    assert(
+      `Record ${this.identifier.type} ${this.identifier.id} (${this.identifier.lid}) is not yet loaded and thus cannot be accessed from the Snapshot during serialization`,
+      record !== null
+    );
+    return record;
   }
 
   get _attributes(): Record<string, unknown> {

packages/request-utils/src/index.ts

@@ -52,7 +52,7 @@ export interface BuildURLConfig {
   namespace: string | null;
 }
 
-let CONFIG: BuildURLConfig = {
+const CONFIG: BuildURLConfig = {
   host: '',
   namespace: '',
 };
@@ -64,7 +64,13 @@ let CONFIG: BuildURLConfig = {
  * These values may still be overridden by passing
  * them to buildBaseURL directly.
  *
- * This method may be called as many times as needed
+ * This method may be called as many times as needed.
+ * host values of `''` or `'/'` are equivalent.
+ *
+ * Except for the value of `/` as host, host should not
+ * end with `/`.
+ *
+ * namespace should not start or end with a `/`.
  *
  * ```ts
  * type BuildURLConfig = {
@@ -73,6 +79,17 @@ let CONFIG: BuildURLConfig = {
  * }
  * ```
  *
+ * Example:
+ *
+ * ```ts
+ * import { setBuildURLConfig } from '@ember-data/request-utils';
+ *
+ * setBuildURLConfig({
+ *   host: 'https://api.example.com',
+ *   namespace: 'api/v1'
+ * });
+ * ```
+ *
  * @method setBuildURLConfig
  * @static
  * @public
@@ -81,7 +98,27 @@ let CONFIG: BuildURLConfig = {
  * @returns void
  */
 export function setBuildURLConfig(config: BuildURLConfig) {
-  CONFIG = config;
+  assert(`setBuildURLConfig: You must pass a config object`, config);
+  assert(
+    `setBuildURLConfig: You must pass a config object with a 'host' or 'namespace' property`,
+    'host' in config || 'namespace' in config
+  );
+
+  CONFIG.host = config.host || '';
+  CONFIG.namespace = config.namespace || '';
+
+  assert(
+    `buildBaseURL: host must NOT end with '/', received '${CONFIG.host}'`,
+    CONFIG.host === '/' || !CONFIG.host.endsWith('/')
+  );
+  assert(
+    `buildBaseURL: namespace must NOT start with '/', received '${CONFIG.namespace}'`,
+    !CONFIG.namespace.startsWith('/')
+  );
+  assert(
+    `buildBaseURL: namespace must NOT end with '/', received '${CONFIG.namespace}'`,
+    !CONFIG.namespace.endsWith('/')
+  );
 }
 
 export interface FindRecordUrlOptions {
@@ -312,8 +349,9 @@ export function buildBaseURL(urlOptions: UrlOptions): string {
   assert(`buildBaseURL: idPath must NOT start with '/', received '${idPath}'`, !idPath.startsWith('/'));
   assert(`buildBaseURL: idPath must NOT end with '/', received '${idPath}'`, !idPath.endsWith('/'));
 
-  const url = [host === '/' ? '' : host, namespace, resourcePath, idPath, fieldPath].filter(Boolean).join('/');
-  return host ? url : `/${url}`;
+  const hasHost = host !== '' && host !== '/';
+  const url = [hasHost ? host : '', namespace, resourcePath, idPath, fieldPath].filter(Boolean).join('/');
+  return hasHost ? url : `/${url}`;
 }
 
 type SerializablePrimitive = string | number | boolean | null;

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

@@ -383,6 +383,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');
       }
@@ -397,7 +401,7 @@ module('Integration | Identifiers - configuration', function (hooks) {
 
     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) => {
@@ -433,7 +437,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(
@@ -452,6 +460,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

@@ -472,12 +472,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');
+  });
+});