WebReflection / dblite

sqlite for node.js without gyp problems
MIT License
209 stars 34 forks source link

Callback of query() not called on UPDATE/INSERT #8

Closed bartve closed 10 years ago

bartve commented 10 years ago

$callback only gets filled/called when the query begins with SELECT or PRAGMA. Shouldn't it also get called after a successful INSERT or UPDATE?

I got the desired behaviour by adjusting the SELECT regex to include UPDATE and INSERT as a quick fix, but I haven't investigated possible other (negative) consequences.

WebReflection commented 10 years ago

You don't need a callback to insert or update. Everything that is not a select or pragma can be treated as if it is synchronous.

bartve commented 10 years ago

I do need a callback. You can only treat query() as synchronous when doing more queries because the queries get queued. As for other Node code execution, how will I know if an update has actually succeeded? I can register a listener for errors (which I have done), but I can't call a function on success. This is a bit strange to me.

dblite.query('UPDATE.....');
functionOnlyOnSuccess();

This won't work, will it? functionOnlyOnSuccess() will not wait for the update to complete and will run "parallel" in the next event loop slot. Even if I register an error listener, the functionOnlyOnSuccess() will be called before the error event is even emitted. So even though UPDATE or INSERT don't return an actual result that can be used in a callback, it's useful to be able to check if the query ran without errors.

WebReflection commented 10 years ago

You don't need to wait for anything. Everything that is not a SELECT or a PRAGMA won't wait for a result and when you do an update you are already sure you've done it because whatever next command you'll operate through that database will be after that update. SQLite does atomic operations so the order of queries matters. A callback wan't guarantee anything more than just a SELECT or PRAGMA after such update so yes, db.query("UPDATE..."); doNextStep();

I undertand the error concern though, that should never be the case but might happen. You need to set an error listener for that and if you want to be sure that the query didn't trigger the error I might do that for you but imho there won't ever be any validation in this wrap.

This project is about using SQLITE console through node in a simplified way ... same stuff you can do in console, you can do here. There's no such thing as callbacks on console, all you put, in order, will get out ;-)

WebReflection commented 10 years ago

the callback

WebReflection commented 10 years ago

just to remind you and myself how it was described so this is not a bug but meant by desing.

I'll keep it open to think about possible solutions. It will probably end up with different behavior only when a callback is specified. Problem is ... if there is an error, that callback will never be fired because the output won't probably be the expected one :-/

Stay tuned

bartve commented 10 years ago

whatever next command you'll operate through that database will be after that update

Exactly what I said, if your next function does a dblite query you're fine. But what if the command is NOT database related?

You need to set an `error

Errors can easily happen. Set a non-nullable field to null, etc. I haven't tried, but wouldn't that cause sqlite to ouput an error, or would it still be "data"? More importantly, on big or complex updates you need to know when it has finished. There's no way to tell now.

the callback

I have read the manual, so I knew it was by design. I just still don't get why :/

Problem is ... if there is an error, that callback will never be fired because the output won't probably be the expected one :-/

Or do it like every other Node module, pass any errors as the first param of the callback and always allow a callback. I do realize this would cause serious backward compatibility issues.

On error the normal callback doesn't fire indeed, but that's why you can add an error listener. Only one of those two will fire. In my code the same callback gets fired on error (I set it as the error handler) and success (callback), but in case of an error I pass an Error object to the function, so I know where it came from and the code in the callback function checks for an error before doing anything else.

WebReflection commented 10 years ago

But what if the command is NOT database related?

It's the same ... you can send as many input as you want without caring about the "when" 'cause behind the scene the sqlite-shell executes ordered commands.

You can do any complex thing and only once you've done you'll eventually receive any other output ... try with a massive insert and query just for ".show" after, when you'll read that, your previous query would have done for sure.

I have read the manual, so I knew it was by design. I just still don't get why :/

Because you can do many queries and commits at once without creating a callback mess to deal with (no matter with what ...)

Look at the BEGIN TRANSACTION example .. how the hell would you do that with an errback?

Thing is that SQLite has transactions to grant execution or rollbacks on errors so when in doubt, go for transactions and after select the status to be sure everything went as expected.

What I am saying is that as a wrapper I don't even know what's going on ... I send an input, receive an output and log an error if occurres ... but yeah, I have to double check that and see if an error as first argument would be a reasonable/possible solution but again, no idea how are you planning to use these callbacks during a transaction.

dblite is simple and will probably be simple by design ... also 'cause I believe you gonna have HUGE problems if you have errors in your queries, and this cannot be a wrapper of sqlite-shell issue, you know what I mean?

Anyway, as I've said, I'll keep this updated and think/test/try some possible solution. Stay tuned!

bartve commented 10 years ago

I think we have some miscommunication going on here. I'll be away for the next couple of days, but I'll write a piece of code demonstrating my point when I'm back.

I like dblite for its simplicity (and portability) and you should definitely keep it that way :)

WebReflection commented 10 years ago

Anyway, version 0.3.3 should invoke the callback, if specified, no matter what kind of query it is ( technically even after a BEGIN TRANSACTION and no idea what would happen ^_^ )

Is this good enough? Cheers

bartve commented 10 years ago

Is this good enough?

Excellent! Looking at issue #9 this is somewhat related indeed. Already replaced my hacked dblite with 0.3.5 and it works like a charm. Thanks!

WebReflection commented 10 years ago

FYI, version 0.4.0 has the err variable. Callbacks there work either with a single argument, or double.

function (rows) {} will keep working as expected for now

function (err, rows) {} will receive the error buffer if returned, null otherwise.