SQL Server adapter: update parameters for latest version of sqlcmd#133
SQL Server adapter: update parameters for latest version of sqlcmd#133johnybx wants to merge 1 commit intotpope:masterfrom
Conversation
|
There's a lot going on here. Let's do the password change and the new query options separately. |
|
Sure, I created new pull request for password change. Once that is merged I will rebase this one. |
| " Legacy parameters left just for backward compatibility with older sqlcmd: secure and | ||
| " trustServerCertificate |
There was a problem hiding this comment.
They're not legacy, keep them as is, and fix your additions to match. See https://learn.microsoft.com/en-us/sql/connect/jdbc/setting-the-connection-properties?view=sql-server-ver16#properties.
There was a problem hiding this comment.
ah now I understand the naming ( I thought that parameters were derived from sqlcmd --help ) - but at least for secure parameter which is supposed to be encrypt there was change
In version 11.2.0 and up, encrypt has been changed from boolean to string, allowing for TDS 8.0 support when the property is set to strict.
Do you want to keep secure a boolean parameter and encrypt as string parameter or there should be just one parameter and type of value will be detected ?
There was a problem hiding this comment.
Since -N doesn't accept a string, and thus there's no way for us to do anything withencrypt=strict, I think we should just leave it as is for now?
There was a problem hiding this comment.
That is actually one of the reasons why I started this pull request - I am using latest sqlcmd:
❯ sqlcmd --version
sqlcmd: 1.0.0and I cannot use -N as boolean parameter because it was changed
❯ sqlcmd -N
sqlcmd: error: --encrypt-connection: expected string value but got "EOL" (<EOL>)
❯ sqlcmd -N X
sqlcmd: error: --encrypt-connection must be one of "default","false","true","disable" but got "X"
❯ sqlcmd -N disable
unable to open tcp connection with host 'localhost:1433': dial tcp [::1]:1433: connect: connection refusedThe idea was that I would add parameter while keeping old one so that the adapter is not broken for people with older version of sqlcmd.
There was a problem hiding this comment.
I rebased the PR and one more thing to note is that encrypt and trustServerCertificate are properties of connection according to specification but format, column-separator and trim-spaces are options of sqlcmd client.
There was a problem hiding this comment.
Oh, yeah, those have no business being in a URL. URLs specify how to connect to the database, not arbitrary command-line flags. There have been multiple requests of this nature lately (#129, #125) so I think we should add some sort of general interface for that.
As for -N, I'm pushing a fix to support -N on ?encrypt=1 and a string argument for anything else.
There was a problem hiding this comment.
sounds good, thanks.
2 participants
Based on this issue #127 and latest docs for
sqlcmdthe password is now supplied inSQLCMDPASSWORDenvironment variable. I also updated the adapter to properly supportencrypt-connection,format,column-separator,trim-spacesandtrust-server-certificateoptions. For backward compatibility I left there old flagssecureandtrustServerCertificate.