From a435738507c1e0417cdcc362052d36775c22d136 Mon Sep 17 00:00:00 2001 From: Oliver Date: Sat, 22 Aug 2026 00:51:51 +1000 Subject: [PATCH] Fix 500 errors for transitions (#12678) --- src/backend/InvenTree/build/api.py | 15 +++ src/backend/InvenTree/build/test_api.py | 43 +++++++++ src/backend/InvenTree/order/api.py | 123 +++++++++++++++++++++--- src/backend/InvenTree/order/test_api.py | 87 +++++++++++++++++ 4 files changed, 253 insertions(+), 15 deletions(-) diff --git a/src/backend/InvenTree/build/api.py b/src/backend/InvenTree/build/api.py index a50d062720..41328bf67b 100644 --- a/src/backend/InvenTree/build/api.py +++ b/src/backend/InvenTree/build/api.py @@ -742,10 +742,25 @@ class BuildOrderContextMixin: try: ctx['build'] = self.get_build() except NotFound: + # Swallowed here (e.g. schema generation may call this without a + # resolvable pk) - create() below is what actually enforces a 404 + # for a real request against a non-existent build. pass return ctx + def create(self, request, *args, **kwargs): + """Ensure the target Build actually exists before attempting the action. + + Without this, a POST against a non-existent pk would fall through to the + action serializer's save(), which unconditionally reads + self.context['build'] - raising an unhandled KeyError (HTTP 500) instead of + the intended 404. + """ + self.get_build() + + return super().create(request, *args, **kwargs) + @extend_schema(responses={201: stock.serializers.StockItemSerializer(many=True)}) class BuildOutputCreate(BuildOrderContextMixin, CreateAPI): diff --git a/src/backend/InvenTree/build/test_api.py b/src/backend/InvenTree/build/test_api.py index 7b7685b0ba..c3ad180436 100644 --- a/src/backend/InvenTree/build/test_api.py +++ b/src/backend/InvenTree/build/test_api.py @@ -3033,3 +3033,46 @@ class BuildAutoAllocateAPITest(InvenTreeAPITestCase): self.assertEqual(fa.count(), 4) allocated = sum(a.quantity for a in fa) self.assertEqual(allocated, 130) # 130 allocated to each line + + +class BuildActionMissingPkTest(InvenTreeAPITestCase): + """Regression tests for a class of bugs in the Build action endpoints. + + BuildOrderContextMixin looks up the target Build in get_serializer_context(), but + silently swallows a not-found result (needed so schema/OPTIONS introspection + doesn't break). Without an explicit check elsewhere, a POST against a + non-existent pk fell through to the action serializer's save(), which + unconditionally reads self.context['build'] - an unhandled KeyError (HTTP 500) + rather than a clean 404. Fixed by BuildOrderContextMixin.create(). + """ + + roles = ['build.add'] + + def test_actions_404_for_missing_build(self): + """Every BuildOrderContextMixin-based action should 404, not 500, for a bad pk.""" + for url_name in [ + 'api-build-issue', + 'api-build-hold', + 'api-build-cancel', + 'api-build-finish', + 'api-build-allocate', + ]: + url = reverse(url_name, kwargs={'pk': 999999}) + self.post(url, {}, expected_code=404) + + def test_output_actions_already_safe(self): + """The build-output actions already guard against a missing build themselves. + + (BuildOutputScrap/Complete/Delete/AutoAllocate/Consume all call + self.get_build() explicitly at the top of a custom post() override, which + already raises NotFound correctly - this test just locks that in.) + """ + for url_name in [ + 'api-build-output-scrap', + 'api-build-output-complete', + 'api-build-output-delete', + 'api-build-auto-allocate', + 'api-build-consume', + ]: + url = reverse(url_name, kwargs={'pk': 999999}) + self.post(url, {}, expected_code=404) diff --git a/src/backend/InvenTree/order/api.py b/src/backend/InvenTree/order/api.py index 11596b6598..492ca7e551 100644 --- a/src/backend/InvenTree/order/api.py +++ b/src/backend/InvenTree/order/api.py @@ -444,16 +444,33 @@ class PurchaseOrderViewSet( queryset = serializers.PurchaseOrderSerializer.annotate_queryset(queryset) return queryset + def get_order(self): + """Return the PurchaseOrder object associated with this API endpoint. + + Note: deliberately a raw lookup rather than self.get_object() - the latter + routes through ParameterListMixin.filter_queryset(), which assumes + self.serializer_class.Meta.model exists. That's true for the default + PurchaseOrderSerializer, but not for the plain-Serializer action classes + (PurchaseOrderHoldSerializer etc.) used by hold/cancel/complete/issue/receive + below, so calling get_object() from those actions raises an unrelated + AttributeError instead of the intended 404. + """ + try: + return models.PurchaseOrder.objects.get(pk=self.kwargs.get('pk', None)) + except (ValueError, models.PurchaseOrder.DoesNotExist): + raise NotFound(_('Purchase order not found')) + def get_serializer_context(self): """Add the PurchaseOrder object to the serializer context.""" context = super().get_serializer_context() # Pass the purchase order through to the serializer for validation try: - context['order'] = models.PurchaseOrder.objects.get( - pk=self.kwargs.get('pk', None) - ) - except Exception: + context['order'] = self.get_order() + except NotFound: + # Swallowed here (e.g. schema generation may call this without a + # resolvable pk) - each action method below is what actually enforces + # a 404 for a real request against a non-existent order. pass context['request'] = self.request @@ -470,6 +487,12 @@ class PurchaseOrderViewSet( ) def hold(self, request, pk=None): """API endpoint to place a PurchaseOrder on hold.""" + # Ensure the target order actually exists (raises NotFound -> 404 otherwise) - + # without this, a non-existent pk would fall through to the serializer's + # save(), which unconditionally reads self.context['order'], raising an + # unhandled KeyError (HTTP 500) instead of a clean 404. + self.get_order() + serializer = self.get_serializer(data=request.data) serializer.is_valid(raise_exception=True) serializer.save() @@ -488,6 +511,8 @@ class PurchaseOrderViewSet( The purchase order must be in a state which can be cancelled """ + self.get_order() + serializer = self.get_serializer(data=request.data) serializer.is_valid(raise_exception=True) serializer.save() @@ -503,6 +528,8 @@ class PurchaseOrderViewSet( ) def complete(self, request, pk=None): """API endpoint to 'complete' a purchase order.""" + self.get_order() + serializer = self.get_serializer(data=request.data) serializer.is_valid(raise_exception=True) serializer.save() @@ -518,6 +545,8 @@ class PurchaseOrderViewSet( ) def issue(self, request, pk=None): """API endpoint to 'issue' (place) a PurchaseOrder.""" + self.get_order() + serializer = self.get_serializer(data=request.data) serializer.is_valid(raise_exception=True) serializer.save() @@ -534,6 +563,8 @@ class PurchaseOrderViewSet( ) def receive(self, request, pk=None): """API endpoint to receive stock items against a PurchaseOrder.""" + self.get_order() + serializer = self.get_serializer(data=request.data) serializer.is_valid(raise_exception=True) items = serializer.save() @@ -1159,6 +1190,13 @@ class SalesOrderContextMixin: queryset = models.SalesOrder.objects.all() + def get_order(self): + """Return the SalesOrder object associated with this API endpoint.""" + try: + return models.SalesOrder.objects.get(pk=self.kwargs.get('pk', None)) + except (ValueError, models.SalesOrder.DoesNotExist): + raise NotFound(_('Sales order not found')) + def get_serializer_context(self): """Add the 'order' reference to the serializer context for any classes which inherit this mixin.""" ctx = super().get_serializer_context() @@ -1166,12 +1204,27 @@ class SalesOrderContextMixin: ctx['request'] = self.request try: - ctx['order'] = models.SalesOrder.objects.get(pk=self.kwargs.get('pk', None)) - except Exception: + ctx['order'] = self.get_order() + except NotFound: + # Swallowed here (e.g. schema generation may call this without a + # resolvable pk) - create() below is what actually enforces a 404 + # for a real request against a non-existent order. pass return ctx + def create(self, request, *args, **kwargs): + """Ensure the target SalesOrder actually exists before attempting the action. + + Without this, a POST against a non-existent pk would fall through to the + action serializer's save(), which unconditionally reads + self.context['order'] - raising an unhandled KeyError (HTTP 500) instead of + the intended 404. + """ + self.get_order() + + return super().create(request, *args, **kwargs) + class SalesOrderHold(SalesOrderContextMixin, CreateAPI): """API endpoint to place a SalesOrder on hold.""" @@ -1716,22 +1769,42 @@ class ReturnOrderContextMixin: queryset = models.ReturnOrder.objects.all() + def get_order(self): + """Return the ReturnOrder object associated with this API endpoint.""" + try: + return models.ReturnOrder.objects.get(pk=self.kwargs.get('pk', None)) + except (ValueError, models.ReturnOrder.DoesNotExist): + raise NotFound(_('Return order not found')) + def get_serializer_context(self): - """Add the PurchaseOrder object to the serializer context.""" + """Add the ReturnOrder object to the serializer context.""" context = super().get_serializer_context() # Pass the ReturnOrder instance through to the serializer for validation try: - context['order'] = models.ReturnOrder.objects.get( - pk=self.kwargs.get('pk', None) - ) - except Exception: + context['order'] = self.get_order() + except NotFound: + # Swallowed here (e.g. schema generation may call this without a + # resolvable pk) - create() below is what actually enforces a 404 + # for a real request against a non-existent order. pass context['request'] = self.request return context + def create(self, request, *args, **kwargs): + """Ensure the target ReturnOrder actually exists before attempting the action. + + Without this, a POST against a non-existent pk would fall through to the + action serializer's save(), which unconditionally reads + self.context['order'] - raising an unhandled KeyError (HTTP 500) instead of + the intended 404. + """ + self.get_order() + + return super().create(request, *args, **kwargs) + class ReturnOrderCancel(ReturnOrderContextMixin, CreateAPI): """API endpoint to cancel a ReturnOrder.""" @@ -1999,22 +2072,42 @@ class TransferOrderContextMixin: queryset = models.TransferOrder.objects.all() + def get_order(self): + """Return the TransferOrder object associated with this API endpoint.""" + try: + return models.TransferOrder.objects.get(pk=self.kwargs.get('pk', None)) + except (ValueError, models.TransferOrder.DoesNotExist): + raise NotFound(_('Transfer order not found')) + def get_serializer_context(self): """Add the TransferOrder object to the serializer context.""" context = super().get_serializer_context() # Pass the Transfer instance through to the serializer for validation try: - context['order'] = models.TransferOrder.objects.get( - pk=self.kwargs.get('pk', None) - ) - except Exception: + context['order'] = self.get_order() + except NotFound: + # Swallowed here (e.g. schema generation may call this without a + # resolvable pk) - create() below is what actually enforces a 404 + # for a real request against a non-existent order. pass context['request'] = self.request return context + def create(self, request, *args, **kwargs): + """Ensure the target TransferOrder actually exists before attempting the action. + + Without this, a POST against a non-existent pk would fall through to the + action serializer's save(), which unconditionally reads + self.context['order'] - raising an unhandled KeyError (HTTP 500) instead of + the intended 404. + """ + self.get_order() + + return super().create(request, *args, **kwargs) + class TransferOrderCancel(TransferOrderContextMixin, CreateAPI): """API endpoint to cancel a TransferOrder.""" diff --git a/src/backend/InvenTree/order/test_api.py b/src/backend/InvenTree/order/test_api.py index 100b4263be..3f8d2e49e7 100644 --- a/src/backend/InvenTree/order/test_api.py +++ b/src/backend/InvenTree/order/test_api.py @@ -5494,3 +5494,90 @@ class SalesOrderAllocationBulkDeleteAPITest(InvenTreeAPITestCase): self.assertEqual( SalesOrderAllocation.objects.filter(pk__in=shipped_ids).count(), 2 ) + + +class OrderActionMissingPkTest(InvenTreeAPITestCase): + """Regression tests for a class of bugs in the order-app action endpoints. + + Each order type's *ContextMixin looks up the target order in + get_serializer_context(), but silently swallows a not-found result (needed so + schema/OPTIONS introspection doesn't break). Without an explicit check + elsewhere, a POST against a non-existent pk fell through to the action + serializer's save(), which unconditionally reads self.context['order'] - an + unhandled KeyError (HTTP 500) rather than a clean 404. + + Fixed by SalesOrderContextMixin/ReturnOrderContextMixin/TransferOrderContextMixin + .create(), and (since PurchaseOrderViewSet's actions are plain ViewSet @action + methods rather than CreateAPI subclasses) an explicit check at the top of each + PurchaseOrderViewSet action method. + """ + + roles = [ + 'purchase_order.add', + 'sales_order.add', + 'return_order.add', + 'transfer_order.add', + ] + + def test_purchase_order_actions_404(self): + """Each PurchaseOrderViewSet action should 404, not 500, for a bad pk. + + Note: PurchaseOrderViewSet.get_order() is a deliberate raw lookup rather + than self.get_object() - the latter routes through + ParameterListMixin.filter_queryset(), which assumes + self.serializer_class.Meta.model exists. That's true for the default + PurchaseOrderSerializer, but not for the plain-Serializer action classes + used here, so self.get_object() would raise an unrelated AttributeError. + """ + for url_name in [ + 'api-po-hold', + 'api-po-cancel', + 'api-po-complete', + 'api-po-issue', + 'api-po-receive', + ]: + url = reverse(url_name, kwargs={'pk': 999999}) + self.post(url, {}, expected_code=404) + + def test_sales_order_actions_404(self): + """Each SalesOrderContextMixin-based action should 404, not 500, for a bad pk.""" + for url_name in [ + 'api-so-hold', + 'api-so-cancel', + 'api-so-issue', + 'api-so-complete', + 'api-so-allocate', + 'api-so-allocate-serials', + ]: + url = reverse(url_name, kwargs={'pk': 999999}) + self.post(url, {}, expected_code=404) + + def test_sales_order_auto_allocate_already_safe(self): + """SalesOrderAutoAllocate overrides post() and already calls get_object() itself.""" + url = reverse('api-so-auto-allocate', kwargs={'pk': 999999}) + self.post(url, {}, expected_code=404) + + def test_return_order_actions_404(self): + """Each ReturnOrderContextMixin-based action should 404, not 500, for a bad pk.""" + for url_name in [ + 'api-return-order-cancel', + 'api-ro-hold', + 'api-return-order-complete', + 'api-return-order-issue', + 'api-return-order-receive', + ]: + url = reverse(url_name, kwargs={'pk': 999999}) + self.post(url, {}, expected_code=404) + + def test_transfer_order_actions_404(self): + """Each TransferOrderContextMixin-based action should 404, not 500, for a bad pk.""" + for url_name in [ + 'api-transfer-order-cancel', + 'api-transfer-order-hold', + 'api-transfer-order-complete', + 'api-transfer-order-issue', + 'api-transfer-order-allocate', + 'api-transfer-order-allocate-serials', + ]: + url = reverse(url_name, kwargs={'pk': 999999}) + self.post(url, {}, expected_code=404)