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

Neo4j::ActiveNode::Query::QueryProxy bug with .first and .last methods #1486

Closed kopylovvlad closed 6 years ago

kopylovvlad commented 6 years ago

QueryProxy's methods .first and .last always return first element. It makes me sad

Additional information which could be helpful if relevant to your issue:

Code example (inline, gist, or repo)

users = User.all
users.size
# => 12 
users.class 
# => Neo4j::ActiveNode::Query::QueryProxy 
users.first.id
# User 
#  MATCH (n:`User`)
#  RETURN n
#  ORDER BY ID(n)
#  LIMIT {limit_1} | {:limit_1=>1}
# HTTP REQUEST: 3ms POST http://localhost:7575/db/data/transaction/commit (1 bytes)
# => 47 

users.last.id
# User 
#  MATCH (n:`User`)
#  RETURN n
#  ORDER BY ID(n)
#  LIMIT {limit_1} | {:limit_1=>1}
# HTTP REQUEST: 2ms POST http://localhost:7575/db/data/transaction/commit (1 bytes)
# => 47 

users.to_a.first.id
# => 47 
users.to_a.last.id
# => 393 
users[0].id
# => 47 
users[-1].id
# => 393 

Runtime information:

Neo4j database version: neo4j gem version: 9.1.4 neo4j-core gem version: 8.1.1 ruby verion: 2.4.1

jorroll commented 6 years ago

Hmmm, looking at the cypher you included with your issue, it appears the problem is that both user.last.id and user.first.id are ORDER BY ID(n).

I imagine a fix is updating user.first.id to be ORDER BY ID(n) ASC and user.last.id to be ORDER BY ID(n) DESC.

I think I've run into this issue a few times myself, though I never diagnosed what was going on.

jorroll commented 6 years ago

Hmm, looks like the #last() definition already is suppose to be ordering in DESC. https://github.com/neo4jrb/neo4j/blob/d3a2412a45c551225b24f4a21760183c3f7e561e/lib/neo4j/active_node/query_methods.rb#L19-L22

jorroll commented 6 years ago

Ah ok, so while the active node .last() query method seems to correctly order DESC, the query proxy .last() query method does not appear to do this.

You can check out QueryProxy#last which actually delegates most of its logic to QueryProxy#first_and_last. I'm not sure what the FIRST and LAST constants are suppose to be doing, but when the query proxy object doesn't currently have any order clause, those two arguments appear to be ignored by first_and_last.

The first_and_last method should probably be updated to something like

def first_and_last(func, target)
  new_query, pluck_proc = if self.query.clause?(:order)
                            [
                              self.query.with(identity),
                              proc { |var| "#{func}(COLLECT(#{var})) as #{var}" }
                            ]
                          elsif func == LAST
                            [
                              self.order({order_property => :DESC}).limit(1),
                              proc { |var| var }
                            ]
                          else
                            [
                              self.order(order_property).limit(1),
                              proc { |var| var }
                            ]
                          end
  query_with_target(target) do |var|
    final_pluck = pluck_proc.call(var)
    new_query.pluck(final_pluck)
  end.first
end

I'm not 100% sure about the .order() syntax, but this is probably a pretty easy PR to put together if you're interested in fixing this.

kopylovvlad commented 6 years ago

@thefliik thank you, for your response. I am happy that you know how to fix this bug. I created PL to master for fix. Here is it: https://github.com/neo4jrb/neo4j/pull/1494

cheerfulstoic commented 6 years ago

1494 merged and fix released as 9.2.1. Feel free to comment if something still isn't right