Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix issues with unregistering nested records. #863

Closed
wants to merge 6 commits into from

3 participants

Greg Fairbanks Evin Grano Tyler Keating
Greg Fairbanks
Collaborator

After upgrading to 1.9, we started having issues with the datastore and nested records. I was able to trace this back to commits 0b5b2db and 62d599a. I've made some changes here that appear to improve the situation , but I haven't completely fixed the problem and haven't been having much luck finding the problem.

I made a gist (https://gist.github.com/4004203) with a small app that exhibits the problem.

I'd like to get some insight into what might be causing the remaining issues.

Greg Fairbanks fairbanksg Fix issues with unregistering nested records.
There were a couple of issues with unregistering nested records:

1) SC.ChildArray did not unregister nested records at all, so toMany
relations with nested records did not work properly.
2) When a child was unregistered from its parent, that did not propagate
to children of the child, leading to problems when nesting was several
levels deep.
3) There were two additional caches that were not being cleared when the
nested record was unregistered.
5821de2
Evin Grano
Owner
fairbanksg added some commits
Greg Fairbanks fairbanksg Fix calculations for (un)registering ChildArray records.
The previous commit incorrectly calculated which records to unregister
and register. All items after the passed in index need to be unregistered,
then all the new items should be registered, followed by the existing items
that were previously unregistered.
8b108d1
Greg Fairbanks fairbanksg Fix drag and drop handling for nested records.
If the item that is dragged is a nested record, special handling is needed
because the data hash will be removed when the record is unregistered. We
need to read the data hash so it doesn't get lost.
9c3c524
Tyler Keating

Hi Greg, I sort of see what is happening. Your example code (thanks it helps a lot!) is creating App.BottomLevel and App.MidLevel as SC.Records and then adding them to the nested record relationships. I haven't figured it out entirely, but since nested records are really just convenience wrappers around Objects, I think you should be creating them as such.

var midlevel = { bProperty: 'BBB' };
var bottomlevel = { cProperty: 'CCC' };
midlevel.bottomLevel = bottomlevel;

Instead of:

var midlevel = App.store.createRecord(App.MidLevel, { bProperty: 'BBB' });
var bottomlevel = App.store.createRecord(App.BottomLevel, { cProperty: 'CCC' });
midlevel.set('bottomLevel', bottomlevel);

Doing it that way solves one problem that I found, which is that attempting to get the nested record property ends up creating a new Record with a default internal id, because the previously assigned manually created record doesn't have an id. You can see this using the gist code, because there are 4 MidLevel records in the store on launch instead of two. Two of which don't have ids (manually created) and two that do (automatically created and used).

But it'll take more time to figure out why removing a midlevel object doesn't remove it from the store, which is why adding a new midlevel object doesn't seem to work correctly (it generates an internal id, finds that a record with a matching id is in the store and uses it).

I'll keep looking at it when I can. I'm also unsure of what the proper CRUD for nested records really should be. Actually, now that I think of it, the way you are creating them should work...

Tyler Keating

Oops.. I wrote the above thinking I was replying on an issue and forgot about your proposed changes. I will look at how that solves the problem.

Tyler Keating

As you've probably seen, it looks like a refactor of nested records which should alleviate all these weird behaviours is in the works. While that is in progress, I thought I would pull in your commits in this branch (except maybe the CollectionView patch), but when I ran the full suite of datastore unit tests, there were a number that failed. I didn't look at them, but are you able to figure out what is happening? I can't bring this in unless all the tests (save 1 that I know is failing currently)

About the CollectionView patch, it's so special case and I'm really worried about it getting forgotten inside the view. I wonder if we can't manage without it or separate it out more so that those who need it can find it.

Thanks.

fairbanksg added some commits
Greg Fairbanks fairbanksg Further changes to unregister child records when a record is unloaded. 85fe1fd
Greg Fairbanks fairbanksg Propagate status changes to child records.
Since nested records are now being properly unregistered, the status of
nested records was getting stale, leading to exceptions because the status
was still set to SC.Record.EMPTY.
c5b4462
Greg Fairbanks fairbanksg Modified unregisterChildFromParent so it can safely be called on any
storeKey, even if it is not a child record.
020a5f8
Tyler Keating publickeating closed this pull request from a commit
Tyler Keating publickeating Closes #863 Fixes merge conflicts and removes extra cycles of calling…
… `unloadRecord` on children and `unregisterChildFromParent` on the parent.
0df9cdd
Tyler Keating

Rebased this onto master and worked through the conflicts. I made some minor changes to the functionality of unloadRecord, which should work better. The only piece I didn't carry over was the piece in SC.CollectionView. I'm not sure about that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 30, 2012
  1. Greg Fairbanks

    Fix issues with unregistering nested records.

    fairbanksg authored
    There were a couple of issues with unregistering nested records:
    
    1) SC.ChildArray did not unregister nested records at all, so toMany
    relations with nested records did not work properly.
    2) When a child was unregistered from its parent, that did not propagate
    to children of the child, leading to problems when nesting was several
    levels deep.
    3) There were two additional caches that were not being cleared when the
    nested record was unregistered.
Commits on Nov 19, 2012
  1. Greg Fairbanks

    Fix calculations for (un)registering ChildArray records.

    fairbanksg authored
    The previous commit incorrectly calculated which records to unregister
    and register. All items after the passed in index need to be unregistered,
    then all the new items should be registered, followed by the existing items
    that were previously unregistered.
  2. Greg Fairbanks

    Fix drag and drop handling for nested records.

    fairbanksg authored
    If the item that is dragged is a nested record, special handling is needed
    because the data hash will be removed when the record is unregistered. We
    need to read the data hash so it doesn't get lost.
Commits on Dec 20, 2012
  1. Greg Fairbanks
Commits on Jan 7, 2013
  1. Greg Fairbanks

    Propagate status changes to child records.

    fairbanksg authored
    Since nested records are now being properly unregistered, the status of
    nested records was getting stale, leading to exceptions because the status
    was still set to SC.Record.EMPTY.
Commits on Jan 9, 2013
  1. Greg Fairbanks

    Modified unregisterChildFromParent so it can safely be called on any

    fairbanksg authored
    storeKey, even if it is not a child record.
This page is out of date. Refresh to see the latest.
2  frameworks/datastore/models/record.js
View
@@ -965,7 +965,7 @@ SC.Record = SC.Object.extend(
// that we don't keep making new storeKeys for the same child record each
// time that it is reloaded.
id = hash[recordType.prototype.primaryKey];
- if (!id) this.generateIdForChild(cr);
+ if (!id) { id = this.generateIdForChild(cr); }
if (!id) { id = psk + '.' + path; }
// If there is an id, there may also be a storeKey. If so, update the
44 frameworks/datastore/system/child_array.js
View
@@ -167,9 +167,22 @@ SC.ChildArray = SC.Object.extend(SC.Enumerable, SC.Array,
record = this.get('record'), newRecs,
pname = this.get('propertyName'),
- cr, recordType;
-
+ cr, recordType, i;
+
newRecs = this._processRecordsToHashes(recs);
+
+ for (i = idx; i < children.length; ++i) {
+ this.unregisterNestedRecord(i);
+ }
+
+ for (i = 0; i < len; ++i) {
+ record.registerNestedRecord(newRecs[i], pname, pname + '.' + (idx + i));
+ }
+
+ for (i = 0; i < children.length - idx - amt; ++i) {
+ record.registerNestedRecord(children[idx + amt + i], pname, pname + '.' + (idx + len + i));
+ }
+
// notify that the record did change...
if (newRecs !== this._prevChildren){
this._performRecordPropertyChange(null, false);
@@ -202,6 +215,33 @@ SC.ChildArray = SC.Object.extend(SC.Enumerable, SC.Array,
},
/**
+ Unregisters a child record from its parent record.
+
+ Since accessing a child (nested) record creates a new data hash for the
+ child and caches the child record and its relationship to the parent record,
+ it's important to clear those caches when the child record is overwritten
+ or removed. This function tells the store to remove the child record from
+ the store's various child record caches.
+
+ You should not need to call this function directly. Simply setting the
+ child record property on the parent to a different value will cause the
+ previous child record to be unregistered.
+
+ @param {Number} idx The index of the child record.
+ */
+ unregisterNestedRecord: function(idx) {
+ var childArray, childRecord, csk, store,
+ record = this.get('record'),
+ pname = this.get('propertyName');
+
+ store = record.get('store');
+ childArray = record.getPath(pname);
+ childRecord = childArray.objectAt(idx);
+ csk = childRecord.get('storeKey');
+ store.unregisterChildFromParent(csk);
+ },
+
+ /**
Calls normalize on each object in the array
*/
normalize: function(){
50 frameworks/datastore/system/store.js
View
@@ -467,9 +467,15 @@ SC.Store = SC.Object.extend( /** @scope SC.Store.prototype */ {
@returns {SC.Store} receiver
*/
writeStatus: function(storeKey, newStatus) {
+ var that = this,
+ ret;
// use writeDataHash for now to handle optimistic lock. maximize code
// reuse.
- return this.writeDataHash(storeKey, null, newStatus);
+ ret = this.writeDataHash(storeKey, null, newStatus);
+ this._propagateToChildren(storeKey, function(storeKey) {
+ that.writeStatus(storeKey, newStatus);
+ });
+ return ret;
},
/**
@@ -673,6 +679,10 @@ SC.Store = SC.Object.extend( /** @scope SC.Store.prototype */ {
}
}
+ this.records = {};
+ this.childRecords = {};
+ this.parentRecords = {};
+
this.set('hasChanges', NO);
},
@@ -1264,6 +1274,8 @@ SC.Store = SC.Object.extend( /** @scope SC.Store.prototype */ {
that.unloadRecord(null, null, storeKey, newStatus);
});
+ this.unregisterChildFromParent(storeKey);
+
return this ;
},
@@ -1446,20 +1458,27 @@ SC.Store = SC.Object.extend( /** @scope SC.Store.prototype */ {
/**
Unregister the Child Record from its Parent. This will cause the Child
Record to be removed from the store.
+
+ @param {Number} childStoreKey storeKey to unregister
*/
unregisterChildFromParent: function(childStoreKey) {
- var crs, oldPk;
+ var crs = this.childRecords,
+ prs = this.parentRecords,
+ recs = this.records,
+ that = this,
+ oldPk;
// Check the child to see if it has a parent
- crs = this.childRecords;
-
- // Remove the parent's connection to the child. This doesn't remove the
- // parent store key from the cache of parent store keys if the parent
- // no longer has any other registered children, because the amount of effort
- // to determine that would not be worth the miniscule memory savings.
- oldPk = crs[childStoreKey];
- if (oldPk) {
- delete this.parentRecords[oldPk][childStoreKey];
+ if (crs) {
+ // Remove the parent's connection to the child. This doesn't remove the
+ // parent store key from the cache of parent store keys if the parent
+ // no longer has any other registered children, because the amount of effort
+ // to determine that would not be worth the miniscule memory savings.
+ oldPk = crs[childStoreKey];
+ if (oldPk && prs) {
+ delete prs[oldPk][childStoreKey];
+ }
+ delete crs[childStoreKey];
}
// Remove the child.
@@ -1467,8 +1486,13 @@ SC.Store = SC.Object.extend( /** @scope SC.Store.prototype */ {
// 2. from the cache of record objects
// 3. from the cache of child record store keys
this.removeDataHash(childStoreKey);
- delete this.records[childStoreKey];
- delete crs[childStoreKey];
+ if (recs) {
+ delete recs[childStoreKey];
+ }
+
+ this._propagateToChildren(childStoreKey, function(storeKey) {
+ that.unregisterChildFromParent(storeKey);
+ });
},
/**
22 frameworks/datastore/tests/models/nested_records/nested_record.js
View
@@ -287,6 +287,7 @@ test("Basic Write As a Hash when Child Record has no primary key", function() {
// Test Child Record creation
var oldCR = testParent3.get('info');
+ var oldKey = oldCR.get('id');
testParent3.set('info', {
type: 'ChildRecordTest',
name: 'New Child Name',
@@ -302,8 +303,7 @@ test("Basic Write As a Hash when Child Record has no primary key", function() {
var storeRef = store.find(NestedRecord.ChildRecordTest, key);
ok(storeRef, 'after a set() with an object, checking that the store has the instance of the child record with proper primary key');
equals(cr, storeRef, "after a set with an object, checking the parent reference is the same as the direct store reference");
- var oldKey = oldCR.get('id');
- ok((oldKey === key), 'check to see that the old child record has the same key as the new child record');
+ equals(oldKey, key, 'check to see that the old child record has the same key as the new child record');
// Check for changes on the child bubble to the parent.
cr.set('name', 'Child Name Change');
@@ -398,11 +398,14 @@ test("Basic Write As a Child Record when Child Record has no primary key", funct
test("Writing over a child record should remove caches in the store.", function() {
// Test Child Record creation
- var cr, key, store = testParent.get('store'), cacheLength;
+ var cr, key, store = testParent.get('store'), cacheLength, idx, storeKeys = [], ids = [], sks;
// Get the child record once before setting it in order to test that this child
// doesn't become abandoned in the store.
cr = testParent.get('info');
+ storeKeys.push(cr.get('storeKey'));
+ ids.push(cr.get('id'));
+ ids = ids.uniq();
// Once we get the child record, certain caches are created in the store.
// Verify the cache lengths to prove that there are no leaked objects.
@@ -425,6 +428,9 @@ test("Writing over a child record should remove caches in the store.", function(
// Overwrite the child record with a new child record with the same guid.
testParent.set('info', {type: 'ChildRecordTest', name: 'New Child Name', value: 'Red Goo', guid: '5001'});
cr = testParent.get('info');
+ storeKeys.push(cr.get('storeKey'));
+ ids.push(cr.get('id'));
+ ids = ids.uniq();
// Verify the cache lengths to prove that there are no leaked objects.
cacheLength = 0;
@@ -446,6 +452,9 @@ test("Writing over a child record should remove caches in the store.", function(
// Overwrite the child record with a new child record with the same guid.
testParent.set('info', store.createRecord(NestedRecord.ChildRecordTest, {type: 'ChildRecordTest', name: 'New Child Name', value: 'Orange Goo', guid: '6001'}));
cr = testParent.get('info');
+ storeKeys.push(cr.get('storeKey'));
+ ids.push(cr.get('id'));
+ ids = ids.uniq();
// Verify the cache lengths to prove that there are no leaked objects.
cacheLength = 0;
@@ -608,11 +617,11 @@ test("Reloading the parent record uses same child record.", function() {
cacheLength = 0;
for (key in store.childRecords) { cacheLength += 1; }
- equals(cacheLength, 1, 'there should only be one child record registered in the store');
+ equals(cacheLength, 0, 'there should be zero child records registered in the store');
cacheLength = 0;
for (key in store.records) { cacheLength += 1; }
- equals(cacheLength, 4, 'there should be four records cached in the store');
+ equals(cacheLength, 2, 'there should be two records cached in the store');
cacheLength = 0;
for (key in store.dataHashes) { if (store.dataHashes[key] !== null) cacheLength += 1; }
@@ -620,7 +629,7 @@ test("Reloading the parent record uses same child record.", function() {
// Reload the record
SC.RunLoop.begin();
- store.loadRecord(NestedRecord.ParentRecordTest, {
+ parentStoreKey = store.loadRecord(NestedRecord.ParentRecordTest, {
name: 'Parent Name 3',
info: {
type: 'ChildRecordTest',
@@ -631,6 +640,7 @@ test("Reloading the parent record uses same child record.", function() {
parentId);
SC.RunLoop.end();
+ testParent3 = store.materializeRecord(parentStoreKey);
child = testParent3.get('info');
equals(testParent3.get('status'), SC.Record.READY_CLEAN, 'parent status should be READY_CLEAN');
equals(child.get('status'), SC.Record.READY_CLEAN, 'child status should be READY_CLEAN');
2  frameworks/datastore/tests/models/nested_records/nested_record_complex.js
View
@@ -246,6 +246,7 @@ function() {
// Test Child Record creation
oldP = testParent.get('person');
+ oldKey = oldP.get('id');
testParent.set('person', {
type: 'Person',
name: 'Al Gore',
@@ -266,7 +267,6 @@ function() {
storeRef = store.find(NestedRecord.Person, key);
ok(storeRef, 'after a set() with an object, checking that the store has the instance of the child record with proper primary key');
equals(p, storeRef, "after a set with an object, checking the parent reference is the same as the direct store reference");
- oldKey = oldP.get('id');
ok((oldKey === key), 'check to see that the old child record has the same key as the new child record');
// Check for changes on the child bubble to the parent.
15 frameworks/desktop/views/collection.js
View
@@ -2994,7 +2994,20 @@ SC.CollectionView = SC.View.extend(SC.CollectionViewDelegate, SC.CollectionConte
objects = [];
shift = 0;
data.indexes.forEach(function(i) {
- objects.push(content.objectAt(i-shift));
+ var o = content.objectAt(i-shift),
+ store, sk;
+ if (SC.get(o, 'isNestedRecord')) {
+ // special case here. removing a nested record from content will
+ // unregister the record from the parent, which removes the data hash.
+ // trying to reinsert that record will fail since the data hash is gone.
+ // to avoid this, read the data hash before removing and keep that around
+ // to reinsert.
+ store = o.get('store');
+ sk = o.get('storeKey');
+ objects.push(store.readDataHash(sk));
+ } else {
+ objects.push(o);
+ }
content.removeAt(i-shift);
shift++;
if (i < idx) idx--;
Something went wrong with that request. Please try again.