Skip to content

fix: bulk load created tables and Xml columns on tedious - #1914

Merged
dhensby merged 1 commit into
tediousjs:masterfrom
dhensby:fix/tedious-bulk-unsized-columns
Oct 13, 2026
Merged

dhensby merged 1 commit into
tediousjs:masterfrom
dhensby:fix/tedious-bulk-unsized-columns

Conversation

@dhensby

@dhensby dhensby commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

On the tedious driver, these bulk loads fail with Invalid column type from bcp client for colid 1. (4816):

  • table.create = true with a VarChar, NVarChar or VarBinary column declared without a length (e.g. t.columns.add('a', sql.NVarChar)). Table#declare creates the column as max, but node-mssql passed no length to bulk.addColumn. tedious resolves a missing length to 4000 or 8000, and the server won't load a sized column into a max one.
  • Every sql.Xml column, into a created table or an existing xml column. Table#_makeBulk turns Xml into an NVarChar and drops the length, so it hits the same mismatch.
  • sql.UDT, sql.Geography and sql.Geometry columns, which _makeBulk turns into a VarBinary without a length, go through the same path. I couldn't verify these locally (see Caveats).

Fix

  • lib/tedious/request.js: a VarChar, NVarChar or VarBinary bulk column without a length is sent as max only when the request creates the table (table.create), the case where node-mssql itself declares it as max. This uses MAX_LENGTH_TYPES, as the TVP path already does.
  • lib/table.js: _makeBulk gives Xml, UDT, Geography and Geometry columns a max length when they have none, matching how Table#declare creates them. msnodesqlv8's output is unchanged: Table#declare gives (MAX) for both a missing length and 65535, and that driver binds from the target's metadata.
  • Existing tables are unchanged. Plain VarChar/NVarChar/VarBinary/NText columns without a length keep the length tedious gives them, so loads into sized or sql_variant columns behave, and perform, exactly as before.

Narrowed after a compatibility audit

The first version of this PR sent every unsized column as max. A before/after audit (25,830 target type × declared type × value combinations, on tedious 19.2.2 and 20.3.4) showed that cost loads that work today:

  • about 1.4–2x slower bulk loads into sized columns, with ~50% more bytes on the wire;
  • a 206 error for every row loaded into a sql_variant column;
  • 4815 becoming 2628, so the server no longer rolled the transaction back.

Re-running the same matrix on this version gives:

  • Existing tables, plain columns: no difference at all from master for unsized NVarChar, VarChar, VarBinary and NText columns, or for any explicitly sized declaration.
  • Existing tables, converted types: the only differences are sql.Xml, sql.UDT, sql.Geography and sql.Geometry columns. Almost all are fixes: 4816 → loads into xml/max targets.
  • create = true: 60 combinations go from failing to loading, and none the other way.

Not fixed here

Loading an unsized VarChar/NVarChar/VarBinary column into an existing varchar(max)/nvarchar(max)/varbinary(max) column still fails with 4816. node-mssql doesn't know the target's column types on this path. The workaround is to declare the column as max (e.g. sql.NVarChar(sql.MAX)); the README bulk section now says so. Fixing it properly would mean reading the target's metadata first, as SqlBulkCopy does, which is a separate change.

Caveats

These apply only to sql.Xml, sql.UDT, sql.Geography and sql.Geometry bulk columns, which are now sent as max:

  • sql_variant target: loading one of these into an existing sql_variant column now fails with 206 ("Operand type clash: nvarchar(max) / varbinary(max) is incompatible with sql_variant"). Workaround: give the column a length.
  • Over-long values into sized columns: a value over 4000 chars / 8000 bytes going into a smaller column now gets the server's 2628 ("String or binary data would be truncated") instead of 4815. 4815 is severity 17 and made the server roll back the surrounding transaction; 2628 leaves the transaction open.
  • Spatial and UDT targets: verified on a real SQL Server 2022 (see Tests). sql.Geography/sql.Geometry into existing geography/geometry columns failed with 4816 on master and load with this change, which is the bug reported in Geography and Geometry. Putting the Genie Back in the Bottle... #764. sql.UDT into hierarchyid loaded on master and still loads as varbinary(max).

One case applies to every type: table.create = true against a table that already exists. node-mssql only creates the table if it is missing, so the existing table's column types are still unknown, and unsized columns are now sent as max. If that table has sized columns, the load is slower; if it has sql_variant columns, it fails with 206. Tables that node-mssql created itself have max columns, so they are unaffected.

Tests

New shared integration tests:

  • bulk load with columns declared without a length (create = true; unsized NVarChar, VarChar, VarBinary and sql.Xml): checks rowsAffected and reads each value back. Fails on master with 4816.
  • bulk load of an Xml column into an existing xml column (fixture bulk_table_xml). Fails on master with 4816.
  • bulk load of spatial columns into existing geography and geometry columns (fixture bulk_table_spatial): bulk loads the bytes the server serializes a geography/geometry point to, and reads them back with STAsText(). Fails on master with 4816.
  • bulk load of a UDT column into an existing hierarchyid column (fixture bulk_table_hierarchy): a guard; passes on master and with this change.
  • bulk load of columns without a length into existing sized columns and bulk load of a column without a length into an existing sql_variant column (fixtures bulk_table_sized, bulk_table_variant): guards for the boundary this change keeps. Without table.create, unsized columns keep the driver's default length, so sized and sql_variant targets still load. Both pass on master and with this change; the earlier, broader version of this PR failed the sql_variant one with 206.

All are registered in both suites. In the msnodesqlv8 suite the Xml, sql_variant and spatial tests run on msnodesqlv8 2.x and are skipped from 3.0 ('… (not supported by msnodesqlv8 3 and later)'). From 3.0, msnodesqlv8 binds a bulk column by the target column's type and has no binding for those types: it rejects them with IMNOD: [msnodesql] Parameter 1: Invalid parameter type.

A unit test, bulk loads converted types as max, checks that _makeBulk() turns Xml into an NVarChar and UDT/Geography/Geometry into a VarBinary, all with a max length. It also checks that an explicit length is kept and that Table#declare creates the same columns as before. It fails on master.

Verification:

  • npm run lint is clean and npm run test-unit passes.
  • Real SQL Server 2022 Developer (16.0.4255.1, Windows Server 2022, the contrib/azure-sql-vm template):
    • tedious 19.2.2 and 20.3.7 (Node 22): 163 passing, 0 failing, including user defined types. With master's lib/tedious/request.js and lib/table.js, the create = true, Xml and spatial tests fail with 4816, and the sized, sql_variant and hierarchyid guards pass.
    • msnodesqlv8 via SQL Server Native Client 11.0:
      • 2.7.0 (Node 18): 143 passing, 0 failing.
      • 3.1.0 (Node 20), 5.2.1 (Node 20), 4.5.0 (Node 22) and 5.6.0 (Node 22): 140 passing, 0 failing, with the three gated tests skipped.
  • Local Azure SQL Edge (no CLR): the spatial and hierarchyid tests fail with CLR is not enabled, the same way user defined types already does. Everything else passes.

Found while bringing the two driver suites in line (#1908, #1910).

🤖 Generated with Claude Code

@dhensby dhensby changed the title fix: bulk load columns declared without a length on tedious fix: bulk load created tables and Xml columns on tedious Oct 5, 2026
@dhensby
dhensby force-pushed the fix/tedious-bulk-unsized-columns branch from d0ce067 to a199662 Compare October 5, 2026 08:12
@dhensby
dhensby force-pushed the fix/tedious-bulk-unsized-columns branch from a199662 to 41e7222 Compare October 13, 2026 11:48
@dhensby
dhensby requested a balanced review from Copilot October 13, 2026 13:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Important existing-table compatibility and spatial/UDT paths lack regression coverage, and the public guidance omits an existing-table caveat.

3 open findings
What changed in this PR

Fixes tedious bulk loading for created max columns and XML/spatial types.

Changes:

  • Sends unsized created-table columns as max.
  • Preserves max length during XML/UDT/spatial conversion.
  • Adds integration coverage, fixtures, cleanup, and usage guidance.
File Description
lib/​tedious/​request.js Applies max metadata to unsized created-table columns.
lib/​table.js Preserves max length for converted bulk types.
test/​common/​tests.js Adds unsized-column and XML bulk tests.
test/​tedious/​tedious.js Registers both new tests.
test/​msnodesqlv8/​msnodesqlv8.js Registers compatible coverage and XML skip.
test/​prepare.sql Adds the XML table fixture.
test/​cleanup.sql Removes new bulk-test tables.
README.md Documents unsized bulk-column behavior.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread README.md Resolved
Comment thread lib/table.js Resolved
Comment thread lib/tedious/request.js Resolved
@dhensby
dhensby marked this pull request as draft October 13, 2026 13:19
@dhensby
dhensby requested a balanced review from Copilot October 13, 2026 15:15

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Two documentation statements contradict the implemented explicit-length behavior.

1 open finding
3 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Low severity Correct Xml bulk-loading comment to reflect MAX assignment

test/​common/​tests.js:1390

This comment describes Xml as being bulk-loaded without a length, but _makeBulk() now assigns MAX before the bulk metadata and declaration are built. Update it to describe the behavior this regression test actually exercises.

🧠 Review effort: Balanced

Comment thread README.md Resolved

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The bulk protocol change intentionally alters compatibility behavior across several SQL Server target types and warrants final human review.

0 open findings

1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Low severity Clarify length behavior when table.create is true

lib/​tedious/​request.js:386

This comment still says existing tables keep tedious's default length, but table.create = true takes the MAX branch even when the conditional CREATE TABLE finds that the table already exists. Clarify that only table.create = false preserves the default length so this does not contradict the documented caveat.

🧠 Review effort: Balanced

A VarChar, NVarChar or VarBinary column added to a Table without a
length (e.g. `sql.NVarChar`) was handed to tedious's bulk load without
one, and tedious resolves a missing length to 4000 or 8000. With
`table.create`, `Table#declare` creates such a column as max, so the
server rejected the load with "Invalid column type from bcp client for
colid 1.". Every `sql.Xml` column failed the same way, as it is bulk
loaded as an NVarChar without a length.

Send these columns as max when `table.create` is set, even if the table
turns out to exist already, and give Xml, UDT, Geography and Geometry
columns, which are bulk loaded as NVarChar or VarBinary, a max length
unless one is given. Without `table.create` the target's column types
are not known, so a column keeps the length tedious gives it: an
unsized VarChar, NVarChar or VarBinary column still loads into a sized
or sql_variant column as before, and loading into an existing max
column still needs the column declared as max.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dhensby
dhensby force-pushed the fix/tedious-bulk-unsized-columns branch from 001ca8d to 0a7a02c Compare October 13, 2026 15:41
@dhensby
dhensby marked this pull request as ready for review October 13, 2026 15:41
@dhensby
dhensby merged commit 6ed1867 into tediousjs:master Oct 13, 2026
47 checks passed
@dhensby
dhensby deleted the fix/tedious-bulk-unsized-columns branch October 13, 2026 16:17
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 12.7.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants