aws / amazon-redshift-python-driver

Redshift Python Connector. It supports Python Database API Specification v2.0.
Apache License 2.0
204 stars 76 forks source link

Will not process sql to grant/revoke built in roles such as sys:dba #152

Closed patatwork365 closed 1 year ago

patatwork365 commented 1 year ago

Driver version

redshift-connector 2.0.910

Redshift version

(['PostgreSQL 8.0.2 on i686-pc-linux-gnu, compiled by GCC gcc (GCC) 3.4.2 20041017 (Red Hat 3.4.2-6.fc3), Redshift 1.0.49087\x00\x00'],)

Client Operating System

Description: Amazon Linux release 2 (Karoo)

Python version

Python 3.7.16

Problem description

Will not process sql to grant/revoke built in roles such as sys:dba

   1187         Result of executing an sql statement:tuple[Any, ...]
   1188         """
-> 1189         self._run_cursor.execute(sql, params, stream=stream)
   1190         return tuple(self._run_cursor._cached_rows)
   1191 

~/venv/lib64/python3.7/site-packages/redshift_connector/cursor.py in execute(self, operation, args, stream, merge_socket_read)
    238                 self._c.execute(self, "begin transaction", None)
    239             self._c.merge_socket_read = merge_socket_read
--> 240             self._c.execute(self, operation, args)
    241         except AttributeError as e:
    242             raise e

~/venv/lib64/python3.7/site-packages/redshift_connector/core.py in execute(self, cursor, operation, vals)
   1622             statement, make_args = cache["statement"][operation] = convert_paramstyle(cursor.paramstyle, operation)
   1623 
-> 1624         args = make_args(vals)
   1625         # change the args to the format that the DB will identify
   1626         # take reference from self.py_types

~/venv/lib64/python3.7/site-packages/redshift_connector/core.py in make_args(vals)
    269 
...
--> 271             return tuple(vals[p] for p in placeholders)
    272 
    273     return "".join(output_query), make_args

KeyError: 'dba'

Reproduction code


conn = redshift_connector.connect(host=host, database=database, user=user, password=password)
sql = 'grant role sys:dba to theallpowerful;'
conn.run(sql)
Brooke-white commented 1 year ago

Hi @patatwork365 ,

I was able to reproduce this issue locally. When stepping through redshift-connector's code with the debugger I was able to identify the issue and a workaround for it: tl;dr add quotes around the role name.

conn = redshift_connector.connect(host=host, database=database, user=user, password=password)
sql = 'grant role "sys:dba" to theallpowerful;'
conn.run(sql)

You've identified an underlying bug in redshift_connector. By default, on a module level redshift_connector is configured to use format paramstyle. When looking with the debugger, this error occurred because the Cursor object on the connection has a default paramstyle of named (which looks for colons), hence the weird error you saw as redshift_connector thought it was a bind parameter.

I think there are a few things going on here that should be looked into:

  1. If the user doesn't pass bind parameters to .execute(), why are we messing with bind parameters? Unless I'm missing something this just adds overhead.
  2. The Cursor object on the Connection should have a default paramstyle that's inline with the module level default. This is confusing the module default is format yet the default for the Cursor on the Connection is named.

2 would be a break in existing behavior for users, so I'm more inclined towards looking into 1 as a possible work around.

patatwork365 commented 1 year ago

Thanks @Brooke-white , I'd already found the quoted workaround and works fine in this instance.

We generate a lot statements from metadata so always nice if they work cleanly and without workarounds which can cause other downstream such as quoted identifiers case mismatch.

Brooke-white commented 1 year ago

completely agreed. we will put a fix for this issue in our next release.

Brooke-white commented 1 year ago

release 2.0.911 includes a fix for this, thank you for your patience