dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r597203702
##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -52,7 +52,10 @@ def __init__(self, *args, **kwargs) -> None:
def set_autocommit(self, conn: Connection, autocommit: bool) -> None: #
noqa: D403
"""MySql connection sets autocommit in a different way."""
- conn.autocommit(autocommit)
+ if hasattr(conn, 'get_autocommit'): # mysqlclient
+ conn.autocommit(autocommit)
+ else: # mysql-connector-python
+ conn.autocommit = autocommit
Review comment:
This was a tricky one!
I totally agree we should avoid generating the side effect twice.
Here are two options I came up with that I think are a little more direct.
First option is you can check the class of `conn`.
I am pretty sure that mysql connector will always inherit from this base
class:
if isinstance(conn, MySQLConnectionAbstract):
conn.autocommit = autocommit
else:
conn.autocommit(autocommit)
Though alternatively you could check if it's MySQLdb:
```
from MySQLdb.connections import Connection as MySQLdbConnection
...
if isinstance(conn, MySQLdbConnection):
conn.autocommit(autocommit)
else:
conn.autocommit = autocommit
```
And the import alias of MySQLdbConnection can be re-used in resolving that
type annotation problem. I'm not sure what's the best practice re aliasing
classes in the event of a collision like we have here but this approach seems
reasonable to me.
Option 2 (and I think I like this one) is check if `autocommit` is a
property. You can do so without calling it like so:
```python
if isinstance(conn.__class__.autocommit, property):
conn.autocommit = autocommit
else:
conn.autocommit(autocommit)
```
I think _also_ checking whether that property has a setter is overkill and
certainly not worth the complexity, but in the course of looking into this I
learned you can do so with `hasattr(conn.__class__.autocommit, 'fset')`.
But I think I'd go with either of these two approaches because checking for
`get_autocommit` and not using it is a little confusing and certainly indirect.
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
