search

OSGeo / gdal

GDAL is an open source MIT licensed translator library for raster and vector geospatial data formats.
https://gdal.org
Other
4.86k stars 2.53k forks source link

MSSQL Connection Strings no longer work with User Id and Password #7179

Closed geographika closed 1 year ago

geographika commented 1 year ago

Expected behavior and actual behavior.

Updating the MapServer MSSQL test suite to use GDAL version 3.6 caused tests to fail, due to data not loading into MS SQL Server.

The following connection string format was used (see https://github.com/MapServer/MapServer/blob/main/msautotest/mssql/create_mssql_db.bat):

ogr2ogr -s_srs epsg:26915 -t_srs epsg:26915 -f MSSQLSpatial "MSSQL:server=%SERVER%;database=msautotest;User Id=sa;Password=%SQLPASSWORD%;" "query/data/bdry_counpy2.shp" -nln "bdry_counpy2"

The following error was returned:

ERROR 1: Unable to initialize connection to the server for MSSQL:server=(local)\SQL2019;database=msautotest;User Id=sa;Password=Password12!;,

It appears as though the changes in #6797 may have resulted in only UID and PWD being valid connection string parameters. User Id and Password are valid options when using the SQL Server Native client (see https://www.connectionstrings.com/sql-server/).

I think it should still be possible to use these parameter names to use the MSSQL driver.

Changing to UID and PWD meant the ogr2ogr import was successful.

Steps to reproduce the problem.

ogr2ogr -s_srs epsg:26915 -t_srs epsg:26915 -f MSSQLSpatial "MSSQL:server=%SERVER%;database=msautotest;User Id=sa;Password=%SQLPASSWORD%;" "query/data/bdry_counpy2.shp" -nln "bdry_counpy2"

Or any OGR command using the MSSQL driver and User Id or Password e.g.

ogrinfo -al "MSSQL:server=(local)\SQL2019;database=msautotest;User Id=sa;PWD=Password12!;"

Operating system

Windows 10 / Appveyor

GDAL version and provenance

3.6 from GISInternals SDK.

rouault commented 1 year ago

@ninsbl Can you have a look at that please ?

ninsbl commented 1 year ago

@ninsbl Can you have a look at that please ?

Yes, sorry. I was not aware of that. Those options were not covered by unit tests, so the issue did not show up earlier. I am currently travelling, but will look at it tomorrow evening...

ninsbl commented 1 year ago

Hi again, I had a look at the issue and I think it could be fixed in different ways, yet ODBC driver managers are unfortunately somewhat moving targets, if I understand correctly. My understanding is that users can name them however they like in the odbcinst.ini (on Linux at least).

Ideally, the env-variable solution should work for any ODBC driver, however, if UID/PWD or User ID/password is valid in the connections string, depends on the driver, and the driver name is not necessarily predictable, as mentioned... We may, though assume default driver names for Microsoft ODBC drivers (like: ODBC Driver 17 for SQL Server). Yet, I am not sure if that is platform independent either...

So, the other solution would be to just allow user id and password in the connection string to work as it did before, and to specify in the documentation that the environment variables only work for FreeTDS ODBC driver...

Any thoughts?

rouault commented 1 year ago

Reviewing https://github.com/OSGeo/gdal/pull/6797 I have a hard time understanding why it would change behavior regarding specifying "User Id" and "password". It seems to only add specific behavior when UID or PWD are specified in the connection string or if MSSQLSPATIAL_UID / MSSQLSPATIAL_PWD configuration options are set. Am I missing something @ninsbl ?

@geographika Is it really a regression due to #6797 ? Did you try to revert just this change ? Maybe your issue is just caused by the CMake build using or not using the native CLI library ?

geographika commented 1 year ago

Looking at the Appveyor builds for MapServer they started failing when the GISInternals package moved from GDAL 3.5 to 3.6. Looking at #6797 I also can't see why it would affect the connection string. Maybe the default driver switched from {SQL Server} to {ODBC Driver 17 for SQL Server} as part of 3.5 to 3.6 (although I can't see why this would happen). Or something else changed in the Appveyor images. It's an easy fix to change to UID and PWD to get them working again.

@ninsbl - unless you can see something when debugging it might be another issue. There is however a minor bug in the update - if a UID or PWD is passed in the connection string it gets added again to the connection string passed to the database. As it is an exact duplicate it makes no difference, but produces a slightly confusing debug log (with --debug ON):

https://github.com/ninsbl/gdal/blob/510d4e38cec2e9299ef33a072319de069446b39d/ogr/ogrsf_frmts/mssqlspatial/ogrmssqlspatialdatasource.cpp#L719 - pszUID gets set here

https://github.com/ninsbl/gdal/blob/510d4e38cec2e9299ef33a072319de069446b39d/ogr/ogrsf_frmts/mssqlspatial/ogrmssqlspatialdatasource.cpp#L866-L871 - pszUID is not null so gets added again.

MSSQLSpatial: Use COPY/BCP: 1
ODBC: SQLDriverConnect(DRIVER={ODBC Driver 17 for SQL Server};server=(local)\SQL2019;database=msautotest;UID=sa;PWD=Password12!;;UID=sa;PWD=Password12!)
ODBC: SQLDisconnect()
szekerest commented 1 year ago

@geographika Regarding to GISInternals, it looks like the gdal cmake build doesn't compile the MSSQL into GDAL as soon as the plugin build was required. Formerly both the default built in driver, and the plugin has been compiled side by side. Installing the plugin overrode the functionality of the built in driver. With the cmake build ecosystem, we can compile against only one ODBC driver which is "ODBC Driver 17 for SQL Server" at the moment. For this reason, either the old ODBC driver and the native client driver is not supported anymore.

geographika commented 1 year ago

Thanks @szekerest for the details. Does this mean that setting a driver={...} no longer has an effect on the connection?

This would correspond with my tests where in GDAL 3.5.0dev I could switch between driver={SQL Server} and use User Id and password and driver={ODBC Driver 17 for SQL Server} and use UID and PWD.

Now only UID PWD work as {ODBC Driver 17 for SQL Server} is "hard-coded" with the build.

szekerest commented 1 year ago

currently yes, but I hope that the cmake build for ogr mssql driver could also be changed to restore the old behavior.

geographika commented 1 year ago

Ah I see the warning at https://github.com/ninsbl/gdal/blob/510d4e38cec2e9299ef33a072319de069446b39d/ogr/ogrsf_frmts/mssqlspatial/CMakeLists.txt#L17

currently yes, but I hope that the cmake build for ogr mssql driver could also be changed to restore the old behavior.

It might not be worth the effort as Microsoft seem to be dropping the native client (although bringing back the OLE DB Driver..):

The SQL Server Native Client (often abbreviated SNAC) has been removed from SQL Server 2022 (16.x) and SQL Server Management Studio 19 (SSMS). The SQL Server Native Client (SQLNCLI or SQLNCLI11) and the legacy Microsoft OLE DB Provider for SQL Server (SQLOLEDB) are not recommended for new development. Switch to the new Microsoft OLE DB Driver (MSOLEDBSQL) for SQL Server or the latest Microsoft ODBC Driver for SQL Server going forward.

https://learn.microsoft.com/en-us/sql/relational-databases/native-client/applications/installing-sql-server-native-client?view=sql-server-ver16

Closing the issue based on above information.