diff --git a/cpp/include/odbc/odbc_statement_legacy.h b/cpp/include/odbc/odbc_statement_legacy.h index bd91f103..0c9b30e1 100644 --- a/cpp/include/odbc/odbc_statement_legacy.h +++ b/cpp/include/odbc/odbc_statement_legacy.h @@ -6,6 +6,7 @@ #include #include #include +#include #include "odbc_common.h" #include "odbc_driver_types.h" @@ -198,6 +199,8 @@ class OdbcStatementLegacy : public IOdbcStatement { bool return_odbc_error(); bool check_odbc_error(SQLRETURN ret); bool return_cancelled(); + bool more_results_after_error() const; + bool look_ahead_after_error(); shared_ptr _query; @@ -212,6 +215,8 @@ class OdbcStatementLegacy : public IOdbcStatement { std::atomic _cancelRequested; // set by Cancel() on the cancel thread before SQLCancelHandle; read by the worker std::atomic _cancelHandleIssued{false}; + // SQLMoreResults already called by look_ahead_after_error, for try_read_next_result + std::optional _pendingMoreResults; std::atomic _pollingEnabled; bool _numericStringEnabled; bool _bigIntAsNativeEnabled; diff --git a/cpp/src/odbc/odbc_statement_legacy.cpp b/cpp/src/odbc/odbc_statement_legacy.cpp index 766efef8..f1225dfe 100644 --- a/cpp/src/odbc/odbc_statement_legacy.cpp +++ b/cpp/src/odbc/odbc_statement_legacy.cpp @@ -698,6 +698,40 @@ bool OdbcStatementLegacy::return_cancelled() { return false; } +// a statement failing part way through a batch, e.g. raiserror severity 16, does not end +// the batch: SQL Server runs the statements after it and SQLMoreResults moves on to them. +// a cancel, timeout or broken connection does end it, as does an error that is only about +// the statement's own state (which SQLMoreResults would just repeat). +bool OdbcStatementLegacy::more_results_after_error() const { + if (!_statement || _cancelRequested.load() || _cancelHandleIssued.load() || _errors->empty()) { + return false; + } + return std::none_of(_errors->begin(), _errors->end(), [](const auto& e) { + const auto& s = e->sqlstate; + return s == "HY008" || s == "HYT00" || s == "HY010" || s == "HY007" || s == "24000" || + s.rfind("08", 0) == 0; + }); +} + +// whether the batch goes on can only be known by asking: call SQLMoreResults now, so a +// batch that ends with the error still ends with it (one callback, more = false). the +// result it moves to is picked up by the next try_read_next_result. the diag records of +// the failed statement were read before this call replaces them. +bool OdbcStatementLegacy::look_ahead_after_error() { + _pendingMoreResults.reset(); + if (!more_results_after_error()) { + return false; + } + const auto ret = _odbcApi->SQLMoreResults(_statement->get_handle()); + SQL_LOG_DEBUG_STREAM("[" << _handle.toString() << "] look_ahead_after_error SQLMoreResults " + << ret); + if (ret == SQL_NO_DATA) { + return false; + } + _pendingMoreResults = ret; + return true; +} + bool OdbcStatementLegacy::check_odbc_error(const SQLRETURN ret) { if (!SQL_SUCCEEDED(ret)) { SQL_LOG_DEBUG_STREAM("check_odbc_error failed: " << ret); @@ -1082,6 +1116,7 @@ bool OdbcStatementLegacy::try_execute_direct(const shared_ptrclear(); _errorHandler->ClearErrors(); + _pendingMoreResults.reset(); _query = q; const auto timeout = q->timeout; @@ -1113,6 +1148,8 @@ bool OdbcStatementLegacy::try_execute_direct(const shared_ptr_end_of_results = true; // reset const auto ret = query_timeout(timeout); if (!check_odbc_error(ret)) { @@ -1173,6 +1210,7 @@ bool OdbcStatementLegacy::try_execute_direct(const shared_ptr(0); } _resultset->_end_of_rows = true; + _resultset->_end_of_results = !look_ahead_after_error(); return false; } @@ -2263,7 +2301,9 @@ bool OdbcStatementLegacy::try_read_next_result() { return false; } const auto& statement = *_statement; - const auto ret = _odbcApi->SQLMoreResults(statement.get_handle()); + const auto ret = _pendingMoreResults ? *_pendingMoreResults + : _odbcApi->SQLMoreResults(statement.get_handle()); + _pendingMoreResults.reset(); switch (ret) { case SQL_NO_DATA: { SQL_LOG_DEBUG_STREAM("OdbcStatementLegacy::try_read_next_result [" @@ -2301,8 +2341,11 @@ bool OdbcStatementLegacy::try_read_next_result() { SQL_LOG_DEBUG_STREAM("OdbcStatementLegacy::try_read_next_result [" << _handle.toString() << "] try_read_next_result SQL_ERROR from SQLMoreResults"); + // don't read this result's columns (that is what fails with HY007), but the + // statements after the failed one may still have results to move on to. return_odbc_error(); - _resultset->_end_of_results = true; + _resultset = make_unique(0); + _resultset->_end_of_results = !look_ahead_after_error(); _resultset->_end_of_rows = true; return false; } diff --git a/lib/procedure-bound.js b/lib/procedure-bound.js index fc09aee5..e7ac8b44 100755 --- a/lib/procedure-bound.js +++ b/lib/procedure-bound.js @@ -88,7 +88,8 @@ class ProcedureBound { function onProcedureRaw (err, results, more, outputParams) { if (chunky.callback) { if (err) { - chunky.callback(err) + // more: a failed statement in the proc need not be the end of it + chunky.callback(err, undefined, undefined, more) } else { chunky.callback(err, driverMgr.objectify(results), outputParams, more) } diff --git a/lib/query-aggregator.js b/lib/query-aggregator.js index eb60aaee..7a9b1996 100755 --- a/lib/query-aggregator.js +++ b/lib/query-aggregator.js @@ -238,6 +238,8 @@ class QueryAggregator { ret.end() const last = ret.lastError() if (last) { + // the last error may not be the end of the batch, so attach the results here too + last._results = ret reject(last) } else { resolve(ret) diff --git a/lib/reader.js b/lib/reader.js index 5af388d7..2b1fac55 100755 --- a/lib/reader.js +++ b/lib/reader.js @@ -71,7 +71,8 @@ class Query extends BasePromises { } /* route non-critical info messages to its own event to prevent streams based readers from halting */ - routeStatementError (errorsAndInfo, callback, notify) { + // moreResults: the batch carries on after these errors, so 'more' is true for all of them + routeStatementError (errorsAndInfo, callback, notify, moreResults = false) { if (!Array.isArray(errorsAndInfo)) { errorsAndInfo = [errorsAndInfo] } @@ -86,12 +87,12 @@ class Query extends BasePromises { errorsAndInfo.forEach(err => { const info = this.isInfo(err) if (callback && !info) { - const more = i < errorsAndInfo.length - 1 + const more = moreResults || i < errorsAndInfo.length - 1 callback(err, null, more) } else { const ev = info ? 'info' : 'error' if (notify) { - const more = i < errorCount - 1 + const more = moreResults || i < errorCount - 1 if (notify.listenerCount(ev) > 0) { notify.emit(ev, err, more) } @@ -143,12 +144,12 @@ class Query extends BasePromises { } }) } - if (errorMessages.length > 0) { - return errorMessages - } else if (infoMessages.length > 0) { + // route info even alongside errors: a print just before a failing statement arrives + // in the same set of diagnostics as the error + if (infoMessages.length > 0) { this.routeStatementError(infoMessages, this.callback, this.notify, false) } - return [] + return errorMessages } async nativeNextResult (queryId) { @@ -159,7 +160,11 @@ class Query extends BasePromises { setImmediate(() => { // may contain info messages e.g. raised by PRINT statements - do not want to reject these const errorMessages = e ? this.dispatchInfoReturnErrors(e) : [] - if (errorMessages.length > 0) { + if (errorMessages.length > 0 && res && !res.endOfResults) { + // a statement in the batch failed but the batch goes on: report and carry on + this.routeStatementError(errorMessages, this.callback, this.notify, true) + resolve(res) + } else if (errorMessages.length > 0) { reject(errorMessages) } else { this.infoFromNextResult = e != null && Array.isArray(e) && e.length > 0 @@ -429,7 +434,7 @@ class Query extends BasePromises { this.notify.setHandle(res.queryResult.handle) if (res.warning) { - this.routeStatementError(res.warning, this.callback, this.notify) + this.routeStatementError(res.warning, this.callback, this.notify, !res.queryResult.endOfResults) if (!this.cancelled) { res.warning.forEach(err => { if (!this.cancelled) { diff --git a/test/batch-errors.test.js b/test/batch-errors.test.js new file mode 100644 index 00000000..ee87f84d --- /dev/null +++ b/test/batch-errors.test.js @@ -0,0 +1,241 @@ +'use strict' + +// a statement failing part way through a batch (raiserror severity 11 - 16, a failed +// insert, ...) does not end the batch: SQL Server runs the statements after it. the +// driver used to stop at the first error, losing every print, error and result set +// that followed, and never raising 'done'. +// errors that do end the batch (throw, conversion errors, timeouts) must still end it. +// ('done' is not raised when a batch ends on an error - that is unchanged here, so those +// tests don't assert on it either way.) + +import { createRequire } from 'module' +const require = createRequire(import.meta.url) +const { TestEnv } = require('./env/test-env') +const env = new TestEnv() +const assert = require('chai').assert + +const sql = require('../lib/sql') +const { configureTestLogging } = require('./common/logging-helper') +configureTestLogging(sql) + +const strip = message => message.replace(/^(\[[^\]]*\])+/, '') + +const twoErrorsBatch = ` +print 'p1' +raiserror('e1', 16, 1) +print 'p2' +raiserror('e2', 16, 1) +print 'p3' +select 1 as n` + +// every event the query raises, in order +function events (conn, query) { + return new Promise(resolve => { + const ev = [] + const q = conn.query(query) + q.on('info', m => ev.push(`info ${strip(m.message)}`)) + q.on('error', (e, more) => ev.push(`error ${strip(e.message)} more=${more}`)) + q.on('meta', m => ev.push(`meta ${m.map(c => c.name).join(',')}`)) + q.on('column', (c, v) => ev.push(`column ${v}`)) + q.on('done', () => ev.push('done')) + q.on('free', () => resolve(ev)) + }) +} + +// every call of the query's callback, in order +function callbacks (conn, query) { + return new Promise(resolve => { + const calls = [] + conn.query(query, (err, rows, more) => { + calls.push({ err: err ? strip(err.message) : null, rows, more }) + if (!more) resolve(calls) + }) + }) +} + +async function assertUsable (conn) { + const res = await conn.promises.query('select 1 as n') + assert.strictEqual(res.first[0].n, 1) +} + +describe('errors part way through a batch', function () { + this.timeout(30000) + + this.beforeEach(async () => { + await env.open() + }) + + this.afterEach(async () => { + await env.close() + }) + + for (const [label, make] of [['polling', q => sql.PollingQuery(q)], ['non polling', q => q]]) { + it(`${label}: prints, errors and rows after a failed statement all arrive`, async function handler () { + const ev = await events(env.theConnection, make(twoErrorsBatch)) + assert.deepStrictEqual(ev, [ + 'info p1', + 'error e1 more=true', + 'info p2', + 'error e2 more=true', + 'info p3', + 'meta n', + 'column 1', + 'done' + ]) + await assertUsable(env.theConnection) + }) + } + + it('promise rejects with the last error, carrying everything else', async function handler () { + const err = await env.theConnection.promises.query(twoErrorsBatch).then(() => null, e => e) + assert(err, 'expected the query to reject') + assert.strictEqual(strip(err.message), 'e2') + const res = err._results + assert.deepStrictEqual(res.errors.map(e => strip(e.message)), ['e1', 'e2']) + assert.deepStrictEqual(res.info.map(strip), ['p1', 'p2', 'p3']) + assert.deepStrictEqual(res.first, [{ n: 1 }]) + await assertUsable(env.theConnection) + }) + + it('callback: more is true until the batch really ends', async function handler () { + const calls = await callbacks(env.theConnection, twoErrorsBatch) + assert.deepStrictEqual(calls.filter(c => c.err).map(c => c.err), ['e1', 'e2']) + assert(calls.slice(0, -1).every(c => c.more), 'only the last call should have more=false') + const last = calls.at(-1) + assert.strictEqual(last.more, false) + assert.deepStrictEqual(last.rows, [{ n: 1 }]) + }) + + it('result sets either side of an error are both returned', async function handler () { + const err = await env.theConnection.promises.query(` + select 1 as a + raiserror('in the middle', 16, 1) + select 2 as b`).then(() => null, e => e) + assert.strictEqual(strip(err.message), 'in the middle') + assert.deepStrictEqual(err._results.results, [[{ a: 1 }], [{ b: 2 }]]) + }) + + it('a failed insert does not stop the rest of the batch', async function handler () { + const err = await env.theConnection.promises.query(` + declare @t table (id int primary key) + insert into @t values (1) + insert into @t values (1) + insert into @t values (2) + select id from @t order by id`).then(() => null, e => e) + assert.include(err.message, 'PRIMARY KEY') + assert.deepStrictEqual(err._results.first, [{ id: 1 }, { id: 2 }]) + }) + + it('stored procedure: statements after a raiserror run, output and return arrive', async function handler () { + const conn = env.theConnection + const name = 'test_batch_errors_proc' + await conn.promises.query(`if object_id('${name}') is not null drop procedure ${name}`) + await conn.promises.query(` + create procedure ${name} @out int output + as + begin + set nocount on + select 'started' as step + print 'before' + raiserror('proc failed a step', 16, 1) + print 'after' + set @out = 42 + select 'still here' as status + return 7 + end`) + try { + const err = await conn.promises.callProc(name, {}).then(() => null, e => e) + assert.strictEqual(strip(err.message), 'proc failed a step') + const res = err._results + assert.deepStrictEqual(res.info.map(strip), ['before', 'after']) + assert.deepStrictEqual(res.results, [[{ step: 'started' }], [{ status: 'still here' }]]) + assert.deepStrictEqual(res.output, [7, 42]) + await assertUsable(conn) + } finally { + await conn.promises.query(`drop procedure ${name}`) + } + }) + + // when the very first result of a batch is the error, SQLExecDirect itself fails. on + // linux / macOS the unixODBC driver manager then answers SQLMoreResults with + // SQL_NO_DATA without asking the driver, so the batch ends there. on windows it goes on. + it('error as the first result: windows carries on, unixODBC ends the batch', async function handler () { + const err = await env.theConnection.promises.query(` + raiserror('first thing fails', 16, 1) + print 'after' + select 1 as n`).then(() => null, e => e) + assert.strictEqual(strip(err.message), 'first thing fails') + const res = err._results + if (process.platform === 'win32') { + assert.deepStrictEqual(res.info.map(strip), ['after']) + assert.deepStrictEqual(res.first, [{ n: 1 }]) + } else { + assert.isNull(res.info) + assert.deepStrictEqual(res.results, []) + } + await assertUsable(env.theConnection) + }) + + it('a print in the same diagnostics as the final error is not dropped', async function handler () { + const conn = env.theConnection + const name = 'test_batch_errors_info' + await conn.promises.query(`if object_id('${name}') is not null drop procedure ${name}`) + // inside a proc with nocount on, the print and the throw arrive together + await conn.promises.query(` + create procedure ${name} + as + begin + set nocount on + print 'about to throw' + ;throw 50002, 'thrown in proc', 1 + end`) + try { + const err = await conn.promises.callProc(name, {}).then(() => null, e => e) + assert.strictEqual(strip(err.message), 'thrown in proc') + assert.deepStrictEqual(err._results.info.map(strip), ['about to throw']) + await assertUsable(conn) + } finally { + await conn.promises.query(`drop procedure ${name}`) + } + }) + + it('throw ends the batch: nothing after it runs', async function handler () { + const ev = await events(env.theConnection, ` + print 'p1' + ;throw 50001, 'thrown', 1 + print 'never' + select 1 as never`) + assert.deepStrictEqual(ev.filter(e => e !== 'done'), ['info p1', 'error thrown more=false']) + await assertUsable(env.theConnection) + }) + + it('a batch aborting conversion error ends the batch', async function handler () { + const ev = await events(env.theConnection, ` + print 'p1' + select cast('oops' as int) as n + print 'never'`) + assert.strictEqual(ev[0], 'info p1') + assert(ev.some(e => e.startsWith('error') && e.includes('Conversion failed')), ev.join('\n')) + assert(!ev.includes('info never'), ev.join('\n')) + await assertUsable(env.theConnection) + }) + + it('several compile errors are still all reported', async function handler () { + const ev = await events(env.theConnection, 'select no_such_column_a; select no_such_column_b') + assert.deepStrictEqual(ev.filter(e => e !== 'done'), [ + "error Invalid column name 'no_such_column_a'. more=true", + "error Invalid column name 'no_such_column_b'. more=false" + ]) + await assertUsable(env.theConnection) + }) + + it('a timeout ends the batch: the statements after it do not run', async function handler () { + const ev = await events(env.theConnection, { + query_str: "waitfor delay '00:00:10'; print 'never'", + query_timeout: 1 + }) + assert(ev.some(e => e.includes('Query timeout expired')), ev.join('\n')) + assert(!ev.includes('info never'), ev.join('\n')) + await assertUsable(env.theConnection) + }) +})