Fix interleaved repo delete with RAC via a transaction

The RepositoryActionCount table can have entries added while a repository deletion is in progress. We now perform the repository deletion under a transaction and explicitly test for RAC entries in the deletion unit test (which doesn't test interleaving, but it was missing this check).

Fixes #494

data/database.py

@@ -119,6 +119,7 @@ db = Proxy()
 read_slave = Proxy()
 db_random_func = CallableProxy()
 db_for_update = CallableProxy()
+db_transaction = CallableProxy()
 
 
 def validate_database_url(url, db_kwargs, connect_timeout=5):
@@ -164,6 +165,10 @@ def configure(config_object):
   if read_slave_uri is not None:
     read_slave.initialize(_db_from_url(read_slave_uri, db_kwargs))
 
+  def _db_transaction():
+    return config_object['DB_TRANSACTION_FACTORY'](db)
+
+  db_transaction.initialize(_db_transaction)
 
 def random_string_generator(length=16):
   def random_string():
@@ -373,14 +378,15 @@ class Repository(BaseModel):
       return sorted_models.index(cmp_fk.model_class.__name__)
     filtered_ops.sort(key=sorted_model_key)
 
-    for query, fk in filtered_ops:
+    with db_transaction():
-      model = fk.model_class
+      for query, fk in filtered_ops:
-      if fk.null and not delete_nullable:
+        model = fk.model_class
-        model.update(**{fk.name: None}).where(query).execute()
+        if fk.null and not delete_nullable:
-      else:
+          model.update(**{fk.name: None}).where(query).execute()
-        model.delete().where(query).execute()
+        else:
+          model.delete().where(query).execute()
 
-    return self.delete().where(self._pk_expr()).execute()
+      return self.delete().where(self._pk_expr()).execute()
 
 class Star(BaseModel):
   user = ForeignKeyField(User, index=True)

data/model/__init__.py

@@ -1,4 +1,4 @@
-from data.database import db
+from data.database import db, db_transaction
 
 
 class DataModelException(Exception):
@@ -80,10 +80,6 @@ class Config(object):
 config = Config()
 
 
-def db_transaction():
-  return config.app_config['DB_TRANSACTION_FACTORY'](db)
-
-
 # There MUST NOT be any circular dependencies between these subsections. If there are fix it by
 # moving the minimal number of things to _basequery
 # TODO document the methods and modules for each one of the submodules below.

test/test_api_usage.py

@@ -1,6 +1,7 @@
 # coding=utf-8
 
 import unittest
+import datetime
 import json as py_json
 
 from urllib import urlencode
@@ -15,6 +16,7 @@ from endpoints.trigger import BuildTriggerHandler
 from app import app
 from initdb import setup_database_for_testing, finished_database_for_testing
 from data import database, model
+from data.database import RepositoryActionCount
 
 from endpoints.api.team import TeamMember, TeamMemberList, TeamMemberInvite, OrganizationTeam
 from endpoints.api.tag import RepositoryTagImages, RepositoryTag, RevertTag, ListRepositoryTags
@@ -1468,6 +1470,10 @@ class TestDeleteRepository(ApiTestCase):
   def test_deleterepo(self):
     self.login(ADMIN_ACCESS_USER)
 
+    # Verify the repo exists.
+    self.getResponse(Repository,
+                     params=dict(repository=self.SIMPLE_REPO))
+
     self.deleteResponse(Repository, params=dict(repository=self.SIMPLE_REPO))
 
     # Verify the repo was deleted.
@@ -1478,6 +1484,10 @@ class TestDeleteRepository(ApiTestCase):
   def test_deleterepo2(self):
     self.login(ADMIN_ACCESS_USER)
 
+    # Verify the repo exists.
+    self.getResponse(Repository,
+                     params=dict(repository=self.COMPLEX_REPO))
+
     self.deleteResponse(Repository, params=dict(repository=self.COMPLEX_REPO))
 
     # Verify the repo was deleted.
@@ -1488,7 +1498,11 @@ class TestDeleteRepository(ApiTestCase):
   def test_populate_and_delete_repo(self):
     self.login(ADMIN_ACCESS_USER)
 
-    # Make sure the repository has come images and tags.
+    # Verify the repo exists.
+    self.getResponse(Repository,
+                     params=dict(repository=self.COMPLEX_REPO))
+
+    # Make sure the repository has some images and tags.
     self.assertTrue(len(list(model.image.get_repository_images(ADMIN_ACCESS_USER, 'complex'))) > 0)
     self.assertTrue(len(list(model.tag.list_repository_tags(ADMIN_ACCESS_USER, 'complex'))) > 0)
 
@@ -1524,6 +1538,13 @@ class TestDeleteRepository(ApiTestCase):
     model.repository.create_email_authorization_for_repo(ADMIN_ACCESS_USER, 'complex', 'a@b.com')
     model.repository.create_email_authorization_for_repo(ADMIN_ACCESS_USER, 'complex', 'b@c.com')
 
+    # Create some repository action count entries.
+    RepositoryActionCount.create(repository=repository, date=datetime.datetime.now(), count=1)
+    RepositoryActionCount.create(repository=repository,
+                                 date=datetime.datetime.now() - datetime.timedelta(days=1), count=2)
+    RepositoryActionCount.create(repository=repository,
+                                 date=datetime.datetime.now() - datetime.timedelta(days=3), count=6)
+
     # Delete the repository.
     self.deleteResponse(Repository, params=dict(repository=self.COMPLEX_REPO))