From 6302748ec8eb882dc8cfa64376c2d5ab6c3f8449 Mon Sep 17 00:00:00 2001 From: Oliver Date: Tue, 28 Jul 2026 16:30:32 +1000 Subject: [PATCH] [bug] Unique constraint for serial numbers (#12483) * Unique constraint for serial numbers * Revert unrelated changes * Updated tests --- .../0126_serial_number_concurrency_guard.py | 34 ++ src/backend/InvenTree/stock/models.py | 99 ++++- .../InvenTree/stock/test_migrations.py | 2 +- src/backend/InvenTree/stock/tests.py | 391 +++++++++++++++++- 4 files changed, 519 insertions(+), 7 deletions(-) create mode 100644 src/backend/InvenTree/stock/migrations/0126_serial_number_concurrency_guard.py diff --git a/src/backend/InvenTree/stock/migrations/0126_serial_number_concurrency_guard.py b/src/backend/InvenTree/stock/migrations/0126_serial_number_concurrency_guard.py new file mode 100644 index 0000000000..e1f2295f45 --- /dev/null +++ b/src/backend/InvenTree/stock/migrations/0126_serial_number_concurrency_guard.py @@ -0,0 +1,34 @@ +# Generated by Django 5.2.16 on 2026-07-27 08:58 + +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("build", "0059_build_tags"), + ("company", "0080_company_tags"), + ("order", "0121_add_line_item_discount"), + ("part", "0152_alter_partpricing_currency"), + ("stock", "0125_remove_mptt_fields"), + ( + "taggit", + "0006_rename_taggeditem_content_type_object_id_taggit_tagg_content_8fc721_idx", + ), + ("users", "0015_alter_userprofile_type"), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.AddConstraint( + model_name="stockitem", + constraint=models.UniqueConstraint( + condition=models.Q( + ("serial__isnull", False), models.Q(("serial", ""), _negated=True) + ), + fields=("part", "serial"), + name="stock_item_unique_part_serial", + ), + ), + ] diff --git a/src/backend/InvenTree/stock/models.py b/src/backend/InvenTree/stock/models.py index 6898453b0e..4867363c02 100644 --- a/src/backend/InvenTree/stock/models.py +++ b/src/backend/InvenTree/stock/models.py @@ -13,7 +13,7 @@ from django.contrib.auth.models import User from django.core.exceptions import ValidationError from django.core.validators import MinValueValidator from django.db import models, transaction -from django.db.models import Q, QuerySet, Sum +from django.db.models import Q, QuerySet, Sum, UniqueConstraint from django.db.models.functions import Coalesce from django.db.models.signals import post_delete, post_save from django.db.utils import IntegrityError, OperationalError @@ -468,6 +468,23 @@ class StockItem( """Model meta options.""" verbose_name = _('Stock Item') + constraints = [ + # Baseline, database-enforced duplicate-serial guard: a given Part + # can never have two StockItem rows sharing the same serial number. + # This alone does not cover the "unique across a Part variant tree" + # (or globally-unique) semantics controlled by the + # SERIAL_NUMBER_GLOBALLY_UNIQUE setting - see + # StockItem._lock_serial_numbers() for that; a UniqueConstraint + # cannot reference the joined Part.tree_id field. + UniqueConstraint( + fields=['part', 'serial'], + # Non-serialized StockItems may store serial as either NULL or + # '' depending on the creation path - exclude both, since + # "no serial" is not a value this constraint should govern + condition=Q(serial__isnull=False) & ~Q(serial=''), + name='stock_item_unique_part_serial', + ) + ] class MPTTMeta: """MPTT metaclass options.""" @@ -655,7 +672,68 @@ class StockItem( & Q(expiry_date__lt=InvenTree.helpers.current_date()) ) + @staticmethod + def _lock_serial_numbers(part: PartModels.Part, serials: list) -> None: + """Serialize concurrent serial number creation, and re-validate under the lock. + + Must be called from within an atomic transaction. Serial number + uniqueness is scoped by the SERIAL_NUMBER_GLOBALLY_UNIQUE setting - + either across an entire Part variant tree (the default), or globally. + That scope depends on Part.tree_id, which is not a field on + StockItem, so it cannot be expressed as a database-level + UniqueConstraint on StockItem directly (a UniqueConstraint cannot + reference a joined field). Instead, this select_for_update()s + existing rows that already represent the relevant scope - every Part + in the tree, or (for the globally-unique case, where there is no + single tree to lock) the SERIAL_NUMBER_GLOBALLY_UNIQUE setting's own + row - so concurrent creation attempts within the same scope + serialize against each other. It then re-checks for conflicts + against StockItem while holding that lock. This closes the race + where two concurrent requests both read "no conflict" before either + has committed its creation. + + Raises: + ValidationError: If any of the provided serial numbers now conflict + """ + if get_global_setting('SERIAL_NUMBER_GLOBALLY_UNIQUE', False): + # There is no single Part tree covering "globally unique" - lock + # the setting's own row instead, so all concurrent global-scope + # creation attempts serialize against each other regardless of + # which part is involved. This row is guaranteed to already + # exist: reaching this branch means the setting is currently + # True, which can only happen if it was explicitly set (and + # therefore persisted) at some point. + setting, _created = common.models.InvenTreeSetting.objects.get_or_create( + key='SERIAL_NUMBER_GLOBALLY_UNIQUE', defaults={'value': str(True)} + ) + common.models.InvenTreeSetting.objects.select_for_update().get( + pk=setting.pk + ) + else: + # Lock every Part in this variant tree (this always includes + # 'part' itself), so concurrent creation attempts for any part in + # the same tree serialize against each other + list( + PartModels.Part.objects + .select_for_update() + .filter(tree_id=part.tree_id) + .order_by('pk') + ) + + # Re-validate for conflicts now that the lock is held - any + # concurrent request for the same scope has either already committed + # (and will now show up here) or is blocked behind this lock (and + # will see this request's result once it releases) + conflicts = part.find_conflicting_serial_numbers(serials) + + if conflicts: + msg = _('The following serial numbers already exist or are invalid') + msg += ' : ' + msg += ','.join(str(x) for x in conflicts) + raise ValidationError({'serial_numbers': msg}) + @classmethod + @transaction.atomic def _create_serial_numbers(cls, serials: list, **kwargs) -> QuerySet: """Create multiple stock items with the provided serial numbers. @@ -672,8 +750,11 @@ class StockItem( This method uses bulk_create to create multiple StockItem objects in a single query, which is much more efficient than creating them one-by-one. - However, it does not perform any validation checks on the provided serial numbers, - and also does not generate any "stock tracking entries". + Concurrent calls for an overlapping set of serial numbers are + serialized against each other (see _lock_serial_numbers()), so unlike + other validation checks, this method's duplicate-serial protection is + safe even when the caller's own pre-check raced against another + request. It does not generate any "stock tracking entries". Note: This is an 'internal' function and should not be used by external code / plugins. """ @@ -722,6 +803,10 @@ class StockItem( if 'part' not in data: raise ValidationError({'part': _('Part must be specified')}) + # Serialize against any other concurrent request creating an + # overlapping set of serial numbers, and re-validate under that lock + cls._lock_serial_numbers(data['part'], serials) + parent = kwargs.pop('parent', None) or data.get('parent') data['parent'] = parent @@ -741,7 +826,13 @@ class StockItem( items.append(StockItem(**data)) # Create the StockItem objects in bulk - items = bulk_create_and_fetch(StockItem, items) + # (the IntegrityError catch is a defense-in-depth backstop against + # the database-level UniqueConstraint - _lock_serial_numbers() above + # should already have ruled out any conflict) + try: + items = bulk_create_and_fetch(StockItem, items) + except IntegrityError as exc: + raise ValidationError({'serial_numbers': str(exc)}) from exc # Trigger a 'created' event for the new items # Note that instead of a single event for each item, diff --git a/src/backend/InvenTree/stock/test_migrations.py b/src/backend/InvenTree/stock/test_migrations.py index f2269a54a3..c5372b7204 100644 --- a/src/backend/InvenTree/stock/test_migrations.py +++ b/src/backend/InvenTree/stock/test_migrations.py @@ -189,7 +189,7 @@ class TestTestResultMigration(MigratorTestCase): for jj in range(4): si = StockItem.objects.create( part=child, - serial=str(1 + ii * jj), + serial=str(1 + jj), quantity=1, tree_id=0, level=0, diff --git a/src/backend/InvenTree/stock/tests.py b/src/backend/InvenTree/stock/tests.py index edc2b620c3..fe8d3926bb 100644 --- a/src/backend/InvenTree/stock/tests.py +++ b/src/backend/InvenTree/stock/tests.py @@ -1,14 +1,19 @@ """Tests for stock app.""" import datetime +import threading +from unittest import mock +from django.contrib.auth import get_user_model from django.core.exceptions import ValidationError -from django.db import transaction +from django.db import connection, transaction from django.db.models import Sum -from django.test import override_settings +from django.test import TransactionTestCase, override_settings, skipUnlessDBFeature +from django.urls import reverse from django_q.models import OrmQ from djmoney.money import Money +from rest_framework.test import APIClient from build.models import Build from common.models import InvenTreeSetting @@ -1966,3 +1971,385 @@ class AdminTest(AdminTestCase): def test_admin(self): """Test the admin URL.""" self.helper(model=StockLocationType) + + +@skipUnlessDBFeature('has_select_for_update') +class StockItemSerialConcurrencyTest(TransactionTestCase): + """Genuine cross-transaction regression test for StockItem._create_serial_numbers(). + + Uses two real threads (each with its own database connection) to reproduce + the reported race: two concurrent requests to create a StockItem with the + *same* serial number for the same Part could both check for conflicts + before either had committed its bulk_create - producing two StockItem rows + sharing one (part, serial) pair. + + _create_serial_numbers() now locks (select_for_update, on the Part rows + covering the relevant scope - see StockItem._lock_serial_numbers()) and + re-validates for conflicts under that lock before creating anything, and + a database-level UniqueConstraint on (part, serial) backstops it, so only + one of two concurrent requests for the same serial number may succeed. + """ + + fixtures = ['users'] + + def setUp(self): + """Create a single trackable Part to create serialized stock against.""" + super().setUp() + + self.part = Part.objects.create( + name='Concurrency serial part', + description='Part for serial creation concurrency test', + trackable=True, + ) + + def test_concurrent_creation_does_not_duplicate_serial(self): + """Two concurrent requests for the same serial number must not both succeed.""" + start_barrier = threading.Barrier(2, timeout=5) + errors = [] + results = [] + + # Wrap StockItem._lock_serial_numbers() so both threads reach the + # (real, database-level) row lock at the same time - one wins the + # lock and proceeds, the other blocks until the winner's transaction + # completes. + original_lock_serial_numbers = StockItem._lock_serial_numbers + + def synced_lock_serial_numbers(part, serials): + start_barrier.wait(timeout=5) + return original_lock_serial_numbers(part, serials) + + def create(): + try: + StockItem._create_serial_numbers(['SN-RACE'], part=self.part) + results.append('ok') + except ValidationError: + results.append('rejected') + except Exception as exc: # pragma: no cover - surfaced via errors list + errors.append(exc) + finally: + connection.close() + + thread_a = threading.Thread(target=create) + thread_b = threading.Thread(target=create) + + with mock.patch.object( + StockItem, '_lock_serial_numbers', synced_lock_serial_numbers + ): + thread_a.start() + thread_b.start() + + thread_a.join(timeout=5) + thread_b.join(timeout=5) + + self.assertFalse(thread_a.is_alive()) + self.assertFalse(thread_b.is_alive()) + self.assertEqual(errors, []) + + # Exactly one request must have been rejected as a duplicate + self.assertEqual(sorted(results), ['ok', 'rejected']) + + # The serial number must only have been created once + self.assertEqual( + StockItem.objects.filter(part=self.part, serial='SN-RACE').count(), 1 + ) + + +@skipUnlessDBFeature('has_select_for_update') +class StockItemSerialBatchConcurrencyTest(TransactionTestCase): + """Genuine cross-transaction regression test for overlapping batch serial creation. + + Extends the single-serial race in StockItemSerialConcurrencyTest to the + more realistic case of *bulk* serial number creation: three concurrent + requests, each creating 10 serial numbers, with the requested ranges + overlapping pairwise (but not all three sharing any single serial): + + A: 1-10 + B: 6-15 (overlaps A on 6-10, and C on 11-15) + C: 11-20 + + Since _create_serial_numbers() creates its whole batch atomically (a + conflict anywhere in the batch aborts the batch entirely), and A and C do + not share any serial with each other, exactly one of the following must + occur: + + - B is rejected, and A and C both succeed in full, or + - B succeeds in full, and both A and C are rejected + + In neither case can two of the three batches both succeed, and in neither + case can any serial number be created more than once. Since all three + batches target the same Part, StockItem._lock_serial_numbers() locks the + same Part rows for each of them (rather than locking anything specific to + the requested serials), so the three requests are fully serialized + against each other regardless of which serials they overlap on. + """ + + fixtures = ['users'] + + def setUp(self): + """Create a single trackable Part to create serialized stock against.""" + super().setUp() + + self.part = Part.objects.create( + name='Concurrency batch serial part', + description='Part for batch serial creation concurrency test', + trackable=True, + ) + + def test_concurrent_overlapping_batches_do_not_duplicate_serials(self): + """Three concurrent, partially-overlapping batch creation requests must stay consistent.""" + start_barrier = threading.Barrier(3, timeout=10) + errors = [] + results = {} + results_lock = threading.Lock() + + # Wrap StockItem._lock_serial_numbers() so all three threads reach the + # (real, database-level) row locks at the same time, maximising + # contention across the overlapping sets of serial numbers. + original_lock_serial_numbers = StockItem._lock_serial_numbers + + def synced_lock_serial_numbers(part, serials): + start_barrier.wait(timeout=10) + return original_lock_serial_numbers(part, serials) + + batches = { + 'a': [str(i) for i in range(1, 11)], # 1-10 + 'b': [str(i) for i in range(6, 16)], # 6-15 + 'c': [str(i) for i in range(11, 21)], # 11-20 + } + + def create(name, serials): + try: + StockItem._create_serial_numbers(serials, part=self.part) + with results_lock: + results[name] = 'ok' + except ValidationError: + with results_lock: + results[name] = 'rejected' + except Exception as exc: # pragma: no cover - surfaced via errors list + with results_lock: + errors.append(exc) + finally: + connection.close() + + threads = [ + threading.Thread(target=create, args=(name, serials)) + for name, serials in batches.items() + ] + + with mock.patch.object( + StockItem, '_lock_serial_numbers', synced_lock_serial_numbers + ): + for thread in threads: + thread.start() + + for thread in threads: + thread.join(timeout=10) + + for thread in threads: + self.assertFalse(thread.is_alive()) + self.assertEqual(errors, []) + self.assertEqual(set(results.keys()), {'a', 'b', 'c'}) + + # Batch 'b' overlaps both 'a' and 'c', which do not overlap each + # other - so either 'b' alone wins, or both 'a' and 'c' win, never + # any other combination + ok = {name for name, outcome in results.items() if outcome == 'ok'} + self.assertIn(ok, [{'b'}, {'a', 'c'}]) + + # No serial number was created more than once + created_serials = list( + StockItem.objects.filter(part=self.part).values_list('serial', flat=True) + ) + self.assertEqual(len(created_serials), len(set(created_serials))) + + # The created serials are exactly the union of the winning batches - + # each successful batch's serials all exist, and (since a rejected + # batch creates nothing at all) nothing beyond that union exists + expected_serials = set() + for name in ok: + expected_serials.update(batches[name]) + + self.assertEqual(set(created_serials), expected_serials) + + +@skipUnlessDBFeature('has_select_for_update') +class StockItemSerialGloballyUniqueConcurrencyTest(TransactionTestCase): + """Genuine cross-transaction regression test for the globally-unique locking path. + + Reproduces the same race as StockItemSerialConcurrencyTest, but with + SERIAL_NUMBER_GLOBALLY_UNIQUE enabled and the two concurrent requests + targeting *different* parts (in different variant trees). There is no + single Part tree covering this scope, so StockItem._lock_serial_numbers() + instead locks the SERIAL_NUMBER_GLOBALLY_UNIQUE setting's own row - this + verifies that still serializes the two requests correctly. + """ + + fixtures = ['users'] + + def setUp(self): + """Create two unrelated trackable Parts (in different variant trees).""" + super().setUp() + + InvenTreeSetting.set_setting('SERIAL_NUMBER_GLOBALLY_UNIQUE', True, None) + self.addCleanup( + InvenTreeSetting.set_setting, 'SERIAL_NUMBER_GLOBALLY_UNIQUE', False, None + ) + + self.part_a = Part.objects.create( + name='Globally-unique concurrency part A', + description='Part A for globally-unique serial concurrency test', + trackable=True, + ) + self.part_b = Part.objects.create( + name='Globally-unique concurrency part B', + description='Part B for globally-unique serial concurrency test', + trackable=True, + ) + + def test_concurrent_creation_across_parts_does_not_duplicate_serial(self): + """Two concurrent requests for different parts must not both succeed.""" + start_barrier = threading.Barrier(2, timeout=5) + errors = [] + results = {} + results_lock = threading.Lock() + + original_lock_serial_numbers = StockItem._lock_serial_numbers + + def synced_lock_serial_numbers(part, serials): + start_barrier.wait(timeout=5) + return original_lock_serial_numbers(part, serials) + + def create(name, part): + try: + StockItem._create_serial_numbers(['SN-RACE'], part=part) + with results_lock: + results[name] = 'ok' + except ValidationError: + with results_lock: + results[name] = 'rejected' + except Exception as exc: # pragma: no cover - surfaced via errors list + with results_lock: + errors.append(exc) + finally: + connection.close() + + thread_a = threading.Thread(target=create, args=('a', self.part_a)) + thread_b = threading.Thread(target=create, args=('b', self.part_b)) + + with mock.patch.object( + StockItem, '_lock_serial_numbers', synced_lock_serial_numbers + ): + thread_a.start() + thread_b.start() + + thread_a.join(timeout=5) + thread_b.join(timeout=5) + + self.assertFalse(thread_a.is_alive()) + self.assertFalse(thread_b.is_alive()) + self.assertEqual(errors, []) + + # Exactly one request must have been rejected as a duplicate, even + # though the two parts are unrelated + self.assertEqual(sorted(results.values()), ['ok', 'rejected']) + + # The serial number must only have been created once, across both parts + self.assertEqual( + StockItem.objects.filter( + part__in=[self.part_a, self.part_b], serial='SN-RACE' + ).count(), + 1, + ) + + +@skipUnlessDBFeature('has_select_for_update') +class StockItemSerialAPIConcurrencyTest(TransactionTestCase): + """Genuine cross-transaction regression test driven through the real StockItem creation API. + + Same race as StockItemSerialConcurrencyTest, but issued as five + concurrent HTTP POST requests against the 'api-stock-list' endpoint + (stock.api.StockList.create()) rather than calling + StockItem._create_serial_numbers() directly - this exercises the full + view/serializer stack (permission checks, serial number extraction, + pre-validation) under concurrency, not just the locking primitive itself. + """ + + fixtures = ['users'] + + def setUp(self): + """Create a superuser and a single trackable Part to create serialized stock against.""" + super().setUp() + + self.user = get_user_model().objects.get(username='sue_the_superuser') + + self.part = Part.objects.create( + name='API concurrency serial part', + description='Part for API serial creation concurrency test', + trackable=True, + ) + + def test_concurrent_api_creation_does_not_duplicate_serial(self): + """Five concurrent API requests for the same serial number must not all succeed.""" + n_threads = 5 + start_barrier = threading.Barrier(n_threads, timeout=10) + errors = [] + results = [] + results_lock = threading.Lock() + + url = reverse('api-stock-list') + + # Wrap StockItem._lock_serial_numbers() so all five threads reach the + # (real, database-level) row lock at the same time, regardless of how + # long each request takes to reach that point. + original_lock_serial_numbers = StockItem._lock_serial_numbers + + def synced_lock_serial_numbers(part, serials): + start_barrier.wait(timeout=10) + return original_lock_serial_numbers(part, serials) + + def create(): + client = APIClient() + client.force_authenticate(user=self.user) + + try: + response = client.post( + url, + { + 'part': self.part.pk, + 'quantity': 1, + 'serial_numbers': 'SN-API-RACE', + }, + format='json', + ) + with results_lock: + results.append(response.status_code) + except Exception as exc: # pragma: no cover - surfaced via errors list + with results_lock: + errors.append(exc) + finally: + connection.close() + + threads = [threading.Thread(target=create) for _ in range(n_threads)] + + with mock.patch.object( + StockItem, '_lock_serial_numbers', synced_lock_serial_numbers + ): + for thread in threads: + thread.start() + + for thread in threads: + thread.join(timeout=15) + + for thread in threads: + self.assertFalse(thread.is_alive()) + self.assertEqual(errors, []) + + # Exactly one request must have succeeded; the rest must have been + # rejected as duplicates + self.assertEqual(results.count(201), 1) + self.assertEqual(results.count(400), n_threads - 1) + + # The serial number must only have been created once + self.assertEqual( + StockItem.objects.filter(part=self.part, serial='SN-API-RACE').count(), 1 + )