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

Fix "Multiple Parents Found" issue when moving a relationship. #14060

Merged
merged 1 commit into from
Mar 1, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/models/account.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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) }
Copy link
Member

Choose a reason for hiding this comment

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

Am I reading this correctly that these two places are the only ones where we want might want to have multiple children/parents?

Copy link
Member

Choose a reason for hiding this comment

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

Doh, I just read the comment above. So anyway, it makes sense, but to your point, there is probably untested code that might need to be changed. It's such a hard regex to find bad uses of parent=. 😢

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, those are some of the intentional "multiple parents", so having a method that is specifically designed for that seems clearer.

end

def remove_group(owner)
Expand Down
21 changes: 12 additions & 9 deletions app/models/mixins/relationship_mixin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

but now this method does not use the parameter parent??

Copy link
Member Author

Choose a reason for hiding this comment

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

what? it does...not sure what you mean.

Copy link
Member

Choose a reason for hiding this comment

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

my bad. was reading GitHub diffs incorrectly.

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
Expand Down
3 changes: 1 addition & 2 deletions lib/extensions/ar_miq_set.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
56 changes: 56 additions & 0 deletions spec/models/mixins/relationship_mixin_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Expand Down