How to handle errors/warnings based on severity level about node-sqlserver-v8 HOT 22 CLOSED

I checked in a patch for this issue and released to npm along with node 9 support

from node-sqlserver-v8.

Looking at how tedious does this in stream mode, the token is parsed and compared to the following object to determine the token type.

module.exports.TYPE = {
ALTMETADATA: 0x88,
ALTROW: 0xD3,
COLMETADATA: 0x81,
COLINFO: 0xA5,
DONE: 0xFD,
DONEPROC: 0xFE,
DONEINPROC: 0xFF,
ENVCHANGE: 0xE3,
ERROR: 0xAA,
INFO: 0xAB,
LOGINACK: 0xAD,
NBCROW: 0xD2,
OFFSET: 0x78,
ORDER: 0xA9,
RETURNSTATUS: 0x79,
RETURNVALUE: 0xAC,
ROW: 0xD1,
SSPI: 0xED,
TABNAME: 0xA4
};

from node-sqlserver-v8.

Thanks. Can you give me couple of examples. I think we certainly can do better than situation presently. Most warnings and information messages should come across with a recognisable state. I think easiest think would be to channel those onto a separate event leaving errors on the subscription causing the stream to halt. I’m nearly complete on TVP proc Params which has taken a lot of my recent time. I will try to at least improve situation on that release.

from node-sqlserver-v8.

Fantastic, I will share some examples later that you can use to test with!

from node-sqlserver-v8.

OK so I was using your driver via node mssql, i.e. require('mssql/msnodesqlv8'); and running stored procedures through there. I have isolated the issue to how they use queryRaw() to actually execute the final query. It is only with queryRaw that I see any issues.

To provide an example I have written a couple of simple queries (see attached) that produce JOIN HINT or NULLS ELIMINATED warnings. I have then used the msnodesqlv8 package directly rather than through mssql package.

In the queries you will see option(recompile) as this stops the queries from being cached and the warnings omitted.

There are a couple of examples using queryRaw, preparedQuery, and stored procedure. I have exclcuded everything but the error handling for each query.

You will see that it is only the JOIN HINT via queryRaw that actually raises an error.

So an obvious solution for me is to switch to use msnodesqlv8 directly rather than mssql package. However it still might be nice to handle error/warnings differently.

Final question - is the way i execute the stored procedure definitely in streaming mode? Reason I ask is that the second parameter to pm.callproc('tstWarning', ()=>{}) expects a callback function that is run on the result set. I will be using on('row', function(){}) callbacks for each row streamed and so i definitely dont want the entire resultset building up in memory anywhere if possible?

Thanks!
test.zip

from node-sqlserver-v8.

Very elegant code these will go straight into the test suite.

I will take a look at this. Looking at documentation it seems like state 0100 which I believe your code throws can be considered not critical. You could if you have a moment search for where I raise the error event and simply screen these out as a test. The cpp throws an Error object you should see a field state plus message and error code. We should probably channel these into a warning event. That may well also sort out the mssql code.

This is from memory in the driverread file. I apologise for the JS it has been through the wars a bit but most of heavy lifting is done in cpp.

Regarding the proc call yes I suspect your right this looks to be an error. The actual reader code in file above I am certain does not build up large cache of rows if callback is not provided but the proc manager may well be handing in a callback which is a mistake as in streaming rows this will probably force the code to cache. I will certainly look at that as well.

from node-sqlserver-v8.

Thanks! For the errors, there is a function routeStatmentError() on line 18 of driverRead.js that I have modified locally to ignore the "General Warnings" with sqlstate = '01000' as follows:

function routeStatementError (err, callback, notify, more) { if(err.sqlstate == '01000'){ /* could implement warning notification here instead */ } else if (callback) { callback(err, null, more) } else if (notify && notify.listeners('error').length > 0) { notify.emit('error', err) } else { throw new Error(err) } }

This solves the issue for me. For the stored procedure I got a bit lost in the code to work out if it caches or not. It seems like the absence of a callback would prevent the resultset being built up in memory. However callproc() doesnt allow the absence of a callback function.

from node-sqlserver-v8.

In fact, SQL states that a class value of "01" indicates a warning and is accompanied by a return code of SQL_SUCCESS_WITH_INFO. So it is best to exclude any sqlstate beginning '01'. so change the above to:

if(err.sqlstate.substring(0,2) == '01'){ }

from node-sqlserver-v8.

that's great thanks, i will patch this into the tvp branch - I will also sort out the proc callback.

from node-sqlserver-v8.

this is merged onto latest version, thanks for all your help.
tvp suppot is now included

from node-sqlserver-v8.

I have added your tests I believe the issue is resolved on latest version so I am closing this. Thanks for help and tests.

from node-sqlserver-v8.

Sorry, but my tests were too focused on the suppression of the error for those warning messages that I didn't realise that the meta and row callbacks do not get fired. So I am guessing that whatever triggers the routeStatmentError is returning the error and then skipping the rest of the code. But I'm getting a bit lost where this error first gets identified. Can you take a look please? I'll update the test if you can point me in the right direction!

from node-sqlserver-v8.

from node-sqlserver-v8.

Well, for one of the warnings in my previous tests it does push out meta data and row data , and for the other one it does not. See attached. Same queries as before, only using the queryRaw function for this example. You'll see that the NULL ELIMINATED test works fine, but the JOIN HINT one only fires a done callback!
no_callbacks_test.zip

from node-sqlserver-v8.

OK, this seems to be a bug in the actual c++ driver, amazing this has been in there this long.

can you check out master from git and test that version -you could in theory just drop the binaries into your bin folder.

WARNING >> TEST ONE - Query - JOIN HINT WARNING >> { Error: [Microsoft][SQL Server Native Client 11.0][SQL Server]Warning: The join order has been enforced because a local join hint is used. sqlstate: '01000', code: 8625 }
META >> TEST ONE - Query - JOIN HINT WARNING >> [ { size: 10,
name: 'id',
nullable: false,
type: 'number',
sqlType: 'int' },
{ size: 5,
name: 'value',
nullable: false,
type: 'text',
sqlType: 'varchar' } ]
ROW IDX >> TEST ONE - Query - JOIN HINT WARNING >> 0
ROW IDX >> TEST ONE - Query - JOIN HINT WARNING >> 1
DONE >> TEST ONE - Query - JOIN HINT WARNING

from node-sqlserver-v8.

It works!!

You, Sir, are a life saver!! Thank you so much for fixing it this quickly. I was working backwards through the functions and got to that bin folder last night and then gave up and was going to continue digging today - but you have fixed it by the time I am up :)

I'm amazed it has gone unnoticed for so long as well!

I've just dropped the bin files in like you said, but I'll watch here for any more updates. Keep up the good work :)

👍

from node-sqlserver-v8.

Hi,

I just changed to your master branch rather than using the binaries but notice that your implementation of the routeStatementError() warnings doesnt actually emit a warning event and still throws an error.

Below is the evaluated function in my debugger:

It seems that notify.listeners(ev).length > 0 evaluates to false and so a new Error is thrown.

Would you be able to fix this? Also you should bear in mind that node-mssql doesnt have an event 'warning' and it treats all of these ones as 'info' events.

Would be great if you could fix this and then publish the changes to npm that include the new binaries and this fix.

Cheers!

from node-sqlserver-v8.

from node-sqlserver-v8.

Yes it works when I comment out the notify.listeners(ev).length > 0 part.

Also yes I am suggesting that info would make it more compatible with mssql, though I haven't checked what happens with the messages that get passed through notify.emit() to see if it would straight away work with mssql.

For completeness, here is the info for mssql package, which is in agreement with how we implemented warnings for error messages of class 10 or less: https://www.npmjs.com/package/mssql#informational-messages

from node-sqlserver-v8.

I have very little time presently I will try to addresss this over next few days.

from node-sqlserver-v8.

This has been a nice collaborative thread to read. I hate to barge in. If it's worth anything, I too have experienced this issue using mssql package with msnodesqlv8 driver that an information message is thrown as an error by the routeStatementError method of the reader.js file. My original mssql batch call never returns after that point. I suppose for now, I'm going to change the if statement as suggested on Oct 24 and get on with my way.

from node-sqlserver-v8.

thanks for all help with this issue, it was very useful. I will close (again!) for now, please come back if there are any more issues around warning/errors.

from node-sqlserver-v8.