[Django] #26291: loaddata cannot deserialize fixtures with forward references and natural foreign keys
Django
foreign keys
--------------------------------------+--------------------
Reporter: inglesp | Owner: nobody
Type: Uncategorized | Status: new
Component: Core (Serialization) | Version: 1.9
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
The following fixture cannot be deserialized, unless an Author object with
name "John Steinbeck" already exists. This is because when Django tries
to deserialize the first object, it tries to load an Author with name
"John Steinbeck", which fails.
{{{
[
{
"model": "app.book",
"fields": {
"title": "East Of Eden",
"author": ["John Steinbeck"]
}
},
{
"model": "app.author",
"fields": {
"name": "John Steinbeck",
"date_of_birth": "1902-02-27"
}
}
]
}}}
This problem is avoidable with careful ordering of fixture loading, but is
still a problem with fixtures that have circular references.
I have a proposed fix: see PR 6221
--
Ticket URL: <https://code.djangoproject.com/ticket/26291>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
foreign keys
Reporter: inglesp | Owner: nobody
Type: New feature | Status: new
Component: Core | Version: 1.9
(Serialization) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* type: Uncategorized => New feature
* needs_tests: => 0
* needs_docs: => 0
Old description:
> The following fixture cannot be deserialized, unless an Author object
> with name "John Steinbeck" already exists. This is because when Django
> tries to deserialize the first object, it tries to load an Author with
> name "John Steinbeck", which fails.
>
> {{{
> [
> {
> "model": "app.book",
> "fields": {
> "title": "East Of Eden",
> "author": ["John Steinbeck"]
> }
> },
> {
> "model": "app.author",
> "fields": {
> "name": "John Steinbeck",
> "date_of_birth": "1902-02-27"
> }
> }
> ]
> }}}
>
> This problem is avoidable with careful ordering of fixture loading, but
> is still a problem with fixtures that have circular references.
>
> I have a proposed fix: see PR 6221
New description:
The following fixture cannot be deserialized, unless an Author object with
name "John Steinbeck" already exists. This is because when Django tries
to deserialize the first object, it tries to load an Author with name
"John Steinbeck", which fails.
{{{
[
{
"model": "app.book",
"fields": {
"title": "East Of Eden",
"author": ["John Steinbeck"]
}
},
{
"model": "app.author",
"fields": {
"name": "John Steinbeck",
"date_of_birth": "1902-02-27"
}
}
]
}}}
This problem is avoidable with careful ordering of fixture loading, but is
still a problem with fixtures that have circular references.
I have a proposed fix: see [https://github.com/django/django/pull/6221 PR
6221]
--
Comment:
The proposed code looks rather complicated. What you are saying is that
[https://docs.djangoproject.com/en/dev/topics/serialization/#dependencies-
during-serialization the instructions about fixture ordering] aren't
sufficient due to the possibility of circular references, correct? Could
you give an example of that to motivate the feature a bit more?
--
Ticket URL: <https://code.djangoproject.com/ticket/26291#comment:1>
Django
foreign keys
Reporter: inglesp | Owner: nobody
Type: New feature | Status: new
Component: Core | Version: 1.9
(Serialization) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by inglesp):
Thanks for taking a look at this.
There are a handful of problems with Django's current dependency
resolution.
Firstly, `loaddata` cannot handle circular references at all.
Secondly, even without circular references, `dumpdata` can produce data
that `loaddata` cannot load, requiring manual re-ordering of a fixture.
This could happen if a field of a model instance references another
instance of the same model, where the pk of the referenced model is
greater than the pk of the referring model, since `dumpdata` dumps
instances of a model in order of it pk.
Finally, Django's current dependency resolution depends on `loaddata`
importing data that was created by `dumpdata`. If the data comes from
another source, `sort_dependencies`'s logic must be followed.
This last problem is the cause of my own interest in the issue, but I
think that the fact that `dumpdata` can produce data that `loaddata`
cannot load is reason enough to consider a solution.
--
Ticket URL: <https://code.djangoproject.com/ticket/26291#comment:2>
Django
foreign keys
Reporter: inglesp | Owner: nobody
Type: New feature | Status: new
Component: Core | Version: 1.9
(Serialization) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* cc: freakboy3742 (added)
Comment:
Russ, as the author of the natural key serialization support in
35cc439228cd32dfa7a3ec919db01a8a5cd17d33, I wonder if you could take a
look and let us know if you think this looks like a good idea or if you
foresee any problems?
--
Ticket URL: <https://code.djangoproject.com/ticket/26291#comment:3>
Django
foreign keys
Reporter: inglesp | Owner: nobody
Type: New feature | Status: new
Component: Core | Version: 1.9
(Serialization) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by freakboy3742):
@timgraham I've left some comments on the PR about specific implementation
details, but the broad-strokes approach seems sound. Unlike PK references,
Natural keys depend on being able to issue queries on the database, so
there's no way to resolve a forward reference until the object actually
exists. This means that deferring selected objects is the only approach
that is going to work. I can't think of any situation where this will
cause a regression in behavior - it just means a subset of all fixtures
that wasn't previously loadable now will be. AFAICT, the code that Peter
has added should only be activated in the case of fixtures that can't
currently be parsed at all.
The only alternative that I can see would be to improve
`sort_dependencies` so that it only issued fixtures in a format that can
be read again. However, that doesn't improve the general case that Peter
has identified - fixtures generated by an external source.
The dependency resolution issue that Peter flags *should* (AFAIK) only
exist for natural key fixtures (for the reason identified previously), and
MySQL InnoDB data stores (because InnoDB's implementation of referential
integrity is demonstrably broken). On other data stores, forward PK
references aren't an issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/26291#comment:4>
Django
foreign keys
Reporter: inglesp | Owner: nobody
Type: New feature | Status: new
Component: Core | Version: 1.9
(Serialization) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by inglesp):
Thanks for the review -- I've replied on the PR.
I think the only functionality question that needs to be resolved is
whether `django.core.serializers.python.Deserializer` has a
`handle_forward_references` option that defaults to `False`. That was
there for backwards compatibility, but it's probably not necessary, so
I'll remove.
There are some documentation changes required -- I'll work on those in the
next few days.
Also, I did look into improving `sort_dependencies` to output fixtures in
an order that has no forward references (ignoring circular references),
but I think it's fiddly to do efficiently.
--
Ticket URL: <https://code.djangoproject.com/ticket/26291#comment:5>
Django
foreign keys
Reporter: inglesp | Owner: nobody
Type: New feature | Status: new
Component: Core (Serialization) | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* needs_docs: 0 => 1
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/26291#comment:6>
Django
foreign keys
Reporter: inglesp | Owner: nobody
Type: New feature | Status: new
Component: Core (Serialization) | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Changes (by brillgen):
* cc: dev@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/26291#comment:7>
Django
foreign keys
Reporter: Peter Inglesby | Owner: nobody
Type: New feature | Status: new
Component: Core (Serialization) | Version: 1.9
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Changes (by Peter Inglesby):
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26291#comment:8>
Django
foreign keys
Reporter: Peter Inglesby | Owner: Peter
| Inglesby
Type: New feature | Status: assigned
Component: Core | Version: 1.9
(Serialization) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Peter Inglesby):
* status: new => assigned
* owner: nobody => Peter Inglesby
--
Ticket URL: <https://code.djangoproject.com/ticket/26291#comment:9>
Django
foreign keys
Reporter: Peter Inglesby | Owner: Peter
| Inglesby
Type: New feature | Status: assigned
(Serialization) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Asif Saifuddin Auvi):
* version: 1.9 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/26291#comment:10>
Django
foreign keys
Reporter: Peter Inglesby | Owner: Peter
| Inglesby
Type: New feature | Status: assigned
(Serialization) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/26291#comment:10>
Django
foreign keys
Reporter: Peter Inglesby | Owner: Peter
| Inglesby
Type: New feature | Status: assigned
Component: Core | Version: 1.9
(Serialization) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
I did a brief review of the patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/26291#comment:11>
Django
foreign keys
Reporter: Peter Inglesby | Owner: Peter
| Inglesby
Type: New feature | Status: assigned
Component: Core | Version: 1.9
(Serialization) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"73f7d1755ff1da3aac687c7b046e4b5028e505db" 73f7d175]:
{{{
#!CommitTicketReference repository=""
revision="73f7d1755ff1da3aac687c7b046e4b5028e505db"
Extracted deserialize fk/m2m functions from Deserializer.
In preparation for handling forward references (refs #26291).
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26291#comment:12>
Django
foreign keys
Reporter: Peter Inglesby | Owner: Peter
| Inglesby
Component: Core | Version: 1.9
(Serialization) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"312eb5cb11d09c0c41b2740e2e9aef838d60c8b5" 312eb5cb]:
{{{
#!CommitTicketReference repository=""
revision="312eb5cb11d09c0c41b2740e2e9aef838d60c8b5"
Fixed #26291 -- Allowed loaddata to handle forward references in
natural_key fixtures.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26291#comment:13>
Django
foreign keys
Reporter: Peter Inglesby | Owner: Peter
| Inglesby
Type: New feature | Status: closed
Component: Core | Version: 1.9
(Serialization) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"fca36f3c9812a779d0b9d0db315dd056f5d75d87" fca36f3c]:
{{{
#!CommitTicketReference repository=""
revision="fca36f3c9812a779d0b9d0db315dd056f5d75d87"
Refs #26291 -- Added tests for dumpdata with forward references in natural
keys.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26291#comment:14>
Django
foreign keys
Reporter: Peter Inglesby | Owner: Peter
| Inglesby
Type: New feature | Status: closed
Component: Core | Version: 1.9
(Serialization) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"26799c650389bec255df581eb400730916919aa1" 26799c65]:
{{{
#!CommitTicketReference repository=""
revision="26799c650389bec255df581eb400730916919aa1"
Refs #26291 -- Added tests for dumpdata/loaddata with forward references
without natural keys.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26291#comment:15>