[GitHub] [airflow] dstandish commented on a change in pull request #14869: Fixed autocommit calls for mysql-connector-python

[GitBox <gi...@apache.org>]

dstandish commented on a change in pull request #14869:
URL: https://github.com/apache/airflow/pull/14869#discussion_r596598632



##########
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:
       it's a little awkward to check for attr `get_autocommit` and then not use that attr ...  but rather to only use it as a mechanism to infer that `autocommit` is callable
   
   better would be to just check if autocommit is callable directly:
   
   ```suggestion
           if callable(conn.autocommit):
               conn.autocommit(autocommit)
           else:
               conn.autocommit = autocommit
   ```
   

##########
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

Review comment:
       in the code here, presently this `Connection` refers to the airflow model connection but this is not correct
   
   it should be this one: `from MySQLdb.connections import Connection`
   
   now might be a good opportunity to fix this, and also make it a `Union` with the mysql-connector connection type also
   

##########
File path: airflow/providers/mysql/hooks/mysql.py
##########
@@ -63,7 +66,10 @@ def get_autocommit(self, conn: Connection) -> bool:  # noqa: D403
         :return: connection autocommit setting
         :rtype: bool
         """
-        return conn.get_autocommit()
+        if hasattr(conn, 'get_autocommit'):  # mysqlclient

Review comment:
       the comment says `mysqlclient ` but isn't the library actually called `MySQLdb`?  not sure you really need the comments but either way is fine

##########
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
 
     def get_autocommit(self, conn: Connection) -> bool:  # noqa: D403

Review comment:
       here too i'd suggest fixing this type annotation while you're at it




----------------------------------------------------------------
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:
users@infra.apache.org