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

Query#where broken? #248

Open
smackesey opened this issue Jan 7, 2016 · 8 comments
Open

Query#where broken? #248

smackesey opened this issue Jan 7, 2016 · 8 comments

Comments

@smackesey
Copy link

I can't get Neo4j::Server::CypherNode#set_label to work. I investigated and found that the private method match_start_query returns this regardless of the actual node id:

"MATCH n WHERE (ID(n) = {ID_n}) WITH n"

Then I looked into where, which is invoked in match_start_query and found that:

Neo4j::Session.query.match(:n).where(:n => {name: 'joe'}).to_cypher
# => "MATCH n WHERE (n.name = {n_name})"

This doesn't seem like it could be right. Why does 'joe' not appear in the generated query? This appears to be the root of the problem for set_label.

@cheerfulstoic
Copy link
Contributor

The Query API automatically uses Cypher parameters in queries to help with performance and to protect against injection attacks. You can see the parameters like so:

query = Neo4j::Session.query.match(:n).where(:n => {name: 'joe'})
query.to_cypher # Gives you the Cypher
query.send(:merge_params) # Weird, I know

If you run the query MATCH n WHERE (n.name = 'joe') in your Neo4j console does it work?

@smackesey
Copy link
Author

Yes, that query works-- I understand about the params, but set_label still seems broken. In fact this seems related to another issue that I've run into.

The general problem is that some methods that update the state of a Neo4j::Node or Neo4j::Relationship don't seem to show their result on that object immediately. I can see the updated state of the object if I run a query to retrieve the same record from the database. I am seeing this with both Node#set_label and Relationship#update_props, but not with Node#update_props. Is it supposed to function this way? I don't see anything in the docs to indicate it, and I don't think I encountered this when using this library in the past.

To see what I'm talking about, try:

require 'neo4j-core'

neo = Neo4j::Session.open(:server_db)
x = Neo4j::Node.create({name: 'joe'}, :testlabel1)
puts x.labels
x.set_label(:testlabel2)
puts x.labels

y = neo.query.match(:n).where(:n => {name: 'joe'}).return(:n).first[:n]
puts y.labels

x.del
y.del

I see:

testlabel1
testlabel1
testlabel2

@smackesey
Copy link
Author

After running back through a bunch of versions, I found that Neo4j 3.0.0 produces what I would expect:

testlabel1
testlabel2
testlabel2

And throws an exception on the y.del line, also what I would expect, since x and y should be the same.

@cheerfulstoic
Copy link
Contributor

Ah, I see. Yeah, that's probably a bug. We can definitely look at that.

There's another thing I'm struggling with:

I'm currently building a new API which is focused on doing Cypher queries. In the current neo4j-core gem you can do things via Cypher queries but there is also stuff on top to work with the Java APIs provide conveniences like this.

In the new API what I've been focused on is the task of being able to provide a Cypher queries string (and optionally parameters) and getting back results as objects. Currently those are pretty dumb objects and that was by design because the random convenience methods are a bit of a hassle to maintain (as you've discovered). If you look SQL / Mongo gems (I think) you're just getting the ability to query and to get objects back.

The ActiveNode / ActiveRel modules seem to be a place to provide this functionality (like creating/updating/deleting), but obviously it's somewhat heavier than neo4j-core. We're working on making it lighter currently (part of that is my neo4j-core API work and part of that is some stuff that @subvertallchris is working on).

But maybe it would make sense to have another gem that doesn't have a lot of the fancy stuff that ActiveNode / ActiveRel has. Or maybe we should just really dig it and slice into the performance problems. The only thing that is perhaps a bit odd is that in ActiveNode you have a mapping of labels to classes, so if you do things like add_label/remove_label (which don't yet exist there) then it should potentially be turned into an object of another class. But maybe that's still a good place to do it.

Thoughts?

@smackesey
Copy link
Author

Thanks for looking into it Brian.

As for the new API, I actually like the current neo4j-core API. I looked into ActiveNode/ActiveRel a while ago and found it didn't quite suit my needs (something about the way it dealt with labels), and I wanted something that functioned as you described-- allowed me to run Cypher queries and get back simple objects, without layering additional abstractions (i.e. new classes) on top of my objects. Neo4j-core fit the bill.

But I guess I'm a little unclear on what your new API is supposed to do. As you say, neo4j-core at the moment lets you work with queries, but also has some other stuff. Are you just extracting the query-centered parts of Neo4j-core, or doing something different? Also, when you say "just getting the ability to query and get objects back", are you saying that the returned objects have no capacity to write the database?

@cheerfulstoic
Copy link
Contributor

Yeah, the new API is basically just there for making Cypher queries. It uses an adaptor pattern so when Bolt comes around in the near future it will be easy for us to throw in support for it. The current code is pretty harry for a few reasons, hence the new one. It should be pretty straightforward, still. Currently it's like;

adaptor = Neo4j::Core::CypherSession::Adaptors::HTTP.new('https://localhost:7474')

session = Neo4j::Core::CypherSession.new(adaptor)

result = session.query("MATCH (n) WHERE n.prop = {prop} RETURN n LIMIT 10", prop: value)

# or to make multiple queries with one request:

results = session.queries do
  append "MATCH (n) WHERE n.prop = {prop} RETURN n LIMIT 10", prop: value
  append "MATCH (n) WHERE n.prop = {prop} RETURN n LIMIT 10", prop: value
end

And yeah, at the moment the idea is that the raw data from those queries will be wrapped into Node/Relationship/Path objects like they are currently. It's just that those objects don't have any methods which will change the database or reflect changing state. I'm trying to figure out if it's a good idea to have those or not. It will even be that much harder to maintain those extra methods once we have the third adaptor for Bolt.

If you remember what you didn't like about ActiveNode / ActiveRel I'd love to hear about it. It may be something that we can fix

@smackesey
Copy link
Author

I see-- what you are implementing seems like it would be very useful as the basis for the Neo4J adaptor for ROM. In ROM read operations return immutable Relations, which are basically just sets of hashes. Write logic is completely separate. I am kind-of sort-of working on that adaptor right now, but I've been held up because:

  • ROM just hit 1.0 and the docs up to now have been pretty awful
  • I wanted the ability to return plain hashes from Cypher queries in Neo4j core, and I haven't yet figured out how from my exploration of the source
  • Life

I actually think that to make your new API maximally useful, you should avoid new classes altogether and simply return Hashes. Something like this:

res = session.query("MATCH (n)-[r]-() WHERE n.name = 'joe' RETURN n,r")
res.first
# => {
#      "n" => {
#        "id" => 43426,
#        "labels" => [ "label1", "label2" ]
#        "props" => {
#          "name" => 'Joe',
#          #  rest of props...
#        }
#      },
#      "r" => {
#        "id" => 23512,
#        "type" => "reltype1",
#        "props" => {
#          # props here...
#          }
#      }
#    }

At the very least, I hope there is an option to get this sort of result. I guess what I'm hoping for is a very thin client for Neo4J's REST interface.

Also, what is Bolt?

@cheerfulstoic
Copy link
Contributor

Ah, that "Life" is always a big reason ;)

I'm remembering now issue #223 . I think you'll be glad to know that as part of my refactor I put in an option to control how objects are being wrapped. You can see the specs for it here:

https://github.com/neo4jrb/neo4j-core/blob/new_cypher_api/spec/neo4j/core/shared_examples/adaptor.rb#L147

The wrap_level option takes a symbol. Either :none (plain hashes), :core_entity (the objects that I've been talking about above), or :proc (which allows you to define a wrapper_callback Proc on Node/Relationship/Path and define how the objects are wrapped. This will be used in the neo4j gem to wrap ActiveNode and ActiveRel models.

Bolt is the new protocol for communicating with Neo4j. It will be coming out with version 3.0 of Neo4j. It is socket-based and uses a compressed format called PackStream (similar to MessagePack). It should help increate the performance of Neo4j connections. You can read about it here:

https://alpha.neohq.net/docs/server-manual/bolt.html

I'm hoping that we can have full support for Bolt when Neo4j 3.0 comes out.

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

No branches or pull requests

2 participants