From c5f22fb7bf29add03f852fd218f3d7f3eda62935 Mon Sep 17 00:00:00 2001 From: "David J. Hamilton" Date: Wed, 19 Oct 2016 07:45:20 -0700 Subject: [PATCH] Free up `internalModel`s `internalModel`s need to stay around as long as they're connected to a live record via relationships. Once an entire subgraph is removed however, we can free all of it. Also add a couple of "late private" fields to `internalModel`'s constructor for shape preservation. Finally - Add a lot of `toString`s to test classes, as a debugging convenience. - Minor test cleanup Includes work from @igort, @sly7-7 and @pangratz [Fix #3296] [Supercede #3301] --- addon/-private/system/model/internal-model.js | 152 ++++++++++++++++-- addon/-private/system/model/model.js | 13 +- addon/-private/system/model/states.js | 10 -- addon/-private/system/record-array-manager.js | 7 +- addon/-private/system/record-map.js | 2 +- addon/-private/system/store.js | 6 +- tests/integration/adapter/find-all-test.js | 1 + tests/integration/records/load-test.js | 9 +- tests/integration/records/unload-test.js | 121 ++++++++++---- .../relationships/has-many-test.js | 15 +- tests/integration/store-test.js | 5 +- tests/unit/many-array-test.js | 2 + tests/unit/model/rollback-attributes-test.js | 1 + tests/unit/store/unload-test.js | 72 ++++----- 14 files changed, 301 insertions(+), 115 deletions(-) diff --git a/addon/-private/system/model/internal-model.js b/addon/-private/system/model/internal-model.js index 5bc9e621528..ebcf3b03ddb 100644 --- a/addon/-private/system/model/internal-model.js +++ b/addon/-private/system/model/internal-model.js @@ -59,6 +59,16 @@ function extractPivotName(name) { ); } +function areAllModelsUnloaded(internalModels) { + for (let i=0; i { + if (this._relationships.has(key)) { + let related = this._relationships.get(key).members.toArray(); + array = array.concat(related); + } + }); + return array; + } + + + /** + Computes the set of internal models reachable from this internal model. + + Reachability is determined over the relationship graph (ie a graph where + nodes are internal models and edges are belongs to or has many + relationships). + + @return {Array} An array including `this` and all internal models reachable + from `this`. + */ + _allRelatedInternalModels() { + let array = []; + let queue = []; + let bfsId = nextBfsId++; + queue.push(this); + this._bfsId = bfsId; + while (queue.length > 0) { + let node = queue.shift(); + array.push(node); + let related = node._directlyRelatedInternalModels(); + for (let i=0; i false; + assert.equal(env.store.hasRecordForId('person', 1), false, 'The person is unloaded'); + }); - run(function() { + run(() => { env.store.push({ data: { type: 'person', id: '1', attributes: { name: 'Adam Sunderland' + }, + relationships: { + cars: { + data: [ + { type: 'car', id: '1' } + ] + } } } }); - adam = env.store.peekRecord('person', 1); }); - run(function() { + var rematerializedPerson = bob.get('person'); + assert.equal(rematerializedPerson.get('id'), '1'); + // the person is rematerialized; the previous person is *not* re-used + assert.notEqual(rematerializedPerson, adam, 'the person is rematerialized, not recycled'); +}); + + +test('unloading a disconnected subgraph clears the relevant internal models', function(assert) { + env.adapter.shouldBackgroundReloadRecord = () => false; + + run(() => { env.store.push({ data: { - type: 'group', + type: 'person', id: '1', + attributes: { + name: 'Could be Anybody' + }, relationships: { - people: { + cars: { data: [ - { type: 'person', id: '1' } + { type: 'car', id: '1' }, + { type: 'car', id: '2' } ] } } } }); - bob = env.store.peekRecord('group', 1); }); - run(function() { - env.store.findRecord('group', 1).then(function(group) { - assert.equal(group.get('people.length'), 1, 'The inital length of people is correct'); - var person = env.store.peekRecord('person', 1); - run(function() { - person.unloadRecord(); - }); - - assert.equal(group.get('people.length'), 0, 'Person was removed from the people array'); + run(() => { + env.store.push({ + data: { + type: 'car', + id: '1', + attributes: { + make: 'Nissan', + model: 'Altima' + }, + relationships: { + person: { + data: { type: 'person', id: '1' } + } + } + } }); }); + + run(() => { + env.store.push({ + data: { + type: 'car', + id: '2', + attributes: { + make: 'Tesla', + model: 'S' + }, + relationships: { + person: { + data: { type: 'person', id: '1' } + } + } + } + }); + }); + + assert.equal( + env.store._recordMapFor('person').records.length, + 1, + 'one person record is loaded' + ); + assert.equal( + env.store._recordMapFor('car').records.length, + 2, + 'two car records are loaded' + ); + + run(() => { + env.store.peekRecord('person', 1).unloadRecord(); + env.store.peekRecord('car', 1).unloadRecord(); + env.store.peekRecord('car', 2).unloadRecord(); + }); + + assert.equal(env.store._recordMapFor('person').records.length, 0); + assert.equal(env.store._recordMapFor('car').records.length, 0); }); diff --git a/tests/integration/relationships/has-many-test.js b/tests/integration/relationships/has-many-test.js index bf49959d009..74592311f49 100644 --- a/tests/integration/relationships/has-many-test.js +++ b/tests/integration/relationships/has-many-test.js @@ -29,44 +29,53 @@ module("integration/relationships/has_many - Has-Many Relationships", { Contact = DS.Model.extend({ user: belongsTo('user', { async: false }) }); + Contact.reopenClass({ toString: () => 'Contact' }); Email = Contact.extend({ email: attr('string') }); + Email.reopenClass({ toString: () => 'Email' }); Phone = Contact.extend({ number: attr('string') }); + Phone.reopenClass({ toString: () => 'Phone' }); Message = DS.Model.extend({ user: belongsTo('user', { async: false }), created_at: attr('date') }); + Message.reopenClass({ toString: () => 'Message' }); Post = Message.extend({ title: attr('string'), comments: hasMany('comment', { async: false }) }); + Post.reopenClass({ toString: () => 'Post' }); Comment = Message.extend({ body: DS.attr('string'), message: DS.belongsTo('post', { polymorphic: true, async: true }) }); + Comment.reopenClass({ toString: () => 'Comment' }); Book = DS.Model.extend({ title: attr(), chapters: hasMany('chapter', { async: true }) }); + Book.reopenClass({ toString: () => 'Book' }); Chapter = DS.Model.extend({ title: attr(), pages: hasMany('page', { async: false }) }); + Chapter.reopenClass({ toString: () => 'Chapter' }); Page = DS.Model.extend({ number: attr('number'), chapter: belongsTo('chapter', { async: false }) }); + Page.reopenClass({ toString: () => 'Page' }); env = setupStore({ user: User, @@ -2197,10 +2206,12 @@ test("adding and removing records from hasMany relationship #2666", function(ass var Post = DS.Model.extend({ comments: DS.hasMany('comment', { async: true }) }); + Post.reopenClass({ toString: () => 'Post' }); var Comment = DS.Model.extend({ post: DS.belongsTo('post', { async: false }) }); + Comment.reopenClass({ toString: () => 'Comment' }); env = setupStore({ post: Post, @@ -2776,7 +2787,7 @@ test("unloading and reloading a record with hasMany relationship - #3084", funct user = env.store.peekRecord('user', 'user-1'); - assert.equal(get(user, 'messages.firstObject.id'), 'message-1'); - assert.equal(get(message, 'user.id'), 'user-1'); + assert.equal(get(user, 'messages.firstObject.id'), 'message-1', 'user points to message'); + assert.equal(get(message, 'user.id'), 'user-1', 'message points to user'); }); }); diff --git a/tests/integration/store-test.js b/tests/integration/store-test.js index 176de164571..c5767002db5 100644 --- a/tests/integration/store-test.js +++ b/tests/integration/store-test.js @@ -13,6 +13,7 @@ var Person = DS.Model.extend({ name: DS.attr('string'), cars: DS.hasMany('car', { async: false }) }); +Person.reopenClass({ toString: () => 'Person' }); var run = Ember.run; @@ -21,6 +22,7 @@ var Car = DS.Model.extend({ model: DS.attr('string'), person: DS.belongsTo('person', { async: false }) }); +Car.reopenClass({ toString: () => 'Car' }); function initializeStore(adapter) { env = setupStore({ @@ -195,11 +197,8 @@ test("destroying the store correctly cleans everything up", function(assert) { assert.equal(car.get('person'), person, "expected car's person to be the correct person"); assert.equal(person.get('cars.firstObject'), car, " expected persons cars's firstRecord to be the correct car"); - Ember.run(person, person.destroy); Ember.run(store, 'destroy'); - assert.equal(car.get('person'), null, "expected car.person to no longer be present"); - assert.equal(personWillDestroy.called.length, 1, 'expected person to have recieved willDestroy once'); assert.equal(carWillDestroy.called.length, 1, 'expected car to recieve willDestroy once'); assert.equal(carsWillDestroy.called.length, 1, 'expected cars to recieve willDestroy once'); diff --git a/tests/unit/many-array-test.js b/tests/unit/many-array-test.js index cd2fd323cf5..42d9d3dd5df 100644 --- a/tests/unit/many-array-test.js +++ b/tests/unit/many-array-test.js @@ -19,11 +19,13 @@ module("unit/many_array - DS.ManyArray", { title: attr('string'), tags: hasMany('tag', { async: false }) }); + Post.reopenClass({ toString: () => 'Post'}); Tag = DS.Model.extend({ name: attr('string'), post: belongsTo('post', { async: false }) }); + Tag.reopenClass({ toString: () => 'Tag'}); env = setupStore({ post: Post, diff --git a/tests/unit/model/rollback-attributes-test.js b/tests/unit/model/rollback-attributes-test.js index 9c58865b958..178e4499607 100644 --- a/tests/unit/model/rollback-attributes-test.js +++ b/tests/unit/model/rollback-attributes-test.js @@ -15,6 +15,7 @@ module("unit/model/rollbackAttributes - model.rollbackAttributes()", { firstName: DS.attr(), lastName: DS.attr() }); + Person.reopenClass({ toString() { return 'Person'; } }); env = setupStore({ person: Person }); store = env.store; diff --git a/tests/unit/store/unload-test.js b/tests/unit/store/unload-test.js index 9bcd77696c5..fc6a4875be6 100644 --- a/tests/unit/store/unload-test.js +++ b/tests/unit/store/unload-test.js @@ -17,6 +17,8 @@ module("unit/store/unload - Store unloading records", { title: DS.attr('string'), wasFetched: DS.attr('boolean') }); + Record.reopenClass({ toString: () => 'Record'}); + store = createStore({ adapter: DS.Adapter.extend({ findRecord(store, type, id, snapshot) { @@ -47,26 +49,25 @@ testInDebug("unload a dirty record asserts", function(assert) { } }); - store.findRecord('record', 1).then(function(record) { - record.set('title', 'toto2'); - record._internalModel.send('willCommit'); + let record = store.peekRecord('record', 1); + record.set('title', 'toto2'); + record._internalModel.send('willCommit'); - assert.equal(get(record, 'hasDirtyAttributes'), true, "record is dirty"); + assert.equal(get(record, 'hasDirtyAttributes'), true, "record is dirty"); - assert.expectAssertion(function() { - record.unloadRecord(); - }, "You can only unload a record which is not inFlight. `" + record._internalModel.toString() + "`", "can not unload dirty record"); + assert.expectAssertion(function() { + record.unloadRecord(); + }, "You can only unload a record which is not inFlight. `" + record._internalModel.toString() + "`", "can not unload dirty record"); - // force back into safe to unload mode. - run(function() { - record._internalModel.transitionTo('deleted.saved'); - }); + // force back into safe to unload mode. + run(function() { + record._internalModel.transitionTo('deleted.saved'); }); }); }); -test("unload a record", function(assert) { - assert.expect(5); +test('unload a record', function(assert) { + assert.expect(2); run(function() { store.push({ @@ -80,15 +81,11 @@ test("unload a record", function(assert) { }); store.findRecord('record', 1).then(function(record) { assert.equal(get(record, 'id'), 1, "found record with id 1"); - assert.equal(get(record, 'hasDirtyAttributes'), false, "record is not dirty"); run(function() { store.unloadRecord(record); }); - assert.equal(get(record, 'hasDirtyAttributes'), false, "record is not dirty"); - assert.equal(get(record, 'isDeleted'), true, "record is deleted"); - tryToFind = false; return store.findRecord('record', 1).then(function() { assert.equal(tryToFind, true, "not found record with id 1"); @@ -103,22 +100,23 @@ module("DS.Store - unload record with relationships"); test("can commit store after unload record with relationships", function(assert) { assert.expect(1); - var like, product; - - var Brand = DS.Model.extend({ + let Brand = DS.Model.extend({ name: DS.attr('string') }); + Brand.reopenClass({ toString: () => 'Brand'}); - var Product = DS.Model.extend({ + let Product = DS.Model.extend({ description: DS.attr('string'), brand: DS.belongsTo('brand', { async: false }) }); + Product.reopenClass({ toString: () => 'Product'}); - var Like = DS.Model.extend({ + let Like = DS.Model.extend({ product: DS.belongsTo('product', { async: false }) }); + Like.reopenClass({ toString: () => 'Like'}); - var store = createStore({ + let store = createStore({ adapter: DS.Adapter.extend({ findRecord(store, type, id, snapshot) { return Ember.RSVP.resolve({ id: 1, description: 'cuisinart', brand: 1 }); @@ -131,9 +129,8 @@ test("can commit store after unload record with relationships", function(assert) product: Product, like: Like }); - var asyncRecords; - run(function() { + return run(() => { store.push({ data: [{ type: 'brand', @@ -155,20 +152,17 @@ test("can commit store after unload record with relationships", function(assert) }] }); - asyncRecords = Ember.RSVP.hash({ - brand: store.findRecord('brand', 1), - product: store.findRecord('product', 1) - }); - asyncRecords.then(function(records) { - like = store.createRecord('like', { id: 1, product: product }); - records.like = like.save(); - return Ember.RSVP.hash(records); - }).then(function(records) { - store.unloadRecord(records.product); - return store.findRecord('product', 1); - }).then(function(product) { - assert.equal(product.get('description'), 'cuisinart', "The record was unloaded and the adapter's `findRecord` was called"); - store.destroy(); + let product = store.peekRecord('product', 1); + let like = store.createRecord('like', { id: 1, product: product }); + + return like.save(); + }).then(() => { + return run(() => { + store.unloadRecord(store.peekRecord('product', 1)); }); + }).then(() => { + return store.findRecord('product', 1); + }).then((product) => { + assert.equal(product.get('description'), 'cuisinart', "The record was unloaded and the adapter's `findRecord` was called"); }); });