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

Fryguy · 2017-02-24T06:52:22Z

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

jrafanie · 2017-02-24T17:04:22Z

@Fryguy tests are red. Can you describe what this PR is fixing? It's not clear how this fixes the multiple parents found problem?

Fryguy · 2017-02-24T17:40:08Z

Updated the OP and commit with a better description. I had thought the tests were self-explanatory, but I agree this needed to be made clearer.

chessbyte · 2017-02-24T17:43:13Z

app/models/mixins/relationship_mixin.rb

-  alias_method :set_child,  :add_children
-
-  def replace_parent(parent)
+  def parent=(parent)


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

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

my bad. was reading GitHub diffs incorrectly.

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

miq-bot · 2017-02-24T22:12:22Z

Some comments on commit Fryguy@af9e051

spec/models/mixins/relationship_mixin_spec.rb

⚠️ - 204 - Detected puts. Remove all debugging statements.
⚠️ - 233 - Detected puts. Remove all debugging statements.

miq-bot · 2017-02-24T22:13:23Z

Checked commit Fryguy@af9e051 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 3 offenses detected

app/models/mixins/relationship_mixin.rb

❗ - Line 577, Col 3 - Style/Alias - Use alias instead of alias_method in a module body.
❗ - Line 583, Col 3 - Style/Alias - Use alias instead of alias_method in a module body.
❗ - Line 584, Col 3 - Style/Alias - Use alias instead of alias_method in a module body.

Fryguy · 2017-02-24T22:23:40Z

I fixed the tests passing, but I think I need to comb the code for all uses of parent= (and set_parent alias) to make sure that untested code still will work as expected. Luckily the tests in account and miq_widget_set (which were effectively testing miq_set) covered some fixes.

jrafanie · 2017-02-27T20:08:30Z

app/models/account.rb

@@ -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) }


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

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=. 😢

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

Fryguy · 2017-03-01T18:31:26Z

@jrafanie I checked the whole code base for parent=, set_parent, replace_parent, and :parent =>, and I everything looks good.

jrafanie · 2017-03-01T18:38:51Z

@Fryguy can you mark euwe/darga labels?

Fix "Multiple Parents Found" issue when moving a relationship. (cherry picked from commit 31a42b7) https://bugzilla.redhat.com/show_bug.cgi?id=1428079 https://bugzilla.redhat.com/show_bug.cgi?id=1428126 https://bugzilla.redhat.com/show_bug.cgi?id=1428124

simaishi · 2017-03-09T19:07:14Z

Euwe backport details:

$ git log -1
commit b88ad85327212c315d7a3a1c528ec665475ef171
Author: Joe Rafaniello <[email protected]>
Date:   Wed Mar 1 13:38:06 2017 -0500

    Merge pull request #14060 from Fryguy/die_multiple_parents_found
    
    Fix "Multiple Parents Found" issue when moving a relationship.
    (cherry picked from commit 31a42b746f97772c7af10a322d44de3e4fbd7eca)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1428079
    https://bugzilla.redhat.com/show_bug.cgi?id=1428126
    https://bugzilla.redhat.com/show_bug.cgi?id=1428124

Fix "Multiple Parents Found" issue when moving a relationship. (cherry picked from commit 31a42b7) https://bugzilla.redhat.com/show_bug.cgi?id=1428079

Fryguy added bug core labels Feb 24, 2017

Fryguy assigned jrafanie Feb 24, 2017

Fryguy force-pushed the die_multiple_parents_found branch from 0eaa28f to 7cde7e9 Compare February 24, 2017 17:39

Fryguy force-pushed the die_multiple_parents_found branch from 7cde7e9 to b9dd9b5 Compare February 24, 2017 17:42

chessbyte reviewed Feb 24, 2017

View reviewed changes

Fryguy force-pushed the die_multiple_parents_found branch from b9dd9b5 to 652bfa7 Compare February 24, 2017 22:06

Fryguy force-pushed the die_multiple_parents_found branch from 652bfa7 to af9e051 Compare February 24, 2017 22:07

jrafanie reviewed Feb 27, 2017

View reviewed changes

jrafanie merged commit 31a42b7 into ManageIQ:master Mar 1, 2017

jrafanie added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 1, 2017

Fryguy added darga/yes labels Mar 1, 2017

Fryguy deleted the die_multiple_parents_found branch March 1, 2017 18:39

simaishi added euwe/backported and removed euwe/yes labels Mar 9, 2017

This was referenced Mar 24, 2017

Set genealogy parent correctly to avoid multiple parents #14490

Merged

Set genealogy parent correctly to avoid multiple parents for graph refresh #14491

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Fryguy commented Feb 24, 2017 •

edited

Loading

jrafanie commented Feb 24, 2017

Fryguy commented Feb 24, 2017

chessbyte Feb 24, 2017

Fryguy Feb 24, 2017

chessbyte Feb 27, 2017

miq-bot commented Feb 24, 2017

miq-bot commented Feb 24, 2017

Fryguy commented Feb 24, 2017

jrafanie Feb 27, 2017

jrafanie Feb 27, 2017

Fryguy Feb 27, 2017

Fryguy commented Mar 1, 2017

jrafanie commented Mar 1, 2017

simaishi commented Mar 9, 2017

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

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

Conversation

Fryguy commented Feb 24, 2017 • edited Loading

jrafanie commented Feb 24, 2017

Fryguy commented Feb 24, 2017

chessbyte Feb 24, 2017

Choose a reason for hiding this comment

Fryguy Feb 24, 2017

Choose a reason for hiding this comment

chessbyte Feb 27, 2017

Choose a reason for hiding this comment

miq-bot commented Feb 24, 2017

miq-bot commented Feb 24, 2017

Fryguy commented Feb 24, 2017

jrafanie Feb 27, 2017

Choose a reason for hiding this comment

jrafanie Feb 27, 2017

Choose a reason for hiding this comment

Fryguy Feb 27, 2017

Choose a reason for hiding this comment

Fryguy commented Mar 1, 2017

jrafanie commented Mar 1, 2017

simaishi commented Mar 9, 2017

Fryguy commented Feb 24, 2017 •

edited

Loading