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

resultDataContents #206

Closed
ekkis opened this issue Aug 1, 2016 · 8 comments
Closed

resultDataContents #206

ekkis opened this issue Aug 1, 2016 · 8 comments
Labels

Comments

@ekkis
Copy link

ekkis commented Aug 1, 2016

in looking at my distribution's lib-new/GraphDatabase.js, bottom of the cypher method, I see:

236             formats.push(format = lean ? 'row' : 'rest');
237             _results.push({
238               statement: query,
239               parameters: params || {},
240               resultDataContents: [format]
241             });

I tried looking at the sources here on github but I don't read coffee easily and couldn't even find the code, but in essence, line 236 seems to me wants to be format == so it's a comparison, not an assignment. also line 240 should likely be: resultDataContents: formats (or perhaps I'm wrong because I see formats is defined outside the function that generates the statements, so it seems to be a collector whose values are tossed...

in any case "rest" is not a valid value for the endpoint, "graph" is and I don't see a way to pass that into the code. is this borked or how do I do this?

q = { 
    statement: "match (n) return n",
    resultDataContents: ["graph"],
    includeStats: false
};  
q = JSON.stringify({statements: [q]});
db.cypher(q, function(ls) { ... });
@aseemk
Copy link
Member

aseemk commented Aug 1, 2016

Hey @ekkis,

CoffeeScript is definitely terse in cases like this, but it's correct.

In JS, the order of operations is that a ? b : c is evaluated before x = ..., so format = lean ? 'row' : 'rest' is correctly assigning format to either 'row' (if the lean option was true) or 'rest'.

And then resultDataContents should not be formats (plural), because formats is the array of formats corresponding to the batch of (potentially multiple) queries in this request.

Finally, 'rest' is a valid value for the endpoint. It's how you get back node and relationship metadata beyond just property data, e.g. labels and native IDs.

http://neo4j.com/docs/2.3.0/rest-api-transactional.html#rest-api-execute-statements-in-an-open-transaction-in-rest-format-for-the-return

To your question at the end, node-neo4j indeed provides no API today to return data in 'graph' format. If you have this as a feature request, would you mind sharing your use case a bit? The 'rest' format does give you all the data you need, as far as I've ever been able to tell.

Hope that helps. Cheers.

@aseemk aseemk added the question label Aug 1, 2016
@ekkis
Copy link
Author

ekkis commented Aug 1, 2016

aseemk, thank you so much for the quick reply. yes, of course, it's embarrassing that I wouldn't have understood the = vs == from the start, and you're right, of course... "rest" is a valid value (I learned something new today - though the docs are not very good on what the valid list of values are).

as for my use case, I'm porting an app from using the linkurious library (which uses the "graph" format returns) to using node-neo4j. I would prefer not to have to rewrite everything but just receive the data as it expects. I haven't yet taken the time to analyze the result structure of "rest" but it is different from that of "graph". I've forked and cloned your project (wasn't sure, branch v2 or thingdom/v2?) with the intention of patching it to allow returning this format but I'm unsure: do I patch the coffee source in lib-new, or the .js source? I tried modifying both and running npm build to see which produced which but they seem to be independent of each other

@aseemk
Copy link
Member

aseemk commented Aug 1, 2016

No problem at all.

Got it. Thanks for explaining.

Here are the instructions for hacking on this library:

https://github.com/thingdom/node-neo4j/blob/v2/CONTRIBUTING.md#development

npm run build (note you need the run in there; different than npm build) overwrites the .js with the .coffee, so if you're editing the .js directly, don't run npm run build. (Modifying the .coffee is recommended for this reason, if you're comfortable doing so.)

Good luck, and let me know if I can help anymore! I'm going to go ahead close this issue, but feel free to keep posting any q's here. Cheers.

@aseemk aseemk closed this as completed Aug 1, 2016
@aseemk
Copy link
Member

aseemk commented Aug 1, 2016

Btw, the v2 branch in this thingdom/node-neo4j repo is the right branch! I really need to finish it and merge it to master. Time is just hard to find these days. =)

@ekkis
Copy link
Author

ekkis commented Aug 4, 2016

ok, I figured out how to get what I want: support for the 'graph' formatted results the API supports. it was a small patch and is lint clean, however, I've never written coffee and am not smart enough to implement the test. I added the test but cannot make it pass. help? here's the pull request: #207

@ekkis
Copy link
Author

ekkis commented Aug 4, 2016

I'm not sure that overloading the lean attribute was the right thing to do. that attribute doesn't appear in the API and it would have been better if resultDataContents had just been supported directly. I didn't want to have both and removing lean would probably have been an IBC, ergo my choice, but do please feel free to modify it as you will

@ekkis
Copy link
Author

ekkis commented Aug 4, 2016

and yes, I know about time being short... but it would be great (for me) if you could do a release to npm with the patch soon. I'm reading about npm link (thank you for pointing that out in your docs) so I can get by in the meantime

@aseemk
Copy link
Member

aseemk commented Aug 5, 2016

Awesome, @ekkis! Glad you figured this out for your needs, and thanks for opening the PR. Let's move the discussion on your approach to there.

@aseemk aseemk mentioned this issue Aug 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants