Add mutex to page tree move function, fix #1518

* Use django-db-mutex module to implement a mutex that allows only one page tree move operation at a time. Co-authored-by: Timo Ludwig <[email protected]>
digitalfabrik · Feb 19, 2023 · de8846f · de8846f
1 parent 534f226
commit de8846f
Show file tree

Hide file tree

Showing 8 changed files with 107 additions and 61 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,7 @@ UNRELEASED
 * [ [#1989](https://github.com/digitalfabrik/integreat-cms/issues/1989) ] Improve load time of locations endpoint
 * [ [#1578](https://github.com/digitalfabrik/integreat-cms/issues/1578) ] Add option to hide files in global media library
 * [ [#1752](https://github.com/digitalfabrik/integreat-cms/issues/1752) ] Show chapter for internal link suggestions
+* [ [#1518](https://github.com/digitalfabrik/integreat-cms/issues/1518) ] Prevent database corruption when moving 2 pages at the same time
 
 
 2023.2.0

diff --git a/Pipfile.lock b/Pipfile.lock
diff --git a/integreat_cms/cms/models/abstract_tree_node.py b/integreat_cms/cms/models/abstract_tree_node.py
@@ -8,6 +8,9 @@
 from treebeard.exceptions import InvalidPosition
 from treebeard.ns_tree import NS_Node
 
+from db_mutex import DBMutexError, DBMutexTimeoutError
+from db_mutex.db_mutex import db_mutex
+
 from .abstract_base_model import AbstractBaseModel
 from ..constants import position
 
@@ -259,6 +262,8 @@ def move(self, target, pos=None):
         :type pos: str
 
         :raises ~treebeard.exceptions.InvalidPosition: If the node is moved to another region
+        :raises ~db_mutex.exceptions.DBMutexError: If the DB mutex could not be retrieved
+        :raises ~db_mutex.exceptions.DBMutexTimeoutError: If waiting for the DB mutex timed out
         """
         logger.debug("Moving %r to position %r of %r", self, pos, target)
         # Do not allow to move a node outside its region
@@ -282,7 +287,19 @@ def move(self, target, pos=None):
         # Moving a node can modify all other nodes via raw sql queries (which are not recognized by cachalot),
         # so we have to invalidate the whole model manually.
         invalidate_model(self.__class__)
-        super().move(target=target, pos=pos)
+        try:
+            with db_mutex(self.__class__.__name__):
+                super().move(target, pos)
+        except DBMutexError as e:
+            logger.error("Could not retrieve database mutex to save %r", self)
+            raise DBMutexError(
+                _('Could not change position in tree of "{}".').format(self.best_translation.title)
+            ) from e
+        except DBMutexTimeoutError as e:
+            logger.error("Retrieving database mutex timed out while saving %r", self)
+            raise DBMutexTimeoutError(
+                _('Could not change position in tree of "{}".').format(self.best_translation.title)
+            ) from e
         invalidate_model(self.__class__)
 
         # Reload 'self' because lft/rgt may have changed

diff --git a/integreat_cms/cms/views/pages/page_actions.py b/integreat_cms/cms/views/pages/page_actions.py
@@ -16,6 +16,7 @@
 from django.views.decorators.http import require_POST
 from django.db import transaction
 
+from db_mutex import DBMutexError, DBMutexTimeoutError
 from treebeard.exceptions import InvalidPosition, InvalidMoveToDescendant
 
 from ....api.decorators import json_response
@@ -491,7 +492,13 @@ def move_page(request, region_slug, language_slug, page_id, target_id, position)
                 page=page.best_translation.title
             ),
         )
-    except (ValueError, InvalidPosition, InvalidMoveToDescendant) as e:
+    except (
+        ValueError,
+        InvalidPosition,
+        InvalidMoveToDescendant,
+        DBMutexTimeoutError,
+        DBMutexError,
+    ) as e:
         messages.error(request, e)
         logger.exception(e)
 

diff --git a/integreat_cms/core/settings.py b/integreat_cms/core/settings.py
@@ -247,6 +247,7 @@
     "django.contrib.staticfiles",
     # Installed third-party-apps
     "corsheaders",
+    "db_mutex",
     "linkcheck",
     "polymorphic",
     "rules.apps.AutodiscoverRulesConfig",
@@ -1012,3 +1013,11 @@
 
 #: The URL path where XLIFF files are served for download
 XLIFF_URL = "/xliff/"
+
+
+############
+# DB Mutex #
+############
+
+# Our database operations should never exceed 60 seconds
+DB_MUTEX_TTL_SECONDS = 60
diff --git a/integreat_cms/locale/de/LC_MESSAGES/django.po b/integreat_cms/locale/de/LC_MESSAGES/django.po
@@ -2340,6 +2340,10 @@ msgid "The node \"{}\" in region \"{}\" cannot be moved to \"{}\"."
 msgstr ""
 "Der Knoten \"{}\" in Region \"{}\" kann nicht nach \"{}\" verschoben werden."
 
+#: cms/models/abstract_tree_node.py
+msgid "Could not change position in tree of \"{}\"."
+msgstr "Position von \"{}\" im Baum konnte nicht geändert werden."
+
 #: cms/models/chat/chat_message.py
 msgid "sender"
 msgstr "Absender:in"

diff --git a/setup.cfg b/setup.cfg
@@ -49,6 +49,7 @@ install_requires =
 	Django>=3.2,<4.0
 	django-cacheops
 	django-cors-headers
+	django-db-mutex
 	django-debug-toolbar
 	django-linkcheck
 	django-polymorphic

diff --git a/sphinx/conf.py b/sphinx/conf.py
@@ -93,6 +93,7 @@
         "https://sphinx-rtd-tutorial.readthedocs.io/en/latest/",
         None,
     ),
+    "db_mutex": ("https://django-db-mutex.readthedocs.io/en/latest/", None),
     "django": (
         f"https://docs.djangoproject.com/en/{django_version}/",
         f"https://docs.djangoproject.com/en/{django_version}/_objects/",