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

Issue-1611 Fix in updating relations with ids #1612

Merged

Conversation

amitsuryavanshi
Copy link
Member

Fixes #1611

This pull introduces/changes:

  • The way we get what relations to delete on update of relationships
    In case there was a string ID property, the to_i on passed id parameter would cause it be inconsistent with IDs of existing relations and we would end up with deleting all existing relations.

@codecov-commenter
Copy link

codecov-commenter commented Jul 16, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@ca2fade). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1612   +/-   ##
=========================================
  Coverage          ?   94.02%           
=========================================
  Files             ?      126           
  Lines             ?     5721           
  Branches          ?        0           
=========================================
  Hits              ?     5379           
  Misses            ?      342           
  Partials          ?        0           
Impacted Files Coverage Δ
...node/query/query_proxy_methods_of_mass_updating.rb 100.00% <100.00%> (ø)

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 ca2fade...30171e9. Read the comment docs.

end.compact
end

def delete_rels_for_nodes(ids)
def delete_rels_for_nodes(original_ids, node_or_nodes)
Copy link

Choose a reason for hiding this comment

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

Method delete_rels_for_nodes has a Cognitive Complexity of 7 (exceeds 5 allowed). Consider refactoring.

@codeclimate
Copy link

codeclimate bot commented Jul 22, 2020

Code Climate has analyzed commit 30171e9 and detected 0 issues on this pull request.

View more on Code Climate.

@@ -49,31 +49,20 @@ def delete_all_rels
# Deletes the relationships between all nodes for the last step in the QueryProxy chain and replaces them with relationships to the given nodes.
# Executed in the database, callbacks will not be run.
def replace_with(node_or_nodes)
node_hash = idify_hash(node_or_nodes)
node_or_nodes = Array(node_or_nodes).map { |arg| arg.is_a?(ActiveGraph::Node) ? arg : @model.find(arg) }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
node_or_nodes = Array(node_or_nodes).map { |arg| arg.is_a?(ActiveGraph::Node) ? arg : @model.find(arg) }
nodes = Array(node_or_nodes).map { |arg| arg.is_a?(ActiveGraph::Node) ? arg : @model.find(arg) }

This is guaranteed to be an array

@klobuczek klobuczek merged commit b45a569 into neo4jrb:master Jul 27, 2020
@klobuczek klobuczek deleted the issue-1611_fix_update_relations branch July 27, 2020 06:12
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.

Relational query in wrong order
3 participants