Repository: mindsdb/mindsdb
Pull request: Moving MySQL driver to use autocommit
This changes the MySQL integration to use autocommit. This fixes #7234. The rationale in that ticket is that python errors, if caught, could lead to dangling open transactions with unexpected results.  This concern is valid.  If we need transactions we should provide a context manager for them so that exiting the context manager automaticaly closes the transaction.

Due to the nature of this change, there are no extra integration tests required.  Nor is there any additional documentation needed aside from something in the module docstring which I have already added.

## Description
In practical terms, this prevents dangling transactions in the case of unexpected python errors and therefore makes the module more robust.

Fixes #7234 

## Type of change

(Please delete options that are not relevant)

- [X] 🐛 Bug fix (non-breaking change which fixes an issue)
- [ ] ⚡ New feature (non-breaking change which adds functionality)
- [ ] 📢 Breaking change (fix or feature that would cause existing functionality not to work as expected)
- [ ] 📄 This change requires a documentation update

## Verification Process

To ensure the changes are working as expected:

 - [ ]   Test Location: Specify the URL or path for testing.
 - [X]   Verification Steps: Outline the steps or queries needed to validate the change. Include any data, configurations, or actions required to reproduce or see the new functionality.
 
I added an integration test that makes sure that the data which is written persists.  This disconnects and then reconnects in the middle of the test.

The changes here are such that existing MySQL integration tests should test the functionality properly.  I modified one integration test to ensure that writes persist 

## Additional Media:

- [ ] I have attached a brief loom video or screenshots showcasing the new functionality or change.

## Checklist:

- [X] My code follows the style guidelines(PEP 8) of MindsDB.
- [X] I have appropriately commented on my code, especially in complex areas.
- [X] Necessary documentation updates are either made or tracked in issues.
- [X] Relevant unit and integration tests are updated or added.





Problem statement:
[mysql-integration] Enable autocommit
### Short description and motivation for the proposed feature

Currently, when a MySQL query succeeds a commit is made explicitly, and when a query fails a rollback is made. Autocommit is not disabled explicitly, but it's disabled by default by all Python MySQL modules.

Using autocommit is better in principle, and it will avoid that a transaction remains open if a Python error occurs.


### Video or screenshots

_No response_

### Describe some possible solutions

_No response_

### Anything else?

_No response_

Hints from issue discussion:
Looking at this, I am wondering if it would make sense to make sure we have a context manager available so that if someone wants to do multiple statements in a transaction a `with` clause can be used for that.  And then without that, autocommit would be enabled.
I'll leave to @ZoranPandovski to decide if supporting transactions makes sense.

I think that currently commands like START TRANSACTION are run in MindsDB. If we move forward with this, these commands should be sent to the integration instead.

Commit message: Moving MySQL driver to use autocommit

This changes the MySQL integration to use autocommit. This fixes #7234.
The rationale in that ticket is that python errors, if caught, could
lead to dangling open transactions with unexpected results.  This
concern is valid.  If we need transactions we should provide a context
manager for them so that exiting the context manager automaticaly closes
the transaction.

For integration tests I added a test that inserts data, then
disconnects, reconnects, and tries to read it.  This should test data
persistence.

Patch to review:
diff --git a/mindsdb/integrations/handlers/mysql_handler/mysql_handler.py b/mindsdb/integrations/handlers/mysql_handler/mysql_handler.py
index b732f99686f..daf94e7ab49 100644
--- a/mindsdb/integrations/handlers/mysql_handler/mysql_handler.py
+++ b/mindsdb/integrations/handlers/mysql_handler/mysql_handler.py
@@ -1,3 +1,14 @@
+"""
+This is the MySQL integration handler for mindsdb.  It provides the routines
+which provide for interacting with the database.
+
+MindsDB currently does not appear to multiple round trip transactions. This
+makes sense given the niche that the project fulfills.  If this changes, most
+handlers will require modification.  Here we would need a context manager for
+handling transactions.  This would be safer than explicit commits since errors
+would result in rolling back automatically.
+"""
+
 from collections import OrderedDict
 
 import pandas as pd
@@ -73,6 +84,7 @@ def connect(self):
                 config["ssl_key"] = ssl_key
 
         connection = mysql.connector.connect(**config)
+        connection.autocommit = True
         self.is_connected = True
         self.connection = connection
         return self.connection
@@ -131,7 +143,6 @@ def native_query(self, query: str) -> Response:
                     )
                 else:
                     response = Response(RESPONSE_TYPE.OK)
-                connection.commit()
             except Exception as e:
                 log.logger.error(f'Error running query: {query} on {self.connection_data["database"]}!')
                 response = Response(
diff --git a/tests/handler_tests/test_mysql_handler.py b/tests/handler_tests/test_mysql_handler.py
index ded0c05c478..4defc4ecf24 100644
--- a/tests/handler_tests/test_mysql_handler.py
+++ b/tests/handler_tests/test_mysql_handler.py
@@ -136,8 +136,20 @@ def test_create_table(self, handler):
         tables = self.get_table_names(handler)
         assert new_table in tables, f"expected to have {new_table} in database, but got: {tables}"
 
+    def test_insert_table(self, handler):
+        tablename = "test_mdb"
+        res = handler.native_query(f"INSERT INTO {new_table} (test_col) values (1), (2), (3))")
+        self.check_valid_response(res)
+        handler.disconnect()
+        handler.connect()
+        res = handler.query(f"SELECT count(*) FROM {new_table}")
+        self.check_valid_response(res)
+        got_rows = res.data_frame.shape[0]
+        assert got_rows == 3
+
+
     def test_drop_table(self, handler):
-        drop_table = "test_md"
+        drop_table = "test_mdb"
         res = handler.native_query(f"DROP TABLE IF EXISTS {drop_table}")
         self.check_valid_response(res)
         tables = self.get_table_names(handler)


Code review comments:
diff_hunk: @@ -136,8 +136,20 @@ def test_create_table(self, handler):
         tables = self.get_table_names(handler)
         assert new_table in tables, f"expected to have {new_table} in database, but got: {tables}"
 
+    def test_insert_table(self, handler):
+        tablename = "test_mdb"
+        res = handler.native_query(f"INSERT INTO {new_table} (test_col) values (1), (2), (3))")
+        self.check_valid_response(res)
+        handler.disconnect()
+        handler.connect()
+        res = handler.query(f"SELECT count(*) FROM {new_table}")
+        self.check_valid_response(res)
+        got_rows = res.data_frame.shape[0]
+        assert got_rows == 3
+
+
     def test_drop_table(self, handler):
-        drop_table = "test_md"
path: tests/handler_tests/test_mysql_handler.py
text: @author:
I am guessing that this is a typo?

@user1:
It looks so, anyway we will need to change this tests