Prevent deletion of Handle instances via ORM and migration to remove existing Codelist with no Handle
#2919
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mikerkelly
force-pushed
the
mikerkelly/no-delete-handles/enforce
branch
from
c885810 to
c3bbeef
Compare
`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. The only models with deletion in `codelists/actions.py` or `builder/actions.py` are `Codelist` (when discarding the last draft version), `CodelistVersion` (when discarding drafts), and `Search` (when deleting a search in the builder). 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 parent issue #2893. 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. That includes `Handle` when deleted via `Codelist` `CASCADE` as they do not have any dependent models, triggers etc. See https://github.com/django/django/blob/cb1d2854ed2b13799f2b0cc6e04019df181bacd4/django/db/models/deletion.py#L501 Trying locally via deleting a codelist draft in the web UI led to: ``` [debug ] (0.007) DELETE FROM "codelists_handle" WHERE "codelists_handle"."codelist_id" IN (9979); args=(9979,); alias=default [django.db.backends] ``` ... and I can find and delete multiple codelist with multiple handles in the Django shell, which deletes the handles via cascade: ``` >>> from django.db.models import Count >>> five_handle=Codelist.objects.annotate(hcount=Count("handles")).filter(hcount=5) >>> len(five_handle) 2 >>> five_handle.delete() <SUCCESS MESSAGE> >>> five_handle=Codelist.objects.annotate(hcount=Count("handles")).filter(hcount=5) >>> len(five_handle) 0 ``` ... and I cannot delete via the handle instances or `QuerySet` methods directly in the Django shell: ``` >>> three_handle_cl=Codelist.objects.annotate(hcount=Count("handles")).filter(hcount=3).first() >>> three_handle_cl.handles.all().delete() codelists.models.DeleteHandleException: Bulk deletion of Handle instances via the ORM is not permitted. Attempted on QuerySet `<NoDeleteHandleQuerySet [<Handle: Handle object (2800)>, <Handle: Handle object (2811)>, <Handle: Handle object (2908)>]>` >>> three_handle_cl.handles.all().first().delete() codelists.models.DeleteHandleException: May not delete handles - attempted on `Handle object (2800)` for codelist `BNF codes for NSAIDs (Medication Safety Indicator GIB01)` ``` Added automated tests for the overriden methods and manager queryset. There are already automated tests for discarding drafts etc. in `builder/tests/test_views.py`: - test_discard_only_draft_version - test_discard_one_draft_version
mikerkelly
force-pushed
the
mikerkelly/no-delete-handles/enforce
branch
from
c3bbeef to
141d57b
Compare
mikerkelly
added a commit
that referenced
this pull request
Dec 2, 2025
Per #2919 and #2893 `Codelist` should not be able to exist without `Handle`. We found an example, let's clean it up. Let's do this in a migration in a new maintenance app, so that the fix is auditable, reviewable, repeatable, testable, avoids the possibility of manual error, and fixes up development environment databases as well. This also provides an example if we need to do this or other maintenance again. And it avoids cluttering the feature apps and getting squashed. The migration is idempotent and has a noop reversal. Manual testing with the latest sanitised backup: Before the migration migrations state: ``` ~/opencodelists$ just manage showmigrations ... maintenance [ ] 0001_delete_codelists_without_handles ``` Before state in the Django shell, 1 such codelist exists: ``` >>> len(Codelist.objects.all()) 10419 >>> no_handles = Codelist.objects.filter(handles__isnull=True) >>> len(no_handles) 1 ``` Applying the migration: ``` just manage migrate Running migrations: Applying maintenance.0001_delete_codelists_without_handles... Deleted 1 codelists without handles just manage showmigrations ... maintenance [X] 0001_delete_codelists_without_handles ``` After state in the Django shell, no such codelist exists and only 1 codelist has been deleted: ``` len(Codelist.objects.all()) 10418 >>> no_handles = Codelist.objects.filter(handles__isnull=True) >>> len(no_handles) 0 ``` This safely demonstrates the migration works as expected on near-production data and it isn't part of ongoing application code so it does not need automated testing.
mikerkelly
added a commit
that referenced
this pull request
Dec 2, 2025
Per #2919 and #2893 `Codelist` should not be able to exist without `Handle`. We found an example, let's clean it up. Let's do this in a migration in a new maintenance app, so that the fix is auditable, reviewable, repeatable, testable, avoids the possibility of manual error, and fixes up development environment databases as well. This also provides an example if we need to do this or other maintenance again. And it avoids cluttering the feature apps and getting squashed. The migration is idempotent and has a noop reversal. Manual testing with the latest sanitised backup: Before the migration migrations state: ``` ~/opencodelists$ just manage showmigrations ... maintenance [ ] 0001_delete_codelists_without_handles ``` Before state in the Django shell, 1 such codelist exists: ``` >>> len(Codelist.objects.all()) 10419 >>> no_handles = Codelist.objects.filter(handles__isnull=True) >>> len(no_handles) 1 ``` Applying the migration: ``` just manage migrate Running migrations: Applying maintenance.0001_delete_codelists_without_handles... Deleted 1 codelists without handles just manage showmigrations ... maintenance [X] 0001_delete_codelists_without_handles ``` After state in the Django shell, no such codelist exists and only 1 codelist has been deleted: ``` len(Codelist.objects.all()) 10418 >>> no_handles = Codelist.objects.filter(handles__isnull=True) >>> len(no_handles) 0 ``` This safely demonstrates the migration works as expected on near-production data and it isn't part of ongoing application code so it does not need automated testing.
mikerkelly
force-pushed
the
mikerkelly/no-delete-handles/enforce
branch
from
d62b9c0 to
c862a85
Compare
Per #2919 and #2893 `Codelist` should not be able to exist without `Handle`. We found an example, let's clean it up. Let's do this in a migration in a new maintenance app, so that the fix is auditable, reviewable, repeatable, testable, avoids the possibility of manual error, and fixes up development environment databases as well. This also provides an example if we need to do this or other maintenance again. And it avoids cluttering the feature apps and getting squashed. The migration is idempotent and has a noop reversal. Manual testing with the latest sanitised backup (with judicious snipping of noise throughout): Before the migration migrations state: ``` ~/opencodelists$ just manage showmigrations ... maintenance [ ] 0001_delete_codelists_without_handles ``` Before state in the Django shell, 1 such codelist exists: ``` >>> len(Codelist.objects.all()) 10419 >>> no_handles = Codelist.objects.filter(handles__isnull=True) >>> len(no_handles) 1 ``` Applying the migration: ``` just manage migrate maintenance 0001 Running migrations: Applying maintenance.0001_delete_codelists_without_handles... 0001: Deleted 1 codelists without handles just manage showmigrations ... maintenance [X] 0001_delete_codelists_without_handles ``` After state in the Django shell, no such codelist exists and only 1 codelist has been deleted: ``` len(Codelist.objects.all()) 10418 >>> no_handles = Codelist.objects.filter(handles__isnull=True) >>> len(no_handles) 0 ``` Re-applying a very similr migration based on the same file and depending on it (not committed), to to test the `except` branch: ``` just manage migrate maintenance 0002 0002: No codelists without handles to delete ``` This safely demonstrates the migration works as expected on near-production data and also in databases without the faulty codelists. This isn't part of ongoing application code so it does not need automated testing.
mikerkelly
force-pushed
the
mikerkelly/no-delete-handles/enforce
branch
from
c862a85 to
434b13b
Compare
mikerkelly
changed the title
Handle instances via ORMHandle instances via ORM and migration to remove existing Codelist with no Handle
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. Document what we might do instead.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Codelistare created with oneHandle. They may gain more later, but business logic inactions.pydoes not permit deletion of any of them, and parts of the application may expect that there is always a currentHandlefor aCodelist. So we ought not permit it in application code.In this PR are a commit for preventing it via the instance and manager
deletemethods, and for applying a maintenance migration to the database to remove one current example from production. See commit messages for details about testing of both. They are part of the same PR as they're strongly related.Also a docs commit.
Fixes #2893.
Background
The only models with deletion in
codelists/actions.pyorbuilder/actions.pyareCodelist(when discarding the last draft version),CodelistVersion(when discarding drafts), andSearch(when deleting a search in the builder).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 parent issue #2893. This also helps avoid any use of the developer Django shell to deleteHandles.We enforce this constraint only in the Django model layer. That does not stop
Handlesbeing deleted by other means such as via direct SQL, such as viaon_delete=models.CASCADE, seehttps://docs.djangoproject.com/en/5.2/topics/db/queries/#deleting-objects
That includes
Handlewhen deleted viaCodelistCASCADEas they do not have any dependent models, triggers etc. Seehttps://github.com/django/django/blob/cb1d2854ed2b13799f2b0cc6e04019df181bacd4/django/db/models/deletion.py#L501