From d92435e1b7800458771c3501eb7178805da87559 Mon Sep 17 00:00:00 2001 From: Sina Chaichi Maleki Date: Mon, 22 Sep 2025 16:00:07 +0200 Subject: [PATCH] Fixed #36489 -- Fixed stale reverse OneToOne cache in get_or_create/update_or_create. When concurrent get_or_create()/update_or_create() calls raced on a OneToOneField, the loser of the insert could leave an unsaved object cached on the reverse relation (e.g. parent.child with pk=None). This has been fixed by conditionally clearing the reverse cache. --- django/db/models/query.py | 20 ++++- docs/releases/6.1.txt | 8 ++ tests/one_to_one/tests.py | 169 +++++++++++++++++++++++++++++++++++++- 3 files changed, 195 insertions(+), 2 deletions(-) diff --git a/django/db/models/query.py b/django/db/models/query.py index a2af672546..cf76a4611c 100644 --- a/django/db/models/query.py +++ b/django/db/models/query.py @@ -20,6 +20,7 @@ from django.db import ( IntegrityError, NotSupportedError, connections, + models, router, transaction, ) @@ -1011,7 +1012,24 @@ class QuerySet(AltersData): return self.create(**params), True except IntegrityError: try: - return self.get(**kwargs), False + obj = self.get(**kwargs) + + # Conditional reverse OneToOne cache clearing + get_field = self.model._meta.get_field + for field_name, value in kwargs.items(): + if not isinstance(value, models.Model): + continue + try: + field = get_field(field_name) + except exceptions.FieldDoesNotExist: + continue + if getattr(field, "one_to_one", False): + cache_attr = field.remote_field.cache_name + cached = getattr(value, cache_attr, None) + # Clear cache only if object is missing or unsaved + if cached is None or getattr(cached, "pk", None) is None: + value._state.fields_cache.pop(cache_attr, None) + return obj, False except self.model.DoesNotExist: pass raise diff --git a/docs/releases/6.1.txt b/docs/releases/6.1.txt index 1430cb4f17..94862cad0c 100644 --- a/docs/releases/6.1.txt +++ b/docs/releases/6.1.txt @@ -293,6 +293,14 @@ Validators * ... +Bugfixes +======== + +* Fixed a bug in Django 5.2+ where concurrent ``get_or_create()`` and + ``update_or_create()`` calls on a ``OneToOneField`` could leave an unsaved + instance cached on the reverse relation (e.g. ``parent.child`` with + ``pk=None``) (:ticket:`36489`). + .. _backwards-incompatible-6.1: Backwards incompatible changes in 6.1 diff --git a/tests/one_to_one/tests.py b/tests/one_to_one/tests.py index 39f24d6b10..0e74fca1e3 100644 --- a/tests/one_to_one/tests.py +++ b/tests/one_to_one/tests.py @@ -1,6 +1,8 @@ +from unittest.mock import patch + from django.core.exceptions import FieldFetchBlocked from django.db import IntegrityError, connection, transaction -from django.db.models import FETCH_PEERS, RAISE +from django.db.models import FETCH_PEERS, RAISE, QuerySet from django.test import TestCase from .models import ( @@ -695,3 +697,168 @@ class OneToOneTests(TestCase): p1.restaurant._state.fetch_mode, FETCH_PEERS, ) + + def test_get_or_create_race_keeps_reverse_o2o_cache_consistent(self): + """ + Regression test for #36489. + + Simulate two concurrent Restaurant.get_or_create(place=...) calls + racing on the same OneToOneField. The loser of the insert must not + leave an unsaved instance cached on the reverse side (place.restaurant + with pk=None). After get_or_create(), place.restaurant must resolve to + the saved row. + """ + place = Place.objects.create(name="Race Diner", address="Somewhere") + + # We’ll inject a concurrent get_or_create inside the first one by + # patching QuerySet._extract_model_params + original = QuerySet._extract_model_params + triggered = {"done": False} + + def side_effect(self, defaults=None, **kwargs): + # Only re-enter once, and only for the same place kwarg. + if not triggered["done"] and kwargs.get("place") == place: + triggered["done"] = True + # Use a *fresh* instance of place to emulate a separate thread. + Restaurant.objects.get_or_create( + place=Place.objects.get(pk=place.pk), + defaults={"serves_hot_dogs": True, "serves_pizza": False}, + ) + return original(self, defaults, **kwargs) + + with patch( + "django.db.models.query.QuerySet._extract_model_params", + side_effect, + ): + restaurant, created = Restaurant.objects.get_or_create( + place=place, + defaults={"serves_hot_dogs": True, "serves_pizza": False}, + ) + self.assertFalse(created) + + # The reverse cache must not contain an unsaved Restaurant. + # Accessing place.restaurant should yield a saved row with a pk, + # and it should be the same row returned above. + self.assertIsNotNone(restaurant.pk) + self.assertIsNotNone(place.restaurant.pk) + self.assertEqual(place.restaurant.pk, restaurant.pk) + self.assertEqual(restaurant.place_id, place.pk) + + def test_get_or_create_race_does_not_clear_valid_select_related_cache(self): + place = Place.objects.create(name="Silver Square", address="Somewhere") + Restaurant.objects.create(place=place, serves_hot_dogs=True, serves_pizza=False) + + # Prime a valid forward cache via select_related() + # (we don't clear this path). + fetched = Restaurant.objects.select_related("place").get(place=place) + + # Run a no-op get_or_create (no IntegrityError path taken). + _, created = Restaurant.objects.get_or_create( + place=place, defaults={"serves_hot_dogs": False, "serves_pizza": False} + ) + self.assertFalse(created) + + # Using the forward cache should not cause a query. + with self.assertNumQueries(0): + _ = fetched.place.id + + def test_update_or_create_race_keeps_reverse_o2o_cache_consistent(self): + """ + Regression test for #36489 (update_or_create path). + + Simulate two concurrent Restaurant.update_or_create(place=...) calls + racing on the same OneToOneField. The loser of the insert must not + leave an unsaved instance cached on the reverse side (place.restaurant + with pk=None). After update_or_create(), place.restaurant must resolve + to the saved row. + """ + place = Place.objects.create(name="Update Race Diner", address="Somewhere") + + # Inject a concurrent update_or_create inside the first one by + # patching QuerySet._extract_model_params. + original = QuerySet._extract_model_params + triggered = {"done": False} + + def side_effect(self, defaults=None, **kwargs): + if not triggered["done"] and kwargs.get("place") == place: + triggered["done"] = True + # Use a fresh instance of place to emulate a separate thread. + Restaurant.objects.update_or_create( + place=Place.objects.get(pk=place.pk), + defaults={"serves_hot_dogs": True, "serves_pizza": False}, + ) + return original(self, defaults, **kwargs) + + with patch( + "django.db.models.query.QuerySet._extract_model_params", side_effect + ): + restaurant, created = Restaurant.objects.update_or_create( + place=place, + defaults={"serves_hot_dogs": False, "serves_pizza": True}, + ) + self.assertFalse(created) + + # The reverse cache must not contain an unsaved Restaurant. + # Accessing place.restaurant should yield a saved row with a pk, + # and it should be the same row returned above. + self.assertIsNotNone(restaurant.pk) + self.assertIsNotNone(place.restaurant.pk) + self.assertEqual(place.restaurant.pk, restaurant.pk) + self.assertEqual(restaurant.place_id, place.pk) + + def test_update_or_create_race_with_defaults_reverse_o2o_cache_consistent(self): + """ + Regression test for #36489 (defaults/create_defaults path). + + Simulate two concurrent Restaurant.update_or_create(place=...) calls + with non-trivial defaults. The loser of the insert must not leave an + unsaved instance cached on the reverse side + (place.restaurant with pk=None). + After update_or_create(), place.restaurant must resolve + to the saved row, and the defaults must be consistent + with the actual saved object. + """ + place = Place.objects.create(name="Defaults Diner", address="Anywhere") + + # Unique UUID in defaults to detect stale cache. + default_context = {"serves_hot_dogs": True, "serves_pizza": False} + alt_context = {"serves_hot_dogs": False, "serves_pizza": True} + + # Inject a concurrent update_or_create inside the first one. + original = QuerySet._extract_model_params + triggered = {"done": False} + + def side_effect(self, defaults=None, **kwargs): + if not triggered["done"] and kwargs.get("place") == place: + triggered["done"] = True + Restaurant.objects.update_or_create( + place=Place.objects.get(pk=place.pk), + defaults=alt_context, + ) + return original(self, defaults, **kwargs) + + with patch( + "django.db.models.query.QuerySet._extract_model_params", side_effect + ): + restaurant, created = Restaurant.objects.update_or_create( + place=place, + defaults=default_context, + ) + self.assertFalse(created) + + # The reverse cache must not contain an unsaved Restaurant. + self.assertIsNotNone(restaurant.pk) + self.assertIsNotNone(place.restaurant.pk) + self.assertEqual(place.restaurant.pk, restaurant.pk) + + # The object in the cache must be the same row we got back. + self.assertEqual(place.restaurant.pk, restaurant.pk) + self.assertEqual(restaurant.place_id, place.pk) + + # Ensure the actual persisted defaults match the winning insert, + # not some stale in-memory values. + db_restaurant = Restaurant.objects.get(pk=restaurant.pk) + self.assertEqual( + db_restaurant.serves_hot_dogs, place.restaurant.serves_hot_dogs + ) + self.assertEqual(db_restaurant.serves_pizza, place.restaurant.serves_pizza)