Merge pull request #1841 from dhensby/fix/tvp-variant-error

fix: clear error when sql_variant used in TVP columns

Author: dhensby

lib/tedious/request.js

@@ -200,9 +200,13 @@ const parameterCorrection = function (value) {
     }
 
     for (const col of value.columns) {
+      const tediousType = getTediousType(col.type)
+      if (tediousType === tds.TYPES.Variant) {
+        throw new RequestError(`Column '${col.name}' in TVP '${value.schema ? value.schema + '.' : ''}${value.name}' uses sql_variant which is not supported by the tedious driver for TVP column types. Consider using a more specific data type.`, 'EARGS')
+      }
       tvp.columns.push({
         name: col.name,
-        type: getTediousType(col.type),
+        type: tediousType,
         length: col.length,
         scale: col.scale,
         precision: col.precision
@@ -664,9 +668,18 @@ class Request extends BaseRequest {
               } catch (e) {
                 e.message = `Validation failed for parameter '${name}'. ${e.message}`
                 const err = new RequestError(e, 'EPARAM')
+                delete this._cancel
+
+                if (!hasReturned) {
+                  for (const event in errorHandlers) {
+                    connection.removeListener(event, errorHandlers[event])
+                  }
 
-                this.parent.release(connection)
-                return callback(err)
+                  this.parent.release(connection)
+                  hasReturned = true
+                  return callback(err)
+                }
+                return
               }
             }
 
@@ -703,21 +716,23 @@ class Request extends BaseRequest {
 
             req.sqlTextOrProcedure = `declare ${declarations.join(', ')};select ${assigns.join(', ')};${req.sqlTextOrProcedure};${batchHasOutput ? (`select 1 as [___return___], ${selects.join(', ')}`) : ''}`
           }
-        } else {
-          for (const name in this.parameters) {
-            if (!objectHasProperty(this.parameters, name)) {
-              continue
-            }
-            const param = this.parameters[name]
-            if (param.io === 1) {
-              req.addParameter(param.name, getTediousType(param.type), parameterCorrection(param.value), { length: param.length, scale: param.scale, precision: param.precision })
-            } else {
-              req.addOutputParameter(param.name, getTediousType(param.type), parameterCorrection(param.value), { length: param.length, scale: param.scale, precision: param.precision })
-            }
-          }
         }
 
         try {
+          if (!this._isBatch) {
+            for (const name in this.parameters) {
+              if (!objectHasProperty(this.parameters, name)) {
+                continue
+              }
+              const param = this.parameters[name]
+              if (param.io === 1) {
+                req.addParameter(param.name, getTediousType(param.type), parameterCorrection(param.value), { length: param.length, scale: param.scale, precision: param.precision })
+              } else {
+                req.addOutputParameter(param.name, getTediousType(param.type), parameterCorrection(param.value), { length: param.length, scale: param.scale, precision: param.precision })
+              }
+            }
+          }
+
           connection[this._isBatch ? 'execSqlBatch' : 'execSql'](req)
         } catch (error) {
           handleError(true, connection, error)
@@ -983,19 +998,34 @@ class Request extends BaseRequest {
           output[parameterName] = value
         })
 
-        for (const name in this.parameters) {
-          if (!objectHasProperty(this.parameters, name)) {
-            continue
+        try {
+          for (const name in this.parameters) {
+            if (!objectHasProperty(this.parameters, name)) {
+              continue
+            }
+            const param = this.parameters[name]
+            if (param.io === 1) {
+              req.addParameter(param.name, getTediousType(param.type), parameterCorrection(param.value), { length: param.length, scale: param.scale, precision: param.precision })
+            } else {
+              req.addOutputParameter(param.name, getTediousType(param.type), parameterCorrection(param.value), { length: param.length, scale: param.scale, precision: param.precision })
+            }
           }
-          const param = this.parameters[name]
-          if (param.io === 1) {
-            req.addParameter(param.name, getTediousType(param.type), parameterCorrection(param.value), { length: param.length, scale: param.scale, precision: param.precision })
-          } else {
-            req.addOutputParameter(param.name, getTediousType(param.type), parameterCorrection(param.value), { length: param.length, scale: param.scale, precision: param.precision })
+
+          connection.callProcedure(req)
+        } catch (error) {
+          const err = error instanceof RequestError ? error : new RequestError(error, 'EREQUEST')
+          delete this._cancel
+
+          if (!hasReturned) {
+            for (const event in errorHandlers) {
+              connection.removeListener(event, errorHandlers[event])
+            }
+
+            this.parent.release(connection)
+            hasReturned = true
+            callback(err)
           }
         }
-
-        connection.callProcedure(req)
       })
     })
   }

test/common/unit.js

@@ -864,6 +864,32 @@ describe('connection string auth - tedious', () => {
     })
   })
 
+  describe('TVP sql_variant validation', () => {
+    it('throws a clear error when a TVP column uses sql_variant', (done) => {
+      const tvp = new sql.Table('dbo.GridFilter')
+      tvp.columns.add('FieldName', sql.NVarChar(128))
+      tvp.columns.add('Value1', sql.Variant)
+
+      const Request = require('../../lib/tedious/request')
+      const fakeConn = { on: () => {}, removeListener: () => {} }
+      const mockPool = {
+        config: {},
+        connected: true,
+        acquire: (req, cb) => cb(null, fakeConn, {}),
+        release: () => {}
+      }
+      const req = new Request(mockPool)
+      req.input('Filters', tvp)
+
+      req.query('SELECT 1', (err) => {
+        assert.ok(err, 'Expected an error')
+        assert.ok(err.message.includes('sql_variant'), `Error message should mention sql_variant, got: ${err.message}`)
+        assert.ok(err.message.includes('Value1'), `Error message should mention column name, got: ${err.message}`)
+        done()
+      })
+    })
+  })
+
   describe('_poolValidate', () => {
     // Reset Promise in case earlier tests replaced it (e.g. FakePromise)
     before(() => { sql.Promise = Promise })