[SYNPY-1667]Update Error Messaging when Attempting to Delete Rows that do not exist from Tables (#1257)

* add test for raised exceptions

* update how discrepant rows are identified and formatted

* update discrepant rows in test case

* raise lookuperror instead of valueerror

* re escape string

* separate failure tests and properly escape characters

* update var name

* add comment

Author: SageGJ

synapseclient/models/mixins/table_components.py

@@ -4184,9 +4184,16 @@ async def main():
                 existing_rows, on=["ROW_ID", "ROW_VERSION"], how="left", indicator=True
             )
             if not all(merged["_merge"] == "both"):
-                discrepant_idx = merged.loc[merged["_merge"] != "both"].index
-                raise ValueError(
-                    f"Rows with the following ROW_ID and ROW_VERSION pairs were not found in table {self.id}: {', '.join(map(str, discrepant_idx))}."
+                discrepant_rows = (
+                    merged.loc[merged["_merge"] != "both"][["ROW_ID", "ROW_VERSION"]]
+                    .astype(str)
+                    .values
+                )
+                discrepant_tuples = [
+                    f"({', '.join(tuple)})" for tuple in discrepant_rows
+                ]
+                raise LookupError(
+                    f"Rows with the following ROW_ID and ROW_VERSION pairs were not found in table {self.id}: {', '.join(discrepant_tuples)}."
                 )
             client.logger.info(
                 f"Received {len(rows_to_delete)} rows to delete for given dataframe."

tests/integration/synapseclient/models/async/test_table_async.py

@@ -1,6 +1,7 @@
 import json
 import os
 import random
+import re
 import string
 import tempfile
 import uuid
@@ -1849,6 +1850,43 @@ async def test_delete_multiple_rows_via_dataframe(
         # AND only 1 row should exist on the table
         assert len(results) == 1
 
+    async def test_delete_multiple_rows_via_dataframe_exception(
+        self, project_model: Project
+    ) -> None:
+        # GIVEN a table in Synapse
+        table_name = str(uuid.uuid4())
+        table = Table(
+            name=table_name,
+            parent_id=project_model.id,
+            columns=[Column(name="column_string", column_type=ColumnType.STRING)],
+        )
+        table = await table.store_async(synapse_client=self.syn)
+        self.schedule_for_cleanup(table.id)
+
+        # AND data for a column already stored in Synapse
+        data_for_table = pd.DataFrame({"column_string": ["value1", "value2", "value3"]})
+        await table.store_rows_async(
+            values=data_for_table, schema_storage_strategy=None, synapse_client=self.syn
+        )
+
+        # AND row ids and versions that do not exist in the table
+        row_ids = [4, 5]
+        row_versions = [1, 1]
+
+        # And an excpeted error message that should be displayed
+        row_id_version_tuples = ", ".join(
+            [f"({id}, {version})" for id, version in zip(row_ids, row_versions)]
+        )
+        exception_message = f"Rows with the following ROW_ID and ROW_VERSION pairs were not found in table {re.escape(table.id)}: {re.escape(row_id_version_tuples)}."
+
+        # WHEN the rows that do not exist are attempted to be deleted
+        # THEN a value error should be raised and the appropriate message should be displayed
+        with pytest.raises(LookupError, match=exception_message):
+            await table.delete_rows_async(
+                df=pd.DataFrame({"ROW_ID": row_ids, "ROW_VERSION": row_versions}),
+                synapse_client=self.syn,
+            )
+
 
 class TestColumnModifications:
     @pytest.fixture(autouse=True, scope="function")

tests/unit/synapseclient/mixins/unit_test_table_components.py

@@ -1902,15 +1902,42 @@ async def test_delete_rows_async_via_dataframe_pass(self):
                 pd.DataFrame(columns=["INVALID_COL1", "INVALID_COL2"]),  # Both invalid
                 "The dataframe must contain the 'ROW_ID' and 'ROW_VERSION' columns.",
             ),
+        ],
+    )
+    async def test_delete_rows_via_dataframe_fail_missing_columns(self, df, error_msg):
+        # GIVEN a TestClass instance
+        test_instance = self.ClassForTest()
+
+        # WHEN I call delete_rows_async
+        with (
+            patch(
+                "synapseclient.models.mixins.table_components.QueryMixin.query_async",
+                return_value=pd.DataFrame(
+                    {"ROW_ID": ["A", "B"], "ROW_VERSION": [1, 2]}
+                ),
+            ) as mock_query_async,
+            patch.object(self.syn.logger, "info") as mock_logger_info,
+        ):
+            with pytest.raises(ValueError, match=error_msg):
+                result = await test_instance.delete_rows_async(
+                    df=df, synapse_client=self.syn
+                )
+
+                # THEN mock_logger_info should not be called
+                mock_logger_info.assert_not_called()
+
+    @pytest.mark.parametrize(
+        "df, error_msg",
+        [
             (
                 pd.DataFrame(
                     {"ROW_ID": ["C", "D"], "ROW_VERSION": [2, 2]}
                 ),  # Both invalid
-                "Rows with the following ROW_ID and ROW_VERSION pairs were not found in table syn123: 0, 1.",
+                "Rows with the following ROW_ID and ROW_VERSION pairs were not found in table syn123: \\(C, 2\\), \\(D, 2\\).",  # Special characters must be escaped due to use with regex in test
             ),
         ],
     )
-    async def test_delete_rows_via_dataframe_fail(self, df, error_msg):
+    async def test_delete_rows_via_dataframe_fail_missing_rows(self, df, error_msg):
         # GIVEN a TestClass instance
         test_instance = self.ClassForTest()
 
@@ -1924,7 +1951,7 @@ async def test_delete_rows_via_dataframe_fail(self, df, error_msg):
             ) as mock_query_async,
             patch.object(self.syn.logger, "info") as mock_logger_info,
         ):
-            with pytest.raises(ValueError, match=error_msg):
+            with pytest.raises(LookupError, match=error_msg):
                 result = await test_instance.delete_rows_async(
                     df=df, synapse_client=self.syn
                 )