Fixed #36207 -- Fixed refresh_from_db ForeignObject relations #19207
Conversation
tests/composite_pk/test_models.py
Outdated
|
|
||
| def test_refresh_foreign_object(self): | ||
| comment = Comment.objects.get(pk=self.comment_1.pk) | ||
| comment.__dict__.pop("user_id", None) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Should be fixed. Waiting for the CI to validate |
| comment = Comment.objects.get(pk=self.comment_1.pk) | ||
| comment.user = None | ||
| comment.refresh_from_db() | ||
| self.assertEqual(comment.user, self.user_1) |
There was a problem hiding this comment.
The query here seems redundant since we already have self.comment_1.
| comment = Comment.objects.get(pk=self.comment_1.pk) | |
| comment.user = None | |
| comment.refresh_from_db() | |
| self.assertEqual(comment.user, self.user_1) | |
| self.assertEqual(self.comment_1.user, self.user_1) | |
| self.comment_1.user = None | |
| self.comment_1.refresh_from_db() | |
| self.assertEqual(self.comment_1.user, self.user_1) |
which is the same as Jacob's test.
There was a problem hiding this comment.
Thanks for the updates. One thing to note: I think my sketched test is confounding things a little bit, since it's manipulating a ForeignObject field that has in its to_fields fields that participate in a CompositePrimaryKey. That's what's causing the CommentDoesNotExist error. We don't support setting a pk value to something nonexistent and refresh_from_db()'ing. (And my ticket isn't asking for support for that.)
I left a suggestion about an alternative approach.
| @@ -733,6 +779,9 @@ def refresh_from_db(self, using=None, fields=None, from_queryset=None): | |||
| } | |||
| ) | |||
|
|
|||
| if any(type(field) is ForeignObject for field in self._meta.get_fields()): | |||
There was a problem hiding this comment.
This would be a little more resilient against subclassing:
| 8000 | if any(type(field) is ForeignObject for field in self._meta.get_fields()): |
| if any( | |
| isinstance(field, ForeignObject) and not isinstance(field, ForeignKey) | |
| for field in self._meta.get_fields() | |
| ): |
There was a problem hiding this comment.
However, see other note, there might be a simpler way to implement this.
|
|
||
| def test_refresh_foreign_object(self): | ||
| comment = Comment.objects.get(pk=self.comment_1.pk) | ||
| comment.user = None |
There was a problem hiding this comment.
This is probably a better assertion so that we don't get sidetracked by the issue of setting a nonexistent primary key. I didn't realize when sketching out the draft test that the to_fields on the User field included fields that participate in the CompositePrimaryKey of the Comment model.
| comment.user = None | |
| comment.user = self.user_2 |
| @@ -680,6 +681,52 @@ def get_deferred 8000 _fields(self): | |||
| if f.attname not in self.__dict__ | |||
| } | |||
|
|
|||
| def _refresh_from_db_with_foreign_object(self): | |||
There was a problem hiding this comment.
Apologies if my initial sketched test sent us down a rabbit hole with the None stuff. I'm showing that with the improved assertion in my other comment, this is fixed with something like the following (note I would refactor this to do a little less inlining, just trying to communicate the idea):
diff --git a/django/db/models/base.py b/django/db/models/base.py
index d7d207901b..79d01b7bd1 100644
--- a/django/db/models/base.py
+++ b/django/db/models/base.py
@@ -33,6 +33,8 @@ from django.db.models.deletion import CASCADE, Collector
from django.db.models.expressions import DatabaseDefault
from django.db.models.fields.composite import CompositePrimaryKey
from django.db.models.fields.related import (
+ ForeignKey,
+ ForeignObject,
ForeignObjectRel,
OneToOneField,
lazy_related_operation,
@@ -749,7 +751,15 @@ class Model(AltersData, metaclass=ModelBase):
field.delete_cached_value(self)
# Clear cached relations.
- for rel in self._meta.related_objects:
+ for rel in (
+ *self._meta.related_objects,
+ *[
+ field
+ for field in self._meta.fields
+ if isinstance(field, ForeignObject)
+ and not isinstance(field, ForeignKey)
+ ],
+ ):
if (fields is None or rel.name in fields) and rel.is_cached(self):
rel.delete_cached_value(self)There was a problem hiding this comment.
Then a little investigation might be nice to see if we should be "upstreaming" any of these changes instead into whatever is calculating "_meta.related_objects" without breaking anything.
There was a problem hiding this comment.
May be fixed by Jacob's suggestions, but the current patch causes the following to crash:
diff --git a/tests/composite_pk/models/__init__.py b/tests/composite_pk/models/__init__.py
index 5996ae33b0..8848049f29 100644
--- a/tests/composite_pk/models/__init__.py
+++ b/tests/composite_pk/models/__init__.py
@@ -1,8 +1,11 @@
-from .tenant import Comment, Post, Tenant, TimeStamped, Token, User
+from .tenant import (
+ Comment, Post, SelfRefWithForeignObject, Tenant, TimeStamped, Token, User
+)
__all__ = [
"Comment",
"Post",
+ "SelfRefWithForeignObject",
"Tenant",
"TimeStamped",
"Token",
diff --git a/tests/composite_pk/models/tenant.py b/tests/composite_pk/models/tenant.py
index c85869afa7..d5d0e265e2 100644
--- a/tests/composite_pk/models/tenant.py
+++ b/tests/composite_pk/models/tenant.py
@@ -58,3 +58,15 @@ class TimeStamped(models.Model):
id = models.SmallIntegerField(unique=True)
created = models.DateTimeField(auto_now_add=True)
text = models.TextField(default="", blank=True)
+
+
+class SelfRefWithForeignObject(models.Model):
+ selfref = models.ForeignKey("self", models.CASCADE, null=True)
+ tenant_id = models.SmallIntegerField(null=True)
+ tenant = models.ForeignObject(
+ Tenant,
+ from_fields=["tenant_id"],
+ to_fields=["id"],
+ on_delete=models.SET_NULL,
+ related_name="+",
+ )
diff --git a/tests/composite_pk/test_models.py b/tests/composite_pk/test_models.py
index 695401b0b5..42a1f62cf5 100644
--- a/tests/composite_pk/test_models.py
+++ b/tests/composite_pk/test_models.py
@@ -2,7 +2,7 @@ from django.contrib.contenttypes.models import ContentType
from django.core.exceptions import ValidationError
from django.test import TestCase
-from .models import Comment, Tenant, Token, User
+from .models import Comment, Tenant, SelfRefWithForeignObject, Token, User
class CompositePKModelsTests(TestCase):
@@ -161,3 +161,7 @@ class CompositePKModelsTests(TestCase):
comment.user = None
comment.refresh_from_db()
self.assertEqual(comment.user, self.user_1)
+
+ def test_refresh_selfref_fk_and_foreign_object(self):
+ s1 = SelfRefWithForeignObject.objects.create()
+ s1.refresh_from_db()with
E
======================================================================
ERROR: test_refresh_selfref_fk_and_foreign_object (composite_pk.test_models.CompositePKModelsTests.test_refresh_selfref_fk_and_foreign_object)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/clifford/code/django/tests/composite_pk/test_models.py", line 167, in test_refresh_selfref_fk_and_foreign_object
s1.refresh_from_db()
File "/home/clifford/code/django/django/db/models/base.py", line 783, in refresh_from_db
return self._refresh_from_db_with_foreign_object()
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/clifford/code/django/django/db/models/base.py", line 726, in _refresh_from_db_with_foreign_object
setattr(self, field.attname, getattr(db_instance, field.attname))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/clifford/code/django/django/db/models/fields/related_descriptors.py", line 254, in __get__
raise self.RelatedObjectDoesNotExist(
composite_pk.models.tenant.SelfRefWithForeignObject.tenant.RelatedObjectDoesNotExist: SelfRefWithForeignObject has no tenant.
----------------------------------------------------------------------
Ran 1 test in 0.007s
FAILED (errors=1)
There was a problem hiding this comment.
I think the model field needs to have null=True for this to work as intended.
36207 Trac ticket number
ticket-36207
Branch description
Fixes refresh_from_db() for models with ForeignObject and composite primary keys by handling PK preservation and accurate data refresh.
Changes
Added _refresh_from_db_with_foreign_object():
A private method to:
- Collect ForeignObject and composite PK values.
- Query the database using these values.
- Update the instance fields and sync the internal state.
Updated refresh_from_db():
Delegates to the private method only when a ForeignObject is present.
Result
Ensures accurate data refresh for models with ForeignObject and composite PKs and delegate the responsability to a specifc function
Checklist
mainbranch.