Skip to content

Commit c885810

Browse files
committed
Prevent deletion of Handle instances via ORM.
Codelist are created with one Handle. They may gain more later, but business logic in `actions.py` does not permit deletion of any of them, and parts of the application may expect that there is always a current Handle for a Codelist. So we ought not permit it in application code. We don't think that we have application code that does this (see `actions.py`) but have observered it twice in production in recent months, see PR. This also helps avoid any use of the developer Django shell to delete Handles. We enforce this constraint only in the Django model layer. That does not stop Handles being deleted by other means such as via direct SQL, such as via `on_delete=models.CASCADE`, see https://docs.djangoproject.com/en/5.2/topics/db/queries/#deleting-objects > Keep in mind that this will, whenever possible, be executed purely in SQL, > and so the delete() methods of individual object instances will not > necessarily be called during the process. Trying locally via deleting a codelist draft in the UI led to: ``` [debug ] (0.007) DELETE FROM "codelists_handle" WHERE "codelists_handle"."codelist_id" IN (9979); args=(9979,); alias=default [django.db.backends] ```
1 parent 93134f1 commit c885810

File tree

3 files changed

+53
-4
lines changed

3 files changed

+53
-4
lines changed

codelists/models.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
from django.core.validators import MinLengthValidator, RegexValidator
55
from django.db import models
66
from django.db.models import Q
7+
from django.db.models.query import QuerySet
78
from django.urls import reverse
89
from django.utils.functional import cached_property
910
from taggit.managers import TaggableManager
@@ -217,6 +218,18 @@ def has_published_versions(self):
217218
return self.versions.filter(status="published").exists()
218219

219220

221+
class DeleteHandleException(Exception):
222+
"""Raised when attempting to delete Handle via the ORM."""
223+
224+
225+
class NoDeleteHandleQuerySet(QuerySet):
226+
def delete(self):
227+
"""Prevent deletion of Handle QuerySets via ORM."""
228+
raise DeleteHandleException(
229+
"Bulk deletion of Handle instances via the ORM is not permitted."
230+
)
231+
232+
220233
class Handle(models.Model):
221234
codelist = models.ForeignKey(
222235
"Codelist", on_delete=models.CASCADE, related_name="handles"
@@ -271,6 +284,27 @@ class Meta:
271284
def owner(self):
272285
return self.organisation or self.user
273286

287+
# Attempt to stop deletion of Handle instances via the ORM.
288+
# Codelist are created with one Handle. They may gain more later, but
289+
# business logic in actions.py does not permit deletion of any of them,
290+
# and parts of the application may expect that there is always a current
291+
# Handle for a Codelist.
292+
# Note that this does not stop deletion via on_delete=models.CASCADE, see
293+
# https://docs.djangoproject.com/en/5.2/topics/db/queries/#deleting-objects
294+
# We enforce this constraint only in the Django model layer. That does not
295+
# stop Handles being deleted by other means such as via direct SQL, such
296+
# as via a cascade.
297+
298+
# Prevent deletion via QuerySet.
299+
objects = NoDeleteHandleQuerySet.as_manager()
300+
301+
# Prevent deletion via instance method.
302+
def delete(self, using=None, keep_parents=False):
303+
"""Prevent deletion of instances via the ORM."""
304+
raise DeleteHandleException(
305+
f"May not delete handles - attempted on {self} for codelist {self.codelist}"
306+
)
307+
274308

275309
class Status(models.TextChoices):
276310
DRAFT = "draft" # the version is being edited in the builder

codelists/tests/test_api.py

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,10 @@
33
from datetime import datetime
44

55
import pytest
6+
from django.db import connection
67

78
from codelists.actions import update_codelist
8-
from codelists.models import Codelist
9+
from codelists.models import Codelist, Handle
910
from mappings.dmdvmpprevmap.models import Mapping as VmpPrevMapping
1011
from opencodelists.tests.assertions import assert_difference, assert_no_difference
1112

@@ -272,9 +273,13 @@ def test_codelists_get_all_still_works_for_a_codelist_with_a_missing_handle(
272273
# We found a Codelist without an associated Handle,
273274
# which broke this API endpoint.
274275

275-
# Delete the Handle for one codelist.
276+
# Delete the Handle for one codelist. Via SQL as we disallow via ORM.
276277
codelist_to_break = Codelist.objects.get(handles__name="User Codelist From Scratch")
277-
codelist_to_break.current_handle.delete()
278+
table = Handle._meta.db_table
279+
with connection.cursor() as cursor:
280+
cursor.execute(
281+
f'DELETE FROM "{table}" WHERE id = {codelist_to_break.current_handle.pk}'
282+
)
278283

279284
rsp = client.get("/api/v1/codelist/?include-users")
280285
assert rsp.status_code == 200

codelists/tests/test_models.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
from django.db.utils import IntegrityError
44

55
from builder import actions as builder_actions
6-
from codelists.models import CodelistVersion, Handle
6+
from codelists.models import CodelistVersion, DeleteHandleException, Handle
77

88

99
def test_handle_cannot_belong_to_user_and_organisation(codelist, user, organisation):
@@ -150,6 +150,16 @@ def test_handle_name_validation_valid(user_codelist):
150150
assert clean_name == name
151151

152152

153+
def test_handle_delete_not_allowed_method(codelist):
154+
with pytest.raises(DeleteHandleException):
155+
codelist.current_handle.delete()
156+
157+
158+
def test_handle_delete_not_allowed_queryset(codelist):
159+
with pytest.raises(DeleteHandleException):
160+
Handle.objects.first().delete()
161+
162+
153163
def test_old_style_codes(old_style_version):
154164
assert old_style_version.codes == (
155165
"128133004",

0 commit comments

Comments
 (0)