inventree/InvenTree
2
0
Fork 0
mirror of https://github.com/inventree/InvenTree.git synced 2026-08-22 04:55:13 +00:00
Code Activity

Fix 500 errors for transitions (#12678)

Browse Source
This commit is contained in:
Oliver
2026-08-22 00:51:51 +10:00
committed by GitHub
parent 6ca7160723
commit a435738507
4 changed files with 253 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/backend/InvenTree/build/api.py
+15
View File
@@ -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):
src/backend/InvenTree/build/test_api.py
+43
View File
@@ -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)
src/backend/InvenTree/order/api.py
+108 -15
View File
@@ -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."""
src/backend/InvenTree/order/test_api.py
+87
View File
@@ -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)