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

Match has_one direction before deleting rel #1662

Merged
merged 2 commits into from
Sep 1, 2021

Conversation

bobmazanec
Copy link
Contributor

Fixes #1650

When relationship_corresponding_rel matches on the rel class name,
it stops on the first match, ignoring the direction parameter.

When target_class represents the to_node, direction is :in.

When the relationship is a has_one/has_many between a pair
of nodes of the same class, the match could find and return
the to_node's :out association first, causing
delete_reverse_has_one_relationship to delete it.

Checking the direction first prevents that.

Sadly, I worked through #1660 before seeing #1650 and that I had reached the same basic conclusion, only factoring the fix slightly differently than the OP proposed.

Test runs

I added 4 examples—2 simply show that the 'setup' in the 'real' examples works as expected. The other 2 (really the same example but under both creates_unique :all and creates_unique :none) show the problem.

Before

1907 examples, 0 failures, 5 pending

After

Without the fix
[...]
1) between nodes of the same class with creates_unique(:all) given (gen1)-->(gen0) #create (gen2)-->(gen1) adds a new node and relationsihp without affecting (gen1)-->(gen0)
     Failure/Error: expect(@gen1.parent).to(be_present, "gen1 lost its parent")
       gen1 lost its parent
     Shared Example Group: :creating_gen2 called from ./spec/e2e/relationship_spec.rb:650
     # ./spec/e2e/relationship_spec.rb:636:in `block (5 levels) in <top (required)>'

  2) between nodes of the same class with creates_unique(:none) given (gen1)-->(gen0) #create (gen2)-->(gen1) adds a new node and relationsihp without affecting (gen1)-->(gen0)
     Failure/Error: expect(@gen1.parent).to(be_present, "gen1 lost its parent")
       gen1 lost its parent
     Shared Example Group: :creating_gen2 called from ./spec/e2e/relationship_spec.rb:650
     # ./spec/e2e/relationship_spec.rb:636:in `block (5 levels) in <top (required)>'

1911 examples, 2 failures, 5 pending
With the fix

1911 examples, 0 failures, 5 pending

When relationship_corresponding_rel matches on the rel class name,
it stops on the first match, ignoring the direction parameter.

When target_class represents the to_node, direction is :in.

When the relationship is a has_one/has_many between a pair
of nodes of the same class, the match could find and return
the to_node's :out association first, causing
delete_reverse_has_one_relationship() to delete it.

Checking the direction first prevents that.
spec/e2e/relationship_spec.rb Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2021

Codecov Report

Merging #1662 (1ccef51) into 10 (95f249d) will increase coverage by 0.23%.
The diff coverage is 100.00%.

❗ Current head 1ccef51 differs from pull request most recent head 26da404. Consider uploading reports for the commit 26da404 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##               10    #1662      +/-   ##
==========================================
+ Coverage   93.93%   94.16%   +0.23%     
==========================================
  Files         126      126              
  Lines        5737     5643      -94     
==========================================
- Hits         5389     5314      -75     
+ Misses        348      329      -19     
Impacted Files Coverage Δ
lib/active_graph/node/has_n.rb 98.81% <100.00%> (-0.03%) ⬇️
lib/active_graph/migrations/schema_migration.rb 88.88% <0.00%> (-11.12%) ⬇️
lib/active_graph/migrations/helpers/id_property.rb 86.48% <0.00%> (-1.32%) ⬇️
lib/active_graph/core/cypher_error.rb 88.88% <0.00%> (-1.12%) ⬇️
lib/rails/generators/active_graph_generator.rb 65.45% <0.00%> (-0.62%) ⬇️
lib/active_graph/migration.rb 81.39% <0.00%> (-0.43%) ⬇️
lib/active_graph/node/property.rb 93.93% <0.00%> (-0.35%) ⬇️
lib/active_graph/node/id_property.rb 97.61% <0.00%> (-0.30%) ⬇️
lib/active_graph/config.rb 94.11% <0.00%> (-0.12%) ⬇️
lib/active_graph/node/labels.rb 94.73% <0.00%> (-0.11%) ⬇️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95f249d...26da404. Read the comment docs.

@amitsuryavanshi amitsuryavanshi merged commit ff2f8a5 into neo4jrb:10 Sep 1, 2021
@bobmazanec bobmazanec deleted the 10-1650 branch September 1, 2021 15:10
klobuczek pushed a commit that referenced this pull request Nov 5, 2021
* Match has_one direction before deleting rel

When relationship_corresponding_rel matches on the rel class name,
it stops on the first match, ignoring the direction parameter.

When target_class represents the to_node, direction is :in.

When the relationship is a has_one/has_many between a pair
of nodes of the same class, the match could find and return
the to_node's :out association first, causing
delete_reverse_has_one_relationship() to delete it.

Checking the direction first prevents that.

* Move shared_examples_for group into loop using it

per
#1662 (comment)

(cherry picked from commit ff2f8a5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants