fix: Don't notify changes for attributes not registered with the schema (emberjs#9698)

Author: gitKrystan

packages/json-api/src/-private/cache.ts

@@ -656,10 +656,12 @@ export default class JSONAPICache implements Cache {
       this._capabilities.notifyChange(identifier, 'state', null);
     }
 
+    const fields = this._capabilities.schema.fields(identifier);
+
     // if no cache entry existed, no record exists / property has been accessed
     // and thus we do not need to notify changes to any properties.
     if (calculateChanges && existed && data.attributes) {
-      changedKeys = calculateChangedKeys(cached, data.attributes);
+      changedKeys = calculateChangedKeys(cached, data.attributes, fields);
     }
 
     cached.remoteAttrs = Object.assign(
@@ -682,7 +684,7 @@ export default class JSONAPICache implements Cache {
     }
 
     if (data.relationships) {
-      setupRelationships(this.__graph, this._capabilities, identifier, data);
+      setupRelationships(this.__graph, fields, identifier, data);
     }
 
     if (changedKeys?.size) {
@@ -1007,6 +1009,7 @@ export default class JSONAPICache implements Cache {
       }
     }
 
+    const fields = this._capabilities.schema.fields(identifier);
     cached.isNew = false;
     let newCanonicalAttributes: ExistingResourceObject['attributes'];
     if (data) {
@@ -1027,7 +1030,6 @@ export default class JSONAPICache implements Cache {
           if (!DEPRECATE_RELATIONSHIP_REMOTE_UPDATE_CLEARING_LOCAL_STATE) {
             // assert against bad API behavior where a belongsTo relationship
             // is saved but the return payload indicates a different final state.
-            const fields = this._capabilities.schema.fields(identifier);
             fields.forEach((field, name) => {
               if (field.kind === 'belongsTo') {
                 const relationshipData = data.relationships![name]?.data;
@@ -1053,11 +1055,11 @@ export default class JSONAPICache implements Cache {
             cached.inflightRelationships = null;
           }
         }
-        setupRelationships(this.__graph, this._capabilities, identifier, data);
+        setupRelationships(this.__graph, fields, identifier, data);
       }
       newCanonicalAttributes = data.attributes;
     }
-    const changedKeys = newCanonicalAttributes && calculateChangedKeys(cached, newCanonicalAttributes);
+    const changedKeys = newCanonicalAttributes && calculateChangedKeys(cached, newCanonicalAttributes, fields);
 
     cached.remoteAttrs = Object.assign(
       cached.remoteAttrs || (Object.create(null) as Record<string, unknown>),
@@ -1872,7 +1874,8 @@ function notifyAttributes(
   */
 function calculateChangedKeys(
   cached: CachedResource,
-  updates: Exclude<ExistingResourceObject['attributes'], undefined>
+  updates: Exclude<ExistingResourceObject['attributes'], undefined>,
+  fields: ReturnType<Store['schema']['fields']>
 ): Set<string> {
   const changedKeys = new Set<string>();
   const keys = Object.keys(updates);
@@ -1887,6 +1890,10 @@ function calculateChangedKeys(
 
   for (let i = 0; i < length; i++) {
     const key = keys[i];
+    if (!fields.has(key)) {
+      continue;
+    }
+
     const value = updates[key];
 
     // A value in localAttrs means the user has a local change to
@@ -1962,15 +1969,14 @@ function _isLoading(
 
 function setupRelationships(
   graph: Graph,
-  capabilities: CacheCapabilitiesManager,
+  fields: ReturnType<Store['schema']['fields']>,
   identifier: StableRecordIdentifier,
   data: ExistingResourceObject
 ) {
   // TODO @runspired iterating by definitions instead of by payload keys
   // allows relationship payloads to be ignored silently if no relationship
   // definition exists. Ensure there's a test for this and then consider
   // moving this to an assertion. This check should possibly live in the graph.
-  const fields = capabilities.schema.fields(identifier);
   for (const [name, field] of fields) {
     if (!isRelationship(field)) continue;
 

packages/model/src/-private/model.ts

@@ -1738,7 +1738,7 @@ class Model extends EmberObject implements MinimalLegacyRecord {
     this.eachComputedProperty((name, meta) => {
       if (isAttributeSchema(meta)) {
         assert(
-          "You may not set `id` as an attribute on your model. Please remove any lines that look like: `id: attr('<type>')` from " +
+          "You may not set 'id' as an attribute on your model. Please remove any lines that look like: `id: attr('<type>')` from " +
             this.toString(),
           name !== 'id'
         );

tests/main/eslint.config.mjs

@@ -23,6 +23,7 @@ const AllowedImports = [
   '@ember/test-helpers',
   '@ember/test-waiters',
   '@glimmer/component',
+  '@glimmer/tracking',
   'qunit',
 ];
 

tests/main/tests/acceptance/relationships/tracking-record-state-test.js

@@ -3,7 +3,6 @@ import { action } from '@ember/object';
 import { inject } from '@ember/service';
 import { click, findAll, render } from '@ember/test-helpers';
 import Component from '@glimmer/component';
-// eslint-disable-next-line no-restricted-imports
 import { tracked } from '@glimmer/tracking';
 
 import { module, test } from 'qunit';

tests/main/tests/acceptance/tracking-create-record-test.js

@@ -2,7 +2,6 @@ import { setComponentTemplate } from '@ember/component';
 import { inject as service } from '@ember/service';
 import { render, settled } from '@ember/test-helpers';
 import Component from '@glimmer/component';
-// eslint-disable-next-line no-restricted-imports
 import { tracked } from '@glimmer/tracking';
 
 import { module, test } from 'qunit';

tests/main/tests/helpers/create-tracking-context.gjs

@@ -1,6 +1,5 @@
 import { render, settled } from '@ember/test-helpers';
 import Component from '@glimmer/component';
-// eslint-disable-next-line no-restricted-imports
 import { tracked } from '@glimmer/tracking';
 
 // eslint-disable-next-line @typescript-eslint/require-await

tests/main/tests/integration/cache/json-api-cache-2-test.ts

@@ -0,0 +1,70 @@
+import { settled } from '@ember/test-helpers';
+import { tracked } from '@glimmer/tracking';
+
+import { module, test } from 'qunit';
+
+import { setupRenderingTest } from 'ember-qunit';
+
+import Model, { attr } from '@ember-data/model';
+import type Store from '@ember-data/store';
+import { Type } from '@warp-drive/core-types/symbols';
+
+import { reactiveContext } from '../../helpers/reactive-context';
+
+class User extends Model {
+  @attr declare name: string;
+
+  // Not an attr, so shouldn't notify if a similarly named attribute changes.
+  @tracked brotherName: string | undefined;
+
+  declare [Type]: 'user';
+}
+
+module('@ember-data/json-api | Cache (2)', function (hooks) {
+  setupRenderingTest(hooks);
+
+  hooks.beforeEach(function () {
+    this.owner.register('model:user', User);
+  });
+
+  test('Does not notify for attributes not in schema', async function (assert) {
+    const store = this.owner.lookup('service:store') as Store;
+    const user: User = store.push({
+      data: {
+        id: '1',
+        type: 'user',
+        attributes: {
+          name: 'Aino',
+        },
+      },
+    });
+
+    const rc = await reactiveContext.call(this, user, {
+      type: 'user',
+      identity: null,
+      fields: [
+        { name: 'name', kind: 'field' },
+        { name: 'brotherName', kind: 'field' },
+      ],
+    });
+
+    const { counters } = rc;
+
+    assert.deepEqual(counters, { name: 1, brotherName: 1 }, 'Test setup: counters initialized');
+
+    store.push({
+      data: {
+        id: '1',
+        type: 'user',
+        attributes: {
+          name: 'Aino',
+          brotherName: 'Ellis',
+        },
+      },
+    });
+
+    await settled();
+
+    assert.deepEqual(counters, { name: 1, brotherName: 1 }, 'brotherName did not notify');
+  });
+});

tests/main/tests/unit/model-test.js

@@ -210,7 +210,7 @@ module('unit/model - Model', function (hooks) {
       assert.expectAssertion(() => {
         const ModelClass = store.modelFor('test-model');
         get(ModelClass, 'attributes');
-      }, /You may not set `id` as an attribute on your model/);
+      }, /You may not set 'id' as an attribute on your model/);
 
       assert.expectAssertion(() => {
         store.push({

tests/main/tests/unit/store/push-test.js

@@ -527,24 +527,6 @@ module('unit/store/push - Store#push', function (hooks) {
     assert.notOk(store._instanceCache.peek(pushResult, { bucket: 'record' }), 'record is not materialized');
   });
 
-  test('_push does not require a modelName to resolve to a modelClass', function (assert) {
-    const store = this.owner.lookup('service:store');
-    const originalCall = store.modelFor;
-    store.modelFor = function () {
-      assert.notOk('modelFor was triggered as a result of a call to store._push');
-    };
-
-    store._push({
-      data: {
-        id: '1',
-        type: 'person',
-      },
-    });
-
-    store.modelFor = originalCall;
-    assert.ok('We made it');
-  });
-
   test('_push returns an array of identifiers if an array is pushed', function (assert) {
     const store = this.owner.lookup('service:store');
     const pushResult = store._push({