mirror of
https://github.com/django/django.git
synced 2025-11-18 19:01:40 +00:00
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.
This commit is contained in:
Sina Chaichi Maleki
parent
d506e4a528
commit
d92435e1b7
3 changed files with 195 additions and 2 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue