From af9e051cb6d0d5b90b0e5448cc8ca15e5557011d Mon Sep 17 00:00:00 2001 From: Jason Frey Date: Fri, 24 Feb 2017 01:50:55 -0500 Subject: [PATCH] Fix "Multiple Parents Found" issue when moving a relationship. Originally, `.parent=` was written in terms of `add_children`. However, `add_children` supports duplicating nodes in the tree. This is reasonable because you may have a tree where multiple entries make sense, and for those things that have multiple entries, you wouldn't call `.parent`, you would probably call `.parents` and it is expected that calling `.parent` would blow up with the "Multiple Parents Found" error. However, the expectation of calling `.parent=` is that it will become the new, sole, parent, and so implementing it via add_children is wrong because it creates an oddity in that you can call `.parent=` and then not be able to call `.parent` afterwards. https://bugzilla.redhat.com/show_bug.cgi?id=1379464 https://bugzilla.redhat.com/show_bug.cgi?id=1391850 https://bugzilla.redhat.com/show_bug.cgi?id=1406431 --- app/models/account.rb | 4 +- app/models/mixins/relationship_mixin.rb | 21 ++++--- lib/extensions/ar_miq_set.rb | 3 +- spec/models/mixins/relationship_mixin_spec.rb | 56 +++++++++++++++++++ 4 files changed, 71 insertions(+), 13 deletions(-) diff --git a/app/models/account.rb b/app/models/account.rb index 1b3f332a5d6..d0133877670 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -104,7 +104,7 @@ def users end def add_user(owns) - with_valid_account_type('group') { set_child(owns) } + with_valid_account_type('group') { add_child(owns) } end def remove_user(owns) @@ -120,7 +120,7 @@ def groups end def add_group(owner) - with_valid_account_type('user') { set_parent(owner) } + with_valid_account_type('user') { add_parent(owner) } end def remove_group(owner) diff --git a/app/models/mixins/relationship_mixin.rb b/app/models/mixins/relationship_mixin.rb index f09406d9114..b3bce0ecd57 100644 --- a/app/models/mixins/relationship_mixin.rb +++ b/app/models/mixins/relationship_mixin.rb @@ -522,7 +522,7 @@ def child_types(*args) Relationship.resource_types(child_rels(*args)) end - def parent=(parent) + def add_parent(parent) parent.with_relationship_type(relationship_type) { parent.add_child(self) } end @@ -560,25 +560,28 @@ def add_children(*child_objs) end alias_method :add_child, :add_children - # - # Backward compatibility methods - # - - alias_method :set_parent, :parent= - alias_method :set_child, :add_children - - def replace_parent(parent) + def parent=(parent) if parent.nil? remove_all_parents else parent.with_relationship_type(relationship_type) do parent_rel = parent.init_relationship init_relationship(parent_rel) # TODO: Deal with any multi-instances + + parent.clear_relationships_cache end end clear_relationships_cache end + alias_method :replace_parent, :parent= + + # + # Backward compatibility methods + # + + alias_method :set_parent, :parent= + alias_method :set_child, :add_children def replace_children(*child_objs) child_objs = child_objs.flatten diff --git a/lib/extensions/ar_miq_set.rb b/lib/extensions/ar_miq_set.rb index 2cb7d34e874..8546e569c71 100644 --- a/lib/extensions/ar_miq_set.rb +++ b/lib/extensions/ar_miq_set.rb @@ -33,8 +33,7 @@ def sets end # module SingletonMethods def make_memberof(set) - raise "object of type #{self.class} may not be a member of a set of type #{set.class}" unless self.kind_of?(set.class.model_class) - with_relationship_type("membership") { self.parent = set } + set.add_member(self) end end # module ActsAsMiqSetMember diff --git a/spec/models/mixins/relationship_mixin_spec.rb b/spec/models/mixins/relationship_mixin_spec.rb index d77e9a4233d..0c3e2a5351d 100644 --- a/spec/models/mixins/relationship_mixin_spec.rb +++ b/spec/models/mixins/relationship_mixin_spec.rb @@ -196,6 +196,62 @@ end end + describe "#add_parent" do + let(:folder1) { FactoryGirl.create(:ems_folder) } + let(:folder2) { FactoryGirl.create(:ems_folder) } + let(:vm) { FactoryGirl.create(:vm) } + + it "puts an object under another object" do + vm.with_relationship_type(test_rel_type) do + vm.add_parent(folder1) + + expect(vm.parents).to eq([folder1]) + expect(vm.parent).to eq(folder1) + end + + expect(folder1.with_relationship_type(test_rel_type) { folder1.children }).to eq([vm]) + end + + it "allows an object to be placed under multiple parents" do + vm.with_relationship_type(test_rel_type) do + vm.add_parent(folder1) + vm.add_parent(folder2) + + expect(vm.parents).to match_array([folder1, folder2]) + expect { vm.parent }.to raise_error(RuntimeError, "Multiple parents found.") + end + + expect(folder1.with_relationship_type(test_rel_type) { folder1.children }).to eq([vm]) + expect(folder2.with_relationship_type(test_rel_type) { folder2.children }).to eq([vm]) + end + end + + describe "#parent=" do + let(:folder) { FactoryGirl.create(:ems_folder) } + let(:vm) { FactoryGirl.create(:vm) } + + it "puts an object under another object" do + vm.with_relationship_type(test_rel_type) do + vm.parent = folder + expect(vm.parent).to eq(folder) + end + + expect(folder.with_relationship_type(test_rel_type) { folder.children }).to eq([vm]) + end + + it "moves an object that already has a parent under an another object" do + vm.with_relationship_type(test_rel_type) { vm.parent = FactoryGirl.create(:ems_folder) } + vm.reload + + vm.with_relationship_type(test_rel_type) do + vm.parent = folder + expect(vm.parent).to eq(folder) + end + + expect(folder.with_relationship_type(test_rel_type) { folder.children }).to eq([vm]) + end + end + it "#replace_children" do new_vms = (0...2).collect { FactoryGirl.create(:vm_vmware) } vms[0].with_relationship_type(test_rel_type) do |v|