Prevent deletion of Handle instances via ORM and migration to remove existing Codelist with no Handle

DEVELOPERS.md

@@ -305,7 +305,35 @@ Only images that have changed will be committed to github - but it's worth notin
 
 ## Other production maintenance tasks
 
-### Manually creating and updating codelist versions
+### Modifying the production database
+
+Sometimes, we may need to modify the production database to fix integrity
+errors due to bugs or to perform some management actions. We should not make
+any such changes by directly logging into the Django management shell or a
+dbshell. Entering commands manually is error-prone and not auditable,
+reviewable, repeatable, or testable.
+
+It may be okay to directly run management commands in production, discuss it
+with the team. Safest is probably to make the change via a migration, which
+also provides an audit trail and makes it easier to review, test, and
+potentially repeat. Create any such migrations in the `maintenance` app, see
+the history of that app for examples.
+
+You can create a fresh empty migration in a development environment with:
+
+```
+just manage makemigrations maintenance --empty --name XXX
+```
+
+Populate the logic and mark it as dependent on the latest migrations from other
+relevant apps (likely just `codelists` and potentially previous migrations in
+the same app. Ideally it is idempotent and reversible (even if the reverse is
+just a noop). These don't necessarily need automated testing if you do
+sufficiently robust manual testing on near-production data. This is because
+they are not used in any other components and we don't expect them to change
+once merged.
+
+#### Manually creating and updating codelist versions
 
 Currently OpenCodelists handles multiple coding system releases, however it can't always handle
 new versions of codelists when the coding system has changed significantly since the original
@@ -350,23 +378,8 @@ not organisation codelists.
 
 #### Process for deleting a single version of a codelist
 
-First, visit the codelist URL of interest
-and note down the version ID hash on the page.
-
-On dokku3:
-
-1. Start `shell_plus` which loads the database models for you:
-   ```sh
-   $ dokku run opencodelists python manage.py shell_plus
-   ```
-2. Access the specific version of the codelist:
-   ```pycon
-   >>> version = CodelistVersion.objects.get_by_hash("<hash>")
-   ```
-3. Delete the **codelist version**:
-   ```pycon
-   >>> version.delete()
-   ```
+Write a migration to delete the single version of the codelist and make it
+easy for anyone doing the same in future to repeat this.
 
 #### Process for deleting a codelist entirely
 

codelists/models.py

@@ -4,6 +4,7 @@
 from django.core.validators import MinLengthValidator, RegexValidator
 from django.db import models
 from django.db.models import Q
+from django.db.models.query import QuerySet
 from django.urls import reverse
 from django.utils.functional import cached_property
 from taggit.managers import TaggableManager
@@ -217,6 +218,19 @@ def has_published_versions(self):
         return self.versions.filter(status="published").exists()
 
 
+class DeleteHandleException(Exception):
+    """Raised when attempting to delete Handle via the ORM."""
+
+
+class NoDeleteHandleQuerySet(QuerySet):
+    def delete(self):
+        """Prevent deletion of Handle QuerySets via ORM."""
+        raise DeleteHandleException(
+            "Bulk deletion of Handle instances via the ORM is not permitted. "
+            f"Attempted on QuerySet `{self}`"
+        )
+
+
 class Handle(models.Model):
     codelist = models.ForeignKey(
         "Codelist", on_delete=models.CASCADE, related_name="handles"
@@ -271,6 +285,27 @@ class Meta:
     def owner(self):
         return self.organisation or self.user
 
+    # Attempt to stop deletion of Handle instances via the 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.
+    # Note that this does not stop deletion via on_delete=models.CASCADE, see
+    # https://docs.djangoproject.com/en/5.2/topics/db/queries/#deleting-objects
+    # 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 a cascade.
+
+    # Prevent deletion via QuerySet.
+    objects = NoDeleteHandleQuerySet.as_manager()
+
+    # Prevent deletion via instance method.
+    def delete(self, using=None, keep_parents=False):
+        """Prevent deletion of instances via the ORM."""
+        raise DeleteHandleException(
+            f"May not delete handles - attempted on `{self}` for codelist `{self.codelist}`"
+        )
+
 
 class Status(models.TextChoices):
     DRAFT = "draft"  # the version is being edited in the builder

codelists/tests/test_api.py

@@ -3,9 +3,10 @@
 from datetime import datetime
 
 import pytest
+from django.db import connection
 
 from codelists.actions import update_codelist
-from codelists.models import Codelist
+from codelists.models import Codelist, Handle
 from mappings.dmdvmpprevmap.models import Mapping as VmpPrevMapping
 from opencodelists.tests.assertions import assert_difference, assert_no_difference
 
@@ -272,9 +273,13 @@ def test_codelists_get_all_still_works_for_a_codelist_with_a_missing_handle(
     # We found a Codelist without an associated Handle,
     # which broke this API endpoint.
 
-    # Delete the Handle for one codelist.
+    # Delete the Handle for one codelist. Via SQL as we disallow via ORM.
     codelist_to_break = Codelist.objects.get(handles__name="User Codelist From Scratch")
-    codelist_to_break.current_handle.delete()
+    table = Handle._meta.db_table
+    with connection.cursor() as cursor:
+        cursor.execute(
+            f'DELETE FROM "{table}" WHERE id = {codelist_to_break.current_handle.pk}'
+        )
 
     rsp = client.get("/api/v1/codelist/?include-users")
     assert rsp.status_code == 200

codelists/tests/test_models.py

@@ -3,7 +3,7 @@
 from django.db.utils import IntegrityError
 
 from builder import actions as builder_actions
-from codelists.models import CodelistVersion, Handle
+from codelists.models import CodelistVersion, DeleteHandleException, Handle
 
 
 def test_handle_cannot_belong_to_user_and_organisation(codelist, user, organisation):
@@ -150,6 +150,16 @@ def test_handle_name_validation_valid(user_codelist):
     assert clean_name == name
 
 
+def test_handle_delete_not_allowed_method(codelist):
+    with pytest.raises(DeleteHandleException):
+        codelist.current_handle.delete()
+
+
+def test_handle_delete_not_allowed_queryset(codelist):
+    with pytest.raises(DeleteHandleException):
+        Handle.objects.first().delete()
+
+
 def test_old_style_codes(old_style_version):
     assert old_style_version.codes == (
         "128133004",

maintenance/migrations/0001_delete_codelists_without_handles.py

@@ -0,0 +1,30 @@
+# Delete codelists without handles, see https://github.com/opensafely-core/opencodelists/issues/2893
+from django.db import migrations
+
+
+def delete_codelists_without_handles(apps, schema_editor):
+    """Delete codelists that have no associated handles."""
+    Codelist = apps.get_model('codelists', 'Codelist')
+    codelists_without_handles = Codelist.objects.filter(handles__isnull=True)
+    # Deletes via CASCADE bypassing ORM `delete``methods.
+    deleted_stats = codelists_without_handles.delete()
+
+    # Provide some feedback.
+    try:
+        deleted_count = deleted_stats[1]['codelists.Codelist']
+        print(f"0001: Deleted {deleted_count} codelists without handles")
+    except KeyError:
+        print("0001: No codelists without handles to delete")
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('codelists', '0063_alter_handle_name'),
+    ]
+
+    operations = [
+        migrations.RunPython(
+            delete_codelists_without_handles,
+            migrations.RunPython.noop
+        ),
+    ]

opencodelists/settings.py

@@ -110,6 +110,7 @@ def get_env_var(name):
     "mappings.bnfdmd",
     "mappings.ctv3sctmap2",
     "mappings.dmdvmpprevmap",
+    "maintenance",
     "corsheaders",
     "crispy_forms",
     "crispy_bootstrap4",