From 2b3283d5d2d488bf8f16870c0ecf5b888a10041c Mon Sep 17 00:00:00 2001 From: Oliver Date: Wed, 12 Aug 2026 16:38:04 +1000 Subject: [PATCH] [bug] OptionalField race condition (#12627) * [bug] OptionalField race condition Fixes subtle bug where OptionalField entries can be silenty dropped from an API request due to concurrent requests / race conditions * Additional unit tests * Additional guard in metadata.py * include extra kwargs * Adjust import/exporting options * Fix attribute sharing across class instances --- src/backend/InvenTree/InvenTree/metadata.py | 56 ++++++++++++----- .../InvenTree/InvenTree/serializers.py | 11 +++- .../InvenTree/InvenTree/test_serializers.py | 63 +++++++++++++++++++ src/backend/InvenTree/common/test_api.py | 51 +++++++++++++++ .../InvenTree/data_exporter/serializers.py | 14 ++++- .../InvenTree/generic/states/fields.py | 47 ++++++++++++-- src/backend/InvenTree/report/serializers.py | 14 ++++- 7 files changed, 231 insertions(+), 25 deletions(-) diff --git a/src/backend/InvenTree/InvenTree/metadata.py b/src/backend/InvenTree/InvenTree/metadata.py index 4d8451e2db..d74645aa05 100644 --- a/src/backend/InvenTree/InvenTree/metadata.py +++ b/src/backend/InvenTree/InvenTree/metadata.py @@ -219,8 +219,15 @@ class InvenTreeMetadata(SimpleMetadata): serializer_info = super().get_serializer_info(serializer) - # Look for any dynamic fields which were not available when the serializer was instantiated - if hasattr(serializer, 'Meta'): + # Look for any dynamic fields which were not available when the serializer was + # instantiated - this lets an OPTIONS/schema response document an OptionalField + # (e.g. `part_detail`) even when it wasn't included on this particular instance, + # by rehydrating it directly from the class attribute. + if ( + hasattr(serializer, 'Meta') + and not getattr(serializer, '_is_importing', False) + and not getattr(serializer, '_exporting_data', False) + ): for field_name in serializer.Meta.fields: if field_name in serializer_info: # Already know about this one @@ -280,14 +287,26 @@ class InvenTreeMetadata(SimpleMetadata): elif name in model_default_values: serializer_info[name]['default'] = model_default_values[name] - for field_key, model_key in extra_attributes.items(): - field_value = getattr(serializer.fields[name], field_key, None) - model_value = getattr(field, model_key, None) + # Note: `name` may be present in `serializer_info` (above) without + # being a live entry in `serializer.fields` - the 'dynamic fields' + # lookup a few lines up adds metadata for OptionalFields that were + # excluded from *this* serializer instance (e.g. no matching query + # parameter was supplied), by rehydrating them directly from the + # class attribute. A model field can coincidentally share its name + # with such an OptionalField (e.g. `Group.permissions`, a real M2M + # field, vs. `GroupSerializer.permissions`, a computed OptionalField) + # - only touch `serializer.fields[name]` once we know it's real. + if name in serializer.fields: + for field_key, model_key in extra_attributes.items(): + field_value = getattr( + serializer.fields[name], field_key, None + ) + model_value = getattr(field, model_key, None) - if value := self.override_value( - name, field_key, field_value, model_value - ): - serializer_info[name][field_key] = value + if value := self.override_value( + name, field_key, field_value, model_value + ): + serializer_info[name][field_key] = value # Iterate through relations for name, relation in model_fields.relations.items(): @@ -311,14 +330,19 @@ class InvenTreeMetadata(SimpleMetadata): relation.model_field.get_limit_choices_to() ) - for field_key, model_key in extra_attributes.items(): - field_value = getattr(serializer.fields[name], field_key, None) - model_value = getattr(relation.model_field, model_key, None) + # See the comment above, in the 'simple fields' loop - `name` being in + # `serializer_info` doesn't guarantee it's a live `serializer.fields` + # entry (it may be a rehydrated, excluded OptionalField that happens to + # share its name with a real model relation). + if name in serializer.fields: + for field_key, model_key in extra_attributes.items(): + field_value = getattr(serializer.fields[name], field_key, None) + model_value = getattr(relation.model_field, model_key, None) - if value := self.override_value( - name, field_key, field_value, model_value - ): - serializer_info[name][field_key] = value + if value := self.override_value( + name, field_key, field_value, model_value + ): + serializer_info[name][field_key] = value if name in model_default_values: serializer_info[name]['default'] = model_default_values[name] diff --git a/src/backend/InvenTree/InvenTree/serializers.py b/src/backend/InvenTree/InvenTree/serializers.py index 064f879f4b..b1f0f5ce80 100644 --- a/src/backend/InvenTree/InvenTree/serializers.py +++ b/src/backend/InvenTree/InvenTree/serializers.py @@ -297,7 +297,16 @@ class FilterableSerializerMixin: def get_field_names(self, declared_fields, info): """Remove unused fields before returning field names.""" - field_names = super().get_field_names(declared_fields, info) + # Note: when `Meta.fields` is a list/tuple, DRF's base `get_field_names` + # returns that *exact* list object rather than a copy - a single list + # shared by every instance of this serializer class, across every thread. + # Copy it before mutating below - otherwise concurrent requests that + # disagree on whether an OptionalField (e.g. `tags`) should be included + # append/remove it on each other's shared list. A request whose own + # append lands can still have the field silently removed again by a + # concurrent request's `.remove()` before its own field-building loop + # (in DRF's `get_fields()`, which iterates this same list) reaches it. + field_names = list(super().get_field_names(declared_fields, info)) # Add any optional fields which are included for field_name in self.optional_fields: diff --git a/src/backend/InvenTree/InvenTree/test_serializers.py b/src/backend/InvenTree/InvenTree/test_serializers.py index bdebc9e7fa..9228cc6bbb 100644 --- a/src/backend/InvenTree/InvenTree/test_serializers.py +++ b/src/backend/InvenTree/InvenTree/test_serializers.py @@ -1,5 +1,9 @@ """Low level tests for serializers.""" +import sys +import threading +from concurrent.futures import ThreadPoolExecutor + from django.contrib import admin from django.contrib.auth.models import User from django.urls import path, reverse @@ -153,3 +157,62 @@ class FilteredSerializers(InvenTreeAPITestCase): response = self.client.get(url) self.assertContains(response, 'field_f') self.assertEqual(response.data[0]['field_f'], 'sample123') + + +class ConcurrentOptionalFieldTests(InvenTreeAPITestCase): + """Regression test for a race condition in `FilterableSerializerMixin.get_field_names`. + + When `Meta.fields` is a plain list (as it is for every real serializer in this + codebase), DRF's `ModelSerializer.get_field_names()` returns that *exact* list + object rather than a copy - a single list shared by every instance of the + serializer class, across every thread. `get_field_names()` used to `.append()`/ + `.remove()` an OptionalField's name directly on that shared list. + + Two concurrent requests that disagree on whether an OptionalField (here, + `field_b`) should be included could then corrupt each other's output: one + request's `.append('field_b')` could be immediately undone by another, + concurrent request's `.remove('field_b')` on the *same* list object, before + the first request's own field-building loop (DRF's `get_fields()`, which + iterates this same list) reached it - silently dropping the field from a + response that should have included it. + """ + + def test_concurrent_optional_field_inclusion(self): + """Serializers built concurrently with conflicting field_b inclusion must not corrupt each other. + + Builds many `SampleSerializer` instances in parallel threads, alternating + whether `field_b` should be included (passed directly as a constructor + kwarg, per `FilterableSerializerMixin.is_field_included`, so this needs no + HTTP request/response machinery). Every instance's rendered `.data` must + match what *that* instance asked for, regardless of what other concurrently + running instances asked for. + """ + errors = [] + lock = threading.Lock() + + def worker(include: bool): + serializer = SampleSerializer(self.user, field_b=include) + has_field_b = 'field_b' in serializer.data + if has_field_b != include: + with lock: + errors.append((include, has_field_b)) + + # Force frequent thread switches - the race window between the shared + # list being fixed up and it being iterated over is only a handful of + # bytecodes wide, so the default switch interval rarely lands inside it. + old_interval = sys.getswitchinterval() + sys.setswitchinterval(1e-6) + try: + with ThreadPoolExecutor(max_workers=16) as executor: + futures = [executor.submit(worker, i % 2 == 0) for i in range(2000)] + for future in futures: + future.result() + finally: + sys.setswitchinterval(old_interval) + + self.assertEqual( + errors, + [], + f'{len(errors)} / 2000 concurrently-built serializers had the wrong ' + f"'field_b' inclusion (expected, got) pairs shown above", + ) diff --git a/src/backend/InvenTree/common/test_api.py b/src/backend/InvenTree/common/test_api.py index 6e5272c7dd..71602098ca 100644 --- a/src/backend/InvenTree/common/test_api.py +++ b/src/backend/InvenTree/common/test_api.py @@ -1436,6 +1436,57 @@ class TagAPITests(InvenTreeAPITestCase): self.assertIn(self.part_a.pk, pks) self.assertNotIn(self.part_b.pk, pks) + # ------------------------------------------------------------------ + # 'tags' as an OptionalField (data inclusion, not filtering) + # ------------------------------------------------------------------ + # + # Every serializer below wires up its 'tags' field via + # `common.filters.enable_tags_filter()`, with `default_include=False` - + # so a plain detail request should never include tag data, and it should + # only appear when the caller explicitly asks for it via `?tags=true`. + + def test_part_detail_tags_excluded_by_default(self): + """A plain part detail request should not include tag data.""" + url = reverse('api-part-detail', kwargs={'pk': self.part_a.pk}) + + response = self.get(url, expected_code=200) + self.assertNotIn('tags', response.data) + + def test_part_detail_tags_included_via_query_param(self): + """Requesting '?tags=true' on part detail should include the part's tag names.""" + url = reverse('api-part-detail', kwargs={'pk': self.part_a.pk}) + + response = self.get(url, data={'tags': 'true'}, expected_code=200) + self.assertIn('tags', response.data) + self.assertEqual(set(response.data['tags']), {'apple', 'banana'}) + + # An untagged part should report an empty list, not omit the field + url = reverse('api-part-detail', kwargs={'pk': self.part_c.pk}) + response = self.get(url, data={'tags': 'true'}, expected_code=200) + self.assertIn('tags', response.data) + self.assertEqual(response.data['tags'], []) + + def test_part_list_tags_query_param_collides_with_tag_filter(self): + """On the list endpoint, '?tags=true' is *not* the OptionalField inclusion flag. + + `PartFilter` (the list endpoint's FilterSet) declares its own 'tags' field + (a `TagsFilter`, for filtering by tag name - see the `test_part_filter_*` + tests above), which shadows the serializer's 'tags' OptionalField: both are + wired to the same query parameter name. django-filter processes the + FilterSet before the serializer runs, so '?tags=true' is filtered as "must + have a tag named 'true'" - which nothing does - rather than being treated + as a request to include each part's tag data. + + This is presumably not the intended behaviour for a client trying to + request tag data on a list endpoint, but it is the current, real + behaviour - this test locks it in so a change to either `PartFilter` or + `enable_tags_filter()` is a deliberate decision rather than an accident. + """ + url = reverse('api-part-list') + + response = self.get(url, data={'tags': 'true'}, expected_code=200) + self.assertEqual(response.data, []) + class SelectionListLockedTest(InvenTreeAPITestCase): """Tests that a locked SelectionList rejects all entry mutations.""" diff --git a/src/backend/InvenTree/data_exporter/serializers.py b/src/backend/InvenTree/data_exporter/serializers.py index de535a74a1..c5d01d6196 100644 --- a/src/backend/InvenTree/data_exporter/serializers.py +++ b/src/backend/InvenTree/data_exporter/serializers.py @@ -23,8 +23,18 @@ class DataExportOptionsSerializer(serializers.Serializer): - The exact nature of the available fields depends on which plugin is selected. - The selected plugin may 'extend' the fields available in the serializer. """ - # Reset fields to a known state - self.Meta.fields = ['export_format', 'export_plugin'] + + # Give this instance its own 'Meta.fields' list, appended to below depending + # on which plugin is selected. `Meta` is otherwise a single class-level + # object shared by every instance of this serializer - mutating its 'fields' + # list in place would let concurrent requests selecting different plugins + # corrupt each other's field list. + class Meta(self.Meta): + """Per-instance metaclass options for this serializer.""" + + fields = ['export_format', 'export_plugin'] + + self.Meta = Meta # Generate a list of plugins to choose from # If a model type is provided, use this to filter the list of plugins diff --git a/src/backend/InvenTree/generic/states/fields.py b/src/backend/InvenTree/generic/states/fields.py index 95b5803fd0..3a69531b4a 100644 --- a/src/backend/InvenTree/generic/states/fields.py +++ b/src/backend/InvenTree/generic/states/fields.py @@ -218,6 +218,12 @@ class InvenTreeCustomStatusSerializerMixin: _custom_fields_follower: Optional[list] = None _is_gathering = False + # Maps leader field name -> a throwaway instance of that field, built by + # `build_standard_field` as it constructs each leader field. Used by a + # later 'follower' (*_custom_key) field, built later in the same pass, + # to inherit choices/read_only state - see `build_standard_field` below. + _custom_leader_fields: Optional[dict] = None + def update(self, instance, validated_data): """Ensure the custom field is updated if the leader was changed.""" self.gather_custom_fields() @@ -288,6 +294,31 @@ class InvenTreeCustomStatusSerializerMixin: """Use custom field for custom status model. This is required because of DRF overwriting all fields with choice sets. + + Note: This method is called *while* the serializer's `fields` cached_property + is still being constructed (DRF builds fields one at a time, in `Meta.fields` + order). It must not access `self.fields` (or anything that does, like + `self.gather_custom_fields()`) - doing so would re-enter the `fields` + cached_property while it is already being computed, kicking off a second, + fully independent rebuild of the whole field set. Under concurrent access, + whichever of the two competing builds finishes last silently wins and gets + cached - intermittently dropping unrelated fields built earlier in the + original pass (e.g. an OptionalField like `tags`) if the second build + finishes without them. + + Instead, a 'leader' field (e.g. `status`) records a throwaway instance of + itself on `self._custom_leader_fields` as it is built, so that its + 'follower' field (`status_custom_key`), built later in the same pass, can + read its choices/read_only state directly - without touching `self.fields`. + + That throwaway instance must be built the same way DRF's own `get_fields()` + builds the *real* one - which includes merging in `Meta.extra_kwargs` / + `Meta.read_only_fields` (e.g. `status` is typically listed as read-only + there). DRF applies that merge itself, in `get_fields()`, *after* + `build_field()` returns - a step this method is never otherwise party to. + Both `get_extra_kwargs()` and `include_extra_kwargs()` are pure functions + of `self.Meta` / plain dicts, so - unlike `self.fields` - they're safe to + call here. """ field_cls, field_kwargs = super().build_standard_field(field_name, model_field) if issubclass(field_cls, ChoiceField) and isinstance( @@ -296,6 +327,14 @@ class InvenTreeCustomStatusSerializerMixin: field_cls = CustomChoiceField field_kwargs['choice_mdl'] = model_field.model field_kwargs['choice_field'] = model_field.name + + if self._custom_leader_fields is None: + self._custom_leader_fields = {} + leader_extra_kwargs = self.get_extra_kwargs().get(field_name, {}) + leader_kwargs = self.include_extra_kwargs( + dict(field_kwargs), leader_extra_kwargs + ) + self._custom_leader_fields[field_name] = field_cls(**leader_kwargs) elif isinstance(model_field, ExtraInvenTreeCustomStatusModelField): field_cls = ExtraCustomChoiceField field_kwargs['choice_mdl'] = model_field.model @@ -303,10 +342,10 @@ class InvenTreeCustomStatusSerializerMixin: field_kwargs['is_custom'] = True # Inherit choices from leader - self.gather_custom_fields() - if self._custom_fields and field_name in self._custom_fields: - leader_field_name = field_name.replace('_custom_key', '') - leader_field = self.fields[leader_field_name] + leader_field_name = field_name.replace('_custom_key', '') + leader_field = (self._custom_leader_fields or {}).get(leader_field_name) + + if leader_field is not None: if hasattr(leader_field, 'choices'): field_kwargs['choices'] = list(leader_field.choices.items()) elif hasattr(model_field.model, leader_field_name): diff --git a/src/backend/InvenTree/report/serializers.py b/src/backend/InvenTree/report/serializers.py index 60753db8b9..fbe0160823 100644 --- a/src/backend/InvenTree/report/serializers.py +++ b/src/backend/InvenTree/report/serializers.py @@ -138,8 +138,18 @@ class LabelPrintSerializer(serializers.Serializer): def __init__(self, *args, **kwargs): """Override the constructor to add the extra plugin fields.""" - # Reset to a known state - self.Meta.fields = ['template', 'items', 'plugin'] + + # Give this instance its own 'Meta.fields' list, appended to below depending + # on which plugin is selected. `Meta` is otherwise a single class-level + # object shared by every instance of this serializer - mutating its 'fields' + # list in place would let concurrent requests selecting different plugins + # corrupt each other's field list. + class Meta(self.Meta): + """Per-instance metaclass options.""" + + fields = ['template', 'items', 'plugin'] + + self.Meta = Meta if plugin_serializer := kwargs.pop('plugin_serializer', None): for key, field in plugin_serializer.fields.items():