fix(adapter-mssql): set VarBinary type when null Bytes? argument is passed #29557

[AnupamKumar-1]

Fixes #29557

When setting a Bytes? (@db.VarBinary(Max)) field to null, the adapter was sending the parameter without a type, causing MSSQL to default to nvarchar and throw an implicit conversion error.

Fixed by explicitly passing sql.VarBinary as the type for null bytes arguments in performIO.

All 87 tests passing locally.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: ddf6cb5f-fda4-497a-8907-1e21c5df53dd

📥 Commits

Reviewing files that changed from the base of the PR and between 6aaef74 and cc62488.

📒 Files selected for processing (2)

• packages/adapter-mssql/src/mssql.test.ts
• packages/adapter-mssql/src/mssql.ts

---

Summary by CodeRabbit

• Bug Fixes

  • Improved parameter binding for null binary/bytes values in the MSSQL adapter, ensuring UPDATE statements that set binary columns to NULL execute correctly.

• Tests

  • Added an integration-style MSSQL test that verifies null bytes behavior using a real database connection, and automatically skips when the required test database connection string isn’t provided.

Walkthrough

This PR fixes a bug where updating a Bytes? field mapped to @db.VarBinary(Max) with null using the MSSQL adapter throws an implicit type conversion error. The fix modifies the performIO method to explicitly bind null byte arguments as VarBinary instead of allowing default type inference, which was incorrectly treating them as nvarchar. An integration test validates the fix by creating a table with a binary column, inserting data, updating the column to null, and confirming the operation succeeds.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and accurately describes the main fix: handling VarBinary type when null Bytes arguments are passed in the MSSQL adapter.
Description check	✅ Passed	The description is related to the changeset, explaining the bug fix, root cause, and implementation approach in the MSSQL adapter.
Linked Issues check	✅ Passed	The code changes directly address all objectives from issue #29557: fixing null Bytes field updates by explicitly setting VarBinary type in performIO to prevent nvarchar conversion errors.
Out of Scope Changes check	✅ Passed	All changes are scoped to the MSSQL adapter and directly related to fixing the null Bytes parameter type issue described in #29557, with no extraneous modifications.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

✨ Simplify code

• Create PR with simplified code

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/adapter-mssql/src/mssql.test.ts`:
- Around line 176-183: Replace hardcoded MSSQL credentials used to instantiate
PrismaMssql in the test with environment-driven configuration: read server,
port, user, password, database, and options from env vars (e.g., MSSQL_HOST,
MSSQL_USER, MSSQL_PASSWORD, MSSQL_DB) when creating the factory (PrismaMssql),
and if required creds are not set, skip the integration test gracefully (use the
test framework's skip/skipIf mechanism) with a clear message so CI or local runs
without credentials don't fail.
- Around line 219-225: The teardown that drops dbo.PrismaTest and calls
adapter.dispose currently runs inline and will be skipped if the test fails;
move that cleanup into a finally block or a Vitest afterEach hook so it always
executes. Specifically, ensure the adapter.executeRaw({... sql: `DROP TABLE
dbo.PrismaTest` ...}) and adapter.dispose() calls are performed from a finally
block surrounding the test logic (or from an afterEach that has access to the
same adapter instance), so cleanup always runs even on test failures.
- Around line 208-217: The test uses a non-standard Vitest assertion
`.resolves.not.toThrow()` on the promise returned by adapter.executeRaw; change
it to a valid pattern by either awaiting the call (await
adapter.executeRaw({...})) to ensure it doesn't reject, or use the expect
wrapper with a valid matcher like
`expect(adapter.executeRaw({...})).resolves.toBeDefined()` so the async update
via adapter.executeRaw (sql: `UPDATE dbo.PrismaTest...`, args/argTypes) is
correctly asserted.
- Around line 202-205: The test's ArgType objects in
packages/adapter-mssql/src/mssql.test.ts are missing the required arity property
and use unsafe `dbType: null`; update each argTypes array (both the INSERT case
around the first argTypes block and the UPDATE case around the second argTypes
block) to include the correct arity (e.g., arity: 1 for scalar parameters) and
remove the dbType field (or leave it undefined) so the shape matches the ArgType
type; additionally replace any hardcoded MSSQL connection credentials in the
test setup with environment variables (process.env.MSSQL_USER, MSSQL_PASS,
MSSQL_HOST, etc.) so credentials are not embedded in the test.

In `@packages/adapter-mssql/src/mssql.ts`:
- Around line 55-62: The loop handling parameter binding in mssql.ts currently
special-cases null byte parameters only with sql.VarBinary; update the null
handling in the for loop that iterates over query.args and checks
argType.scalarType === 'bytes' so it binds nulls using an explicit binary type
for all binary column types (sql.VarBinary, sql.Binary, sql.Image) rather than
relying on implicit inference; simplest fix: when query.args[i] === null and
argType.scalarType === 'bytes' call req.input with a binary type (default to
sql.VarBinary) instead of letting mapArg decide, or if a dbType is available on
argType use that to choose between sql.VarBinary/sql.Binary/sql.Image before
calling req.input.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 46cbcd79-44da-4b04-b046-a0bd2621df52

📥 Commits

Reviewing files that changed from the base of the PR and between 918cf5d and fc8baf4.

📒 Files selected for processing (2)

• packages/adapter-mssql/src/mssql.test.ts
• packages/adapter-mssql/src/mssql.ts

[daltino]

Great fix for a real pain point — implicit conversion errors in MSSQL are notoriously cryptic, and this tackles the root cause cleanly. The approach is correct and the test is a welcome addition. I have a few suggestions worth considering before merging.

---

Review

packages/adapter-mssql/src/mssql.ts

The core fix is correct. When mssql receives a null value with no type hint, it falls back to nvarchar, which can't be implicitly converted to varbinary. Explicitly passing sql.VarBinary resolves the ambiguity.

Suggestion 1 — generalise the null-type pattern

The same implicit-conversion problem could affect other binary/typed columns when they're null. Right now you're special-casing bytes, but consider whether other scalar types need the same treatment. At minimum, a comment explaining why bytes is special-cased (vs. relying on mapArg for everything else) would help future maintainers:

// mssql cannot infer the correct type for a null binary parameter and
// defaults to nvarchar, causing an implicit conversion error. We must
// supply the type explicitly.
if (query.args[i] === null && argType.scalarType === 'bytes') {
  req.input(`P${i + 1}`, sql.VarBinary, null)
} else {
  req.input(`P${i + 1}`, mapArg(query.args[i], argType))
}

Suggestion 2 — check whether mapArg already receives null and handles it elsewhere

It's worth verifying (and documenting) that mapArg does not already attempt to set a type when the value is null. If it silently passes through null without a type for other scalar types too, there may be a latent issue with e.g. DateTime? or Float? fields. A brief comment or a link to the mapArg implementation would add confidence.

Suggestion 3 — minor: extract the condition

Purely cosmetic, but pulling the special-case into a helper makes the loop body easier to scan:

function inputParam(req, name, arg, argType) {
  if (arg === null && argType.scalarType === 'bytes') {
    req.input(name, sql.VarBinary, null)
  } else {
    req.input(name, mapArg(arg, argType))
  }
}

Not blocking — just a readability nudge.

---

packages/adapter-mssql/src/mssql.test.ts

The test validates the right behaviour (no throw on null assignment) and has a sensible skip-if-no-connection guard.

Suggestion 4 — the test requires a live DB connection

The existing mutex tests in this file are fully mocked, so they run anywhere. The new test silently skips when TEST_MSSQL_URI is unset, which is fine for local dev but means CI without a real MSSQL instance will never exercise this path. Consider:

• Adding a comment explaining the skip is intentional and pointing to wherever integration tests are run with a live DB.
• Or — if this can be mocked — verifying that req.input is called with (name, sql.VarBinary, null) rather than relying on a full round-trip. A unit-style mock test would run in every CI environment.

Suggestion 5 — schema/teardown robustness

await prisma.$executeRawUnsafe(`
  CREATE TABLE TestVarBinary (id INT PRIMARY KEY, data VARBINARY(MAX))
`)

If the test fails before the DROP TABLE, subsequent runs will fail on the CREATE TABLE with "object already exists". Wrap the setup/teardown in a try/finally, or use IF OBJECT_ID(...) IS NOT NULL DROP TABLE ... in the teardown.

Suggestion 6 — disconnect on failure

If any assertion throws before await prisma.$disconnect(), the connection leaks. Wrap the body in try/finally:

try {
  // ... test body
} finally {
  await prisma.$disconnect()
}

---

Summary

Bug fix correctness	✅
Handles the described regression	✅
Test added	✅
Test isolation (teardown safety)	⚠️ needs try/finally
Coverage in always-on CI	⚠️ worth discussing
Generalisation / future-proofing	💡 optional but worth a comment

The fix itself is solid. Addressing the teardown robustness (Suggestion 5 & 6) would be the main thing I'd want to see before merging. Everything else is optional polish.

[AnupamKumar-1]

@daltino Thanks for the thorough review!

• Addressed suggestions 5 & 6: cleanup now uses IF OBJECT_ID(...) IS NOT NULL DROP TABLE to make teardown idempotent, and dispose() is executed in a nested finally block so the connection is always released even if cleanup fails.
• Addressed suggestion 4: added a comment clarifying that the test is intentionally skipped when TEST_MSSQL_URI is not provided and that MSSQL integration tests run separately against a real MSSQL instance.
• Addressed suggestion 1: added a comment explaining why bytes requires special handling when the value is null (mssql defaults to nvarchar rather than VARBINARY, which causes the implicit conversion error).

Tested locally against a real MSSQL instance running in Docker; all tests pass, including the new integration test (87/87).

Thanks again for the review.

[aqrln]

Thanks!