node-neo4j icon indicating copy to clipboard operation
node-neo4j copied to clipboard

v2 implementation!

Open aseemk opened this issue 11 years ago • 39 comments

Fixes #143.

API docs: https://github.com/thingdom/node-neo4j/blob/v2/API_v2.md

Core:
  • [x] GraphDatabase class, incl. w/ custom options (e.g. proxy, agent, etc.)
  • [x] HTTP requests (fixes #100), including w/ custom headers (fixes #97; fixes #13)
  • [x] Node & Relationship classes; parsing responses
  • [x] Error classes; best error reporting ever (fixes #73)
Cypher:
  • [x] New endpoint
  • [x] Transactions (fixes #111) + helpful state tracking
  • [x] Batching (manual; incl. w/ transactions)
Schema management:
  • [x] Labels
  • [x] Properties
  • [x] Relationship types
  • [x] Indexes
  • [x] Constraints
  • ~~Legacy indexing~~ (punted)
Advanced:
  • [x] HTTP streaming (incl. w/ proxy support, gzip decompression, etc.)
  • [x] Neo4j 2.2 auth management (docs)
  • ~~Cypher streaming (issue #50)~~ (punted)
  • [ ] Remove streaming by default to support promises (and thus ES7 async/await)
Testing:
  • [x] Neo4j 2.1
  • [x] Neo4j 2.2 (forced password reset, changed error formats)
  • [x] Neo4j 2.3 (changed transaction error effects, removed index hint failure)
  • [x] Node 0.10
  • [x] Node 0.12
  • [x] io.js (fixes #149)
  • [x] Node v4 & v5
Documentation:
  • [x] Readme update (and possibly a deeper "getting started")
  • [ ] Full/proper API docs (as opposed to the current spec)
  • [x] Changelog / v1 migration guide
  • [x] node-neo4j-template update (https://github.com/aseemk/node-neo4j-template/pull/18)
  • [x] Code style (via hooked-up CoffeeLint)

aseemk avatar Nov 07 '14 09:11 aseemk

My two cents: examples should be standard javascript without streamline. I had to look up the documentation for what streamline does to understand your docs.

Also worth asking: what about streaming the response JSON? It looks like Oboe.js supports reading an existing HTTP stream (docs), but not in the browser. Is that fine?

Oboe.js does work in the browser :)

It seems to be implied that transaction requests immediately (i.e. synchronously) return a stream. This is not technically correct. Due to buffering and batching it is currently asynchronous.

tx.cypher {query, params, headers, raw, commit}, _

I also would prefer that headers, raw, and commit were bundled into an options param. That way you aren't forced to use null params if you want to commit without headers or raw.

brian-gates avatar Nov 07 '14 16:11 brian-gates

My two cents: examples should be standard javascript without streamline.

Great point. Will fix.

Oboe.js does work in the browser :)

Does it have to be the one making the HTTP request though? The API docs seem to imply that the "bring your own stream" feature is only in Node — but do you know if that's actually true in practice?

http://oboejs.com/api#byo-stream

It seems to be implied that transaction requests immediately (i.e. synchronously) return a stream. This is not technically correct. Due to buffering and batching it is currently asynchronous.

Really? This example shows cypher.transaction() returning a stream right away:

https://github.com/brian-gates/cypher-stream#query-batching

I also would prefer that headers, raw, and commit were bundled into an options param. That way you aren't forced to use null params if you want to commit without headers or raw.

I'm not sure if I'm misunderstanding you, but typically, options in options objects can simply be omitted if they're not needed. So you'd never be forced to pass null params.

E.g. see jQuery.ajax(settings): http://api.jquery.com/jquery.ajax/

I'll clarify this in the doc though. And the current doc is just a rough outline of the API, but the final/proper API docs will definitely specify precisely which options are needed vs. which are optional. =)

aseemk avatar Nov 07 '14 17:11 aseemk

Does it have to be the one making the HTTP request though? The API docs seem to imply that the "bring your own stream" feature is only in Node — but do you know if that's actually true in practice?

I've only ever tested it where Oboe is making the request. That works. Not sure about anything else.

Really? This example shows cypher.transaction() returning a stream right away: https://github.com/brian-gates/cypher-stream#query-batching

The transaction stream is synchronous. Streams per statement are asynchronous, which is what your documentation appears to be referring to.

https://github.com/brian-gates/cypher-stream#stream-per-statement

I'm not sure if I'm misunderstanding you, but typically, options in options objects can simply be omitted if they're not needed. So you'd never be forced to pass null params.

Oh, ignore me, that's just CoffeeScript throwing me for a loop :)

brian-gates avatar Nov 07 '14 22:11 brian-gates

Cool, thanks. I won't worry about Oboe in the browser then. Non-"mainline" scenario aside, there's also nothing fundamentally limiting; it should always be theoretically possible to still stream the JSON parsing even if you're not the one making the HTTP request.

Great point about statement streams. Any reason though that a stream couldn't be returned immediately, and "wired up" under the hood later whenever the underlying CypherStream is emitted?

(This assumes that we'd provide our own wrapper stream rather than the underlying CypherStream directly, but I think that's probably a wise API thing to do anyway. Or maybe we document that we're returning a CypherStream directly, and we just proxy all events, etc.)

Thanks for all the great feedback again!

aseemk avatar Nov 07 '14 22:11 aseemk

Any reason though that a stream couldn't be returned immediately, and "wired up" under the hood later whenever the underlying CypherStream is emitted?

The streams API prevents me from returning anything synchronously from my write.

That said, I was planning on adding support for accepting a stream to pipe to as an alternative to callbacks. This would allow your library to generate and return a proxy stream immediately:

var stream = new require('stream').PassThrough(); // or you could use highland.
cypher({ statement: statement, pipe: stream });
return stream;

brian-gates avatar Nov 07 '14 22:11 brian-gates

Nice. An API like that would be great. Thanks Brian.

aseemk avatar Nov 07 '14 23:11 aseemk

One concrete area where the current v2 API is lacking is query batching: sending more than one query (statement) in a single HTTP request.

That can always be layered in as an additional, new API (e.g. db.beginBatch() or even tx.beginBatch(), similar to transactions), but I'm wondering if it'd be better to support it better.

How important is batching to people? What are the use cases?

(From our side at @FiftyThree, we never batch, and in fact have even been encouraged not to, in one case where we parallelize five queries instead. Batched queries run serially.)

aseemk avatar Jan 30 '15 23:01 aseemk

Even automatic batching under the hood will run into complexity given request headers, as I realized in https://github.com/brian-gates/cypher-stream/issues/10#issuecomment-72090757. So it'd probably need to be explicit, at least for our cases (so that e.g. we could have a single "query name" header for the entire batch of queries).

But even so, how valuable is automatic batching under the hood, given Neo4j 2.2's performance improvements coming down the pipe?

/cc @jexp if you have any insight. Thanks!

aseemk avatar Jan 30 '15 23:01 aseemk

From my experience, batching is exceedingly useful in very specific and known circumstances. Stuff like import scripts. In such situations, simple automatic batching could just as easily require explicit invocation if the implementation required it.

It might be useful to intelligently batch queries based on some provided transaction-safe specifications. For example, an API might benefit from batching requests to GET /users, GET /comments, and GET /posts. A singleton database instance could easily debounce and batch many queries to one. How much of a benefit this might actually have has yet to be tested. Higher traffic would yield greater returns of fewer HTTP requests at the cost of a debounce period. Maybe more intelligent query caching would be seen as well within Neo4j if duplicate queries were run simultaneously.

In any case, it would seem query-specific headers via transactions is not something easily (or possibly?) supported.

brian-gates avatar Feb 02 '15 05:02 brian-gates

Thanks @brian-gates. Still chewing on your comments about automatic/intelligent batching, but also still curious how much of it is still applicable with Neo4j 2.2's perf improvements. cc @jexp

Had a great conversation with fellow FiftyThirds @chanadian, @ryanwe, and @gasi last night thinking through manual/explicit batching. I'm convinced it's valuable and not hard to add now; will do so.

In any case, it would seem query-specific headers via transactions is not something easily (or possibly?) supported.

Can you clarify? Wasn't sure if you were referring to the API design here. Query-specific headers in transactions are supported, if so. =)

var tx = db.beginTransaction();
tx.cypher({query: '...', headers: {'X-Foo': 'Bar'}}, cb);
// then later:
tx.cypher({query: '...', headers: {'X-Foo': 'Baz'}, commit: true}, cb);

Edit: but I agree with batching: headers are per batch, not per query.

aseemk avatar Feb 06 '15 14:02 aseemk

Query-specific headers in transactions are supported

Oops, I meant batches! You're right.

Still chewing on your comments about automatic/intelligent batching.

A concrete example might help:

cypher('match (n:User) return n', { channel: 'read' });

// in some other file/route
cypher('match (n:Comment) return n', { channel: 'read' });

If both of those were run within a very small time window, they could be safely batched together.

brian-gates avatar Feb 06 '15 18:02 brian-gates

Got it, @brian-gates, thanks for the example. I think I'll hold off on automatic batching for now, but should be possible to add it anytime. I'd love to see Neo4j 2.2 numbers first.

Implemented manual batching — thanks @ryanwe, @gasi, and @chanadian:

https://github.com/thingdom/node-neo4j/blob/v2/API_v2.md#batching

And with that, all of callback-based Cypher + transactions is implemented and tested!

https://travis-ci.org/thingdom/node-neo4j/jobs/49912854

Published the current snapshot to npm as 2.0.0-alpha1 if anyone wants to try it out. Documentation still light right now, but the tests show lots of usage:

https://github.com/thingdom/node-neo4j/tree/v2/test-new

Feedback welcome!

aseemk avatar Feb 08 '15 03:02 aseemk

One thing I've been feeling is that this v2 API should be consistent in its naming abbrevations. Right now, we have property and properties, but params and config.

I think parameters and configuration are a bit much (and same with statement vs. query), and React has shown the convenience of props.

The only one I'm unsure of is prop vs. property for indexes and constraints...

I'm willing to move to prop for the consistency, but thoughts/feedback welcome.

aseemk avatar Mar 07 '15 17:03 aseemk

Hmm another ideas for a short version would be key(s) when talking about the property-key and prop(s) for the actual property? As I've seen some confusion about property used for the key and/or the values or pairs.

For indexes/constraint (creation) it might be ok to go with key there as well?

jexp avatar Mar 08 '15 10:03 jexp

Good point @jexp — I actually am already using key and value for cases that require both (e.g. legacy indexing), which is another inconsistency.

For JavaScript, the name properties (or props) is inevitable and required, for the dictionary/hash case (e.g. node.properties.uuid). Neither keys or values would be appropriate there.

aseemk avatar Mar 08 '15 17:03 aseemk

node-neo4j v2 is now feature complete! Published 2.0.0-RC1 to npm.

Over the last week, added support for Neo4j 2.2 auth management (e.g. changing passwords), implemented index and constraint methods, and tightened errors and transactions up even more.

At this point, all 100+ tests pass across both Neo4j 2.1 + 2.2 and Node 0.10, 0.12, and even io.js! =) Node 0.12 and io.js have just one HTTP streaming bug that I'm still trying to figure out, but that's it.

There's a small list of minor improvements I'm considering making, but I consider most of the implementation to be done now. (Legacy indexing and Cypher streaming will probably come post-release.) I'll be focusing my attention next on documentation and the template app.

So try this out if you haven't already, and feedback welcome!

aseemk avatar Mar 12 '15 04:03 aseemk

Wow @aseemk and @brian-gates this is really impressive. Great effort. Thanks so much for making it happen. Would you mind writing a blog post and helping us putting the right information on the neo4j.com developer pages? (As well as implementing the tiny demo application there) ?

Thank You

jexp avatar Mar 12 '15 07:03 jexp

I believe I can take no credit here, to be fair =)

On Thu, Mar 12, 2015 at 12:07 AM, Michael Hunger [email protected] wrote:

Wow @aseemk and @brian-gates this is really impressive. Great effort. Thanks so much for making it happen. Would you mind writing a blog post and helping us putting the right information on the neo4j.com developer pages? (As well as implementing the tiny demo application there) ?

Thank You

Reply to this email directly or view it on GitHub: https://github.com/thingdom/node-neo4j/pull/145#issuecomment-78433220

brian-gates avatar Mar 12 '15 07:03 brian-gates

Thanks @jexp!

I will definitely implement that demo app. Thanks.

aseemk avatar Mar 29 '15 04:03 aseemk

This is awesome. Great work!

gpierrick avatar Jun 11 '15 15:06 gpierrick

Thank you, @gpierrick! I appreciate it.

aseemk avatar Jun 12 '15 01:06 aseemk

Is v2 beta usable in development? Is the test coverage good enough to not expect crazy errors?

Just starting with neo4j, so thanks for providing awesome bindings in node!

brigand avatar Jun 26 '15 06:06 brigand

We've been running v2 in production at FiftyThree for half a year now. No issues. Enjoy. =)

aseemk avatar Jun 26 '15 14:06 aseemk

Hi @aseemk when do you think this can become an official final release?

Also I pinged you via email/skype about one issue :) I know you're busy.

jexp avatar Jun 29 '15 07:06 jexp

Great! Thanks for the response.

When I get a better handle on it I'll try to help with the docs.

brigand avatar Jun 29 '15 07:06 brigand

Hey @jexp: (good) docs are the only thing remaining. I keep meaning to get to them, just so hard to find combined time + energy. (E.g. going through apartment application + moving process.)

I'm flying to Seattle next Monday, so I think that offline time on the plane could be a good opportunity to just sit down and hopefully knock this out then. =)

aseemk avatar Jul 06 '15 14:07 aseemk

Btw @jexp, I didn't get any email from you? (And I'm not usually on Skype, but always on gchat.) Sorry if I missed it, though!

aseemk avatar Jul 06 '15 14:07 aseemk

Something to consider based on this Stack Overflow q: should node-neo4j proactively bind GraphDatabase.prototype methods to instances when they're constructed? Instantiation perf is almost certainly irrelevant for the GraphDatabase class, and it might be convenient.

aseemk avatar Aug 05 '15 22:08 aseemk

Is there any plans about the release date?

gyzerok avatar Aug 24 '15 12:08 gyzerok

No date: it'll be merged as soon as the docs are finished.

Sorry that's taken me a while — I'm slow at writing prose to begin with, and it's been hard to find dedicated time lately with life things.

But, I made some good progress this weekend. Sneak peeks:

screenshot 2015-08-24 08 27 52
[...]
screenshot 2015-08-24 08 29 16

I want to make the docs really good, because v2 is packed with a lot of goodness that isn't obvious. I also want to do my part to help people write robust code from the start.

Hope that helps. Thanks for your patience. =)

P.S. But if I may ask, does not having this "officially" released make a big difference for you?

aseemk avatar Aug 24 '15 15:08 aseemk