[bug] Build line fix (#12892)

* Fix BuildLineSerializer

- use SubquerySum not Sum

* Add regression test

* Adjust existing API test thresholds

* Refactor assertion message in test_api.py

Simplified assertion message for GROUP BY check in count queries.

* Fix assertion message in count query test
This commit is contained in:
Oliver
2026-09-20 23:41:09 +10:00
committed by GitHub
parent 10422dc37b
commit ce52e57fba
2 changed files with 71 additions and 13 deletions
+4 -2
View File
@@ -13,7 +13,6 @@ from django.db.models import (
F, F,
FloatField, FloatField,
Q, Q,
Sum,
Value, Value,
When, When,
) )
@@ -22,6 +21,7 @@ from django.utils.translation import gettext_lazy as _
from rest_framework import serializers from rest_framework import serializers
from rest_framework.serializers import ValidationError from rest_framework.serializers import ValidationError
from sql_util.utils import SubquerySum
import common.filters import common.filters
import company.serializers import company.serializers
@@ -1574,7 +1574,9 @@ class BuildLineSerializer(
# Annotate the "allocated" quantity # Annotate the "allocated" quantity
queryset = queryset.annotate( queryset = queryset.annotate(
allocated=Coalesce( allocated=Coalesce(
Sum('allocations__quantity'), 0, output_field=models.DecimalField() SubquerySum('allocations__quantity'),
0,
output_field=models.DecimalField(),
) )
) )
+67 -11
View File
@@ -3,7 +3,9 @@
from datetime import datetime, timedelta from datetime import datetime, timedelta
from typing import Optional from typing import Optional
from django.db import connection
from django.db.models import Sum from django.db.models import Sum
from django.test.utils import CaptureQueriesContext
from django.urls import reverse from django.urls import reverse
from django_q.models import OrmQ from django_q.models import OrmQ
@@ -1858,14 +1860,11 @@ class BuildLineTests(BuildAPITest):
self.assertEqual(len(response.data), BuildLine.objects.count()) self.assertEqual(len(response.data), BuildLine.objects.count())
# Filter by 'available' status # Filter by 'available' status
# Note: The max_query_time is bumped up here, as postgresql backend has some strange issues (only during testing) response = self.get(url, data={'available': True}, max_query_time=10)
# TODO: This needs to be addressed in the future, as 25 seconds is an unacceptably long time for a query to take in testing
response = self.get(url, data={'available': True}, max_query_time=25)
n_t = len(response.data) n_t = len(response.data)
self.assertGreater(n_t, 0) self.assertGreater(n_t, 0)
# Note: The max_query_time is bumped up here, as postgresql backend has some strange issues (only during testing) response = self.get(url, data={'available': False}, max_query_time=10)
response = self.get(url, data={'available': False}, max_query_time=25)
n_f = len(response.data) n_f = len(response.data)
self.assertGreater(n_f, 0) self.assertGreater(n_f, 0)
@@ -2189,24 +2188,81 @@ class BuildLineTests(BuildAPITest):
for line in lines: for line in lines:
StockItem.objects.create(part=line.bom_item.sub_part, quantity=60) StockItem.objects.create(part=line.bom_item.sub_part, quantity=60)
# TODO: 2025-10-02: Work out why this query takes so long with PostgreSQL (in CI)
# Note: The max_query_time is bumped up here, as postgresql backend has some strange issues (only during testing)
response = self.get( response = self.get(
url, {'build': build.pk, 'available': True}, max_query_time=30 url, {'build': build.pk, 'available': True}, max_query_time=10
) )
# We expect 2 lines to have "available" stock # We expect 2 lines to have "available" stock
self.assertEqual(len(response.data), 2) self.assertEqual(len(response.data), 2)
# TODO: 2025-10-02: Work out why this query takes so long with PostgreSQL (in CI)
# Note: The max_query_time is bumped up here, as postgresql backend has some strange issues (only during testing)
response = self.get( response = self.get(
url, {'build': build.pk, 'available': False}, max_query_time=30 url, {'build': build.pk, 'available': False}, max_query_time=10
) )
self.assertEqual(len(response.data), 1) self.assertEqual(len(response.data), 1)
self.assertEqual(response.data[0]['pk'], lines[0].pk) self.assertEqual(response.data[0]['pk'], lines[0].pk)
def test_list_unfiltered_count_query_has_no_group_by(self):
"""Regression test for a bug where the (unfiltered) BuildLine list endpoint was catastrophically slow.
The specific trigger: a plain GET against the list endpoint with no 'build'
filter (BuildLineMixin.get_source_build() then returns None, so
BuildLineSerializer.annotate_queryset() runs across *every* BuildLine in the
database, unscoped) - e.g. an external API client just paging through results.
"""
# Build line with a genuine stock allocation, to also confirm the 'allocated'
# value itself is still computed correctly after the Sum -> SubquerySum swap.
assembly = Part.objects.create(
name='Regression Test Assembly',
description='Assembly for BuildLine count() regression test',
assembly=True,
)
component = Part.objects.create(
name='Regression Test Component',
description='Component for BuildLine count() regression test',
component=True,
)
BomItem.objects.create(part=assembly, sub_part=component, quantity=1)
build = Build.objects.create(
part=assembly,
reference='BO-9996',
quantity=1,
title='BuildLine count() regression build',
)
line = build.build_lines.first()
stock_item = StockItem.objects.create(part=component, quantity=10)
BuildItem.objects.create(build_line=line, stock_item=stock_item, quantity=1)
url = reverse('api-build-line-list')
# Deliberately *no* 'build' filter - see docstring above
with CaptureQueriesContext(connection) as ctx:
response = self.get(url, {'limit': 1}, expected_code=200)
self.assertEqual(response.data['count'], BuildLine.objects.count())
count_queries = [
q
for q in ctx.captured_queries
if 'build_buildline' in q['sql'].lower()
and 'select count(' in q['sql'].lower()
]
self.assertTrue(count_queries, 'Expected a COUNT query for list pagination')
for query in count_queries:
self.assertNotIn(
'GROUP BY',
query['sql'].upper(),
'BuildLine count() query should not require a GROUP BY',
)
# Confirm the 'allocated' value is still computed correctly
response = self.get(url, {'build': build.pk}, expected_code=200)
line_data = next(item for item in response.data if item['pk'] == line.pk)
self.assertEqual(line_data['allocated'], 1)
class BuildConsumeTest(BuildAPITest): class BuildConsumeTest(BuildAPITest):
"""Test consuming allocated stock.""" """Test consuming allocated stock."""