Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion .github/scripts/release-notes.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,27 @@ const groups = [
]
const skipped = ['chore', 'ci', 'build', 'test', 'docs', 'style', 'refactor', 'revert']

// a PR merged with a merge commit ("Merge pull request #123 from owner/branch") has its
// title on the message's first body line: take that, as a squash merge's "title (#123)"
function prTitles (prev) {
const log = git('log', '--first-parent', '--format=%s%x1f%b%x1e', `${prev}..HEAD`)
const titles = []
for (const entry of log.split('\x1e')) {
const [subject = '', body = ''] = entry.trim().split('\x1f')
if (!subject) continue
const merged = subject.match(/^Merge pull request #(\d+) from /)
if (merged) {
const title = body.split('\n').map(l => l.trim()).find(Boolean)
titles.push(title ? `${title} (#${merged[1]})` : subject)
} else if (!/^Merge (remote-tracking )?branch /.test(subject)) {
titles.push(subject)
}
}
return titles
}

function changes (prev) {
const subjects = git('log', '--first-parent', '--format=%s', `${prev}..HEAD`).split('\n').filter(Boolean)
const subjects = prTitles(prev)
const byGroup = new Map(groups.map(g => [g.title, []]))
const other = []
for (const subject of subjects) {
Expand Down
1 change: 1 addition & 0 deletions cpp/include/odbc/odbc_statement_legacy.h
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ class OdbcStatementLegacy : public IOdbcStatement {
bool try_read_string(bool binary, size_t row_id, size_t column);

bool return_odbc_error();
void keep_diagnostics();
bool check_odbc_error(SQLRETURN ret);
bool return_cancelled();
bool more_results_after_error() const;
Expand Down
10 changes: 9 additions & 1 deletion cpp/src/js/workers/bind_query_worker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ void BindQueryWorker::Execute() {
SetError(errorMessage);
has_error_ = true;
}
} else {
// an execution that worked can come with info (e.g. 8153, when the driver has read
// to the end of a short result): it goes back with the metadata
errorDetails_ = connection_->GetErrors();
}
} catch (const std::exception& e) {
SQL_LOG_ERROR("Exception in BindQueryWorker::Execute: " + std::string(e.what()));
Expand All @@ -80,7 +84,11 @@ void BindQueryWorker::OnOK() {

try {
const auto metadata = GetMetadata();
Callback().Call({env.Null(), metadata});
if (errorDetails_.empty()) {
Callback().Call({env.Null(), metadata});
} else {
Callback().Call({GetErrors(env), metadata});
}
} catch (const std::exception& e) {
// Call the callback with an error
Callback().Call({Napi::Error::New(env, e.what()).Value(), env.Null()});
Expand Down
12 changes: 10 additions & 2 deletions cpp/src/js/workers/fetch_rows_worker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ void FetchRowsWorker::Execute() {
SetError(errorMessage);
has_error_ = true;
}
} else {
// a fetch that worked can still come with info (e.g. 8153 at the end of the rows):
// it goes back alongside the rows, as nextResultSet does
errorDetails_ = connection_->GetErrors();
}
} catch (const std::exception& e) {
SQL_LOG_ERROR("Exception in FetchRowsWorker::Execute: " + std::string(e.what()));
Expand Down Expand Up @@ -82,8 +86,12 @@ void FetchRowsWorker::OnOK() {
return;
}
const auto result = JsObjectMapper::fromQueryResult(env, resultset);
// Call the callback with the result
Callback().Call({env.Null(), result});
// Call the callback with the result, and the info the fetch came with if any
if (errorDetails_.empty()) {
Callback().Call({env.Null(), result});
} else {
Callback().Call({GetErrors(env), result});
}
} catch (const std::exception& e) {
// Call the callback with an error
Callback().Call({Napi::Error::New(env, e.what()).Value(), env.Null()});
Expand Down
48 changes: 48 additions & 0 deletions cpp/src/odbc/odbc_statement_legacy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,9 @@ bool OdbcStatementLegacy::TryReadRows(std::shared_ptr<QueryResult> result,
SQL_LOG_FUNC_TRACER();
lock_guard<recursive_mutex> lock(g_i_mutex);
SQL_LOG_DEBUG_STREAM("OdbcStatementLegacy::TryReadRows [" << _handle.toString() << "]");
// what this fetch reports is only what its own calls came with
_errors->clear();
_errorHandler->ClearErrors();
auto res = try_read_columns(number_rows);
assign_result(result, _resultset);
return res;
Expand Down Expand Up @@ -236,6 +239,7 @@ bool OdbcStatementLegacy::fetch_read(const size_t number_rows) {
}
if (ret == SQL_NO_DATA) {
SQL_LOG_DEBUG_STREAM("[" << _handle.toString() << "] fetch_read SQL_NO_DATA " << ret);
keep_diagnostics();
_resultset->_end_of_rows = true;
return true;
}
Expand All @@ -245,6 +249,9 @@ bool OdbcStatementLegacy::fetch_read(const size_t number_rows) {
// fprintf(stderr, "fetch_read check_odbc_error\n");
return false;
}
if (ret == SQL_SUCCESS_WITH_INFO) {
keep_diagnostics();
}
_resultset->_end_of_rows = false;
res = true;

Expand Down Expand Up @@ -272,13 +279,17 @@ bool OdbcStatementLegacy::prepared_read() {
const auto ret = _odbcApi->SQLFetchScroll(statement.get_handle(), SQL_FETCH_NEXT, 0);
// cerr << " row_count " << row_count << endl;
if (ret == SQL_NO_DATA) {
keep_diagnostics();
_resultset->_end_of_rows = true;
return true;
}
_resultset->_end_of_rows = false;
auto res = true;
if (!check_odbc_error(ret))
return false;
if (ret == SQL_SUCCESS_WITH_INFO) {
keep_diagnostics();
}
const auto column_count = static_cast<int>(_resultset->get_column_count());
for (auto c = 0; c < column_count; ++c) {
const auto& definition = _resultset->get_meta_data(c);
Expand Down Expand Up @@ -677,6 +688,33 @@ bool OdbcStatementLegacy::return_odbc_error() {
return false;
}

// keep the diagnostics a call came with when it did not fail. msodbcsql hands a warning
// raised at the end of a result set - 8153 "null value is eliminated by an aggregate",
// 3607 "division by zero occurred" - to the SQLFetch that returns SQL_NO_DATA, and only
// there. they are added to what this call has collected so far (read_errors replaces),
// so one fetch batch keeps every message its fetches came with, in order.
void OdbcStatementLegacy::keep_diagnostics() {
if (!_statement) {
return;
}
auto found = make_shared<vector<shared_ptr<OdbcError>>>();
_statement->read_errors(_odbcApi, found);
if (found->empty()) {
return;
}
SQL_LOG_DEBUG_STREAM("[" << _handle.toString() << "] keep_diagnostics: " << found->size());
for (const auto& error : *found) {
_errors->push_back(error);
_errorHandler->AddError(error);
}
const auto is_error = std::any_of(found->begin(), found->end(), [](const auto& e) {
return e->sqlstate.rfind("01", 0) != 0;
});
if (is_error) {
set_state(OdbcStatementState::STATEMENT_ERROR);
}
}

// a non polling cancel (SQLCancelHandle from the cancel thread) that lands while no
// ODBC call is running on this statement - e.g. between fetch batches - does not make
// the next call return HY008: the driver just closes the cursor, so the next SQLFetch
Expand Down Expand Up @@ -1087,6 +1125,15 @@ bool OdbcStatementLegacy::bind_fetch(const shared_ptr<BoundDatumSet>& param_set)
SQL_LOG_DEBUG_STREAM("[" << _handle.toString() << "] bind_fetch failed to execute");
return false;
}
// this execution has its results still to read: the result set's end flags are the
// last execution's (taken with info, they would say this one has nothing to read)
_resultset->_end_of_rows = false;
_resultset->_end_of_results = false;
if (ret == SQL_SUCCESS_WITH_INFO) {
// e.g. 8153 when the driver has read ahead to the end of a short result: read it now,
// before SQLRowCount clears it
keep_diagnostics();
}

ret = _odbcApi->SQLRowCount(statement.get_handle(), &_resultset->_row_count);
auto result = check_odbc_error(ret);
Expand Down Expand Up @@ -2322,6 +2369,7 @@ bool OdbcStatementLegacy::try_read_next_result() {
case SQL_NO_DATA: {
SQL_LOG_DEBUG_STREAM("OdbcStatementLegacy::try_read_next_result ["
<< _handle.toString() << "] try_read_next_result SQL_NO_DATA " << ret);
keep_diagnostics();
_resultset->_end_of_results = true;
_resultset->_end_of_rows = true;
if (_prepared) {
Expand Down
18 changes: 17 additions & 1 deletion lib/reader.js
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,16 @@ class Query extends BasePromises {
asArrays: true,
batchSize: rowBatchSize,
asObjects: false
}, cb))
}, (e, d) => {
// a batch can come with info, e.g. 8153 "null value is eliminated" at the end of its
// rows: keep it with the batch, to emit after the batch's rows (continueBatch)
if (d && Array.isArray(e) && e.length > 0 && e.every(m => this.isInfo(m))) {
d.info = e
cb(null, d)
} else {
cb(e, d)
}
}))
}

close () {
Expand Down Expand Up @@ -477,6 +486,13 @@ class Query extends BasePromises {
const d = this.batchData
this.dispatchRows(d)
if (this.park(() => this.continueBatch())) return
if (d.info) {
// the info the batch came with follows its rows, as the server sent it
const info = d.info
d.info = null
this.dispatchInfoReturnErrors(info)
if (this.park(() => this.continueBatch())) return
}
if (!d.endOfRows) {
this.dispatch()
} else if (!d.endOfResults) {
Expand Down
104 changes: 102 additions & 2 deletions test/contract/HANDOVER.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,106 @@ the contract below. Workarounds for a missing or doubled `done` (node-mssql 12.7
break. Then decide the node-mssql guard: an RC on `next` for dhensby, node-mssql-shaped
scenarios, or their suite as a canary job.

## Progress — 2026-10-22: node-mssql against v6

node-mssql's own msnodesqlv8 suite (`npx mocha --exit -t 30000 test/msnodesqlv8` in a clone,
with `node_modules/msnodesqlv8` a junction to this repo) against branch
`fix/info-dropped-on-fetch`:

| node-mssql | ODBC 18 | SQL Server Native Client 11 (their CI's driver) |
|---|---|---|
| master (f20c197) | 151 passing, 6 failing | **155 passing, 2 failing** |
| v12.7.10 (latest release) | | **155 passing, 2 failing** |
| v12.7.7 (no 5.6 workarounds) | | **154 passing, 2 failing** |

The 2 are environmental (SQL login tests; LocalDB is trusted-connection only). With ODBC 18,
4 more fail on their exact-message asserts (the driver prefix), as a 30 s timeout because the
assert throws in a callback - not a hang. Use SNAC 11 for a clean baseline: in `.mssql.json`,
`Driver={SQL Server Native Client 11.0}`. Note `npm install` in the clone removes the junction.

`test/contract/mssql/tap.js`, loaded with `--require` into their run, records every call
node-mssql makes on us and the transcript it saw (`MSNODESQLV8_TAP_OUT=<file.jsonl>`). On master:
154 tests, 873 calls - `queryRaw` with `{query_str, query_timeout}` (269), `query` with a
callback (267, pool validation and transactions), `setUseUTC`, `close`, bulk through
`tableMgr().bind` / `insertRows`. Never our `prepare` or `callproc`; no chunked column
(`more=true`). Their stream back pressure pauses on 50-row boundaries (our batch size).

**For the release notes, and dhensby:** node-mssql still emits `done` on our query itself after
a cancel of a paused query is accepted (`if (!err && req.isPaused()) req.emit('done')`). v6
raises its own `done` after the HY008, so their listeners now see
`cancel-cb ok | done (theirs) | error HY008 | done | free` - 4 of their tests. They pass (they
use `once('done')`), but the workaround should go for msnodesqlv8 >= 6.

They also read our internals: `query.queryWorker.running` (`stopped()`), `req.isPaused()`,
`req.getCancelSent()`, and assume a fetch batch of 50 rows. v6 keeps all of them; a change to
any is a break for node-mssql.

Their suite skips for msnodesqlv8: raiserror through a request, XACT_ABORT transactions,
timeouts, concurrent requests, type validation, several bulk column types. Candidates for our
corpus, as nothing of theirs guards them.

## Progress — 2026-10-22: info the driver sends is no longer dropped

Branch `fix/info-dropped-on-fetch`. New corpus group `info/` (45 transcripts) and
`scripts/prepared-info`. The reference for "what the server sent" is .NET SqlClient, which reads
TDS directly and reports every message token in order (a PowerShell probe; classic `sqlcmd` is
ODBC and has the same blind spot, so it is not a reference).

**What was lost, and why:**
- **Warnings raised at the end of a result set** — 8153 "Null value is eliminated by an
aggregate", 3607 "Division by zero occurred", 3606 "Arithmetic overflow occurred" — were
dropped **every time**. msodbcsql hands them to the `SQLFetch` that returns `SQL_NO_DATA`, as
a diagnostic record, and `fetch_read` returned on `SQL_NO_DATA` without reading it. Found by a
temporary probe in `RealOdbcApi` that logged `SQL_DIAG_NUMBER` after every statement call,
whatever it returned. A mid-rowset warning (row 75 of 120) is sent at the end too.
- And a fetch that worked could not carry diagnostics to JS at all: `FetchRowsWorker` called
back `(null, result)`, and `BasePromises.op` rejects whatever comes first.
- **Prepared executions**: the driver hands the same warning to `SQLExecute`
(`SQL_SUCCESS_WITH_INFO`), short result or long. `bind_fetch` passed it as success and
`SQLRowCount` then cleared it; and `BindQueryWorker` also called back `(null, meta)`.
- **1756 "Skipping FOREIGN KEY constraint … temporary table" was not lost by us.** It is raised
when the batch is compiled, so a cached plan raises nothing: SqlClient gets it the first time
and not the second, on identical text. The "0 of 20" in the 2026-10-18 notes was the plan
cache. With unique text it arrives every time. Scenarios with compile-time warnings put
`{unique}` in a comment (the recorder replaces it on every run); `info/fk-on-temp-table-cached`
pins that a cached plan sends none.
- PRINT and RAISERROR (any severity, `with nowait`) were always delivered, in order, including
60 messages over several packets and inside procedures.

**The fix:**
- Native: `keep_diagnostics()` appends whatever records a call that did not fail came with
(`read_errors` replaces). Called on `SQL_NO_DATA` and `SQL_SUCCESS_WITH_INFO` from `SQLFetch`
and `SQLFetchScroll`, on `SQL_NO_DATA` from `SQLMoreResults`, and on `SQL_SUCCESS_WITH_INFO`
from `SQLExecute` in `bind_fetch`. `TryReadRows` now clears the last call's errors first.
`FetchRowsWorker` and `BindQueryWorker` call back `(info, result)` when a call worked with info,
as `NextResultWorker` already did.
- `bind_fetch` resets the result set's end flags after a successful execute. They were the last
execution's, and with info `beginQuery` read "end of rows and results" as a failed query with
nothing to read: the second execution with a warning lost its rows (found by `prepared-info`).
- JS: `nativeGetRows` keeps a batch's info with the batch (`d.info`); `continueBatch` emits it
after the batch's rows, before the next fetch or result — the server's order. A pause holds it
until the resume (`info/warning-paused-*`).
- `test/warnings.test.js` "TEST TWO - NULL ELIMNATED WARNING" asserted `res.info === null`,
i.e. it pinned the bug. It now expects the 8153.

**Known, not fixed:**
- Prepared: the info comes *before* the `meta`, because the driver hands it to `SQLExecute`;
ODBC does not say where in the stream it was (`scripts/prepared-info`).
- `keep_diagnostics` appends, but a later real error in the same fetch goes through
`return_odbc_error`, which replaces: info before an error *within one fetch call* would be lost.
Not seen in any scenario.
- `QueryWorker` (direct execute) still calls back `(null, meta)` on success; info at execute
reaches JS because `try_execute_direct` returns false on `SQL_SUCCESS_WITH_INFO`. Works, but it
is not the same shape as the other workers.
- `cancel/after-error-mid-batch` shows the cancel-cb / HY008 race (about 1 in 10, on master's
native code too): it has an `--- or` alternative now.

**To review in the `info/` transcripts** (all `unreviewed`):
- each PRINT, and each statement with no result (a DDL), is followed by `rowcount 0`; an insert
then a print gives `rowcount 1 x2`. Contract question: should a PRINT end a "result"?
- callback mode, `info/warning-then-error-then-result.callback`: result 1's callback comes after
the error's callback, and the error's empty result gets `cb ok rows=0 more=true`.

## Summary at first handover (2026-10-15)

The normal streaming path is fine: plain reads, multiple result sets, and pause/resume part
Expand Down Expand Up @@ -123,7 +223,7 @@ at execute, the error comes with `more=false` and the rest of the batch is lost:
the #475 fix in 5.6.0 (`odbc_statement_legacy.cpp`, the look-ahead after a failed execute)
does not hold on Linux. The same 4 scenarios differ with the base commit's `lib/`, so it is
older than the queue work. Pinned as `errors/{raiserror-then-result,dup-key-d}.*.linux.txt`
`scripts/issue-478.linux.txt` (its case D) and `cancel/after-error-mid-batch.events.linux.txt` (the batch is gone, so the cancel finds nothing), `# status: known-bug`, so that CI is green
`scripts/issue-478.linux.txt` (its case D) and `cancel/after-error-mid-batch.events.linux.txt` (the batch is gone, so the cancel finds nothing) and `info/print-after-error.{events,callback}.linux.txt` (2026-10-22: the print and result after the error are lost), `# status: known-bug`, so that CI is green
and the fix shows up as a diff. Filed as #483.

To run the corpus on WSL: a `git worktree` under `/tmp/<x>/node_modules/msnodesqlv8` (the
Expand Down Expand Up @@ -223,7 +323,7 @@ described with no columns at prepare, so its executions return no rows (`rowcoun

## Next

### 1. Nothing the driver sends is dropped (the next session's theme)
### 1. Nothing the driver sends is dropped (done 2026-10-22, see Progress; the 1756 below was the plan cache)

Any information the driver sends should reach the caller. Today some is silently lost:
- Info from some DDL is mostly lost, on master too: 1756 "Skipping FOREIGN KEY constraint …
Expand Down
11 changes: 11 additions & 0 deletions test/contract/corpus/cancel/after-error-mid-batch.events.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
# status: agreed
# sql: raiserror('boom', 16, 1); waitfor delay '00:00:01'; select 2 as n
# at: {"error":{"1":["cancel"]}}
# the cancel callback and the HY008 from the call in flight are two native completions;
# either can reach JS first (about 1 run in 10, on master too)
submitted
error 42000 50000 boom more=true
action cancel
Expand All @@ -10,3 +12,12 @@ error HY008 0 Operation canceled more=false
done
free
check ok
--- or
submitted
error 42000 50000 boom more=true
action cancel
error HY008 0 Operation canceled more=false
cancel-cb ok
done
free
check ok
12 changes: 12 additions & 0 deletions test/contract/corpus/info/divide-by-zero-warning.callback.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# scenario: info/divide-by-zero-warning.callback
# status: unreviewed
# sql: set ansi_warnings off; set arithabort off; select 1 / 0 as n; set ansi_warnings on; set arithabort on
submitted
meta n
row
column 0 null
info 01000 3607 Division by zero occurred.
cb ok rows=1 more=false
done
free
check ok
11 changes: 11 additions & 0 deletions test/contract/corpus/info/divide-by-zero-warning.events.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# scenario: info/divide-by-zero-warning.events
# status: unreviewed
# sql: set ansi_warnings off; set arithabort off; select 1 / 0 as n; set ansi_warnings on; set arithabort on
submitted
meta n
row
column 0 null
info 01000 3607 Division by zero occurred.
done
free
check ok
Loading
Loading