fix: prints, errors and results after a failed statement in a batch were lost

cpp/include/odbc/odbc_statement_legacy.h

@@ -6,6 +6,7 @@
 #include <string>
 #include <vector>
 #include <atomic>
+#include <optional>
 
 #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<QueryOperationParams> _query;
 
@@ -212,6 +215,8 @@ class OdbcStatementLegacy : public IOdbcStatement {
   std::atomic<bool> _cancelRequested;
   // set by Cancel() on the cancel thread before SQLCancelHandle; read by the worker
   std::atomic<bool> _cancelHandleIssued{false};
+  // SQLMoreResults already called by look_ahead_after_error, for try_read_next_result
+  std::optional<SQLRETURN> _pendingMoreResults;
   std::atomic<bool> _pollingEnabled;
   bool _numericStringEnabled;
   bool _bigIntAsNativeEnabled;

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_ptr<QueryOperationPara
   // cout << "id " << _statementId << " try_execute_direct" << endl;
   _errors->clear();
   _errorHandler->ClearErrors();
+  _pendingMoreResults.reset();
   _query = q;
 
   const auto timeout = q->timeout;
@@ -1113,6 +1148,8 @@ bool OdbcStatementLegacy::try_execute_direct(const shared_ptr<QueryOperationPara
                                << "] bind_params failed, statement needs cleanup");
       return false;
     }
+    // output params are read by Unbind() when the batch ends, which can be after an error
+    _boundParamsSet = param_set;
     _resultset->_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<QueryOperationPara
       _resultset = make_unique<ResultSet>(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<ResultSet>(0);
+      _resultset->_end_of_results = !look_ahead_after_error();
       _resultset->_end_of_rows = true;
       return false;
     }

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)
         }

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)

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) {