Repository: saltstack/salt
Pull request: Resolves "[BUG] regressions in mysql_cache module #61222"
### What does this PR do?

### What issues does this PR fix or reference?
Fixes: #61222 

### Merge requirements satisfied?
**[NOTICE] Bug fixes or features added to Salt require tests.**
<!-- Please review the [test documentation](https://docs.saltproject.io/en/master/topics/tutorials/writing_tests.html) for details on how to implement tests into Salt's test suite. -->
- [ ] Docs
- [ ] Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
- [x] Tests written/updated

### Commits signed with GPG?
Yes/No

Please review [Salt's Contributing Guide](https://docs.saltproject.io/en/master/topics/development/contributing.html) for best practices.

See GitHub's [page on GPG signing](https://help.github.com/articles/signing-commits-using-gpg/) for more information about signing commits with GPG.


Problem statement:
[BUG] regressions in mysql_cache module
Two recent regressions in the mysql_cache module have broken it fairly significantly.  One is a generic function argument ordering bug which breaks the module globally, while the other only manifests when using PyMySQL (instead of MySQLdb) as the db connector.

I can't seem to push PRs anymore, so here's a tiny patch instead :)
```
index 87ad78eade..7f01cc2473 100644
--- a/salt/cache/mysql_cache.py
+++ b/salt/cache/mysql_cache.py
@@ -90,7 +90,7 @@ def __virtual__():
     return bool(MySQLdb), "No python mysql client installed." if MySQLdb is None else ""


-def run_query(conn, query, retries=3, args=None):
+def run_query(conn, query, args=None, retries=3):
     """
     Get a cursor and run a query. Reconnect up to `retries` times if
     needed.
@@ -203,7 +203,6 @@ def store(bank, key, data):
     query = "REPLACE INTO {} (bank, etcd_key, data) values(%s,%s,%s)".format(
         __context__["mysql_table_name"]
     )
-    query = salt.utils.stringutils.to_bytes(query)
     args = (bank, key, data)

     cur, cnt = run_query(__context__.get("mysql_client"), query, args)
```

Hints from issue discussion:
@sagetherage - not sure if calling to_bytes() with surrogatepass is an acceptable choice, but it solved the immediate problem for me, and I can't see any major downside to it off the top of my head.
@waynew @terminalimage @sagetherage - please note that the current codebase (as of 3004.1, which is what AWS Linux has RPMs of in their repos) is ENTIRELY, 100% broken WRT mysql_cache and needs the above patch to work at all, so this bug is a total showstopper for anyone using the MySQL minion cache backend, so I'd recommend getting this fix in sooner rather than later...
@rallytime @sagetherage @waynew - I believe this should be classified as "high priority", given that the mysql cache module is 100% broken in 3004.1
There is no 3004.1. Latest version is 3004.0.
My bad - Amazon versioned their RPM as `3004-1` and I unthinkingly transcribed it as `3004.1` :)  I've verified the actual RPM is built from version`3004.0`
Corrected the patch in the first comment.

The previous patch assumed that the line`query = salt.utils.stringutils.to_bytes(query)` even BELONGED in the original version, but this assumption was wrong - NONE of the other defined methods (e.g. `fetch()`, `ls()`, `_init_client()`, etc) force a conversion to bytes, so I can't see why this one does so, and in fact removing it makes for a much cleaner "pass it through for the mysql DBI implementation to handle" approach.

Commit message: - format to make the linter happy

Patch to review:
diff --git a/salt/cache/mysql_cache.py b/salt/cache/mysql_cache.py
index 87ad78eade0b..f512cb5677e6 100644
--- a/salt/cache/mysql_cache.py
+++ b/salt/cache/mysql_cache.py
@@ -90,7 +90,7 @@ def __virtual__():
     return bool(MySQLdb), "No python mysql client installed." if MySQLdb is None else ""
 
 
-def run_query(conn, query, retries=3, args=None):
+def run_query(conn, query, args=None, retries=3):
     """
     Get a cursor and run a query. Reconnect up to `retries` times if
     needed.
@@ -118,7 +118,8 @@ def run_query(conn, query, retries=3, args=None):
         else:
             log.info("mysql_cache: recreating db connection due to: %r", e)
         __context__["mysql_client"] = MySQLdb.connect(**__context__["mysql_kwargs"])
-        return run_query(__context__["mysql_client"], query, args, retries - 1)
+        return run_query(conn=__context__.get("mysql_client"),
+                query=query, args=args, retries=(retries - 1))
     except Exception as e:  # pylint: disable=broad-except
         if len(query) > 150:
             query = query[:150] + "<...>"
@@ -136,7 +137,7 @@ def _create_table():
         __context__["mysql_kwargs"]["db"],
         __context__["mysql_table_name"],
     )
-    cur, _ = run_query(__context__.get("mysql_client"), query)
+    cur, _ = run_query(conn=__context__.get("mysql_client"), query=query)
     r = cur.fetchone()
     cur.close()
     if r[0] == 1:
@@ -151,7 +152,7 @@ def _create_table():
         __context__["mysql_table_name"]
     )
     log.info("mysql_cache: creating table %s", __context__["mysql_table_name"])
-    cur, _ = run_query(__context__["mysql_client"], query)
+    cur, _ = run_query(conn=__context__.get("mysql_client"), query=query)
     cur.close()
 
 
@@ -203,10 +204,9 @@ def store(bank, key, data):
     query = "REPLACE INTO {} (bank, etcd_key, data) values(%s,%s,%s)".format(
         __context__["mysql_table_name"]
     )
-    query = salt.utils.stringutils.to_bytes(query)
     args = (bank, key, data)
 
-    cur, cnt = run_query(__context__.get("mysql_client"), query, args)
+    cur, cnt = run_query(conn=__context__.get("mysql_client"), query=query, args=args)
     cur.close()
     if cnt not in (1, 2):
         raise SaltCacheError("Error storing {} {} returned {}".format(bank, key, cnt))
@@ -220,7 +220,7 @@ def fetch(bank, key):
     query = "SELECT data FROM {} WHERE bank='{}' AND etcd_key='{}'".format(
         __context__["mysql_table_name"], bank, key
     )
-    cur, _ = run_query(__context__.get("mysql_client"), query)
+    cur, _ = run_query(conn=__context__.get("mysql_client"), query=query)
     r = cur.fetchone()
     cur.close()
     if r is None:
@@ -239,7 +239,7 @@ def flush(bank, key=None):
     if key is not None:
         query += " AND etcd_key='{}'".format(key)
 
-    cur, _ = run_query(__context__.get("mysql_client"), query)
+    cur, _ = run_query(conn=__context__.get("mysql_client"), query=query)
     cur.close()
 
 
@@ -252,7 +252,7 @@ def ls(bank):
     query = "SELECT etcd_key FROM {} WHERE bank='{}'".format(
         __context__["mysql_table_name"], bank
     )
-    cur, _ = run_query(__context__.get("mysql_client"), query)
+    cur, _ = run_query(conn=__context__.get("mysql_client"), query=query)
     out = [row[0] for row in cur.fetchall()]
     cur.close()
     return out
@@ -266,7 +266,7 @@ def contains(bank, key):
     query = "SELECT COUNT(data) FROM {} WHERE bank='{}' AND etcd_key='{}'".format(
         __context__["mysql_table_name"], bank, key
     )
-    cur, _ = run_query(__context__.get("mysql_client"), query)
+    cur, _ = run_query(conn=__context__.get("mysql_client"), query=query)
     r = cur.fetchone()
     cur.close()
     return r[0] == 1
diff --git a/tests/pytests/unit/cache/test_mysql_cache.py b/tests/pytests/unit/cache/test_mysql_cache.py
index 29047f5cbb32..7944b22a280f 100644
--- a/tests/pytests/unit/cache/test_mysql_cache.py
+++ b/tests/pytests/unit/cache/test_mysql_cache.py
@@ -64,7 +64,7 @@ def test_store(master_config):
                 expected_calls = [
                     call(
                         mock_connect_client,
-                        b"REPLACE INTO salt (bank, etcd_key, data) values(%s,%s,%s)",
+                        "REPLACE INTO salt (bank, etcd_key, data) values(%s,%s,%s)",
                         ("minions/minion", "key1", b"\xa4data"),
                     )
                 ]
@@ -81,7 +81,7 @@ def test_store(master_config):
                 expected_calls = [
                     call(
                         mock_connect_client,
-                        b"REPLACE INTO salt (bank, etcd_key, data) values(%s,%s,%s)",
+                        "REPLACE INTO salt (bank, etcd_key, data) values(%s,%s,%s)",
                         ("minions/minion", "key2", b"\xa4data"),
                     )
                 ]


Code review comments:
diff_hunk: @@ -118,7 +118,8 @@ def run_query(conn, query, retries=3, args=None):
         else:
             log.info("mysql_cache: recreating db connection due to: %r", e)
         __context__["mysql_client"] = MySQLdb.connect(**__context__["mysql_kwargs"])
-        return run_query(__context__["mysql_client"], query, args, retries - 1)
+        return run_query(conn=__context__.get("mysql_client"),
+                query=query, args=args, retries=(retries - 1))
path: salt/cache/mysql_cache.py
text: @user1:
Just nitpicking at this point, but since the `conn` and `query` args are positional, we might want to pass them in as such.  Here and in the rest of the instances of `run_query`.

```suggestion
        return run_query(__context__.get("mysql_client"),
                query, args=args, retries=(retries - 1))
```

@author:
Makes sense.