Handle AbortRequest to ensure error details are returned
This commit is contained in:
Jeremy Stretch
parent
e17227d67d
commit
f68c4f63a7
|
|
@ -1,7 +1,7 @@
|
|||
import json
|
||||
|
||||
from django.conf import settings
|
||||
from django.test import tag
|
||||
from django.test import override_settings, tag
|
||||
from django.urls import reverse
|
||||
from django.utils.translation import gettext as _
|
||||
from rest_framework import status
|
||||
|
|
@ -553,6 +553,73 @@ class SiteTestCase(APIViewTestCases.APIViewTestCase):
|
|||
self.assertTrue(Site.objects.filter(pk=site1.pk).exists(), 'Site 1 should not have been deleted')
|
||||
self.assertTrue(Site.objects.filter(pk=site2.pk).exists(), 'Site 2 should not have been deleted')
|
||||
|
||||
def test_bulk_update_objects_abort_request(self):
|
||||
"""
|
||||
PATCH a set of objects where a signal receiver raises AbortRequest for more than one of
|
||||
them. Each failure must be correlated to its own object (proving the batch continues past
|
||||
the first abort) and no object may be modified.
|
||||
"""
|
||||
# This tag may only be assigned to Regions, so assigning it to a Site raises AbortRequest
|
||||
# from extras.signals.validate_assigned_tags.
|
||||
restricted_tag = Tag.objects.create(name='Regions Only', slug='regions-only')
|
||||
restricted_tag.object_types.set([ObjectType.objects.get_for_model(Region)])
|
||||
|
||||
site1 = Site.objects.get(slug='site-1')
|
||||
site2 = Site.objects.get(slug='site-2')
|
||||
|
||||
obj_perm = ObjectPermission(name='Test permission', actions=['change'])
|
||||
obj_perm.save()
|
||||
obj_perm.users.add(self.user)
|
||||
obj_perm.object_types.add(ObjectType.objects.get_for_model(self.model))
|
||||
self.add_permissions('extras.view_tag')
|
||||
|
||||
data = [
|
||||
{'id': site1.pk, 'tags': [{'name': 'Regions Only'}]},
|
||||
{'id': site2.pk, 'tags': [{'name': 'Regions Only'}]},
|
||||
]
|
||||
response = self.client.patch(self._get_list_url(), data, format='json', **self.header)
|
||||
|
||||
self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
|
||||
self.assertIn('detail', response.data)
|
||||
self.assertEqual([e['id'] for e in response.data['errors']], [site1.pk, site2.pk])
|
||||
for error in response.data['errors']:
|
||||
# Reported as a non-field error, in the same list-of-messages form as field errors
|
||||
self.assertIsInstance(error['errors']['__all__'], list)
|
||||
|
||||
# Neither site may have been tagged (whole batch rolled back)
|
||||
self.assertFalse(site1.tags.exists(), 'Site 1 should not have been tagged')
|
||||
self.assertFalse(site2.tags.exists(), 'Site 2 should not have been tagged')
|
||||
|
||||
def test_bulk_delete_objects_abort_request(self):
|
||||
"""
|
||||
DELETE a set of objects where a protection rule blocks more than one of them. Each failure
|
||||
must be correlated to its own object and no object may be deleted.
|
||||
"""
|
||||
site1 = Site.objects.get(slug='site-1')
|
||||
site2 = Site.objects.get(slug='site-2')
|
||||
|
||||
obj_perm = ObjectPermission(name='Test permission', actions=['delete'])
|
||||
obj_perm.save()
|
||||
obj_perm.users.add(self.user)
|
||||
obj_perm.object_types.add(ObjectType.objects.get_for_model(self.model))
|
||||
|
||||
# Neither site has a description, so the rule blocks both deletions via AbortRequest raised
|
||||
# from core.signals.handle_deleted_object.
|
||||
protection_rules = {'dcim.site': [{'description': {'required': True}}]}
|
||||
data = [{'id': site1.pk}, {'id': site2.pk}]
|
||||
with override_settings(PROTECTION_RULES=protection_rules):
|
||||
response = self.client.delete(self._get_list_url(), data, format='json', **self.header)
|
||||
|
||||
self.assertHttpStatus(response, status.HTTP_409_CONFLICT)
|
||||
self.assertIn('detail', response.data)
|
||||
self.assertEqual([e['id'] for e in response.data['errors']], [site1.pk, site2.pk])
|
||||
for error in response.data['errors']:
|
||||
self.assertIsInstance(error['errors']['__all__'], list)
|
||||
|
||||
# Neither site may have been deleted
|
||||
self.assertTrue(Site.objects.filter(pk=site1.pk).exists(), 'Site 1 should not have been deleted')
|
||||
self.assertTrue(Site.objects.filter(pk=site2.pk).exists(), 'Site 2 should not have been deleted')
|
||||
|
||||
|
||||
class LocationTestCase(APIViewTestCases.APIViewTestCase):
|
||||
model = Location
|
||||
|
|
@ -2347,6 +2414,41 @@ class DeviceTestCase(APIViewTestCases.APIViewTestCase):
|
|||
self.assertEqual(response.data['errors'][0]['index'], 1)
|
||||
self.assertIn('errors', response.data['errors'][0])
|
||||
|
||||
def test_bulk_create_objects_abort_request(self):
|
||||
"""
|
||||
POST a set of Device objects where a signal receiver raises AbortRequest for more than one
|
||||
of them. Each failure must be correlated to its position in the request (proving the batch
|
||||
continues past the first abort) and no object may be created.
|
||||
"""
|
||||
# This tag may only be assigned to Regions, so assigning it to a Device raises AbortRequest
|
||||
# from extras.signals.validate_assigned_tags.
|
||||
restricted_tag = Tag.objects.create(name='Regions Only', slug='regions-only')
|
||||
restricted_tag.object_types.set([ObjectType.objects.get_for_model(Region)])
|
||||
|
||||
obj_perm = ObjectPermission(name='Test permission', actions=['add'])
|
||||
obj_perm.save()
|
||||
obj_perm.users.add(self.user)
|
||||
obj_perm.object_types.add(ObjectType.objects.get_for_model(self.model))
|
||||
self.add_permissions('extras.view_tag')
|
||||
|
||||
initial_count = self._get_queryset().count()
|
||||
data = [
|
||||
{**self.create_data[0], 'tags': [{'name': 'Regions Only'}]},
|
||||
{**self.create_data[1], 'tags': [{'name': 'Regions Only'}]},
|
||||
]
|
||||
response = self.client.post(self._get_list_url(), data, format='json', **self.header)
|
||||
|
||||
self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
|
||||
self.assertIn('detail', response.data)
|
||||
self.assertEqual([e['index'] for e in response.data['errors']], [0, 1])
|
||||
for error in response.data['errors']:
|
||||
self.assertIsInstance(error['errors']['__all__'], list)
|
||||
|
||||
self.assertEqual(
|
||||
self._get_queryset().count(), initial_count,
|
||||
'No objects should be created when any sibling is aborted',
|
||||
)
|
||||
|
||||
|
||||
class ModuleTestCase(APIViewTestCases.APIViewTestCase):
|
||||
model = Module
|
||||
|
|
|
|||
|
|
@ -599,9 +599,9 @@ class EventRuleTestCase(RQQueueTestMixin, APITestCase):
|
|||
|
||||
def test_bulk_delete_abort_discards_events(self):
|
||||
"""
|
||||
Check that a bulk delete aborted by an exception (rather than by a per-object error) also
|
||||
queues no background tasks. A protection rule raises AbortRequest from a signal receiver,
|
||||
which propagates out of the per-object loop.
|
||||
Check that a bulk delete blocked by a signal receiver raising AbortRequest (rather than by a
|
||||
database constraint) also queues no background tasks for the objects that were provisionally
|
||||
deleted before the failure.
|
||||
"""
|
||||
sites = (
|
||||
Site(name='Site 1', slug='site-1', description='Has a description'),
|
||||
|
|
@ -617,9 +617,12 @@ class EventRuleTestCase(RQQueueTestMixin, APITestCase):
|
|||
protection_rules = {'dcim.site': [{'description': {'required': True}}]}
|
||||
with override_settings(PROTECTION_RULES=protection_rules):
|
||||
response = self.client.delete(url, data, format='json', **self.header)
|
||||
self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
|
||||
self.assertHttpStatus(response, status.HTTP_409_CONFLICT)
|
||||
self.assertEqual(Site.objects.count(), 2)
|
||||
|
||||
# The failure is correlated to the blocked object only
|
||||
self.assertEqual([e['id'] for e in response.data['errors']], [sites[1].pk])
|
||||
|
||||
# No task may be queued for a deletion that was rolled back
|
||||
self.assertEqual(self.queue.count, 0)
|
||||
|
||||
|
|
|
|||
|
|
@ -16,7 +16,7 @@ from extras.models import ExportTemplate
|
|||
from netbox.api.serializers import BulkOperationSerializer
|
||||
from netbox.api.serializers.bulk import get_bulk_update_serializer_class
|
||||
from netbox.jobs import AsyncAPIJob
|
||||
from utilities.exceptions import RQWorkerNotRunningException
|
||||
from utilities.exceptions import AbortRequest, RQWorkerNotRunningException
|
||||
from utilities.request import copy_safe_request
|
||||
from utilities.rqworker import any_workers_for_queue
|
||||
|
||||
|
|
@ -260,14 +260,18 @@ class SequentialBulkCreatesMixin:
|
|||
total = len(request.data)
|
||||
for i, data in enumerate(request.data):
|
||||
serializer = self.get_serializer(data=data)
|
||||
if serializer.is_valid():
|
||||
if not serializer.is_valid():
|
||||
errors.append({'index': i, 'errors': serializer.errors})
|
||||
continue
|
||||
try:
|
||||
# Provisionally create even when a prior item failed, so subsequent
|
||||
# cross-object validators (e.g. rack space checks) see a realistic state.
|
||||
# All creates are rolled back together if any item in the batch fails.
|
||||
self.perform_create(serializer)
|
||||
return_data.append(serializer.data)
|
||||
except AbortRequest as e:
|
||||
errors.append({'index': i, 'errors': {'__all__': [str(e.message)]}})
|
||||
else:
|
||||
errors.append({'index': i, 'errors': serializer.errors})
|
||||
return_data.append(serializer.data)
|
||||
|
||||
if errors:
|
||||
transaction.set_rollback(True)
|
||||
|
|
@ -370,11 +374,20 @@ class BulkUpdateModelMixin:
|
|||
if hasattr(obj, 'snapshot'):
|
||||
obj.snapshot()
|
||||
serializer = self.get_serializer(obj, data=data, partial=partial)
|
||||
if serializer.is_valid():
|
||||
self.perform_update(serializer)
|
||||
updated_pks.append(obj.pk)
|
||||
else:
|
||||
if not serializer.is_valid():
|
||||
errors.append({'id': obj.pk, 'errors': serializer.errors})
|
||||
continue
|
||||
try:
|
||||
self.perform_update(serializer)
|
||||
except AbortRequest as e:
|
||||
# Raised by a signal receiver rather than by validation (e.g. assigning a tag
|
||||
# which is restricted to other object types). perform_update() wraps its write
|
||||
# in its own atomic block, so the connection is rolled back to that savepoint
|
||||
# and the remaining objects in the batch can still be evaluated. The message is
|
||||
# coerced to a string because a few receivers pass an exception rather than text.
|
||||
errors.append({'id': obj.pk, 'errors': {'__all__': [str(e.message)]}})
|
||||
else:
|
||||
updated_pks.append(obj.pk)
|
||||
if errors:
|
||||
transaction.set_rollback(True)
|
||||
return updated_pks, errors
|
||||
|
|
@ -479,11 +492,20 @@ class BulkDestroyModelMixin:
|
|||
errors.append({
|
||||
'id': pk,
|
||||
'errors': {
|
||||
'__all__': _(
|
||||
'Unable to delete: {n} dependent object(s) prevent deletion.'
|
||||
).format(n=len(protected)),
|
||||
'__all__': [
|
||||
_('Unable to delete: {n} dependent object(s) prevent deletion.').format(
|
||||
n=len(protected)
|
||||
),
|
||||
],
|
||||
},
|
||||
})
|
||||
except AbortRequest as e:
|
||||
# Raised by a signal receiver rather than by a database constraint (e.g. a
|
||||
# PROTECTION_RULES violation caught in core.signals.handle_deleted_object).
|
||||
# perform_destroy() wraps its delete in its own atomic block, so the connection
|
||||
# is rolled back to that savepoint and the remaining objects in the batch can
|
||||
# still be evaluated.
|
||||
errors.append({'id': pk, 'errors': {'__all__': [str(e.message)]}})
|
||||
if errors:
|
||||
transaction.set_rollback(True)
|
||||
return errors, total
|
||||
|
|
|
|||
Loading…
Reference in New Issue