TVP Schema Name Fix and Request Timeout Improvements#1763
TVP Schema Name Fix and Request Timeout Improvements#1763crs2007 wants to merge 1 commit intotediousjs:masterfrom
Conversation
Improves TVP type name handling to avoid schema duplication and assigns per-request timeout to tedious Request if set. Adds tests for long requestTimeout and TVP with schema-qualified names, and increases Mocha test timeouts for long-running tests.
dhensby
left a comment
dhensby
left a comment
There was a problem hiding this comment.
I've gone and rebased this as well as reworded the commits and fixed the code linting issues.
I'm a bit concerned about the tests here, and I'm not sure the TVP fix is fixed...
| 'Fix default requestTimeout is above 15s' (done) { | ||
| const req = new TestRequest() | ||
| req.requestTimeout = 25000 // 25 seconds | ||
| const start = Date.now() | ||
| req.query("WAITFOR DELAY '00:00:20.000';").then(result => { | ||
| const elapsed = Date.now() - start | ||
| assert.ok(!result.error, 'Should not error for long WAITFOR DELAY with high timeout') | ||
| assert(elapsed >= 20000, 'Query should take at least 20 seconds') | ||
| done() | ||
| }).catch(done) |
There was a problem hiding this comment.
I don't think it's good for us to add 20+ seconds to our test suite to test that we can set a high timeout on the request.
I'd rather we just set a low timeout (1 second, 2 seconds?!) and keep our test suite as fast as reasonably possible.
| (async () => { | ||
| let pool | ||
| try { | ||
| pool = await sql.connect(readConfig()) | ||
| const request = pool.request() | ||
| const tvp = new sql.Table('AI.UDT_StringArray') | ||
| tvp.columns.add('Name', sql.NVarChar(128), { nullable: false }) | ||
| tvp.rows.add('TestValue1') | ||
| tvp.rows.add('TestValue2') | ||
| request.input('InputList', tvp) | ||
| await request.execute('AI.USP_TestProcedure') | ||
| } catch (err) { | ||
| if (err && /Cannot find data type UDT_StringArray/.test(err.message)) { | ||
| return done() | ||
| } | ||
| if (err && /could not find|does not exist|invalid object/i.test(err.message)) { | ||
| return done() | ||
| } | ||
| done(err) | ||
| } finally { | ||
| if (pool) await sql.close() | ||
| } | ||
| })() |
There was a problem hiding this comment.
Perhaps a nitpick, but I don't like this mixed async/callback style. The tests are written in an outdated way, but this mixing feels (and looks) nasty.
Also, isn't there already a pool ready, so there's no need to be initiating a pool here? Could we be using assert.match()?
2 participants
Pull Request: TVP Schema Name Fix and Request Timeout Improvements
Overview
This PR addresses and resolves the following issues:
Key Changes
When passing Table-Valued Parameters (TVPs), the schema and type name are now correctly preserved and included in the parameter declaration. This ensures that sp_executesql receives the fully qualified type name (including schema), resolving issues with custom TVP types. See
lib/tedious/request.js.requestTimeoutandconnectionTimeoutacross both Tedious and msnodesqlv8 drivers.query_timeoutis set in seconds, derived fromconfig.requestTimeout.Impact