Repository: netbox-community/netbox
Pull request: 7503 do device validate-create in serial
<!--
    Thank you for your interest in contributing to NetBox! Please note that
    our contribution policy requires that a feature request or bug report be
    approved and assigned prior to opening a pull request. This helps avoid
    waste time and effort on a proposed change that we might not be able to
    accept.

    IF YOUR PULL REQUEST DOES NOT REFERENCE AN ISSUE WHICH HAS BEEN ASSIGNED
    TO YOU, IT WILL BE CLOSED AUTOMATICALLY.

    Please specify your assigned issue number on the line below.
-->
### Fixes: #7503 

<!--
    Please include a summary of the proposed changes below.
-->
Makes Device ViewSet to object validation and creation serial, instead of validating all at once then creating all at once - to catch conditions like overlapping rack locations.

Problem statement:
API Bulk-Create of Devices does not check Rack-Space in its own context
### NetBox version

v3.0.6

### Python version

3.9

### Steps to Reproduce

1. Create Rack with 42U
2. Create Device-Type with Height of 2U
3. POST /dcim/devices/:
```json
[
  {
    "device_role": 1,
    "device_type": <your_id>,
    "site": 1,
    "rack": <your_id>,
    "face": "front",
    "position": 1
  },
  {
    "device_role": 1,
    "device_type": <your_id>,
    "site": 1,
    "rack": <your_id>,
    "face": "front",
    "position": 2
  }
]
```


### Expected Behavior

A ValidationError with >400 HTTP Status, as both Devices overlap each other in the Rack.

### Observed Behavior

API returns 201 HTTP Status and both Devices are there.
Further Handling other than deleting them or moving them to another position is not possible.
This also breaks SVG-rendering of Racks:
```python
  File "/opt/netbox/netbox/dcim/api/views.py", line 218, in elevation
    drawing = rack.get_elevation_svg(
  File "/opt/netbox/netbox/dcim/models/racks.py", line 392, in get_elevation_svg
    return elevation.render(face, unit_width, unit_height, legend_width)
  File "/opt/netbox/netbox/dcim/svg.py", line 192, in render
    for unit in self.merge_elevations(face):
  File "/opt/netbox/netbox/dcim/svg.py", line 156, in merge_elevations
    elevation = self.rack.get_rack_units(face=face, expand_devices=False)
  File "/opt/netbox/netbox/dcim/models/racks.py", line 313, in get_rack_units
    elevation[device.position]['device'] = device
KeyError: 2
```


This Issue does not occur on API POST /dcim/devices/ with only one Device.
This Issue does not occur in Frontend-Import-View for Devices.
In both these circumstances the transaction is rolled back due to insufficient Space.

Hints from issue discussion:
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. NetBox is governed by a small group of core maintainers which means not all opened issues may receive direct feedback. Please see our [contributing guide](https://github.com/netbox-community/netbox/blob/develop/CONTRIBUTING.md).
We'll probably have to sit on this until we figure out what the implementation of #51 looks like.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. NetBox is governed by a small group of core maintainers which means not all opened issues may receive direct feedback. **Do not** attempt to circumvent this process by "bumping" the issue; doing so will result in its immediate closure and you may be barred from participating in any future discussions. Please see our [contributing guide](https://github.com/netbox-community/netbox/blob/develop/CONTRIBUTING.md).
Caught up with @arthanson on his approach for this in #10223. While valid, I worry that the approach in that PR is going to be difficult to maintain long-term. We identified an alternative approach which involves changing the order in which objects are validated and created within the REST API endpoint.

Rather than validating the new set of devices individually and then attempting to create them all (which can result in the above exception), we could create each device immediately upon its validation. This would mean device1 has already been created when device2 is being validated, which should result in the clean detection and reporting of any validation failures.

Commit message: 7503 add test

Patch to review:
diff --git a/netbox/dcim/api/views.py b/netbox/dcim/api/views.py
index a62315b5736..5d87d26a940 100644
--- a/netbox/dcim/api/views.py
+++ b/netbox/dcim/api/views.py
@@ -1,12 +1,14 @@
+from django.db import transaction
 from django.http import Http404, HttpResponse
 from django.shortcuts import get_object_or_404
-from drf_spectacular.utils import extend_schema, extend_schema_view, OpenApiParameter
 from drf_spectacular.types import OpenApiTypes
+from drf_spectacular.utils import extend_schema, OpenApiParameter
+from rest_framework import status
 from rest_framework.decorators import action
 from rest_framework.renderers import JSONRenderer
 from rest_framework.response import Response
-from rest_framework.status import HTTP_400_BAD_REQUEST
 from rest_framework.routers import APIRootView
+from rest_framework.status import HTTP_400_BAD_REQUEST
 from rest_framework.viewsets import ViewSet
 
 from circuits.models import Circuit
@@ -14,7 +16,6 @@
 from dcim.constants import CABLE_TRACE_SVG_DEFAULT_WIDTH
 from dcim.models import *
 from dcim.svg import CableTraceSVG
-from extras.api.nested_serializers import NestedConfigTemplateSerializer
 from extras.api.mixins import ConfigContextQuerySetMixin, ConfigTemplateRenderMixin
 from ipam.models import Prefix, VLAN
 from netbox.api.authentication import IsAuthenticatedOrLoginNotRequired
@@ -414,6 +415,25 @@ def get_serializer_class(self):
 
         return serializers.DeviceWithConfigContextSerializer
 
+    @transaction.atomic
+    def create(self, request, *args, **kwargs):
+        # do validate / create for each item in serial instead of validating all data at once
+        if is_bulk := isinstance(request.data, list):
+            return_data = []
+        data_list = request.data if is_bulk else [request.data, ]
+
+        for data in data_list:
+            serializer = self.get_serializer(data=data)
+            serializer.is_valid(raise_exception=True)
+            self.perform_create(serializer)
+            headers = self.get_success_headers(serializer.data)
+            if is_bulk:
+                return_data.append(serializer.data)
+            else:
+                return_data = serializer.data
+
+        return Response(return_data, status=status.HTTP_201_CREATED, headers=headers)
+
     @action(detail=True, methods=['post'], url_path='render-config', renderer_classes=[JSONRenderer, TextRenderer])
     def render_config(self, request, pk):
         """
diff --git a/netbox/dcim/tests/test_api.py b/netbox/dcim/tests/test_api.py
index 3445b7e7581..af15e134387 100644
--- a/netbox/dcim/tests/test_api.py
+++ b/netbox/dcim/tests/test_api.py
@@ -1115,7 +1115,7 @@ def setUpTestData(cls):
 
         device_types = (
             DeviceType(manufacturer=manufacturer, model='Device Type 1', slug='device-type-1'),
-            DeviceType(manufacturer=manufacturer, model='Device Type 2', slug='device-type-2'),
+            DeviceType(manufacturer=manufacturer, model='Device Type 2', slug='device-type-2', u_height=2),
         )
         DeviceType.objects.bulk_create(device_types)
 
@@ -1229,6 +1229,39 @@ def test_unique_name_per_site_constraint(self):
 
         self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
 
+    def test_rack_fit(self):
+        """
+        Check that creating multiple devices with overlapping position fails.
+        """
+        device = Device.objects.first()
+        device_type = DeviceType.objects.all()[1]
+        data = [
+            {
+                'device_type': device_type.pk,
+                'device_role': device.device_role.pk,
+                'site': device.site.pk,
+                'name': 'Test Device 7',
+                'rack': device.rack.pk,
+                'face': 'front',
+                'position': 1
+            },
+            {
+                'device_type': device_type.pk,
+                'device_role': device.device_role.pk,
+                'site': device.site.pk,
+                'name': 'Test Device 8',
+                'rack': device.rack.pk,
+                'face': 'front',
+                'position': 2
+            }
+        ]
+
+        self.add_permissions('dcim.add_device')
+        url = reverse('dcim-api:device-list')
+        response = self.client.post(url, data, format='json', **self.header)
+
+        self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
+
 
 class ModuleTest(APIViewTestCases.APIViewTestCase):
     model = Module


Code review comments:
diff_hunk: @@ -414,6 +415,25 @@ def get_serializer_class(self):
 
         return serializers.DeviceWithConfigContextSerializer
 
+    @transaction.atomic
+    def create(self, request, *args, **kwargs):
path: netbox/dcim/api/views.py
text: @user1:
IMO we should move this to a mixin class in case we need it for other views in the future.