Code coverage

[mschae]

Hey @florinpatrascu,

this PR changes the return value when structs (such as nodes, relationships) are returned.

Formerly a struct would just be [sig: <signature>, fields: <fields>]. Now I changed it to return one of Boltex.Structs, e.g. %Boltex.Struct.Node{id: <id>, labels: <labels>, properties: <properties>}. Regular (/unknown) structures will just be %Boltex.Struct.Generic{signature: <signature>, fields: <fields>}.

This is still WIP (I'm going to add the two remaining documented struct types and renume from the current Boltex.Structs to Boltex.Struct but I wanted to discuss if that change is ok with you. If the new structs are a dealbreaker I can easily roll that back. The late addition was caused that only now is it properly documented in the bolt procotol, at which point I wanted to turn it into proper data structures.

Other than that I improved run_statement so that it stops as soon as it detects an error (formerly it would still send the PULL ALL message, resulting in an :ignored by the server. I adjusted ack_failure (and the newly added cancel) accordingly, so it should not need any changes on your end.

Let me know!

Thanks, Michael

[florinpatrascu]

ouch :( This: Now I changed it to return one of Boltex.Structs, e.g. %Boltex.Struct.Node{id: <id>, labels: <labels>, properties: <properties>}. ... totally throws away my implementation :( Sorry. I implemented all the Neo4j structures, node, Rels, Paths, etc, starting from the raw results of Boltex and added a ton of tests too, to make sure I am not going sideways. Ugh .. let me think, but it is pretty much rendering half of my code unusable. On the other hand, I definitely don't want to block your development strategy, if this is where you want to take Boltex. I can fork it again and maintain my own version until I can find the time to refactor Bolt.Sips, no problem, just let me know. Many thanks for the heads-up!

[florinpatrascu]

I you need Nodes, Rels, etc., why don't you want to use Bolt.Sips? :)

[mschae]

He, that's why I wanted to discuss this early.

I actually don't think this changes too much. You do a lot more, like adding the field name in maps, etc.

Just an example, here is a before and after:

Before:

# MATCH (n) RETURN (n)
[
  success: %{"fields" => ["n"]},
  record: [[sig: 78, fields: [0, ["Test"], %{"foo" => "bar"}]]],
  success: %{"type" => "r"}
]

After:

# MATCH (n) RETURN (n)
[
  success: %{"fields" => ["n"]},
  record: [%Boltex.Struct.Node{id: 0, labels: ["Test"], properties: %{"foo" => "bar"}}], 
  success: %{"type" => "r"}
]

As you can see, the basic return structure is almost unchanged. Just the value of the struct have changed.

Either way, I think now that structures are properly defined in the protocol I do want to return the corresponding data structures at some point. However: I also realise that it was very ad-hoc. How about I pry the struct implementation into a different PR and wait with it until you can take a look?

[florinpatrascu]

Sure, thank you.

[mschae]

Alright, this branch is refactored to only include the connection fixes (amongst others) and tests.

As always, I would appreciate if you gave this a go. :)

The structs part is optional and we can get there when we get there. :)

[florinpatrascu]

Thank you, Michael. I'll check this as soon as I get home; was heading out for some beers. Family Day here, and used most of it for working on Bolt.Sips but I'll get back to it as soon as I return :) Thanks again!

[coveralls]

Coverage increased (+11.8%) to 82.993% when pulling 945971b24b2ab4e4031e23cb426a24e228442a68 on mschae/improved-connections into 4077c6cb858df4e9de8f27cd0024b6638a543934 on master.

[mschae]

Awesome, no rush, enjoy the well deserved beers. Like I said this is now only improved query running and expanded test coverage and documentation. Should not be a breaking change, do let me know if it is.

I did discover some more minor bugs when writing tests, so this PR is still critical.

I'll work on the structs elsewhere but there is absolutely no pressure from my end to get that changed anytime soon. And again, I am certain it won't render Bolt.Sips obsolete as it merely changes one data structure, where Bolt.Sips is doing so much more.

[florinpatrascu]

I though above I commented on a specific sha, but it seems it went to the main thread. So much for using the iPhone for this :) Thank you, Michael, I'll definitely integrate the commit as soon as possible :)

[florinpatrascu]

(my wife will kill me now, had to stop the car to write this comment as the thought I just had is not letting my brain relax :)) What if rather than using Boltex in Bolt.Sips, we rather join the efforts, I bring some of Bolt.Sips functionality in Boltex (if you thing is worth it) and make Boltex the better drive instead? Mind you, I wanted to write this before the beers :) Bolt.Sips is not sponsored by anyone, so no contractual obligations there, is just the result of my passion for programming and the intention to do well for the OS community - I don't mind not having my name on a driver, if that driver has a better chance to become the best one instead :) With this I'm done for few hours, at least. 🍻

[coveralls]

Coverage increased (+18.7%) to 89.928% when pulling 4897b8b4559025e56003aa9a5a184ef9e018762f on mschae/improved-connections into 4077c6cb858df4e9de8f27cd0024b6638a543934 on master.

[mschae]

Ha, now I'm scared to write back. Don't want to get you into more trouble and/or keep you from beers.

Either way, I generally much like the idea of bringing you in on boltex. That being said I do like the division of Boltex and Bolt.Sips (especially around transactions, error handling, connection management, etc.). So while I am very open to adding you as a maintainer and keep improving Boltex, I still like the idea of keeping it low level and Bolt.Sips being the more advanced solution.

I hope that makes sense. I'll think on this a bit more, let's discuss tomorrow in more detail when you have time again. :)

[coveralls]

Coverage increased (+18.7%) to 89.928% when pulling cc2ab2cefb556775b2048ee526f57e6248a75af0 on mschae/improved-connections into 4077c6cb858df4e9de8f27cd0024b6638a543934 on master.

[florinpatrascu]

If Boltex's responsibilities will be to just implement the Bolt protocol, at low level, then indeed, we don't have to mix the two. I too like the separation of concerns between the two, unless they start to overlap too much, resulting in just confusing the user. The idea was to avoid working on parallel threads to end up arriving to the same point, but at different times :) Anyway, even if we'd like to join the two, I'd still need to maintain Bolt.Sips for a while, to allowing the users a smooth transition, so no matter what, I have to make sure Bolt.Sips has the latest Boltex incorporated properly. Therefore, if you can estimate how far you planning to go with the struct-datatypes branch, then I'll start looking into using it in Bolt.Sips and refactor/replace the support for the raw Bolt structures (and related tests) - just need to find a couple of week-ends for that. Ok, I'll go check this branch now.

[florinpatrascu]

getting lots of errors but I guess I'll better wait till tomorrow and have a better look at this. One question tho': do I still need to call Boltex.Bolt.ack_failure/3 at my end in case or errors? Because I still do, and this is where most of the exceptions are happening. Anyway, I'll check again tomorrow, .. fresh eyes :P

[mschae]

Sorry to hear it.

Yes, you do still need to call ack_failure. The only difference is that it doesn't first expect an :ignored message, because that was caused by the pull all that we are now omitting when the query itself already yielded an error.

I'm happy to help if you provide instructions for me to test the new branch.

As for your comment above: Full ack! I do think it's best to keep Boltex low level and I do think it's best for Bolt.Sips to stay around for the foreseeable future. If the new structs are too advanced for a low level driver we can just shelf that idea for now.

Thanks!

[coveralls]

Coverage increased (+14.9%) to 86.131% when pulling a66756c2aef78a35759753ad7715f06d31c94d07 on mschae/improved-connections into 4077c6cb858df4e9de8f27cd0024b6638a543934 on master.

[mschae]

Hey @florinpatrascu,

since this PR covers some crucial bug fixes and test coverage I decided to roll back the improved connection handling. That part is now separated out into #17.

That means that this branch should integrate into Bolt.Sips without issues. We should still strive to integrate #17 asap, but this buys us a little time. Once again, feel free to let me know if you want help integrating #17.

Thanks, Michael

[florinpatrascu]

Hi @mschae - sorry, couldn't spend more time last night on this, and now I'm at work. However, last night I stopped after realizing that your last commit had an issue in run_statement the line with List.last(more_data) ... sorry for the lack of better details, just looking at my notes from last night. Probably this new commit replaced that?! The problem last night was when I was starting transactions. I.e. Sending a 'BEGIN' command, the return in that case is not a list, hence the error in Boltex. But I couldn't continue, as it was already too late. Will look today again, in the afternoon and let you know how it goes. Thank you.

[mschae]

Hey @florinpatrascu,

Thanks so much, I didn't account for transactions! Will add a test with transactions and fix the other PR.

To clarify: The last commit just rolls back the "improvement" which you found out to introduce a bug. So I went back to what we have in master and just added some code coverage (and the reset) function.

So yeah, if this looks good let's merge and get out. I still want to improve connection handling but I'll first make sure it works with transactions.

Thanks again

[florinpatrascu]

I believe #17 was mostly working, but since my driver is using transactions intensively, I was overwhelmed by the flurry of errors, definitely side effects of the broken transactional support, and couldn't continue debugging further after I spotted that error in the run_statement, due to lack of time. I'll test as soon as I get home and let you know the results, I'm sure it should work with these last set of changes. Sorry for the delay, the time zone differences suck, when you don't need them :)

[mschae]

@florinpatrascu The transaction-related error is now also fixed in the other branch.

So whatever is easiest for you: You can test this branch first (which should be easy) and then move on to #17 or you can try #17 rightaway (which will just render this PR obsolete).

No rush at all. I broke these out to give you all the time you need on #17 as this is the only PR that contains somewhat critical fixes around bigger records (hence nodes or edges with lots of data).

[mschae]

PS: I added a test in #17 to make sure it works in transactions, so that bug shouldn't happen again.

[florinpatrascu]

Thank you! I'll test with this one, of course but like yourself I'd rather focus on making sure #17 works too an use the latter instead, as the changes you introduced are needed and the code looks much better. After debugging it I became more familiar with #17 therefore I'll focus on it :)

[mschae]

Cool I'll go ahead and close this one then for the time being. If we see that the "improved" connection handling yielded more errors we can go ahead and re-open this.

Thanks again, looks like the test coverage in Bolt.Sips saved my ass. :) Will continue to work on testing more cases in Bolt so I can catch those issues up-stream.

[florinpatrascu]

I just finished testing the integration with mschae/improved-connections, and it woks well. I'll start looking into #17, and comment there about my findings. Thank you!