From cf0c23b2f80ab7d28aa84e1ede6359406010ca1a Mon Sep 17 00:00:00 2001
From: Riccardo Coccioli <rcoccioli@wikimedia.org>
Date: Thu, 14 Nov 2024 12:24:40 +0100
Subject: [PATCH] mysql_legacy: fix pymysql queries

* Fix `set_master_use_gtid()` query, its value it's part of the syntax,
  avoid pymysql quoting it.
* Fix query formatting in `set_replication_parameters()`.
* Fix check in `replication_lag()` that would raise if the lag is 0.0s.

Change-Id: I2a02f98dbbbce8c65edbfe9383a263d028fa9193
---
 spicerack/mysql_legacy.py                 | 17 +++++++--------
 spicerack/tests/unit/test_mysql_legacy.py | 25 ++++++++++++++---------
 2 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/spicerack/mysql_legacy.py b/spicerack/mysql_legacy.py
index 3a23d27..4102a68 100644
--- a/spicerack/mysql_legacy.py
+++ b/spicerack/mysql_legacy.py
@@ -479,7 +479,8 @@ def set_master_use_gtid(self, setting: MasterUseGTID) -> None:
         if not isinstance(setting, MasterUseGTID):
             raise MysqlLegacyError(f"Only instances of MasterUseGTID are accepted, got: {type(setting)}")
 
-        self.execute("CHANGE MASTER TO MASTER_USE_GTID=%s", [setting.value])
+        # Not using placeholder replacements ad MariaDB requires it to be a syntax word and raises if it's quoted
+        self.execute(f"CHANGE MASTER TO MASTER_USE_GTID={setting.value}")
 
     def stop_mysql(self) -> Iterator[tuple[NodeSet, MsgTreeElem]]:
         """Stops mariadb service.
@@ -614,13 +615,13 @@ def set_replication_parameters(self, *, replication_info: ReplicationInfo, user:
         """Sets the replication parameters for the MySQL instance."""
         self.execute(
             (
-                "CHANGE MASTER TO master_host=%s(primary), "
-                "master_port=%s(port), "
+                "CHANGE MASTER TO master_host=%(primary)s, "
+                "master_port=%(port)s, "
                 "master_ssl=1, "
-                "master_log_file=%s(binlog), "
-                "master_log_pos=%s(position), "
-                "master_user=%s(user), "
-                "master_password=%s(password)"
+                "master_log_file=%(binlog)s, "
+                "master_log_pos=%(position)s, "
+                "master_user=%(user)s, "
+                "master_password=%(password)s"
             ),
             {
                 "primary": replication_info.primary,
@@ -689,7 +690,7 @@ def replication_lag(self) -> Decimal:
         if row is None:
             raise MysqlLegacyError("The replication lag query returned no data")
 
-        if not row["lag"]:
+        if "lag" not in row or row["lag"] is None:
             raise MysqlLegacyError(f"Unable to get lag information from: {row}")
 
         return row["lag"]
diff --git a/spicerack/tests/unit/test_mysql_legacy.py b/spicerack/tests/unit/test_mysql_legacy.py
index bddec89..e4a8652 100644
--- a/spicerack/tests/unit/test_mysql_legacy.py
+++ b/spicerack/tests/unit/test_mysql_legacy.py
@@ -319,8 +319,7 @@ def test_set_master_use_gtid_ok(self, setting, expected):
         """It should execute MASTER_USE_GTID with the given value."""
         self.mocked_cursor.execute.return_value = 0
         self.single_instance.set_master_use_gtid(setting)
-        query = "CHANGE MASTER TO MASTER_USE_GTID=%s"
-        self.mocked_cursor.execute.assert_any_call(query, [expected])
+        self.mocked_cursor.execute.assert_any_call(f"CHANGE MASTER TO MASTER_USE_GTID={expected}", None)
 
     def test_set_master_use_gtid_invalid(self):
         """It should raise MysqlLegacyError if called with an invalid setting."""
@@ -463,13 +462,13 @@ def test_set_replication_parameters(self):
         call_args = self.mocked_cursor.execute.call_args_list[0].args
         for part in (
             "CHANGE MASTER TO",
-            "master_host=%s(primary)",
-            "master_port=%s(port)",
+            "master_host=%(primary)s",
+            "master_port=%(port)s",
             "master_ssl=1",
-            "master_log_file=%s(binlog)",
-            "master_log_pos=%s(position)",
-            "master_user=%s(user)",
-            "password=%s(password)",
+            "master_log_file=%(binlog)s",
+            "master_log_pos=%(position)s",
+            "master_user=%(user)s",
+            "password=%(password)s",
         ):
             assert part in call_args[0]
 
@@ -574,10 +573,16 @@ def test_replication_lag_no_data(self):
 
         self.mocked_cursor.fetchone.assert_not_called()
 
-    def test_replication_lag_no_lag(self):
+    @pytest.mark.parametrize(
+        "row",
+        (
+            {"lag": None},
+            {"invalid": Decimal(0.1234)},
+        ),
+    )
+    def test_replication_lag_no_lag(self, row):
         """It should raise a MysqlLegacyError if the returned lag is None."""
         self.mocked_cursor.execute.side_effect = [1, 0]
-        row = {"lag": None}
         self.mocked_cursor.fetchone.return_value = row
         error_message = f"Unable to get lag information from: {row}"