Error when upgrading from 3.24 to 3.28 when a model has an isValid property

[kategengler]

When upgrading an app from 3.24 to 3.28, I saw an error:

Uncaught TypeError: Cannot read properties of undefined (reading 'isValid')
    at Model.get isValid (vendor.js:99707:32)
    at vendor.js:55315:29
    at track (vendor.js:76227:7)
    at Model.desc.get (vendor.js:55314:40)
    at mergeProps (vendor.js:32534:40)
    at mergeMixins (vendor.js:32496:11)
    at mergeMixins (vendor.js:32498:11)
    at applyMixin (vendor.js:32643:5)
    at Mixin.apply (vendor.js:32889:14)
    at Function.proto (vendor.js:46713:31)
  | eachComputedProperty | @ | vendor.js:46670
  | get relationshipsObject | @ | vendor.js:100840
  | desc.get | @ | vendor.js:99473
  | get relationshipsByName | @ | vendor.js:100825
  | desc.get | @ | vendor.js:99473
  | eachRelationship | @ | vendor.js:100909
  | (anonymous) | @ | vendor.js:128380
  | discoverEmberDataModels | @ | vendor.js:128377

(For anyone searching -- in tests, without No try/catch checked this presented as mirage failing to start).

I tracked this down to an isValid computed property defined in Ember classic syntax on a model.

import Model from '@ember-data/model';
import { computed } from '@ember/object';

export default Model.extend({
  isValid: computed('...
});

Renaming the computed property from isValid to anything else removes the error. I have no problem just renaming, but was surprised that the user-defined isValid interfered with something called by ember-data.

[runspired]

So it appears the issue here is that mirage looks up models and accesses the static properties on them without using the container. This has a number of ill-effects and was never officially supported though it is something we felt we need to explicitly deprecate. The deprecation RFC is here: emberjs/rfcs#741 and this second RFC was also motivated by the sort of bug discovered here. emberjs/rfcs#738

All this to say that I'm not surprised that this lookup results in mergeMixins being called since the proto chain must be realized when a static property or method access occurs, and I am also not surprised that this merging does a read access of isValid during the merge (this is a bug I previously surfaced to @pzuraq)

Prior reading on this and a related bug in Ember:

• Issue with mergeMixins when accessing a service in a getter ember.js#18860
• Overwriting a getter from a mixin calls the original getter ember.js#18829
• Perf: Bugfix: Don't read before write for anything with a descriptor ember.js#19504

BUT! While I'm not surprised at the pathology of this triggering, it does mean we had a blind spot around it being able to be triggered by collisions with the properties that are on Model out of the box. Those properties are intended to be overridable and not being so would even harm the implementation of the Links & Attrs RFC that @mansona is currently working on in #7855

I built this twiddle to show that native class syntax does not have this issue but classic syntax does. This means that folks who hit this bug can resolve it by refactoring to native syntax

import Model, { attr } from '@ember-data/model';

class User extends Model {
  @attr isValid;
}
/*
// replace the class definition with this one to trigger the error
const User = Model.extend({
  isValid: attr(),
});
*/

const attrs = User.attributes;
console.log(attrs);

export default User;

[kategengler]

Since it can also be worked around by changing the attribute name (if not intentionally overriding) and native-class syntax is the way-forward, I'm comfortable closing this issue (but will leave it open for you to do so if you agree).

Thanks for looking into this and writing it up so clearly!