diff --git a/docs/development/netbox-internals.md b/docs/development/netbox-internals.md index 883ce34..4ae27b4 100644 --- a/docs/development/netbox-internals.md +++ b/docs/development/netbox-internals.md @@ -22,9 +22,11 @@ not a list of unsupported APIs alone. | `utilities.request.copy_safe_request` | `views/scripts.py`, `api/views.py` | Copies the request for a worker and strips sensitive headers. The copy must be picklable. | | `netbox.api.viewsets.mixins.discard_events_on_rollback` | `api/views.py` | Discards request events when a declaration-writing API action rolls back. Never used for disposable-write permission probes. | | `netbox.api.viewsets.NetBoxModelViewSet.perform_update` and `perform_destroy` | `api/views.py` | Lets the lock-order mixin acquire the Project lock before NetBox's conditional row lock. That row lock must remain inside these hooks. The canary detects a missing hook, not a change in where it takes the lock. A Script File update also gates and queues its Project's refresh here, which bulk updates reach because NetBox calls the hook once per object. | +| `netbox.api.viewsets.mixins.BulkUpdateModelMixin.get_bulk_update_queryset` | `api/views.py` | Orders Script bulk updates by primary key to match synchronization. NetBox v4.7.4 and later also lock the whole batch in that order before saving. | | `utilities.exceptions.PermissionsViolation` | `permissions.py`, `api/views.py` | Rejects child writes outside the user's object-permission scope through NetBox's enclosing rollback path. | | `utilities.exceptions.AbortRequest` | `models/projects.py`, `forms/model_forms/projects.py` | Refuses a form or model save. REST returns 400, and the UI displays the message on the form. Documented in `docs/plugins/development/exceptions.md` and handled by `NetBoxModelViewSet.dispatch`, `ObjectEditView.post` and both bulk views. | | `netbox.views.generic.BulkEditView.pre_save_operations` | `views/projects.py` | Runs the source-field permission check before each save in NetBox's bulk-edit transaction. The check reads the undocumented `_nullify` list used by **Set null**. | +| `netbox.views.generic.BulkEditView.get_queryset` | `views/scripts.py` | Uses primary-key order when applying a Script bulk edit. Checks `_apply`, the undocumented name of the Apply button, to preserve the initial confirmation page's default order. | | `utilities.data.normalize_update_fields` | `models/projects.py` | Converts `update_fields` to a frozenset in the forwarded save arguments. Membership checks then cannot exhaust a one-shot iterable before the save. NetBox uses the same pattern. | | `utilities.rqworker.get_queue_for_model` | `api/views.py`, `jobs.py` | Selects the configured queue for both worker checks and enqueue operations. | | `rq.utils.parse_timeout` | `runtime/introspection.py` | Parses the author's `job_timeout` using RQ's own grammar, including strings such as `"30m"`. | diff --git a/netbox_scripts/activation.py b/netbox_scripts/activation.py index 359927c..f28164e 100644 --- a/netbox_scripts/activation.py +++ b/netbox_scripts/activation.py @@ -138,10 +138,11 @@ def synchronize_scripts(*, project, revision, records, using): """ Bring one project's Script rows in line with a validated discovery snapshot. - Database work only, inside the transaction and the project lock the caller holds. A row the - snapshot no longer names is retired rather than deleted, keeping its primary key and the Job - history on it. A row whose recorded fields already match is left untouched. enabled is never - written. + Database work only, inside the transaction and the project lock the caller holds. Every + existing Script of the project is locked in primary-key order before any write, matching the + order used by bulk edits. A row the snapshot no longer names is retired rather than deleted, + keeping its primary key and the Job history on it. A row whose recorded fields already match + is left untouched. enabled is never written. Returns a ScriptSyncResult. Inside a request each written row is change logged and queues an event attributed to the user who asked for the activation, OBJECT_CREATED for a newly @@ -150,7 +151,10 @@ def synchronize_scripts(*, project, revision, records, using): """ written = 0 retired = 0 - rows = {(row.module_path, row.class_name): row for row in project.scripts.using(using)} + rows = { + (row.module_path, row.class_name): row + for row in project.scripts.using(using).order_by('pk').select_for_update() + } for record in records: identity = (record['module_path'], record['class_name']) # script_file_id and script_file_path stay in the revision's snapshot. Which diff --git a/netbox_scripts/api/views.py b/netbox_scripts/api/views.py index 49108eb..fbd72b6 100644 --- a/netbox_scripts/api/views.py +++ b/netbox_scripts/api/views.py @@ -320,6 +320,10 @@ def initial(self, request, *args, **kwargs): # class already applied restricts to what the user may add, and that set is empty. self.queryset = type(self).queryset.restrict(request.user, 'run') + def get_bulk_update_queryset(self): + """Return Scripts in primary-key order to match synchronization.""" + return super().get_bulk_update_queryset().order_by('pk') + # http_method_names is checked on every dispatch, so the action declares its own as an # initkwarg, which applies to this route alone and leaves POST refused on the list route. @extend_schema( diff --git a/netbox_scripts/models/scripts.py b/netbox_scripts/models/scripts.py index 25781a0..b3ab81f 100644 --- a/netbox_scripts/models/scripts.py +++ b/netbox_scripts/models/scripts.py @@ -253,6 +253,8 @@ def save(self, *args, **kwargs): original = ( type(self) .objects.using(using) + # The default ordering joins the Project, and FOR UPDATE would lock that row too. + .order_by('pk') .select_for_update() .filter(pk=self.pk) .values(*PUBLISHED_SCRIPT_FIELDS) diff --git a/netbox_scripts/tests/api/test_scripts.py b/netbox_scripts/tests/api/test_scripts.py index 7ea762d..89cada4 100644 --- a/netbox_scripts/tests/api/test_scripts.py +++ b/netbox_scripts/tests/api/test_scripts.py @@ -1,5 +1,6 @@ from unittest import mock +from django.db.models.signals import pre_save from django.urls import reverse from rest_framework import status @@ -17,6 +18,7 @@ ObjectPermissionTestMixin, PluginAPIViewTestCase, PluginAPIViewTestCases, + create_scripts_out_of_default_order, refresh_count, ) from utilities.api import get_serializer_for_model @@ -268,6 +270,24 @@ def test_patching_requires_the_change_permission(self): ) self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + def test_a_bulk_update_saves_in_primary_key_order(self): + pks = create_scripts_out_of_default_order() + saved = [] + + def record_save(sender, instance, **kwargs): + saved.append(instance.pk) + + pre_save.connect(record_save, sender=NetBoxScript, dispatch_uid='bulk-update-save-order') + self.addCleanup(pre_save.disconnect, record_save, sender=NetBoxScript, dispatch_uid='bulk-update-save-order') + self.add_permissions('netbox_scripts.view_netboxscript', 'netbox_scripts.change_netboxscript') + + response = self.client.patch( + self._get_list_url(), [{'id': pk, 'enabled': False} for pk in pks[::-1]], format='json', **self.header + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(saved, pks) + FILE_DIGEST = 'c' * 64 diff --git a/netbox_scripts/tests/plugin_testing.py b/netbox_scripts/tests/plugin_testing.py index 3a3dff1..669e04e 100644 --- a/netbox_scripts/tests/plugin_testing.py +++ b/netbox_scripts/tests/plugin_testing.py @@ -7,6 +7,7 @@ from core.models import Job, ObjectType from netbox_scripts.jobs import ProjectScriptFileRefreshJob +from netbox_scripts.models import NetBoxScript, ScriptProject from netbox_scripts.runtime.naming import PRIVATE_ROOT from netbox_scripts.storage.locks import advisory_key from users.models import ObjectPermission @@ -44,6 +45,16 @@ def refresh_count(project): return Job.objects.filter(name=ProjectScriptFileRefreshJob.name, data__project_id=project.pk).count() +def create_scripts_out_of_default_order(): + """Create Scripts out of default order, interleaving two Projects, and return their primary keys.""" + zeta = ScriptProject.objects.create(name='Zeta Project', key='zeta-project') + alpha = ScriptProject.objects.create(name='Alpha Project', key='alpha-project') + return [ + NetBoxScript.objects.create(project=project, module_path='deploy', class_name=name, display_name=name).pk + for project, name in ((zeta, 'Zeta'), (alpha, 'Mid'), (zeta, 'Alpha')) + ] + + class SecondSession: """ A separate database session, used to observe what this connection holds. diff --git a/netbox_scripts/tests/storage/test_concurrency.py b/netbox_scripts/tests/storage/test_concurrency.py index 4ce787c..67cc0b6 100644 --- a/netbox_scripts/tests/storage/test_concurrency.py +++ b/netbox_scripts/tests/storage/test_concurrency.py @@ -21,6 +21,7 @@ from django.test import TransactionTestCase, override_settings from core.models import Job +from netbox_scripts.activation import activate_revision from netbox_scripts.choices import RevisionStatusChoices from netbox_scripts.jobs import ProjectStorageCleanupJob from netbox_scripts.models import NetBoxScript, ScriptFile, ScriptProject, ScriptProjectRevision @@ -29,6 +30,7 @@ from netbox_scripts.storage.manifest import compute_digest from netbox_scripts.storage.paths import revision_prefix from netbox_scripts.tests.plugin_testing import IN_MEMORY_STORAGES, SecondSession +from netbox_scripts.tests.test_activation import record from utilities.testing.mixins import RQQueueTestMixin SOURCE = {'hello.py': b'print("hi")\n'} @@ -70,7 +72,7 @@ def cleanup(self, digest, paths): class ScriptEditLockScopeTestCase(SerializationTestCase): - """An ordinary Script edit serializes on its own row, never on the project.""" + """Script edits lock only their own row, and activation locks every Script of its Project before writing.""" def observe_project_lock_during_save(self, instance): """Save one row and report whether another session could take the project lock meanwhile.""" @@ -102,6 +104,59 @@ def test_a_declaration_still_takes_the_project_lock(self): self.assertFalse(self.observe_project_lock_during_save(script_file), 'The declaration lost its lock.') + def test_an_edit_locks_only_its_script_row(self): + script = NetBoxScript.objects.create( + project=self.project, module_path='deploy', class_name='Deploy', display_name='Deploy' + ) + observed = [] + + def observe(sender, instance, **kwargs): + with SecondSession() as other: + observed.append( + { + 'script': other.row_locked(NetBoxScript, instance.pk), + 'project': other.row_locked(ScriptProject, self.project.pk), + } + ) + + pre_save.connect(observe, sender=NetBoxScript, dispatch_uid='script-guard-row-lock') + self.addCleanup(pre_save.disconnect, observe, sender=NetBoxScript, dispatch_uid='script-guard-row-lock') + script.enabled = False + script.save() + + self.assertEqual(observed, [{'script': True, 'project': False}]) + + def test_activation_locks_all_existing_scripts_before_writing(self): + revision = self.materialize() + # Primary keys run against publication order, and Retired is dropped from the snapshot. + zeta, alpha, retired = ( + NetBoxScript.objects.create(project=self.project, module_path='deploy', class_name=name, display_name=name) + for name in ('Zeta', 'Alpha', 'Retired') + ) + ScriptProjectRevision.objects.filter(pk=revision.pk).update( + status=RevisionStatusChoices.VALID, + discovered_scripts=[record(class_name='Alpha'), record(class_name='Zeta', position=1)], + ) + revision.refresh_from_db() + other = ScriptProject.objects.create(name='Audit Devices', key='audit-devices') + bystander = NetBoxScript.objects.create( + project=other, module_path='audit', class_name='Audit', display_name='Audit' + ) + observed = [] + + def observe(sender, instance, **kwargs): + if not observed: + with SecondSession() as session: + observed.append( + {row.pk: session.row_locked(NetBoxScript, row.pk) for row in (zeta, alpha, retired, bystander)} + ) + + pre_save.connect(observe, sender=NetBoxScript, dispatch_uid='activation-row-locks') + self.addCleanup(pre_save.disconnect, observe, sender=NetBoxScript, dispatch_uid='activation-row-locks') + activate_revision(revision) + + self.assertEqual(observed, [{zeta.pk: True, alpha.pk: True, retired.pk: True, bystander.pk: False}]) + class DeclarationGuardRowLockTestCase(SerializationTestCase): """The declaration guard holds the row it read until its own write lands.""" diff --git a/netbox_scripts/tests/test_activation.py b/netbox_scripts/tests/test_activation.py index 44d6299..514e17c 100644 --- a/netbox_scripts/tests/test_activation.py +++ b/netbox_scripts/tests/test_activation.py @@ -235,6 +235,13 @@ def test_job_history_survives_a_retirement_and_a_return(self): self.sync([record()]) self.assertEqual(NetBoxScript.objects.get().jobs.count(), 1) + def test_the_scripts_are_locked_in_primary_key_order_before_any_write(self): + self.sync([record(), record(class_name='AuditDevices', position=1)]) + with CaptureQueriesContext(connection) as captured: + self.sync([record(class_name='AuditDevices', display_name='Audit Devices')]) + first = next(q['sql'] for q in captured.captured_queries if f'"{SCRIPT_TABLE}"' in q['sql']) + self.assertTrue(first.endswith(f'ORDER BY "{SCRIPT_TABLE}"."id" ASC FOR UPDATE'), first) + class ActivationMixin(StorageServiceMixin): """A project with real stored content, plus a way to give a revision recorded scripts.""" diff --git a/netbox_scripts/tests/views/test_scripts.py b/netbox_scripts/tests/views/test_scripts.py index b5f8f1e..b54ad52 100644 --- a/netbox_scripts/tests/views/test_scripts.py +++ b/netbox_scripts/tests/views/test_scripts.py @@ -1,6 +1,6 @@ from django.contrib.auth import get_user_model from django.db import connection -from django.db.models.signals import post_save +from django.db.models.signals import post_save, pre_save from django.test import Client, override_settings from django.test.utils import CaptureQueriesContext from django.urls import NoReverseMatch, reverse @@ -9,7 +9,12 @@ from core.tables import ObjectChangeTable from netbox_scripts.models import NetBoxScript, ScriptFile, ScriptProject from netbox_scripts.permissions import SELECTION_REFUSAL -from netbox_scripts.tests.plugin_testing import ObjectPermissionTestMixin, PluginTestCases, refresh_count +from netbox_scripts.tests.plugin_testing import ( + ObjectPermissionTestMixin, + PluginTestCases, + create_scripts_out_of_default_order, + refresh_count, +) from users.models import ObjectPermission from utilities.testing import TestCase, create_tags, post_data @@ -80,6 +85,34 @@ def publish_between(sender, instance, **kwargs): self.assertEqual(clobbered.display_name, 'Renamed By Sync') self.assertFalse(clobbered.enabled) + def test_a_bulk_edit_saves_in_primary_key_order(self): + pks = create_scripts_out_of_default_order() + saved = [] + + def record_save(sender, instance, **kwargs): + saved.append(instance.pk) + + pre_save.connect(record_save, sender=NetBoxScript, dispatch_uid='bulk-edit-save-order') + self.addCleanup(pre_save.disconnect, record_save, sender=NetBoxScript, dispatch_uid='bulk-edit-save-order') + self.add_permissions('netbox_scripts.view_netboxscript', 'netbox_scripts.change_netboxscript') + + response = self.client.post( + self._get_url('bulk_edit'), + data={'pk': pks[::-1], '_apply': True, **post_data({'enabled': False})}, + ) + + self.assertHttpStatus(response, 302) + self.assertEqual(saved, pks) + + def test_the_bulk_edit_confirmation_preserves_default_order(self): + pks = create_scripts_out_of_default_order() + self.add_permissions('netbox_scripts.view_netboxscript', 'netbox_scripts.change_netboxscript') + + response = self.client.post(self._get_url('bulk_edit'), data={'pk': pks}) + + self.assertHttpStatus(response, 200) + self.assertEqual([row.record.pk for row in response.context['table'].rows], [pks[1], pks[2], pks[0]]) + class NetBoxScriptViewTestCase(TestCase): """The detail view and the change-log rendering that reverses a Script's URL.""" diff --git a/netbox_scripts/views/scripts.py b/netbox_scripts/views/scripts.py index 3d6eb41..21b45f8 100644 --- a/netbox_scripts/views/scripts.py +++ b/netbox_scripts/views/scripts.py @@ -95,6 +95,11 @@ class NetBoxScriptBulkEditView(generic.BulkEditView): table = NetBoxScriptTable form = NetBoxScriptBulkEditForm + def get_queryset(self, request): + """Return Scripts in primary-key order when a bulk edit is applied, to match synchronization.""" + queryset = super().get_queryset(request) + return queryset.order_by('pk') if '_apply' in request.POST else queryset + @register_model_view(NetBoxScript, 'run', path='run') class NetBoxScriptRunView(generic.ObjectView): diff --git a/scripts/check_netbox_internals.py b/scripts/check_netbox_internals.py index d0eb2be..2fbf1c5 100644 --- a/scripts/check_netbox_internals.py +++ b/scripts/check_netbox_internals.py @@ -45,12 +45,17 @@ 'netbox.api.viewsets:NetBoxModelViewSet.perform_update', 'netbox.api.viewsets:NetBoxModelViewSet.perform_destroy', ), + ( + 'netbox.api.viewsets.mixins.BulkUpdateModelMixin.get_bulk_update_queryset', + 'netbox.api.viewsets.mixins:BulkUpdateModelMixin.get_bulk_update_queryset', + ), ('utilities.exceptions.PermissionsViolation', 'utilities.exceptions:PermissionsViolation'), ('utilities.exceptions.AbortRequest', 'utilities.exceptions:AbortRequest'), ( 'netbox.views.generic.BulkEditView.pre_save_operations', 'netbox.views.generic:BulkEditView.pre_save_operations', ), + ('netbox.views.generic.BulkEditView.get_queryset', 'netbox.views.generic:BulkEditView.get_queryset'), ('utilities.data.normalize_update_fields', 'utilities.data:normalize_update_fields'), ('utilities.rqworker.get_queue_for_model', 'utilities.rqworker:get_queue_for_model'), ('rq.utils.parse_timeout', 'rq.utils:parse_timeout'),