Re: [Django] #34907: loaddata crashes on objects with natural keys when don't exist on passed database.
Django
database.
-------------------------------------+-------------------------------------
Reporter: Florian | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: natural key, multi- | Triage Stage:
db, loaddata | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by AbId KhAn):
* owner: AbId KhAn => (none)
* status: assigned => new
* component: Core (Serialization) => Database layer (models, ORM)
* stage: Accepted => Unreviewed
--
Ticket URL: <https://code.djangoproject.com/ticket/34907#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
database.
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core | Version: 4.2
(Serialization) |
Severity: Normal | Resolution:
Keywords: natural key, multi- | Triage Stage: Accepted
db, loaddata |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Florian):
Thanks for looking into it. The patch seems to fix it for me (filling in
some small details like importing the router):
{{{#!diff
diff --git a/django/core/serializers/python.py
b/django/core/serializers/python.py
index 7ec894aa00..2ef5a1765a 100644
--- a/django/core/serializers/python.py
+++ b/django/core/serializers/python.py
@@ -6,7 +6,7 @@ other serializers.
from django.apps import apps
from django.core.serializers import base
-from django.db import DEFAULT_DB_ALIAS, models
+from django.db import DEFAULT_DB_ALIAS, models, router
from django.utils.encoding import is_protected_type
@@ -118,6 +118,8 @@ def Deserializer(
else:
raise
data = {}
+ if not router.allow_migrate(using, Model):
+ continue
if "pk" in d:
try:
data[Model._meta.pk.attname] =
Model._meta.pk.to_python(d.get("pk"))
}}}
The test script outputs 'Ok' when running all tests with this patch
applied.
I can't really figure out what would be a logical unit test for the bug.
I've been looking at `tests/serializers/test_natural.py` for inspiration.
But I guess I'm not familiar enough with this part to come up with a good
test case that fails without this patch. What I've tried is modifying
`natural_key_test` to use a different database (modifications marked with
comments):
{{{
class TestRouter:
def allow_migrate(self, db, app_label, model_name=None, **hints):
return False
@override_settings(DATABASE_ROUTES=[TestRouter]) # added router
def natural_key_test_multidb(self, format):
book1 = {
"data": "978-1590597255",
"title": "The Definitive Guide to Django: Web Development Done
Right",
}
book2 = {"data": "978-1590599969", "title": "Practical Django
Projects"}
# Create the books.
adrian = NaturalKeyAnchor.objects.create(**book1)
james = NaturalKeyAnchor.objects.create(**book2)
# Serialize the books.
string_data = serializers.serialize(
format,
NaturalKeyAnchor.objects.all(),
indent=2,
use_natural_foreign_keys=True,
use_natural_primary_keys=True,
)
# Delete one book (to prove that the natural key generation will only
# restore the primary keys of books found in the database via the
# get_natural_key manager method).
james.delete()
# Deserialize and test.
books = list(serializers.deserialize(format, string_data,
using='other')) # added using
self.assertCountEqual(
[(book.object.title, book.object.pk) for book in books],
[
(book1["title"], None), # None i.s.o. adrian.pk
(book2["title"], None),
],
)
}}}
It feels natural to me that `None` is returned when using a different
database in which the object doesn't exist. This test succeeds already
without the patch. I guess what's missing is that the router is not used
when setting up the database, but I'm currently somewhat lost in getting
it to do that. Any hints?
I'm going to look into it some more tomorrow.
Django
database.
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Core | Version: 4.2
(Serialization) |
Severity: Normal | Resolution:
Keywords: natural key, multi- | Triage Stage: Accepted
db, loaddata |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => gauravvjn
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34907#comment:4>
Django
database.
-------------------------------------+-------------------------------------
| Jain
Type: Bug | Status: assigned
Component: Core | Version: 4.2
(Serialization) |
Severity: Normal | Resolution:
Keywords: natural key, multi- | Triage Stage: Accepted
db, loaddata |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/17528
--
Ticket URL: <https://code.djangoproject.com/ticket/34907#comment:5>
Django
database.
-------------------------------------+-------------------------------------
Type: Bug | Status: new
(Serialization) |
Severity: Normal | Resolution:
Keywords: natural key, multi- | Triage Stage: Accepted
db, loaddata |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Gaurav Jain):
* cc: Gaurav Jain (added)
* owner: Gaurav Jain => (none)
* has_patch: 1 => 0
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/34907#comment:6>
Django
database.
-------------------------------------+-------------------------------------
Reporter: Florian | Owner: (none)
Type: Bug | Status: new
Component: Core | Version: 4.2
(Serialization) |
Severity: Normal | Resolution:
Keywords: natural key, multi- | Triage Stage: Accepted
db, loaddata |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Gaurav Jain):
Hey @Florian, it looks like there's a typo in DATABASE_ROUTES. Although
the test works without the patch, applying it breaks other tests connected
to **loaddata**, particularly those linked to a multidb setup, such as
**django/tests/multiple_database/tests.py:
FixtureTestCase.test_pseudo_empty_fixtures**.
Due to the patch suppressing this error, loaddata proceeds silently with
the subsequent steps. Consequently, it fails to assert the expected string
in the mentioned test case and We're encountering a '**'RuntimeWarning: No
fixture data found for 'pets'. (The file format might be invalid.)''**
--
Ticket URL: <https://code.djangoproject.com/ticket/34907#comment:7>
Django
database.
-------------------------------------+-------------------------------------
| Jain
Type: Bug | Status: assigned
(Serialization) |
Severity: Normal | Resolution:
Keywords: natural key, multi- | Triage Stage: Accepted
db, loaddata |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Gaurav Jain (removed)
* owner: (none) => Gaurav Jain
* has_patch: 0 => 1
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34907#comment:8>
Django
database.
-------------------------------------+-------------------------------------
Reporter: Florian | Owner: Gaurav
| Jain
Type: Bug | Status: assigned
Component: Core | Version: 4.2
(Serialization) |
Severity: Normal | Resolution:
Keywords: natural key, multi- | Triage Stage: Accepted
db, loaddata |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34907#comment:9>
Django
database.
-------------------------------------+-------------------------------------
Reporter: Florian | Owner: Gaurav
| Jain
Type: Bug | Status: assigned
Component: Core | Version: 4.2
(Serialization) |
Severity: Normal | Resolution:
Keywords: natural key, multi- | Triage Stage: Accepted
db, loaddata |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Gaurav Jain):
@Mariusz Felisiak Patch has been updated. All checks have passed.
--
Ticket URL: <https://code.djangoproject.com/ticket/34907#comment:10>