fix: include instance name in Login7 message and traceID in prelogin
#1668
Conversation
TPVallancourt
changed the title
|
Hi @arthurschreiber , are there any other things I need to do get this incorporated into the library? |
arthurschreiber
changed the title
Login7 message
|
@TPVallancourt This seems to break the appveyor build on Windows 🤔 |
src/connection.ts
Outdated
|
|
||
| payload.hostname = this.config.options.workstationId || os.hostname(); | ||
| payload.serverName = this.routingData ? this.routingData.server : this.config.server; | ||
| payload.serverName = this.routingData ? `${this.routingData.server}\\${this.routingData.instance}` : this.config.server; |
There was a problem hiding this comment.
Is instance here guaranteed to always be a non-empty string? What happens if it's empty?
TPVallancourt
changed the title
Login7 messageLogin7 message and traceID in prelogin
|
@TPVallancourt Hi, just curious if this is planned to be merged and released soon? I am currently relying on this PR for Fabric Warehouse connections |
|
Hi @arthurschreiber ! Finally found some time to think about this PR some more. Microsoft appears to have pushed out an update this week which has made Fabric a lot faster and more reliable, so it seemed like a good time to revisit this. The big change since you last reviewed is that I added an Additionally, I think it would be good to add integration testing to a Fabric warehouse - not sure what the protocol is for setting something like that up for this repo. Any ideas? Thank you! |
| connectionId: 0, | ||
| clientTimeZone: new Date().getTimezoneOffset(), | ||
| clientLcid: 0x00000409 | ||
| clientLcid: 0x00000409, |
There was a problem hiding this comment.
That is one hell of an opaque magic number…
There was a problem hiding this comment.
According to the MS-TDS spec, the ClientLCID value is no longer used to set language parameters and is ignored.
|
Hey to everyone, Really looking forward to use the official tedious implementation to connect to Microsoft Fabric. Thanks! (: |
| fixedData.writeUInt32LE(0, offset); | ||
| } | ||
|
|
||
| if (this.isFabric) { |
There was a problem hiding this comment.
From just a code read, I'm trying to understand how tedious works with feature extensions before this change. It seems like the previous code is writing the feature extension data to the completely wrong place (the middle instead of the end) compared to what the spec defines. My guess is that SQL Server and Azure SQL DB endpoints are just a lot more forgiving in the name of backwards compatibility with existing clients.
Is there even any need for the isFabric logic? I would think the changes would work on both Fabric and non-Fabric endpoints.
Fix for #1563
isFabricoption toConnectionConfigurationwhich controls which Login7 packet is created. It appears that the packet which is required for Fabric does not work for SQLServerisFabric, write FeatureExtensions to the end of the Login7 Packet matching the behavior of the go-mssqldb library. The header must still be written in the same place as it was previously. The value in the header is adjusted to reflect that FeatureExtensions now appear at the end of the packetinstance. Previously we just savedserverwhich removes the instance name from the redirect URL. This is the proper string for TLS negotiation, but Login7 needs theserverand theinstancetogether.preloginpayload, include a randomly generated traceID, Starting with TDS37.0 it seems that traceID is no longer optional for Fabric.