From 8027d2fb487c761043068a6eacc10260316edd41 Mon Sep 17 00:00:00 2001 From: Dan Kubb Date: Thu, 9 Jul 2009 00:21:11 -0700 Subject: [PATCH] Fixed element swapping in Collection * Minor refactoring of specs to ensure Collection#[]= remains not a kicker. [#955 state:resolved] --- lib/dm-core/associations/many_to_many.rb | 10 +- lib/dm-core/collection.rb | 34 +++--- spec/lib/collection_helpers.rb | 8 ++ spec/public/shared/collection_shared_spec.rb | 107 +++++++++---------- 4 files changed, 77 insertions(+), 82 deletions(-) diff --git a/lib/dm-core/associations/many_to_many.rb b/lib/dm-core/associations/many_to_many.rb index fe83f7b2..6ca5d729 100644 --- a/lib/dm-core/associations/many_to_many.rb +++ b/lib/dm-core/associations/many_to_many.rb @@ -315,12 +315,6 @@ def _create(safe, attributes) # TODO: document # @api private def _save(safe) - resources = if loaded? - self - else - head + tail - end - # delete only intermediaries linked to the target orphans unless @orphans.empty? || intermediaries(@orphans).send(safe ? :destroy : :destroy!) return false @@ -328,11 +322,11 @@ def _save(safe) if via.respond_to?(:resource_for) super - resources.all? { |resource| create_intermediary(safe, via => resource) } + loaded_entries.all? { |resource| create_intermediary(safe, via => resource) } else if intermediary = create_intermediary(safe) inverse = via.inverse - resources.map { |resource| inverse.set(resource, intermediary) } + loaded_entries.map { |resource| inverse.set(resource, intermediary) } end super diff --git a/lib/dm-core/collection.rb b/lib/dm-core/collection.rb index 39ec5495..7316fe0d 100644 --- a/lib/dm-core/collection.rb +++ b/lib/dm-core/collection.rb @@ -72,16 +72,10 @@ def model def reload(query = nil) query = query.nil? ? self.query.dup : self.query.merge(query) - resources = if loaded? - self - else - head + tail - end - # make sure the Identity Map contains all the existing resources identity_map = repository.identity_map(model) - resources.each do |resource| + loaded_entries.each do |resource| identity_map[resource.key] = resource end @@ -411,11 +405,15 @@ def slice!(*args) # # @api public def []=(*args) - # orphan resources being replaced - orphan_resources(superclass_slice(*args[0..-2])) + orphans = Array(superclass_slice(*args[0..-2])) # relate new resources - relate_resources(super) + resources = relate_resources(super) + + # orphan resources that no longer exist in the collection elsewhere + orphan_resources(orphans - loaded_entries) + + resources end alias splice []= @@ -1032,6 +1030,16 @@ def lazy_load self end + # Loaded Resources in the collection + # + # @return [Array] + # Resources in the collection + # + # @api private + def loaded_entries + loaded? ? self : head + tail + end + ## # Initializes a new Collection # @@ -1104,11 +1112,7 @@ def _update(dirty_attributes) # # @api private def _save(safe) - resources = if loaded? - self - else - head + tail - end + resources = loaded_entries # FIXME: remove this once the writer method on the child side # is used to store the reference to the parent. diff --git a/spec/lib/collection_helpers.rb b/spec/lib/collection_helpers.rb index 1e0aa2cd..885dfbc8 100644 --- a/spec/lib/collection_helpers.rb +++ b/spec/lib/collection_helpers.rb @@ -5,6 +5,14 @@ def self.extended(base) base.class_inheritable_accessor :loaded base.loaded = false end + + def should_not_be_a_kicker + unless loaded + it 'should not be a kicker' do + @articles.should_not be_loaded + end + end + end end end end diff --git a/spec/public/shared/collection_shared_spec.rb b/spec/public/shared/collection_shared_spec.rb index 903cd1c6..26db4d49 100644 --- a/spec/public/shared/collection_shared_spec.rb +++ b/spec/public/shared/collection_shared_spec.rb @@ -161,13 +161,7 @@ @return = @resource = @articles.at(0) end - # FIXME: this is spec order dependent, move this into a helper method - # and execute in the before :all block - unless loaded - it 'should not be a kicker' do - @articles.should_not be_loaded - end - end + should_not_be_a_kicker it 'should return a Resource' do @return.should be_kind_of(DataMapper::Resource) @@ -187,13 +181,7 @@ @return = @resource = @articles.unshift(@other).at(0) end - # FIXME: this is spec order dependent, move this into a helper method - # and execute in the before :all block - unless loaded - it 'should not be a kicker' do - @articles.should_not be_loaded - end - end + should_not_be_a_kicker it 'should return a Resource' do @return.should be_kind_of(DataMapper::Resource) @@ -213,13 +201,7 @@ @return = @resource = @articles.at(-1) end - # FIXME: this is spec order dependent, move this into a helper method - # and execute in the before :all block - unless loaded - it 'should not be a kicker' do - @articles.should_not be_loaded - end - end + should_not_be_a_kicker it 'should return a Resource' do @return.should be_kind_of(DataMapper::Resource) @@ -239,13 +221,7 @@ @return = @resource = @articles.push(@other).at(-1) end - # FIXME: this is spec order dependent, move this into a helper method - # and execute in the before :all block - unless loaded - it 'should not be a kicker' do - @articles.should_not be_loaded - end - end + should_not_be_a_kicker it 'should return a Resource' do @return.should be_kind_of(DataMapper::Resource) @@ -619,10 +595,6 @@ @articles.should be_empty end - it 'should be a kicker' do - @articles.should be_loaded - end - it 'should bypass validation' do pending 'TODO: not sure how to best spec this' end @@ -648,10 +620,6 @@ @limited.should be_empty end - it 'should be a kicker' do - @limited.should be_loaded - end - it 'should bypass validation' do pending 'TODO: not sure how to best spec this' end @@ -1342,13 +1310,7 @@ @return = @articles.base_model end - # FIXME: this is spec order dependent, move this into a helper method - # and execute in the before :all block - unless loaded - it 'should not be a kicker' do - @articles.should_not be_loaded - end - end + should_not_be_a_kicker it 'should return expected object' do @return.should == @article_model @@ -2454,13 +2416,18 @@ before :all do unless @skip orphans = (1..10).map do |number| - @articles.create(:content => "Article #{number}") - @articles.pop # remove the article from the tail + articles = @articles.dup + articles.create(:content => "Article #{number}") + articles.pop # remove the article from the tail end @articles.unshift(*orphans.first(5)) @articles.concat(orphans.last(5)) + unless loaded + @articles.should_not be_loaded + end + @copy = @articles.dup @new = @article_model.new(:content => 'New Article') end @@ -2476,6 +2443,8 @@ end end + should_not_be_a_kicker + it 'should return a Resource' do @return.should be_kind_of(DataMapper::Resource) end @@ -2511,6 +2480,8 @@ end end + should_not_be_a_kicker + it 'should return a Resource' do @return.should be_kind_of(DataMapper::Resource) end @@ -2542,6 +2513,8 @@ end end + should_not_be_a_kicker + it 'should return a Resource' do @return.should be_kind_of(DataMapper::Resource) end @@ -2573,6 +2546,8 @@ end end + should_not_be_a_kicker + it 'should return a Resource' do @return.should be_kind_of(DataMapper::Resource) end @@ -2608,6 +2583,8 @@ end end + should_not_be_a_kicker + it 'should return a Resource' do @return.should be_kind_of(DataMapper::Resource) end @@ -2639,6 +2616,8 @@ end end + should_not_be_a_kicker + it 'should return a Resource' do @return.should be_kind_of(DataMapper::Resource) end @@ -2662,6 +2641,28 @@ end end + describe '#[]=' do + describe 'when swapping resources' do + before :all do + rescue_if 'TODO', @skip do + @articles.create(:content => 'Another Article') + + @entries = @articles.entries + + @articles[0], @articles[1] = @articles[1], @articles[0] + end + end + + it 'should include the Resource in the Collection' do + @articles.should == @entries.reverse + end + + it 'should relate the Resource to the Collection' do + @articles.each { |resource| resource.collection.should equal(@articles) } + end + end + end + it { @articles.should respond_to(:unshift) } describe '#unshift' do @@ -2752,13 +2753,7 @@ @return = @articles.update! end - # FIXME: this is spec order dependent, move this into a helper method - # and execute in the before :all block - unless loaded - it 'should not be a kicker' do - @articles.should_not be_loaded - end - end + should_not_be_a_kicker it 'should return true' do @return.should be_true @@ -2836,13 +2831,7 @@ @return = @articles.update!(:title => nil) end - # FIXME: this is spec order dependent, move this into a helper method - # and execute in the before :all block - unless loaded - it 'should not be a kicker' do - @articles.should_not be_loaded - end - end + should_not_be_a_kicker it 'should return false' do @return.should be_false