Fix issue in association_proxy with == objects

[yourpalal]

• rename hash variable to proxy_hash_key
• rename cache variable to association_query_results
• create proc on its own line, with simpler code
• instead of using inject and complicating things, use each + self.association_proxy_cache[proxy_hash_key]. This also means that things won't go wrong if 2 objects in the query are == to each other but have different query results

Fixes #1529

This pull introduces/changes:

• edits association_proxy method in HasN and makes it a bit clearer, and fixes #1529
• see commit message for more detailed breakdown

Unfortunately, I'm not really sure how I could add a test for this issue in the repo. With some guidance I may be able to add one. The key would be making at least 3 database objects, 2 of which are == (see the pseudo-code in #1529)

[klobuczek]

@yourpalal strictly speaking you don't need to add specs for equivalent backwards compatible code refactoring. However it looks like you are trying to overcome a bug here. The gem cannot work without the id_property (either uuid or neo_id). I think the right solution here is some kind of exception when an object without such an id is found. You can create a spec for the case by inserting records without uuid into the database with pure cypher. I have already fixed the 1 failing spec in your PR I probably am going to merge it today, so please refresh master later.

[klobuczek]

@yourpalal I am just thinking.. maybe another solution could be ignoring all records without id_property. @amitsuryavanshi thoughts?

[yourpalal]

Ok, I'll give it a go and add in a test case. This might take a bit because I'll need to install vagrant etc.

As for ignoring records without id_property set, I know that the id_property is essential, and that was an issue in our codebase. However, this was only the condition that exposed the bug in the neo4j.rb code. The same underlying issue would come up any time that two objects (representing different neo4j nodes) are == . I know it's a little silly, but I think that since the code can be written not to depend on the fact that each node in neo4j is == only to itself, we may as well write it that way. IMO the change also makes the code a bit less complicated. Essentially, simpler code with fewer demands on its input is what I was going for here.

I think raising an error when nodes are found without an id could be good, but it would kind of also break backwards compatibility, so it might require some more discussion IMO. For instance, if we upgraded to a new version of the gem that raised an error in that scenario, we would have discovered this issue sooner, but in a pretty unpleasant way. Then again, this bug meant that query association results were being mixed up & coming back incorrect, so that's pretty unpleasant too! Perhaps if such an error is added, a rake task should also be added that generates uuids for all instances of a model where the uuid field is blank. eg.

Neo4j::ActiveBase.run_transaction do
  model_class_name.constantize.where(uuid: nil).find_each do |n|
      n.update! uuid: SecureRandom.uuid
  end
end

[amitsuryavanshi]

I am thinking, what percentage of database users would have this kind of issue of nodes without id_property. It could be very very less, so IMO, we will be supporting 99% or even more users if we ignored nodes without id_property.

[yourpalal]

Just to be clear, when discussing "ignoring nodes without id_property" do you mean in the association_proxy method in particular? If so, I need to point out that not creating association proxies for the nodes with a blank id will not solve this bug. The fact that id is relevant is due to two things:

1. implementation detail of the method (using == and reduce to determine the return value)
2. default implementation of == (based on id property)

Of these two issues, 1. can be fixed very easily in the gem while making the code simpler/easier to read. The implementation of == is ultimately up to users and although it's not generally going to be changed, it's really irrelevant to what this method does, so why base the correct functioning of association_proxy on the implementation of ==?

[klobuczek]

@yourpalal a word of advise: if you created a PR without all the renaming and reformating, just fixing the problem with the least character changes your PR would've been merged long time ago. It is just too hard to see the significant changes. I know that this might not be the most satisfying way, but 2 PRs (one with the fix and another one with the esthetics) would probably be the most successful.

[yourpalal]

@klobuczek Ok! I got the impression this change wasn't wanted. I'll split it up into 2 PRs, and force-push the smallest possible change to this branch.

[codecov-io]

Codecov Report

Merging #1530 into master will not change coverage. The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #1530   +/-   ##
=======================================
  Coverage   93.33%   93.33%           
=======================================
  Files         104      104           
  Lines        4815     4815           
=======================================
  Hits         4494     4494           
  Misses        321      321

Impacted Files	Coverage Δ
lib/neo4j/active_node/has_n.rb	98.4% <100%> (ø)	: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 291c79a...9a00095. Read the comment docs.

[yourpalal]

Style changes extracted into #1547. Probably that one will have a little merge conflict after this change is merged, but I can fix that when the time comes.

[klobuczek]

@yourpalal really sorry for taking that long to review this. Could you merge master to your branch and add a spec that fails on master but passes on your branch. That would be extremely helpful. We have fixed few problems since then around some relationships not being deleted when using the association= methods, so there is a chance this problem is fixed. I have looked into #1529 but the case is not as perfectly clear as a spec would be.

[yourpalal]

Hi @klobuczek I will try to get it updated soon :) Thanks for getting back to me!