From 4278f14e739021a5849892be44c8b8f5b0f8355e Mon Sep 17 00:00:00 2001
From: Chris Brozdowski <Chris.Broz@ucsf.edu>
Date: Wed, 7 Feb 2024 09:45:35 -0800
Subject: [PATCH] Cautious delete bugfixes (#821)

* #820

* Update changelog

* Blackify

* logger.error -> TypeError

* #818
---
 CHANGELOG.md                    |  2 ++
 src/spyglass/utils/dj_chains.py | 28 ++++++++++++++++---------
 src/spyglass/utils/dj_mixin.py  | 36 +++++++++++++++++++++++----------
 3 files changed, 45 insertions(+), 21 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index f4d9e3259..cbc20cc6e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,8 @@
     - Initial implementation. #711, #762
     - More robust caching of join to downstream tables. #806
     - Overwrite datajoint `delete` method to use `cautious_delete`. #806
+    - Reverse join order for session summary. #821
+    - Add temporary logging of use to `common_usage`. #811, #821
 - Add `deprecation_factory` to facilitate table migration. #717
 - Add Spyglass logger. #730
 - IntervalList: Add secondary key `pipeline` #742
diff --git a/src/spyglass/utils/dj_chains.py b/src/spyglass/utils/dj_chains.py
index d8152d020..457e3e613 100644
--- a/src/spyglass/utils/dj_chains.py
+++ b/src/spyglass/utils/dj_chains.py
@@ -134,13 +134,12 @@ def __init__(self, parent: Table, child: Table, connection=None):
             get_master(child.full_table_name) == ""
             and MERGE_PK in child.heading.names
         ):
-            logger.error("Child is a merge table. Use TableChains instead.")
+            raise TypeError("Child is a merge table. Use TableChains instead.")
 
         self._link_symbol = " -> "
         self.parent = parent
         self.child = child
         self._has_link = child.full_table_name in parent.descendants()
-        self._errors = []
 
     def __str__(self):
         """Return string representation of chain: parent -> child."""
@@ -221,17 +220,26 @@ def objects(self) -> List[dj.FreeTable]:
             else None
         )
 
-    def errors(self) -> List[str]:
-        """Return list of errors for each table in chain."""
-        return self._errors
-
-    def join(self, restricton: str = None) -> dj.expression.QueryExpression:
-        """Return join of tables in chain with restriction applied to parent."""
+    def join(
+        self, restricton: str = None, reverse_order: bool = False
+    ) -> dj.expression.QueryExpression:
+        """Return join of tables in chain with restriction applied to parent.
+
+        Parameters
+        ----------
+        restriction : str, optional
+            Restriction to apply to first table in the order.
+            Defaults to self.parent.restriction.
+        reverse_order : bool, optional
+            If True, join tables in reverse order. Defaults to False.
+        """
         if not self._has_link:
             return None
+
+        objects = self.objects[::-1] if reverse_order else self.objects
         restriction = restricton or self.parent.restriction or True
-        join = self.objects[0] & restriction
-        for table in self.objects[1:]:
+        join = objects[0] & restriction
+        for table in objects[1:]:
             try:
                 join = join.proj() * table
             except dj.DataJointError as e:
diff --git a/src/spyglass/utils/dj_mixin.py b/src/spyglass/utils/dj_mixin.py
index 7632e0249..7a69ba75e 100644
--- a/src/spyglass/utils/dj_mixin.py
+++ b/src/spyglass/utils/dj_mixin.py
@@ -278,7 +278,9 @@ def _get_exp_summary(self):
         empty_pk = {self._member_pk: "NULL"}
 
         format = dj.U(self._session_pk, self._member_pk)
-        sess_link = self._session_connection.join(self.restriction)
+        sess_link = self._session_connection.join(
+            self.restriction, reverse_order=True
+        )
 
         exp_missing = format & (sess_link - SesExp).proj(**empty_pk)
         exp_present = format & (sess_link * SesExp - exp_missing).proj()
@@ -363,17 +365,29 @@ def _usage_table(self):
 
     def _log_use(self, start, merge_deletes=None):
         """Log use of cautious_delete."""
-        self._usage_table.insert1(
-            dict(
-                duration=time() - start,
-                dj_user=dj.config["database.user"],
-                origin=self.full_table_name,
-                restriction=(
-                    str(self.restriction)[:255] if self.restriction else "None"
-                ),
-                merge_deletes=merge_deletes,
-            )
+        if isinstance(merge_deletes, QueryExpression):
+            merge_deletes = merge_deletes.fetch(as_dict=True)
+        safe_insert = dict(
+            duration=time() - start,
+            dj_user=dj.config["database.user"],
+            origin=self.full_table_name,
         )
+        try:
+            self._usage_table.insert1(
+                dict(
+                    **safe_insert,
+                    restriction=(
+                        "".join(self.restriction)[255:]  # handle list
+                        if self.restriction
+                        else "None"
+                    ),
+                    merge_deletes=merge_deletes,
+                )
+            )
+        except (DataJointError, DataError):
+            self._usage_table.insert1(
+                dict(**safe_insert, restriction="Unknown")
+            )
 
     # TODO: Intercept datajoint delete confirmation prompt for merge deletes
     def cautious_delete(self, force_permission: bool = False, *args, **kwargs):