WebReflection / dblite

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

send a misspelled query doesn't return error string #22

Closed risingphoenix closed 9 years ago

risingphoenix commented 10 years ago

Hi

if I send a query like this:

db.query('select jhgfkjhg * from users',function(err,data) {
    console.log(err);
    console.log(data);
});

err is null and data is an array of 0 elements.

even if a set a global listener db.on('error'... the error inside the closure is an array of 0 elements.

if I send a query like this:

db.query('select jhgfkjhg * from users',null,{id:Number},function(err,data) {
    console.log(err);
    console.log(data);
});

returns:

Uncaught node.js Error 
TypeError: Cannot call method 'forEach' of undefined
    at Socket.eval (... \node_modules\dblite\build\dblite.node.js:296:25 ...)

I run all my code under node-webkit

I do something wrong? Thanks for your code

WebReflection commented 10 years ago

This is what I have in console

> var db = require('dblite')(':memory:');
undefined
> db.query('create table users (id INTEGER PRIMARY KEY)');
{ domain: null,
  _events: {},
  _maxListeners: 10,
  close: [Function],
  lastRowID: [Function],
  plain: [Function],
  query: [Function] }
> db.query('select jhgfkjhg * from users', function(err, data){console.log(err);console.log(data);});
{ domain: null,
  _events: {},
  _maxListeners: 10,
  close: [Function],
  lastRowID: [Function],
  plain: [Function],
  query: [Function] }
> [Error: Error: near line 2: near "from": syntax error
]
null

So everything works for me. I need to know which version of node.js and sqlite3 you are using in order to guess more.

Last, but not least, dblite works that whatever you don't need, should not be used. As example, your last operation would expect an array of properties, not null … you don't have properties in the query, don't pass them or pass an empty Array instead of null.

Query error a part, this would be fine db.query('select jhgfkjhg * from users',{id:Number},callback);

as well as this one db.query('select jhgfkjhg * from users',[],{id:Number},callback);

Have a look at the list of overloads in the README.md page ;-)

risingphoenix commented 10 years ago

Thanx for the reply

My OS il Win 7, Node version is 0.10.22 and the SQLite is the the windows command-line 3.8.2. Currently I am using your module inside node-webkit version 0.8.1... maybe that is the problem.

The same error happens if I change my the params with an empty array. The error disappears only if I change the fields argument to null or an empty array.

If I run your last command db.query('select jhgfkjhg * from users', function(err, data){console.log(err);console.log(data);}) the result is always: null and []

Tomorrow I'll do more tests... Many Thanks

WebReflection commented 10 years ago

OK .. so, do not send arguments you don't need. Latest example is good, use that and forget the null arguments since it is not supported. Database speaking, dblite assumes you know what you are doing ;-)

That being said, it looks like node-webkit does not write to the stderr so dblite is unable to catch that.

can you try with both on('error') and a query such:

dblite(':memory:')
  .on('error', function(){
    console.log(arguments);
  })
  .query('qwoidj qwid jojdoqiw dj', function() {
    console.log(arguments);
  })
;

and tell me if anything at all happens? If not, I am not sure there is anything I can do :-(

risingphoenix commented 10 years ago

The only console.log that fires is the second and logs {"0":null}

I've tried to debug your code (only for good purpose ;) ) and the spawn goes only into the .stdout -> 'data' event and in the buffer there's only the SUPER_SECRET value --- e+xntPsFGc51zZxkhlJSPuYsyjtwlSpoculblolr7es5Y633YLGZetH0eZbrPpL/QdoKBJCoxRYzxfb2ve4A1w==---

WebReflection commented 10 years ago

I am sorry, the second one should have been

dblite(':memory:')
  .on('error', function(){
    console.log(arguments);
  })
  .query('qwoidj qwid jojdoqiw dj', function(err, data) {
    console.log(arguments);
  })
;

with both arguments defined since arty is used in order to understand how to deal with the on error listener.

In any case it looks like there's nothing in the stderr … I will try to simulate this on a windows machine and with this node-webkit … if you could test latest snippet and same operation in regular windows node to compare results, I would really appreciate that. Thanks!

risingphoenix commented 10 years ago

Hello WebReflection I have done quite a bit of testing. Simply using the last script without passing through node-webkit, the result is always the same.

So I have tried a simply script on spawn and sqlite3

var test = function() {
  var EOL = require('os').EOL;
  var spawn = require('child_process').spawn;
  var child = spawn('sqlite3.exe');

  child.stdout.on('data', function(buf) {
    console.log('[OUT] '+buf);
  });
  child.stderr.on('data', function(buf) {
    console.log('[ERR] '+buf);
  });
  child.on('close', function(code) {
    console.log('[END] code', code);
  });

  child.stdin.write('select ciao;'+EOL);
  child.stdin.write('.quit'+EOL);
};

test();

And I have noticed that if I comment the last .quit the stderr data event doesn't fire up. So i have tried to execute multiple command trying to force the stderr to drain the buffer like this:

 setTimeout(function() {
    child.stdin.write('select ciao;'+EOL+'select "secret";'+EOL);
  },1);
  setTimeout(function() {
    child.stdin.write('select ciao;'+EOL+'select "secret";'+EOL);
  },100);
  setTimeout(function() {
    child.stdin.write('select ciao;'+EOL+'select "secret";'+EOL);
  },300);
  setTimeout(function() {
    child.stdin.write('.quit'+EOL);
  }, 1000);

and the strange thing is that at the end of the script the log is:

[ERR] Error: near line 1: no such column: ciao
Error: near line 3: no such column: ciao
Error: near line 5: no such column: ciao

As if the commands were all chained in a single script.

Swapping the sqlite3.exe with the wmci (that opens an interactive shell, too) and launching a wrong command, all works correctly, here's the console log

[OUT] wmic:root\cli>
[ERR] cpuasd - Alias non trovato.
[OUT] wmic:root\cli>
[ERR] cpuasd - Alias non trovato.
[OUT] wmic:root\cli>
[ERR] cpuasd - Alias non trovato.
[OUT] wmic:root\cli>

The only difference I see is the "propt" of the shell.

I hope I've given other elements to figure out how to solve the problem...

sangregoriopaolo commented 9 years ago

The same thing happens to me too using dblite inside atom-shell :( Any advice?

sangregoriopaolo commented 9 years ago

Just for providing additional info, trying debugging the issue seems that both ondata and onerror are called, but the ondata is called before so the "wasError" variable has not been set yet, and once the onerror is then called, the $callback has been unset

WebReflection commented 9 years ago

@sangregoriopaolo do you have a basic example I can work on? I am still unable to reproduce the problem, not so easy to fix it. Also exact version of node and sqlite + platform would be great.

Thanks and apologies for any inconvenience.

sangregoriopaolo commented 9 years ago

You should not apologize for something you publish for free! :) Sure I'd be happy to help.

> sqlite3 --version
3.8.5 2014-08-15 22:37:57 c8ade949d4a2eb3bba4702a4a0e17b405e9b6ace

The same thing happens with the latest binary from sqlite3 website too. I'm running OSX Yosemite 10.10.1

What I basically experienced trying to debug your code, is that it seems that the stderr is somehow buffered, or anyway comes up after the response from sqlite comes on the stdout. The order your callbacks got called are first the "on data", and then the "on error" which does not find any callback because it has already been unset by the "on data" method.

Use case:

var dblite = require('dblite');
dblite(':memory:')
  .on('error', function(){
    console.log("Error" + JSON.stringify(arguments) );
  })
  .query('qwoidj qwid jojdoqiw dj', function() {
    console.log("Result" + JSON.stringify(arguments));
  })
;

Executing this returns:

Result{}
Error{"0":"Error: near line 1: near \"qwoidj\": syntax error\n"}

As you can see, result got displayed before. If I can help in any other way please tell me! :) I tried to debug the issue myself, but seems like sqlite populates stdout and stderr in this order, so maybe there is no thing we can do to fix it.

Thanks for your great library, that I'm actually using anyway :)

WebReflection commented 9 years ago

I a not sure why sqlite returns regular output on errors but it's kinda hard to find a way to work around this. If the problem is "bad queries are badly handled" it might end up as a wontfix simply because bad queries, being queries not a user but a developer input, should never happen (and users should never write them :P )

Will try to dig more on it thought, thanks for the help

sangregoriopaolo commented 9 years ago

Exactly! :) Actually I'm using it anyway for that reason. Thanks again for this beatiful library!

rooflack commented 9 years ago

Hi, sorry for interrupting. I'm jumping into this thread just to let you know that I had exactly the same issue on Windows (W7), using the official SQLite3 CLI, and somehow found a workaround (which I don't really like but at least allows me to keep on developing in the meantime). After pulling out a lot of hair, and crying as loud as I could, which I finally realized didn't help, I found this thread, which put me on the right way, hence this post. So here it what happens:

To go back to sangregoriopaolo's example, if you run it on Windows, the "error" line is actually printed out only when the sqlite3 child process ends. If you set ignoreErrors=false, for instance, you never get any error message (or get them only at random times, totally out of context, depending on the way the buffer is flushed. So the workaround I'm using now is that I compiled my own version of the sqlite3 binary, using a modified shell.c where I added a few calls to fflush(stderr) (got the idea from this other thread although I'm not directly using the same code). I don't know how OSX works regarding buffered output, but I thought this sounded similar enough to post this here. As I don't think there is anything to do on dblite's side, it may be worth submitting this to the SQLite team, as I doubt this inconsistency between OSes is meant by design (I'm very new to both Node and SQLite and may have overlooked something, so I'll let you make this call, WebReflection).

Thanks a lot for this very good lib!

WebReflection commented 9 years ago

thank you @rooflack and I am on Linux indeed, hence probably my inability to reproduce problems.

it looks like the effort to work-around might be non reliable or too weird/compromising anyway, but the way I read your comment feels like much more could break and I don't like it so I'll try to figure out how/what to do anyway and see if I can fix All The Things !!! :dancer:

stay tuned

rooflack commented 9 years ago

Thanks @WebReflection for your (very) quick reply. I think indeed that the only safe way to fix this is to have the SQLite guys integrate a new option in the CLI tool for output buffer management (allowing one to force buffered or unbuffered output via a command line option, falling back to the system's default if not provided), and to handle this on dblite's side with a version selector (.withSQLite-style). But again my point of view may be biased by my newbieness to this environment.

Happy new programming year!

WebReflection commented 9 years ago

OK, once again I cannot reproduce this on linux but here a possible work around I need you guys to test.

There is a -bail option that should force sqlite to quit as soon as there is an error. This is not an elegant solution, rather a very ugly hack, but it might be worth exploring its potential.

The idea is that I could simply add such parameter automatically and handle the unexpected exit ignoring it and relaunching sqlite behind the scene so that from user perspective everything is transparent and the error is logged as expected.

This is the test I need you to try:


var dblite = require('dblite').withSQLite('3.8.6+');
var db = dblite(':memory:', '-bail');
db.on('error', function(){console.log('listener'); console.log(arguments);});
db.query('qwoidj qwid jojdoqiw dj', function(err, data) {console.log('query'); console.log(arguments);});

And this is the output I'd expect to be consistent:

> query
{ '0': 
   [Error: Error: near line 1: near "qwoidj": syntax error
   ],
  '1': null }
bye bye

If this is as such everyhwere, I might decide what to do and eventually fix this, specially because I really don't like the fact in windows errors could go on and on and be shown probably too late.

Very bad ... very weird, I hope this is SQLite only and not node spawn in every windows platform.

Thanks for your help

sangregoriopaolo commented 9 years ago

Hi @WebReflection, great idea! I can confirm that running your example on OSX Yosemite provides the same output you expected, so it works! :) Great idea

rooflack commented 9 years ago

Hi @WebReflection,

your code passes, and produces the expected output on W7. I played a bit with the -bail option yesterday with similar results. But I finally abandoned this option because I'm concerned that breaking the "one instance, one spawn" rule could be very dangerous (not to mention the overhead, which I deem to be a minor issue, but may have an impact for some applications). I haven't taken the time yet to dive deeper in the docs nor to test it directly, but what if for instance one of several requests breaks a constraint in the middle of a transaction? would the bailing roll back the whole transaction, or would it be resumed after the respawn?).

Regarding your very last point, I guess this is a "yes and no". In my opinion, it is a bug that SQLite's CLI tool does not handle this consistently accross platforms. But this problem is not specific to this program, and is not even specific to Node, as it relies in implementation differences in C's stdio for the different OSes. The same happens with every program using this lib (for instance Python and some modules had similar issues at some point). The origin being that, unfortunately, the default buffering modes are not part of the C99 specification, so developers of cross-platform tools have to handle this in their own code if buffering matters for their application. (Actually it may even vary between the compilers/toolchains: I wouldn't be surprised if gcc and msvc behaved differently.)

I still think it is worth submitting this to the SQLite devs. They may state that this way of using the CLI tool is out of scope, but if they agree to fix it, this would be by far the cleanest way to handle the issue.

Keep up the good work :)

rooflack commented 9 years ago

Hi @WebReflection,

I don't know if you had submitted this on your side or if it is just a coincidence, but I reported our issue to SQLite's maling list and it appears that the problem has been fixed and will be released soon with the next update of SQLite. If I correctly read the diff below, they chose to just force stderr to be unbuffered, which is even better than flushing it explicitly. It should ensure that the behaviour is consistent accross all systems and matches Linux's default. https://www.sqlite.org/src/info/2a9ea9b4a7d6

Cool. Cool cool cool. No need to compile a homemade exe anymore.

Thanks for your help and for all your work!

WebReflection commented 9 years ago

thanks for the headsup @rooflack , I've sent them an email too but nobody ever answered to me, happy to know you had more luck :-)

Will close this updating the SQLite version once we can confirm the bug is gone.

Cheers

WebReflection commented 9 years ago

closing since nobody came back in a while, I hope this has been solved from sqlite side.

rooflack commented 9 years ago

It's actually been commited in the trunk but not yet released (current version 3.8.8.2 was released after the patch but uses a dedicated branch which does not integrate this). I guess it will be published in their featured release.

rooflack commented 9 years ago

SQLite v3.8.9 was released yesterday (2015-04-08) and includes the aforementioned fix. I just tested it and confirm the default precompiled binary available on their site works as expected, and the behaviour is now consistent on Linux and Windows. Thanks all.

WebReflection commented 9 years ago

\o/