Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX] Fix availability of properties in createRecord init #5413

Merged
merged 12 commits into from
Apr 4, 2018
Merged
3 changes: 3 additions & 0 deletions addon/-private/system/many-array.js
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,9 @@ export default EmberObject.extend(MutableArray, Evented, {
},

objectAt(index) {
if (this.relationship._willUpdateManyArray) {
this.relationship._flushPendingManyArrayUpdates();
}
let internalModel = this.currentState[index];
if (internalModel === undefined) { return; }

Expand Down
106 changes: 105 additions & 1 deletion addon/-private/system/model/internal-model.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { assign, merge } from '@ember/polyfills';
import { A } from '@ember/array';
import { set, get } from '@ember/object';
import { copy } from '@ember/object/internals';
import EmberError from '@ember/error';
Expand All @@ -13,6 +14,7 @@ import RootState from "./states";
import Relationships from "../relationships/state/create";
import Snapshot from "../snapshot";
import OrderedSet from "../ordered-set";
import isArrayLike from "../is-array-like";

import { getOwner } from '../../utils';

Expand Down Expand Up @@ -334,7 +336,7 @@ export default class InternalModel {
return this.currentState.dirtyType;
}

getRecord() {
getRecord(properties) {
if (!this._record && !this._isDematerializing) {
heimdall.increment(materializeRecord);
let token = heimdall.start('InternalModel.getRecord');
Expand All @@ -349,6 +351,40 @@ export default class InternalModel {
adapterError: this.error
};

if (properties !== undefined) {
assert(`You passed '${properties}' as properties for record creation instead of an object.`, typeof properties === 'object' && properties !== null);
let classFields = this.getFields();
let relationships = this._relationships;
let propertyNames = Object.keys(properties);

for (let i = 0; i < propertyNames.length; i++) {
let name = propertyNames[i];
let fieldType = classFields.get(name);
let propertyValue = properties[name];

if (name === 'id') {
this.setId(propertyValue);
continue;
}

switch (fieldType) {
case 'attribute':
this.setDirtyAttribute(name, propertyValue);
break;
case 'belongsTo':
this.setDirtyBelongsTo(name, propertyValue);
relationships.get(name).setHasData(true);
break;
case 'hasMany':
this.setDirtyHasMany(name, propertyValue);
relationships.get(name).setHasData(true);
break;
default:
createOptions[name] = propertyValue;
}
}
}

if (setOwner) {
// ensure that `getOwner(this)` works inside a model instance
setOwner(createOptions, getOwner(this.store));
Expand All @@ -365,6 +401,10 @@ export default class InternalModel {
return this._record;
}

getFields() {
return get(this.modelClass, 'fields');
}

resetRecord() {
this._record = null;
this.isReloading = false;
Expand Down Expand Up @@ -602,6 +642,70 @@ export default class InternalModel {
}
}

getAttributeValue(key) {
if (key in this._attributes) {
return this._attributes[key];
} else if (key in this._inFlightAttributes) {
return this._inFlightAttributes[key];
} else {
return this._data[key];
}
}

setDirtyHasMany(key, records) {
assert(`You must pass an array of records to set a hasMany relationship`, isArrayLike(records));
assert(`All elements of a hasMany relationship must be instances of DS.Model, you passed ${inspect(records)}`, (function() {
return A(records).every((record) => record.hasOwnProperty('_internalModel') === true);
})());

let relationship = this._relationships.get(key);
relationship.clear();
relationship.addInternalModels(records.map(record => get(record, '_internalModel')));
}

setDirtyBelongsTo(key, value) {
if (value === undefined) {
value = null;
}
if (value && value.then) {
this._relationships.get(key).setRecordPromise(value);
} else if (value) {
this._relationships.get(key).setInternalModel(value._internalModel);
} else {
this._relationships.get(key).setInternalModel(value);
}
}

setDirtyAttribute(key, value) {
if (this.isDeleted()) {
throw new EmberError(`Attempted to set '${key}' to '${value}' on the deleted record ${this}`);
}

let oldValue = this.getAttributeValue(key);
let originalValue;

if (value !== oldValue) {
// Add the new value to the changed attributes hash; it will get deleted by
// the 'didSetProperty' handler if it is no different from the original value
this._attributes[key] = value;

if (key in this._inFlightAttributes) {
originalValue = this._inFlightAttributes[key];
} else {
originalValue = this._data[key];
}

this.send('didSetProperty', {
name: key,
oldValue: oldValue,
originalValue: originalValue,
value: value
});
}

return value;
}

get isDestroyed() {
return this._isDestroyed;
}
Expand Down
11 changes: 1 addition & 10 deletions addon/-private/system/relationships/belongs-to.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,16 +118,7 @@ export default function belongsTo(modelName, options) {
return this._internalModel._relationships.get(key).getRecord();
},
set(key, value) {
if (value === undefined) {
value = null;
}
if (value && value.then) {
this._internalModel._relationships.get(key).setRecordPromise(value);
} else if (value) {
this._internalModel._relationships.get(key).setInternalModel(value._internalModel);
} else {
this._internalModel._relationships.get(key).setInternalModel(value);
}
this._internalModel.setDirtyBelongsTo(key, value);

return this._internalModel._relationships.get(key).getRecord();
}
Expand Down
18 changes: 4 additions & 14 deletions addon/-private/system/relationships/has-many.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
/**
@module ember-data
*/

import { A } from '@ember/array';

import { get, computed } from '@ember/object';
import { computed } from '@ember/object';
import { assert, inspect } from '@ember/debug';
import normalizeModelName from "../normalize-model-name";
import isArrayLike from "../is-array-like";

/**
`DS.hasMany` is used to define One-To-Many and Many-To-Many
Expand Down Expand Up @@ -147,15 +143,9 @@ export default function hasMany(type, options) {
return this._internalModel._relationships.get(key).getRecords();
},
set(key, records) {
assert(`You must pass an array of records to set a hasMany relationship`, isArrayLike(records));
assert(`All elements of a hasMany relationship must be instances of DS.Model, you passed ${inspect(records)}`, (function() {
return A(records).every((record) => record.hasOwnProperty('_internalModel') === true);
})());

let relationship = this._internalModel._relationships.get(key);
relationship.clear();
relationship.addInternalModels(records.map(record => get(record, '_internalModel')));
return relationship.getRecords();
this._internalModel.setDirtyHasMany(key, records);

return this._internalModel._relationships.get(key).getRecords();
}
}).meta(meta);
}
44 changes: 42 additions & 2 deletions addon/-private/system/relationships/state/has-many.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ export default class ManyRelationship extends Relationship {
// inverse internal models are unloaded.
this._retainedManyArray = null;
this.__loadingPromise = null;
this._willUpdateManyArray = false;
this._pendingManyArrayUpdates = null;
}

get _loadingPromise() { return this.__loadingPromise; }
Expand Down Expand Up @@ -109,8 +111,46 @@ export default class ManyRelationship extends Relationship {

assertPolymorphicType(this.internalModel, this.relationshipMeta, internalModel);
super.addInternalModel(internalModel, idx);
// make lazy later
this.manyArray._addInternalModels([internalModel], idx);
this.scheduleManyArrayUpdate(internalModel, idx);
}

scheduleManyArrayUpdate(internalModel, idx) {
// ideally we would early exit here, but some tests
// currently suggest that we cannot.
// if (!this._manyArray) {
// return;
// }

let pending = this._pendingManyArrayUpdates = this._pendingManyArrayUpdates || [];
pending.push(internalModel, idx);

if (this._willUpdateManyArray === true) {
return;
}

this._willUpdateManyArray = true;
let backburner = this.store._backburner;

backburner.join(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, it would be nice to remove this (and rely on the auto-run). What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that too many of our APIs are sync, so we can't rely on setTimeout (or microtasks if backburner changes) for settledness at the moment. We need the work to complete by the time someone exits the ember-data world.

This is something we would like to change, and a longterm goal of ours is to move more public APIs to being promise based to unlock optimizations around batching and updating without needing this sort of wonkiness.

backburner.schedule('syncRelationships', this, this._flushPendingManyArrayUpdates);
});
}

_flushPendingManyArrayUpdates() {
if (this._willUpdateManyArray === false) {
return;
}

let pending = this._pendingManyArrayUpdates;
this._pendingManyArrayUpdates = [];
this._willUpdateManyArray = false;

for (let i = 0; i < pending.length; i += 2) {
let internalModel = pending[i];
let idx = pending[i + 1];

this.manyArray._addInternalModels([internalModel], idx);
}
}

removeCanonicalInternalModelFromOwn(internalModel, idx) {
Expand Down
43 changes: 24 additions & 19 deletions addon/-private/system/snapshot.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,25 +24,13 @@ export default class Snapshot {
this._hasManyIds = Object.create(null);
this._internalModel = internalModel;

let record = internalModel.getRecord();
// TODO is there a way we can assign known attributes without
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make an issue so we can track fixing (or removing the TODO)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i left the skipped test for tracking :D
i can open an issue too though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue is #5419

// using `eachAttribute`? This forces us to lookup the model-class
// but for findRecord / findAll these are empty and doing so at
// this point in time is unnecessary.
internalModel.eachAttribute((keyName) => this._attributes[keyName] = internalModel.getAttributeValue(keyName));
Copy link
Member

@pangratz pangratz Apr 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this broke defaultValue not being considered anymore, when the attribute hasn't been accessed yet 😔

So given:

// app/models/foo.js
export default Model.extend({
  bar: attr({ defaultValue: () => "baz" })
});

then

let foo = store.createRecord('foo');

// this assertion breaks after upgrading to v3.2.0-beta.2 :pensive:
assert.deepEqual(foo.toJSON(), { bar: "baz" });

Currently the whole story around defaultValue is not ideal (e.g. #2566) and doing this in the app code by explicitly passing default values when creating a record is better, but I would still consider this a breaking change... What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to open an issue for this, I think it should be fixable...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be very fixable, surprised this broke though, re-examining my changes, will open an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hrm, so I didn't pull the defaultValue logic into internalModel, nor am I sure we should. https://github.com/emberjs/data/pull/5413/files#diff-c2a90350f555423721228dfa13c137f2R130

That said, the proposal in #5419 would be a good direction to address this, as we can access each attribute via the record if it exists, and only copy lazily if it does not.


/**
The underlying record for this snapshot. Can be used to access methods and
properties defined on the record.

Example

```javascript
let json = snapshot.record.toJSON();
```

@property record
@type {DS.Model}
*/
this.record = record;
record.eachAttribute((keyName) => this._attributes[keyName] = get(record, keyName));

/**
/**O
The id of the snapshot's underlying record

Example
Expand Down Expand Up @@ -73,7 +61,24 @@ export default class Snapshot {
*/
this.modelName = internalModel.modelName;

this._changedAttributes = record.changedAttributes();
this._changedAttributes = internalModel.changedAttributes();
}

/**
The underlying record for this snapshot. Can be used to access methods and
properties defined on the record.

Example

```javascript
let json = snapshot.record.toJSON();
```

@property record
@type {DS.Model}
*/
get record() {
return this._internalModel.getRecord();
}

/**
Expand Down
37 changes: 15 additions & 22 deletions addon/-private/system/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -339,34 +339,27 @@ Store = Service.extend({
createRecord(modelName, inputProperties) {
assert(`You need to pass a model name to the store's createRecord method`, isPresent(modelName));
assert(`Passing classes to store methods has been removed. Please pass a dasherized string instead of ${modelName}`, typeof modelName === 'string');
let normalizedModelName = normalizeModelName(modelName);
let properties = copy(inputProperties) || Object.create(null);

// If the passed properties do not include a primary key,
// give the adapter an opportunity to generate one. Typically,
// client-side ID generators will use something like uuid.js
// to avoid conflicts.
return this._backburner.join(() => {
let normalizedModelName = normalizeModelName(modelName);
let properties = copy(inputProperties) || Object.create(null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just notating that we will eventially need to deal with the Ember.copy deprecation that is being worked on in emberjs/rfcs#322. Nothing needs to be done now though (AFAICT)...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, we know. we use it a ton :/


if (isNone(properties.id)) {
properties.id = this._generateId(normalizedModelName, properties);
}
// If the passed properties do not include a primary key,
// give the adapter an opportunity to generate one. Typically,
// client-side ID generators will use something like uuid.js
// to avoid conflicts.

// Coerce ID to a string
properties.id = coerceId(properties.id);
if (isNone(properties.id)) {
properties.id = this._generateId(normalizedModelName, properties);
}

let internalModel = this._buildInternalModel(normalizedModelName, properties.id);
internalModel.loadedData();
let record = internalModel.getRecord();
record.setProperties(properties);
// Coerce ID to a string
properties.id = coerceId(properties.id);

// TODO @runspired this should also be coalesced into some form of internalModel.setState()
internalModel.eachRelationship((key, descriptor) => {
if (properties[key] !== undefined) {
internalModel._relationships.get(key).setHasData(true);
}
let internalModel = this._buildInternalModel(normalizedModelName, properties.id);
internalModel.loadedData();
return internalModel.getRecord(properties);
});

return record;
},

/**
Expand Down
Loading