Login fails when odbc_connect PWD= includes a plus sign

[gordthompson]

Describe the bug

When odbc_connect is given a properly-formed ODBC connection string with a password that contains a plus sign, the plus sign is converted to a space and the login fails.

Optional link from https://docs.sqlalchemy.org which documents the behavior that is expected

No response

SQLAlchemy Version in Use

2.1.0b1.dev0

DBAPI (i.e. the database driver)

pyodbc

Database Vendor and Major Version

MS SQL Server

Python Version

3.8

Operating system

all

To Reproduce

import pyodbc
import sqlalchemy as sa

connection_string = (
    "Driver=ODBC Driver 17 for SQL Server;"
    "Server=192.168.0.199;"
    "Database=test;"
    "UID=howie;"
    "PWD=ab+cd;"
)

# ___ this works ___

cnxn = pyodbc.connect(connection_string)
crsr = cnxn.cursor()
result = crsr.execute("SELECT 'Connected.' AS foo").fetchval()
print(result)

# ___ but this fails ___

connection_url = sa.URL.create(
    "mssql+pyodbc", query=dict(odbc_connect=connection_string)
)
engine = sa.create_engine(connection_url)
with engine.begin() as conn:
    result = conn.exec_driver_sql("SELECT 'Connected.' AS foo").scalar()
    print(result)

Error

Traceback (most recent call last):
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/engine/base.py", line 150, in __init__
    self._dbapi_connection = engine.raw_connection()
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/engine/base.py", line 3298, in raw_connection
    return self.pool.connect()
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/pool/base.py", line 449, in connect
    return _ConnectionFairy._checkout(self)
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/pool/base.py", line 1263, in _checkout
    fairy = _ConnectionRecord.checkout(pool)
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/pool/base.py", line 712, in checkout
    rec = pool._do_get()
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/pool/impl.py", line 180, in _do_get
    self._dec_overflow()
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/util/langhelpers.py", line 144, in __exit__
    raise exc_value.with_traceback(exc_tb)
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/pool/impl.py", line 177, in _do_get
    return self._create_connection()
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/pool/base.py", line 390, in _create_connection
    return _ConnectionRecord(self)
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/pool/base.py", line 674, in __init__
    self.__connect()
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/pool/base.py", line 901, in __connect
    pool.logger.debug("Error on connect(): %s", e)
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/util/langhelpers.py", line 144, in __exit__
    raise exc_value.with_traceback(exc_tb)
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/pool/base.py", line 896, in __connect
    self.dbapi_connection = connection = pool._invoke_creator(self)
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/engine/create.py", line 635, in connect
    return dialect.connect(*cargs, **cparams)
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/engine/default.py", line 629, in connect
    return self.loaded_dbapi.connect(*cargs, **cparams)
pyodbc.InterfaceError: ('28000', "[28000] [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Login failed for user 'howie'. (18456) (SQLDriverConnect)")

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/gord/git/sqla-gerrit/gord_test/so78294946.py", line 26, in <module>
    with engine.begin() as conn:
  File "/usr/lib/python3.8/contextlib.py", line 113, in __enter__
    return next(self.gen)
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/engine/base.py", line 3238, in begin
    with self.connect() as conn:
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/engine/base.py", line 3274, in connect
    return self._connection_cls(self)
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/engine/base.py", line 152, in __init__
    Connection._handle_dbapi_exception_noconnection(
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/engine/base.py", line 2438, in _handle_dbapi_exception_noconnection
    raise sqlalchemy_exception.with_traceback(exc_info[2]) from e
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/engine/base.py", line 150, in __init__
    self._dbapi_connection = engine.raw_connection()
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/engine/base.py", line 3298, in raw_connection
    return self.pool.connect()
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/pool/base.py", line 449, in connect
    return _ConnectionFairy._checkout(self)
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/pool/base.py", line 1263, in _checkout
    fairy = _ConnectionRecord.checkout(pool)
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/pool/base.py", line 712, in checkout
    rec = pool._do_get()
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/pool/impl.py", line 180, in _do_get
    self._dec_overflow()
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/util/langhelpers.py", line 144, in __exit__
    raise exc_value.with_traceback(exc_tb)
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/pool/impl.py", line 177, in _do_get
    return self._create_connection()
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/pool/base.py", line 390, in _create_connection
    return _ConnectionRecord(self)
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/pool/base.py", line 674, in __init__
    self.__connect()
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/pool/base.py", line 901, in __connect
    pool.logger.debug("Error on connect(): %s", e)
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/util/langhelpers.py", line 144, in __exit__
    raise exc_value.with_traceback(exc_tb)
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/pool/base.py", line 896, in __connect
    self.dbapi_connection = connection = pool._invoke_creator(self)
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/engine/create.py", line 635, in connect
    return dialect.connect(*cargs, **cparams)
  File "/home/gord/git/sqla-gerrit/lib/sqlalchemy/engine/default.py", line 629, in connect
    return self.loaded_dbapi.connect(*cargs, **cparams)
sqlalchemy.exc.InterfaceError: (pyodbc.InterfaceError) ('28000', "[28000] [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Login failed for user 'howie'. (18456) (SQLDriverConnect)")
(Background on this error at: https://sqlalche.me/e/21/rvf5)

Additional context

No response

[zzzeek]

pyodbc connector would appear to have been written as:

        if 'odbc_connect' in keys:
            connectors = [util.unquote_plus(keys.pop('odbc_connect'))]

for decades. so we'd have to do something with that. likely would be very backwards incompatible for those who are working around this right now (Which you can do using an escape code for the + sign)

[CaselIT]

Could we just document that the odbc_connect requeires escaping, like user and passwords?

[zzzeek]

we could do that!

[zzzeek]

but it looks like the current implementation is really wrong, is the thing. that unquote is likely left over from when the unquoting wasnt happening elsewhere.

[gordthompson]

but it looks like the current implementation is really wrong, is the thing. that unquote is likely left over from when the unquoting wasnt happening elsewhere.

That is true. The string is now already unquoted by that point, so the unquote_plus() is superfluous. I will submit a Gerrit patch shortly.

[zzzeek]

right but....if someone is sending + sign like you are because they want a space character....

thiis would need LOTS of test cases

[sqla-tester]

Gord Thompson has proposed a fix for this issue in the main branch:

Avoid removing + from odbc_connect parameter values https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5248

[gordthompson]

if someone is sending + sign like you are because they want a space character....

Then the string would have to be completely manually quoted, and the + will have already been unquoted to a space before it hit that line in connectors/pyodbc.py

sqlalchemy/lib/sqlalchemy/connectors/pyodbc.py

Line 78 in ac7d70d

connectors = [unquote_plus(keys.pop("odbc_connect"))]

[zzzeek]

We need to have test cases both for string URLs that include ?connect_args=... as well as the form that you are using. We must exhaustively test that not a single URL that worked before would have any change in behavior whatsoever.

We've yet to have any changes to URL of any kind that have not produced immediate blowback and arguments.