[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

@@ -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,8 +287,20 @@ class InvenTreeMetadata(SimpleMetadata):
                     elif name in model_default_values:
                         serializer_info[name]['default'] = model_default_values[name]
 
+                    # 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)
+                            field_value = getattr(
+                                serializer.fields[name], field_key, None
+                            )
                             model_value = getattr(field, model_key, None)
 
                             if value := self.override_value(
@@ -311,6 +330,11 @@ class InvenTreeMetadata(SimpleMetadata):
                     relation.model_field.get_limit_choices_to()
                 )
 
+                # 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)

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:

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",
+        )

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."""

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

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 = (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):

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():