cleanup

packages/-ember-data/tests/integration/identifiers/scenarios-test.ts

@@ -16,6 +16,7 @@ import { ExistingResourceObject } from '@ember-data/store/-private/ts-interfaces
 import { Dict } from '@ember-data/store/-private/ts-interfaces/utils';
 import { StableRecordIdentifier } from '@ember-data/store/-private/ts-interfaces/identifier';
 import { identifierCacheFor } from '@ember-data/store/-private';
+import { set } from '@ember/object';
 
 function isNonEmptyString(str: any): str is string {
   return typeof str === 'string' && str.length > 0;
@@ -301,12 +302,12 @@ if (IDENTIFIERS) {
               // first treat as id
               // then treat as username
               // if still no entry, fallback to generation
-              lid = secondaryCache[resource.id] || `remote:user:${resource.id}`;
+              lid = secondaryCache[resource.id] || `remote:user:${resource.id}:${localIdInc++}`;
             }
 
             // generate from username if still undefined
             if (!lid && isNonEmptyString(username)) {
-              lid = `remote:user:${username}`;
+              lid = `remote:user:${username}:${localIdInc++}`;
             }
 
             // generate at random if still undefined
@@ -581,6 +582,87 @@ if (IDENTIFIERS) {
           `We only have the lid '${identifierByUsername.lid}' in ['${lids.join("', '")}']`
         );
       });
+
+      test(`secondary-key mutation`, async function(assert) {
+        const adapter = store.adapterFor('application');
+        let hasSaved = false;
+
+        adapter.findRecord = (_, __, id) => {
+          if (hasSaved && id === '@runspired') {
+            throw new Error(`No record found for the username @runspired`);
+          }
+          return resolve({
+            data: {
+              type: 'user',
+              id: '1',
+              attributes: {
+                username: hasSaved ? '@cthoburn' : '@runspired',
+              },
+            },
+          });
+        };
+
+        adapter.updateRecord = () => {
+          hasSaved = true;
+          return resolve({
+            data: {
+              type: 'user',
+              id: '1',
+              attributes: {
+                username: '@cthoburn',
+              },
+            },
+          });
+        };
+
+        function updateUsernameCache(identifier, oldUsername, newUsername) {
+          if (secondaryCache[oldUsername] !== identifier.lid) {
+            throw new Error(`Incorrect username update`);
+          }
+          if (secondaryCache[newUsername] && secondaryCache[newUsername] !== identifier.lid) {
+            throw new Error(`Cannot update username to one used elsewhere`);
+          }
+          secondaryCache[newUsername] = identifier.lid;
+        }
+        function commitUsernameUpdate(identifier, oldUsername, newUsername) {
+          if (secondaryCache[oldUsername] === identifier.lid) {
+            delete secondaryCache[oldUsername];
+          }
+        }
+
+        const user = await store.findRecord('user', '@runspired');
+        const identifier = recordIdentifierFor(user);
+        set(user, 'username', '@cthoburn');
+
+        // eagerly update
+        updateUsernameCache(identifier, '@runspired', '@cthoburn');
+
+        const cthoburn = await store.findRecord('user', '@cthoburn');
+        const runspired = await store.findRecord('user', '@runspired');
+        const user1 = await store.findRecord('user', '1');
+
+        assert.strictEqual(cthoburn, user, 'We can find by the new username');
+        assert.strictEqual(runspired, user, 'We can find by the old username');
+        assert.strictEqual(user1, user, 'We can find by the id');
+
+        await user.save();
+
+        // eliminate the old username
+        commitUsernameUpdate(identifier, '@runspired', '@cthoburn');
+
+        const cthoburnAfter = await store.findRecord('user', '@cthoburn');
+        const user1After = await store.findRecord('user', '1');
+
+        assert.strictEqual(cthoburnAfter, user, 'We can find by the new username');
+        assert.strictEqual(user1After, user, 'We can find by the id');
+
+        try {
+          await store.findRecord('user', '@runspired');
+          assert.ok(false, 'Expected an error to be thrown');
+        } catch (e) {
+          assert.strictEqual(e.message, `No record found for the username @runspired`, 'We throw an error');
+        }
+      });
     });
   });
 }

packages/store/addon/-private/identifiers/cache.ts

@@ -31,7 +31,7 @@ export type MergeMethod = (
   identifier: StableRecordIdentifier,
   identifier2: StableRecordIdentifier,
   newData: ResourceIdentifierObject | ExistingResourceObject
-) => StableRecordIdentifier | false;
+) => StableRecordIdentifier;
 
 let configuredForgetMethod: ForgetMethod | null;
 let configuredGenerationMethod: GenerationMethod | null;
@@ -215,8 +215,11 @@ export class IdentifierCache {
         // even for the "successful" secondary lookup
         // by `_generate()`, since we missed the cache
         // previously
-        if (id !== null) {
-          keyOptions.id[id] = identifier;
+        // we use identifier.id instead of id here
+        // because they may not match and we prefer
+        // what we've set via resource data
+        if (identifier.id !== null) {
+          keyOptions.id[identifier.id] = identifier;
 
           // TODO allow filling out of `id` here
           // for the `username` non-client created
@@ -318,18 +321,11 @@ export class IdentifierCache {
 
       if (existingIdentifier !== undefined) {
         let merged = this._merge(identifier, existingIdentifier, data);
-
-        if (!merged) {
-          throw new Error(
-            `Failed to update the 'id' for the RecordIdentifier '${identifier}' to '${newId}', because that id is already in use by '${existingIdentifier}'`
-          );
-        } else {
-          let abandoned = merged === identifier ? existingIdentifier : identifier;
-          identifier = merged;
-          this.forgetRecordIdentifier(abandoned);
-          keyOptions.id[newId] = identifier;
-          data.lid = identifier.lid;
-        }
+        let abandoned = merged === identifier ? existingIdentifier : identifier;
+        identifier = merged;
+        this.forgetRecordIdentifier(abandoned);
+        keyOptions.id[newId] = identifier;
+        data.lid = identifier.lid;
       }
     }
 
@@ -341,6 +337,10 @@ export class IdentifierCache {
     if (id !== newId && newId !== null) {
       let keyOptions = getTypeIndex(this._cache.types, identifier.type);
       keyOptions.id[newId] = identifier;
+
+      if (id !== null) {
+        delete keyOptions.id[id];
+      }
     }
 
     return identifier;
@@ -442,7 +442,6 @@ function performRecordIdentifierUpdate(
     if (lid !== undefined) {
       let newLid = coerceId(lid);
       if (newLid !== identifier.lid) {
-        debugger;
         throw new Error(
           `The 'lid' for a RecordIdentifier cannot be updated once it has been created. Attempted to set lid for '${wrapper}' to '${lid}'.`
         );
@@ -456,7 +455,7 @@ function performRecordIdentifierUpdate(
         // here we warn and ignore, as this may be a mistake, but we allow the user
         // to have multiple cache-keys pointing at a single lid so we cannot error
         warn(
-          `The 'id' for a RecordIdentifier cannot be updated once it has been set. Attempted to set id for '${wrapper}' to '${newId}'.`,
+          `The 'id' for a RecordIdentifier should not be updated once it has been set. Attempted to set id for '${wrapper}' to '${newId}'.`,
           false,
           { id: 'ember-data:multiple-ids-for-identifier' }
         );

packages/store/addon/-private/system/model/internal-model.ts

@@ -1268,10 +1268,6 @@ export default class InternalModel {
   }
 
   setId(id: string) {
-    assert(
-      "A record's id cannot be changed once it is in the loaded state",
-      this._id === null || this._id === id || this.isNew()
-    );
     let didChange = id !== this._id;
 
     this._id = id;

packages/store/addon/-private/system/store.ts

@@ -54,6 +54,8 @@ import {
   CollectionResourceDocument,
   JsonApiDocument,
 } from '../ts-interfaces/ember-data-json-api';
+import hasValidId from '../utils/has-valid-id';
+import { stringify } from 'querystring';
 const badIdFormatAssertion = '`id` passed to `findRecord()` has to be non-empty string or number';
 const emberRun = emberRunLoop.backburner;
 
@@ -738,7 +740,7 @@ class Store extends Service {
     assert(badIdFormatAssertion, (typeof id === 'string' && id.length > 0) || (typeof id === 'number' && !isNaN(id)));
 
     const normalizedModelName = normalizeModelName(modelName);
-    const normalizedId = coerceId(id);
+    const normalizedId = coerceId(id) as string;
     const internalModel = internalModelFactoryFor(this).lookup(normalizedModelName, normalizedId, null);
     options = options || {};
 
@@ -1116,7 +1118,9 @@ class Store extends Service {
     const normalizedModelName = normalizeModelName(modelName);
     const normalizedId = coerceId(id);
 
-    return internalModelFactoryFor(this).lookup(normalizedModelName, normalizedId, null).recordReference;
+    assert(`You must pass a valid id to getReference`, typeof id === 'string' && stringify.length > 0);
+
+    return internalModelFactoryFor(this).lookup(normalizedModelName, normalizedId as string, null).recordReference;
   }
 
   /**
@@ -1156,7 +1160,7 @@ class Store extends Service {
       typeof modelName === 'string'
     );
     let normalizedModelName = normalizeModelName(modelName);
-    const normalizedId = coerceId(id);
+    const normalizedId = coerceId(id) as string;
 
     if (this.hasRecordForId(normalizedModelName, normalizedId)) {
       return internalModelFactoryFor(this)
@@ -1225,7 +1229,10 @@ class Store extends Service {
 
     const normalizedModelName = normalizeModelName(modelName);
     const trueId = coerceId(id);
-    const internalModel = internalModelFactoryFor(this).peekId(normalizedModelName, trueId, null);
+
+    assert(`You must pass a valid id to hasRecordForId`, typeof trueId === 'string' && trueId.length > 0);
+
+    const internalModel = internalModelFactoryFor(this).peekId(normalizedModelName, trueId as string, null);
 
     return !!internalModel && internalModel.isLoaded();
   }
@@ -1250,8 +1257,12 @@ class Store extends Service {
       typeof modelName === 'string'
     );
 
+    const trueId = coerceId(id);
+
+    assert(`You must pass a valid id to recordForId`, typeof trueId === 'string' && trueId.length > 0);
+
     return internalModelFactoryFor(this)
-      .lookup(modelName, coerceId(id), null)
+      .lookup(modelName, trueId as string, null)
       .getRecord();
   }
 

packages/store/addon/-private/system/store/internal-model-factory.ts

@@ -82,7 +82,9 @@ export default class InternalModelFactory {
       // we cannot merge internalModels when both have records
       // (this may not be strictly true, we could probably swap the internalModel the record points at)
       if (im && otherIm && im.hasRecord && otherIm.hasRecord) {
-        return false;
+        throw new Error(
+          `Failed to update the 'id' for the RecordIdentifier '${identifier}' to '${resourceData.id}', because that id is already in use by '${matchedIdentifier}'`
+        );
       }
 
       // remove otherIm from cache

packages/store/addon/-private/system/store/record-data-store-wrapper.ts

@@ -5,6 +5,7 @@ import { BRAND_SYMBOL } from '../../ts-interfaces/utils/brand';
 import { upgradeForInternal } from '../ts-upgrade-map';
 import RecordData from '../../ts-interfaces/record-data';
 import { internalModelFactoryFor } from './internal-model-factory';
+import hasValidId from '../../utils/has-valid-id';
 
 type StringOrNullOrUndefined = string | null | undefined;
 
@@ -53,7 +54,12 @@ export default class RecordDataStoreWrapper implements IRecordDataStoreWrapper {
     });
   }
 
-  notifyErrorsChange(modelName: string, id: string | null, clientId: string | null) {
+  notifyErrorsChange(modelName: string, id: string, clientId: string | null): void;
+  notifyErrorsChange(modelName: string, id: string | null, clientId: string): void;
+  notifyErrorsChange(modelName: string, id: string | null, clientId: string | null): void {
+    if (!hasValidId(id, clientId)) {
+      throw new Error(MISSING_ID_ARG_ERROR_MESSAGE);
+    }
     let internalModel = internalModelFactoryFor(this._store).peekId(modelName, id, clientId);
 
     if (internalModel) {
@@ -76,7 +82,7 @@ export default class RecordDataStoreWrapper implements IRecordDataStoreWrapper {
       let id = pending[i + 1] || null;
       let clientId = pending[i + 2];
       let key = pending[i + 3] as string;
-      let internalModel = factory.peekId(modelName, id, clientId);
+      let internalModel = factory.peekId(modelName, id as string, clientId);
 
       if (internalModel) {
         internalModel.notifyHasManyChange(key);
@@ -141,7 +147,12 @@ export default class RecordDataStoreWrapper implements IRecordDataStoreWrapper {
     }
   }
 
+  notifyStateChange(modelName: string, id: string, clientId: string | null, key?: string): void;
+  notifyStateChange(modelName: string, id: string | null, clientId: string, key?: string): void;
   notifyStateChange(modelName: string, id: string | null, clientId: string | null, key?: string): void {
+    if (!hasValidId(id, clientId)) {
+      throw new Error(MISSING_ID_ARG_ERROR_MESSAGE);
+    }
     let internalModel = internalModelFactoryFor(this._store).peekId(modelName, id, clientId);
 
     if (internalModel) {
@@ -190,11 +201,3 @@ export default class RecordDataStoreWrapper implements IRecordDataStoreWrapper {
     }
   }
 }
-
-function hasValidId(id?: string | null, clientId?: string | null): id is string {
-  // weed out anything falsey
-  if (!id && !clientId) {
-    return false;
-  }
-  return true;
-}

packages/store/addon/-private/utils/has-valid-id.ts

@@ -0,0 +1,7 @@
+export default function hasValidId(id?: string | null, clientId?: string | null): id is string {
+  // weed out anything falsey
+  if (!id && !clientId) {
+    return false;
+  }
+  return true;
+}