Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update tedious version #1312

Merged
merged 7 commits into from
Nov 11, 2021
Merged

Update tedious version #1312

merged 7 commits into from
Nov 11, 2021

Conversation

dhensby
Copy link
Collaborator

@dhensby dhensby commented Sep 20, 2021

What this does:

Update tedious for a v8 release

@dhensby dhensby added this to the v8.0.0 milestone Sep 20, 2021
@arthurschreiber
Copy link
Contributor

RequestError: Validation failed for parameter 'vch'. No collation was set by the server for the current connection.

That's an interesting error. It should never happen - when connecting to an SQL Server instance you always connect to some database, and that database always has some encoding that should be send to the client, so I'm wondering how the tests run into this. 🤔

@arthurschreiber
Copy link
Contributor

Ok, I figured out that this happens because node-mssql calls .validate on the tedious type directly. I'm not sure this is a good idea. Methods defined on data types should be internally used inside tedious only, and I will deprecate access to them soon-ish. Long term, I want to completely get rid of datatype objects and have the user just specify strings (like varchar, nvarchar, int and so on).

@dhensby
Copy link
Collaborator Author

dhensby commented Sep 22, 2021

Hey @arthurschreiber - thanks for taking the initiative to look into this.

The tight coupling/use of the validate method has always caused problems and I'm not entirely sure why it's done but it's how it is for now... Ideally this lib would abstract the typings away and if you're going to do that too, then I'll have no choice! 😅

@arthurschreiber
Copy link
Contributor

The tight coupling/use of the validate method has always caused problems and I'm not entirely sure why it's done but it's how it is for now... Ideally this lib would abstract the typings away and if you're going to do that too, then I'll have no choice! 😅

It's going to break for sure, because the types returned by validate have changed in some of the recent tedious releases, and now in many cases return Buffer objects instead of whatever they returned before.

Anyway, node-mssql uses it's own logic to cast parameter values to embed them into the query string.

assigns.push(`@${name} = ${cast(param.value, param.type, param)}`)

node-mssql/lib/datatypes.js

Lines 153 to 192 in 319993f

module.exports.cast = (value, type, options) => {
if (value == null) {
return null
}
switch (typeof value) {
case 'string':
return `N'${value.replace(/'/g, '\'\'')}'`
case 'number':
return value
case 'boolean':
return value ? 1 : 0
case 'object':
if (value instanceof Date) {
let ns = value.getUTCMilliseconds() / 1000
if (value.nanosecondDelta != null) {
ns += value.nanosecondDelta
}
const scale = options.scale == null ? 7 : options.scale
if (scale > 0) {
ns = String(ns).substr(1, scale + 1)
} else {
ns = ''
}
return `N'${value.getUTCFullYear()}-${zero(value.getUTCMonth() + 1)}-${zero(value.getUTCDate())} ${zero(value.getUTCHours())}:${zero(value.getUTCMinutes())}:${zero(value.getUTCSeconds())}${ns}'`
} else if (Buffer.isBuffer(value)) {
return `0x${value.toString('hex')}`
}
return null
default:
return null
}
}

Maybe you should also have your own validation logic then? 🤔


On another note, I'm really wondering what this embedding of values into batch statements is all about? Batch statements in TDS don't support parameters, and it looks like this is trying to emulate them by embedding variable declaration into the SQL statements directly? 😮

@Zomono
Copy link

Zomono commented Nov 1, 2021

Is this PR still in progress or is it rejected for now because of the mentioned problems?

@dhensby
Copy link
Collaborator Author

dhensby commented Nov 1, 2021

This PR is still in progress, the upgrade needs to happen, but I need to find the time to rework the validation area appropriately

@dhensby dhensby force-pushed the pulls/bump-tds branch 2 times, most recently from 47bef91 to 8719ae7 Compare November 3, 2021 10:28
@dhensby dhensby changed the title Update tedious to v13 Update tedious version Nov 3, 2021
This was referenced Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants