Add additional detection for transitive deletes and fix those found (#281)

We were not testing for transitive deletes when performing *User*
deletion, which led us to have a few

data/database.py

@@ -651,9 +651,11 @@ class User(BaseModel):
                     TeamSync,
                     RepositorySearchScore,
                     DeletedNamespace,
+                    DeletedRepository,
                     RepoMirrorRule,
                     NamespaceGeoRestriction,
                     ManifestSecurityStatus,
+                    RepoMirrorConfig,
                 }
                 | appr_classes
                 | v22_classes

data/model/test/test_gc.py

@@ -38,6 +38,7 @@ from image.docker.schema2.manifest import DockerSchema2ManifestBuilder
 from image.shared.schemas import parse_manifest_from_bytes
 from util.bytes import Bytes
 
+from test.helpers import check_transitive_modifications
 from test.fixtures import *
 
 
@@ -203,7 +204,8 @@ def assert_gc_integrity(expect_storage_removed=True):
     existing_manifest_count = _get_dangling_manifest_count()
 
     # Yield to the GC test.
-    yield
+    with check_transitive_modifications():
+        yield
 
     # Ensure the number of dangling storages, manifests and labels has not changed.
     updated_storage_count = _get_dangling_storage_count()

data/model/test/test_user.py

@@ -20,6 +20,7 @@ from data.queue import WorkQueue
 from util.timedeltastring import convert_to_timedelta
 from util.timedeltastring import convert_to_timedelta
 from util.security.token import encode_public_private_token
+from test.helpers import check_transitive_modifications
 from test.fixtures import *
 
 
@@ -118,7 +119,8 @@ def test_delete_namespace_via_marker(initialized_db):
     marker_id = mark_namespace_for_deletion(user, [], queue)
 
     # Delete the user.
-    delete_namespace_via_marker(marker_id, [])
+    with check_transitive_modifications():
+        delete_namespace_via_marker(marker_id, [])
 
     # Ensure the user was actually deleted.
     with pytest.raises(User.DoesNotExist):

test/helpers.py

@@ -1,8 +1,13 @@
 import multiprocessing
 import time
 import socket
+import logging
+import re
 
 from contextlib import contextmanager
+
+from playhouse.test_utils import assert_query_count, _QueryLogHandler
+
 from data.database import LogEntryKind, LogEntry3
 
 
@@ -32,6 +37,53 @@ class assert_action_logged(object):
             assert self.existing_count == (updated_count - 1), error_msg
 
 
+class log_queries(object):
+    """ Logs all queries that occur under the context. """
+
+    def __init__(self, query_filters=None):
+        self.filters = query_filters
+
+    def get_queries(self):
+        queries = [q.msg[0] for q in self._handler.queries]
+        if not self.filters:
+            return queries
+
+        filtered_queries = []
+        for query_filter in self.filters:
+            filtered_queries.extend([q for q in queries if re.match(query_filter, q)])
+
+        return filtered_queries
+
+    def __enter__(self):
+        logger = logging.getLogger("peewee")
+        self._handler = _QueryLogHandler()
+        logger.setLevel(logging.DEBUG)
+        logger.addHandler(self._handler)
+        return self
+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        logger = logging.getLogger("peewee")
+        logger.removeHandler(self._handler)
+
+
+class check_transitive_modifications(log_queries):
+    """ Checks for Peewee-generated transition deletion queries and fails if any are found.
+    
+        These kinds of queries (which use subqueries) can lock massively on MySQL, so we detect
+        them and fail.
+    """
+
+    def __init__(self):
+        filters = [r"^DELETE.+IN \(SELECT.+$", r"^UPDATE.+IN \(SELECT.+$"]
+        super(check_transitive_modifications, self).__init__(query_filters=filters)
+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        super(check_transitive_modifications, self).__exit__(exc_type, exc_val, exc_tb)
+        queries = self.get_queries()
+        if queries:
+            raise Exception("Detected transitive deletion or update in queries: %s" % queries)
+
+
 _LIVESERVER_TIMEOUT = 5
 
 

test/test_api_usage.py

@@ -37,7 +37,7 @@ from data.appr_model.models import NEW_MODELS
 from data.database import RepositoryActionCount, Repository as RepositoryTable
 from data.logs_model import logs_model
 from data.registry_model import registry_model
-from test.helpers import assert_action_logged
+from test.helpers import assert_action_logged, log_queries, check_transitive_modifications
 from util.secscan.fake import fake_security_scanner
 
 from endpoints.api.team import (
@@ -2389,45 +2389,6 @@ class TestChangeRepoVisibility(ApiTestCase):
         self.assertEquals(False, json["is_public"])
 
 
-class log_queries(object):
-    def __init__(self, query_filters=None):
-        self.filters = query_filters
-
-    def get_queries(self):
-        queries = [q.msg[0] for q in self._handler.queries]
-        if not self.filters:
-            return queries
-
-        filtered_queries = []
-        for query_filter in self.filters:
-            filtered_queries.extend([q for q in queries if re.match(query_filter, q)])
-
-        return filtered_queries
-
-    def __enter__(self):
-        logger = logging.getLogger("peewee")
-        self._handler = _QueryLogHandler()
-        logger.setLevel(logging.DEBUG)
-        logger.addHandler(self._handler)
-        return self
-
-    def __exit__(self, exc_type, exc_val, exc_tb):
-        logger = logging.getLogger("peewee")
-        logger.removeHandler(self._handler)
-
-
-class check_transitive_modifications(log_queries):
-    def __init__(self):
-        filters = [r"^DELETE.+IN \(SELECT.+$", r"^UPDATE.+IN \(SELECT.+$"]
-        super(check_transitive_modifications, self).__init__(query_filters=filters)
-
-    def __exit__(self, exc_type, exc_val, exc_tb):
-        super(check_transitive_modifications, self).__exit__(exc_type, exc_val, exc_tb)
-        queries = self.get_queries()
-        if queries:
-            raise Exception("Detected transitive deletion or update in queries: %s" % queries)
-
-
 class TestDeleteRepository(ApiTestCase):
     SIMPLE_REPO = ADMIN_ACCESS_USER + "/simple"
     COMPLEX_REPO = ADMIN_ACCESS_USER + "/complex"