test(mssql): add reproduction test for batch insert type inconsistency#29418
test(mssql): add reproduction test for batch insert type inconsistency#29418yangbobo2021 wants to merge 1 commit intoprisma:mainfrom
Conversation
…y (issue prisma#29354) This commit adds a test case that reproduces the root cause of issue prisma#29354: when performing batch inserts with Float field values, the mssql adapter parameterizes values inconsistently depending on their range. The test verifies that when req.input() is called without an explicit SQL type, the mssql package auto-detects types: - Values within int32 range -> SQL int - Values exceeding int32 range -> SQL nvarchar This type inconsistency causes SQL Server type precedence issues in batch insert operations. Root cause location: packages/adapter-mssql/src/mssql.ts:56 Current code: req.input(`P${i + 1}`, mapArg(query.args[i], query.argTypes[i])) Suggested fix: req.input(`P${i + 1}`, sql.Float, mapArg(...)) Test execution: export TEST_MSSQL_URI="mssql://SA:Pr1sm4_Pr1sm4@localhost:1433/master" pnpm --filter @prisma/adapter-mssql test issue-29354-batch-insert
Summary by CodeRabbit
WalkthroughA new Vitest integration test file is added for the MSSQL adapter to reproduce Issue Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/adapter-mssql/src/issue-29354-batch-insert.test.ts`:
- Around line 112-141: The simulated type-detection block (detectedTypes,
INT32_MAX, hasInconsistentTypes, uniqueTypes derived from testValues) is
illustrative only and doesn't inspect mssql internals; update the test by adding
a brief clarifying comment above this block that the primary verification is the
actual batch insert behavior (the subsequent insert attempt/assertions) and that
the detectedTypes logic is only for human-readable illustration of the expected
mismatch rather than a real assertion of mssql parameter types.
- Around line 162-170: The verification query and console.log should be guarded
when the batch insert fails: detect whether the insert try/catch (the block that
catches the batch insert error at the referenced catch) recorded a failure
(e.g., set a boolean like insertSucceeded) and only run the SELECT/console.log
of result.recordset.map(r => r.maxIndividualLoss) if insertSucceeded is true;
otherwise skip the SELECT and log a concise message indicating the insert failed
so the test output is clear. Ensure you reference the same pool variable and the
Issue29354Test table name when implementing the guard.
- Around line 57-62: The test body calls test.skip() conditionally, which is
misuse; instead remove the test.skip() call inside the async test and simply
early-return when the env var is missing (or declare the test as skipped at
definition time using test.skip with the same title), i.e., inside the test
handler for "should reproduce parameter type inconsistency in batch insert"
check process.env.TEST_MSSQL_URI and if falsy do a plain return (or move the
condition to use test.skip('should reproduce parameter type inconsistency in
batch insert') before the test is defined) so you stop executing but do not
invoke test.skip() mid-test.
- Around line 58-75: The test reads testMssqlUri but then uses a hardcoded
config, so either parse TEST_MSSQL_URI into the sql.config used by the test or
explicitly make the env var only a run-flag and document that; update the code
around the testMssqlUri variable and the config object so that if TEST_MSSQL_URI
is present you parse it (host, port, user, password, database, options/pool) and
populate the config, otherwise fall back to the existing hardcoded values, or
alternatively add a clear comment and rename testMssqlUri to something like
RUN_MSSQL_TEST to reflect it is only a flag. Ensure the parsing logic handles
typical mssql connection strings/URLs and populates the same config keys
referenced in the test.
🪄 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: 1126532f-d9ff-4270-9a3c-4502c52dde4e
📒 Files selected for processing (1)
packages/adapter-mssql/src/issue-29354-batch-insert.test.ts
| test('should reproduce parameter type inconsistency in batch insert', async () => { | ||
| const testMssqlUri = process.env.TEST_MSSQL_URI | ||
| if (!testMssqlUri) { | ||
| test.skip() | ||
| return | ||
| } |
There was a problem hiding this comment.
Incorrect usage of test.skip() inside test body.
In Vitest, test.skip() is meant to define a skipped test at declaration time, not for conditional skipping inside a test body. Calling it mid-test may cause unexpected behavior. The return statement mitigates this, but the test.skip() call itself is problematic.
Suggested fix: Use early return or skipIf pattern
Option 1 - Simple early return (preferred for this case):
test('should reproduce parameter type inconsistency in batch insert', async () => {
const testMssqlUri = process.env.TEST_MSSQL_URI
if (!testMssqlUri) {
- test.skip()
+ console.log('Skipping: TEST_MSSQL_URI not set')
return
}Option 2 - Use describe.skipIf at the describe block level:
-describe('Issue `#29354`: Batch Insert Type Inconsistency', () => {
+describe.skipIf(!process.env.TEST_MSSQL_URI)('Issue `#29354`: Batch Insert Type Inconsistency', () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('should reproduce parameter type inconsistency in batch insert', async () => { | |
| const testMssqlUri = process.env.TEST_MSSQL_URI | |
| if (!testMssqlUri) { | |
| test.skip() | |
| return | |
| } | |
| test('should reproduce parameter type inconsistency in batch insert', async () => { | |
| const testMssqlUri = process.env.TEST_MSSQL_URI | |
| if (!testMssqlUri) { | |
| console.log('Skipping: TEST_MSSQL_URI not set') | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/adapter-mssql/src/issue-29354-batch-insert.test.ts` around lines 57
- 62, The test body calls test.skip() conditionally, which is misuse; instead
remove the test.skip() call inside the async test and simply early-return when
the env var is missing (or declare the test as skipped at definition time using
test.skip with the same title), i.e., inside the test handler for "should
reproduce parameter type inconsistency in batch insert" check
process.env.TEST_MSSQL_URI and if falsy do a plain return (or move the condition
to use test.skip('should reproduce parameter type inconsistency in batch
insert') before the test is defined) so you stop executing but do not invoke
test.skip() mid-test.
| const testMssqlUri = process.env.TEST_MSSQL_URI | ||
| if (!testMssqlUri) { | ||
| test.skip() | ||
| return | ||
| } | ||
|
|
||
| const config: sql.config = { | ||
| server: 'localhost', | ||
| port: 1433, | ||
| user: 'SA', | ||
| password: 'Pr1sm4_Pr1sm4', | ||
| database: 'master', | ||
| options: { | ||
| enableArithAbort: false, | ||
| trustServerCertificate: true, | ||
| }, | ||
| pool: { max: 1 }, | ||
| } |
There was a problem hiding this comment.
TEST_MSSQL_URI is checked but never used; config is hardcoded.
The test checks for TEST_MSSQL_URI to determine whether to run, but then ignores it entirely and uses hardcoded connection parameters. This is misleading—if someone sets a different URI (e.g., different host, port, or credentials), the test will ignore their configuration and potentially fail or connect to the wrong server.
Either parse the URI to extract connection parameters, or document that the env var is only a "run this test" flag and the connection details are fixed.
Suggested fix: Parse the URI or clarify intent
Option 1 - Parse the URI:
test('should reproduce parameter type inconsistency in batch insert', async () => {
const testMssqlUri = process.env.TEST_MSSQL_URI
if (!testMssqlUri) {
console.log('Skipping: TEST_MSSQL_URI not set')
return
}
- const config: sql.config = {
- server: 'localhost',
- port: 1433,
- user: 'SA',
- password: 'Pr1sm4_Pr1sm4',
- database: 'master',
- options: {
- enableArithAbort: false,
- trustServerCertificate: true,
- },
- pool: { max: 1 },
- }
+ const url = new URL(testMssqlUri)
+ const config: sql.config = {
+ server: url.hostname,
+ port: parseInt(url.port, 10) || 1433,
+ user: decodeURIComponent(url.username),
+ password: decodeURIComponent(url.password),
+ database: url.pathname.slice(1) || 'master',
+ options: {
+ enableArithAbort: false,
+ trustServerCertificate: true,
+ },
+ pool: { max: 1 },
+ }Option 2 - Clarify the env var is just a flag:
+ // Note: TEST_MSSQL_URI is used only as a flag to enable this test.
+ // Connection uses fixed localhost config for reproducibility.
const config: sql.config = {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const testMssqlUri = process.env.TEST_MSSQL_URI | |
| if (!testMssqlUri) { | |
| test.skip() | |
| return | |
| } | |
| const config: sql.config = { | |
| server: 'localhost', | |
| port: 1433, | |
| user: 'SA', | |
| password: 'Pr1sm4_Pr1sm4', | |
| database: 'master', | |
| options: { | |
| enableArithAbort: false, | |
| trustServerCertificate: true, | |
| }, | |
| pool: { max: 1 }, | |
| } | |
| const testMssqlUri = process.env.TEST_MSSQL_URI | |
| if (!testMssqlUri) { | |
| test.skip() | |
| return | |
| } | |
| const url = new URL(testMssqlUri) | |
| const config: sql.config = { | |
| server: url.hostname, | |
| port: parseInt(url.port, 10) || 1433, | |
| user: decodeURIComponent(url.username), | |
| password: decodeURIComponent(url.password), | |
| database: url.pathname.slice(1) || 'master', | |
| options: { | |
| enableArithAbort: false, | |
| trustServerCertificate: true, | |
| }, | |
| pool: { max: 1 }, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/adapter-mssql/src/issue-29354-batch-insert.test.ts` around lines 58
- 75, The test reads testMssqlUri but then uses a hardcoded config, so either
parse TEST_MSSQL_URI into the sql.config used by the test or explicitly make the
env var only a run-flag and document that; update the code around the
testMssqlUri variable and the config object so that if TEST_MSSQL_URI is present
you parse it (host, port, user, password, database, options/pool) and populate
the config, otherwise fall back to the existing hardcoded values, or
alternatively add a clear comment and rename testMssqlUri to something like
RUN_MSSQL_TEST to reflect it is only a flag. Ensure the parsing logic handles
typical mssql connection strings/URLs and populates the same config keys
referenced in the test.
| // STEP 4: Verify parameter types (THE ROOT CAUSE) | ||
| // mssql package doesn't expose parameter types directly, so we infer from behavior | ||
| console.log('Parameter inference:') | ||
|
|
||
| const INT32_MAX = 2147483647 | ||
| const detectedTypes = testValues.map((value) => { | ||
| if (typeof value === 'number' && value <= INT32_MAX && value >= -INT32_MAX - 1) { | ||
| return 'int' | ||
| } else { | ||
| return 'nvarchar' | ||
| } | ||
| }) | ||
|
|
||
| detectedTypes.forEach((type, index) => { | ||
| console.log(` @P${index + 1}: ${type} (value: ${testValues[index]})`) | ||
| }) | ||
|
|
||
| // STEP 5: Verify BUG - parameter types are INCONSISTENT | ||
| const uniqueTypes = new Set(detectedTypes) | ||
| const hasInconsistentTypes = uniqueTypes.size > 1 | ||
|
|
||
| console.log('\nResult:') | ||
| if (hasInconsistentTypes) { | ||
| console.log(' ❌ BUG CONFIRMED: Parameter types are INCONSISTENT') | ||
| console.log(` ❌ Types detected: ${Array.from(uniqueTypes).join(', ')}`) | ||
| console.log(' ❌ This causes SQL Server type precedence issues in batch insert') | ||
| } else { | ||
| console.log(' ✅ Parameter types are consistent') | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type detection is simulated, not verified from mssql internals.
The test simulates what mssql's auto-detection should do based on int32 range checks, but doesn't actually verify the types that mssql assigns to parameters. The assertion expect(hasInconsistentTypes).toBe(true) passes based on the simulation logic, not on actual mssql behavior.
This is acknowledged in the comments, and the actual insert attempt (lines 152-160) does provide behavioral evidence. Consider adding a comment clarifying that the primary verification is whether the insert succeeds/fails, and the simulated type detection is illustrative.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/adapter-mssql/src/issue-29354-batch-insert.test.ts` around lines 112
- 141, The simulated type-detection block (detectedTypes, INT32_MAX,
hasInconsistentTypes, uniqueTypes derived from testValues) is illustrative only
and doesn't inspect mssql internals; update the test by adding a brief
clarifying comment above this block that the primary verification is the actual
batch insert behavior (the subsequent insert attempt/assertions) and that the
detectedTypes logic is only for human-readable illustration of the expected
mismatch rather than a real assertion of mssql parameter types.
| // STEP 7: Verify inserted values | ||
| const result = await pool.request().query('SELECT maxIndividualLoss FROM Issue29354Test ORDER BY id') | ||
| console.log( | ||
| '\nInserted values:', | ||
| result.recordset.map((r: any) => r.maxIndividualLoss), | ||
| ) | ||
|
|
||
| // STEP 8: Clean up | ||
| await pool.request().query('DROP TABLE Issue29354Test') |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider guarding the verification query when insert fails.
If the batch insert fails (caught at lines 157-160), the verification query at line 163 will return an empty recordset, and result.recordset.map(...) will produce an empty array. This works but could be confusing when reviewing test output. Consider skipping verification or adding context when the insert fails.
Optional: Add conditional logging
} catch (error: any) {
console.log(`\n❌ Batch insert failed: ${error.message}`)
console.log(' This demonstrates the type inconsistency issue')
+ // Skip verification when insert fails
+ await pool.request().query('DROP TABLE Issue29354Test')
+ expect(hasInconsistentTypes).toBe(true)
+ return
}
// STEP 7: Verify inserted values🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/adapter-mssql/src/issue-29354-batch-insert.test.ts` around lines 162
- 170, The verification query and console.log should be guarded when the batch
insert fails: detect whether the insert try/catch (the block that catches the
batch insert error at the referenced catch) recorded a failure (e.g., set a
boolean like insertSucceeded) and only run the SELECT/console.log of
result.recordset.map(r => r.maxIndividualLoss) if insertSucceeded is true;
otherwise skip the SELECT and log a concise message indicating the insert failed
so the test output is clear. Ensure you reference the same pool variable and the
Issue29354Test table name when implementing the guard.
There was a problem hiding this comment.
we should avoid spamming console.log from tests, this should just have an concise and meaningful failing assertion
3 participants
This PR adds a reproduction test for #29354.
Problem
When performing batch inserts with Float field values, the mssql adapter produces inconsistent parameter types depending on the value range:
Root Cause
`packages/adapter-mssql/src/mssql.ts:56` calls `req.input()` without an explicit SQL type:
```typescript
req.input(`P${i + 1}`, mapArg(query.args[i], query.argTypes[i]))
```
This allows the mssql package to auto-detect types based on values, causing type inconsistency in batch operations.
Test Case
The test demonstrates that with values `[450000000, 3850000000, 1600000000]`:
Execution
```bash
export TEST_MSSQL_URI="mssql://SA:Pr1sm4_Pr1sm4@localhost:1433/master"
pnpm --filter @prisma/adapter-mssql test issue-29354-batch-insert
```
Suggested Fix
Add explicit SQL type to `req.input()`:
```typescript
req.input(`P${i + 1}`, sql.Float, mapArg(query.args[i], query.argTypes[i]))
```
Would you like me to create a follow-up PR with the fix?