neo4jrb / activegraph

An active model wrapper for the Neo4j Graph Database for Ruby.

http://neo4jrb.io

MIT License

1.4k stars 276 forks source link

Issue 1531 on assignment of relationship objects, deleting rels that are required to be deleted #1537

Closed amitsuryavanshi closed 4 years ago

amitsuryavanshi commented 5 years ago

…

Fixes #1531

This pull introduces/changes:

Into the way relationships are updated when we assign relationship objects eg. user.posts = [new_posts] would fist check difference in existing user.posts with new_posts and create/delete only relevant posts. Earlier implementation would delete all existing posts relationships and create new relationships.

subvertallchris commented 5 years ago

Are there performance considerations to think through? It’s been a while since I’ve looked through this code but based on your description, it sounds like this would need to get the IDs of all relationships to make its comparison. If a node has a significant number of existing rels, this could result in a large amount of data coming down the pipe.

codecov-io commented 5 years ago

Codecov Report

Merging #1537 into master will increase coverage by 0.03%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1537      +/-   ##
==========================================
+ Coverage   93.23%   93.26%   +0.03%     
==========================================
  Files         105      105              
  Lines        4757     4769      +12     
==========================================
+ Hits         4435     4448      +13     
+ Misses        322      321       -1

Impacted Files	Coverage Δ
...node/query/query_proxy_methods_of_mass_updating.rb	`100% <100%> (ø)`	:arrow_up:
lib/neo4j/active_node/query/query_proxy_methods.rb	`97.2% <0%> (+0.69%)`	:arrow_up:

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 160b5ee...79d554b. Read the comment docs.

amitsuryavanshi commented 5 years ago

@subvertallchris I followed activerecord implementation https://github.com/rails/rails/blob/master/activerecord/lib/active_record/associations/collection_association.rb#L244 Assuming they would have done some benchmarking on performance.

subvertallchris commented 5 years ago

I still have some questions about how it's loading nodes to make that comparison but I'm so uninvolved that I shouldn't even be poking my nose in at this point. Sorry for the distraction. :-)

amitsuryavanshi commented 5 years ago

@subvertallchris It is loading the nodes the same way as earlier. It is just at a few steps earlier than before. Your comments are always welcomed :)

klobuczek commented 5 years ago

I had another comment that this might be a breaking change if the relationship had properties which would currently be cleared, but with the new implementation would be preserved.

amitsuryavanshi commented 4 years ago

Closing this as this has been addressed with https://github.com/neo4jrb/neo4j/pull/1584.