Opened 3 years ago

Last modified 3 days ago

#24607 new Bug

Serialization (and deserialization) of MTI models doesn't work with natural keys

Reported by: Denys Duchier Owned by:
Component: Core (Serialization) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I am using Django1.8. The exact error message is:

ValueError: Problem installing fixture '/home/denys/src/GSM/tmp/bug/person.json': "<Person: >" needs to have a value for field "user" before this many-to-many relationship can be used.

I have created a very small example to reproduce the problem; it is available on github at:

https://github.com/denys-duchier/django-loaddata-bug.git

to create the database, migrate, populate the database, then dumpdata, execute:

step1.bash

to recreate the database, migrate, and attempt the loaddata, execute:

step2.bash

that is when the error occurs.

Change History (18)

comment:1 Changed 3 years ago by Tim Graham

Component: UncategorizedCore (Serialization)
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

The issue seems to be caused by the fact that --natural-primary causes the Person's 'id' to be omitted in the fixture dump (since AbstractBaseUser defines natural_key()), but without it, there's no way to connect Person instances to the corresponding User.

comment:2 Changed 3 years ago by Denys Duchier

ah... I see, the issue is in get_dump_object

Last edited 10 months ago by Tim Graham (previous) (diff)

comment:3 Changed 3 years ago by Denys Duchier

I am thinking that when we have use_natural_foreign_keys and the pk is a relation, we want to recurse.

comment:4 Changed 3 years ago by Denys Duchier

I have created a PR that takes a first stab at this problem:

https://github.com/django/django/pull/4473

comment:5 Changed 3 years ago by Denys Duchier

comment:6 Changed 3 years ago by Tim Graham

Has patch: set

comment:7 Changed 2 years ago by Tim Graham

Patch needs improvement: set

Waiting for a response from the reporter on the pull request, but it seems to me the issue is that use_natural_primary_keys is not compatible with serializing multi-table inheritance models since the child models require the primary key to be linked to their parent. I'm not sure that adding a lot of complexity to essential "undo" the option for the affected models is worth it. Could we simply document the limitation?

comment:8 Changed 17 months ago by Levi Cameron

As it stands right now, use_natural_primary_keys doesn't work with multi-table inheritance with or without use_natural_foreign_keys -- but there's no fundamental reason stopping this and the most recent patch at least allows it to work if both use_natural_primary_keys and use_natural_foreign_keys are enabled.

The patch changes serialization requires use_natural_primary_keys and use_natural_foreign_keys to both be set in the case of multi-table inheritance. Possible options where use_natural_primary_keys is set but use_natural_foreign_keys isn't:

  1. use_natural_primary_keys implies use_natural_foreign_keys
    1. Always (this is what the patch did)
    2. If there are any multi-table inheritance models defined that have a natural key
    3. Only for references to models that are involved in any multi-table inheritance
      • May be a little surprising if you add an inherited model and it changes the dump for the parent model changes
  2. use_natural_primary_keys does not imply use_natural_foreign_keys
    • dumps may break if load/dump order is not identical as the real PKs may not match (ie is likely to break)
    • If there are any multi-table inheritance models with natural keys then a warning can be emitted
  3. use_natural_primary_keys is ignored for models that are involved in any multi-table inheritance and have a natural_key defined
    • May be a little surprising if you add an inherited model and it changes the dump for the parent model changes

Option 2 involves the least change and with the warning follows the principle of least surprise. It is an improvement over the current situation and is as backwards-compatible as possible.

If the patch was bought up to date with the latest django version, and it was changed to use option 2 (so use_natural_primary_keys did not turn on use_natural_foreign_keys but gave a warning if a likely problem was detected) then would this be acceptable?

(As an aside: if use_natural_primary_keys is set but use_natural_foreign_keys isn't then even for tables without multi-table inheritance it would seem that there is a sensitivity to the load/dump order in the referenced/referencing tables. The documentation at https://docs.djangoproject.com/en/dev/topics/serialization/ doesn't mention this anywhere)

Last edited 17 months ago by Levi Cameron (previous) (diff)

comment:9 Changed 15 months ago by Simon Charette

Summary: dumpdata (with natural keys) / loaddata failure for model inheriting from UserSerialization (and deserialization) of MTI models doesn't work with natural keys
Version: 1.8master

comment:11 Changed 14 months ago by Tim Graham

Patch needs improvement: unset

comment:12 Changed 13 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 74a575eb:

Fixed #24607 -- Serialized natural keys in multi-table inheritance models.

comment:13 Changed 9 months ago by Tim Graham <timograham@…>

In 0595bca:

Fixed #27742 -- Reverted "Fixed #24607 -- Serialized natural keys in multi-table inheritance models."

This reverts commit 74a575eb7296fb04e1fc2bd4e3f68dee3c66ee0a as it causes
unexpected migrations and doesn't seem to be the best solution.

comment:14 Changed 9 months ago by Tim Graham <timograham@…>

In eeb28f47:

[1.11.x] Fixed #27742 -- Reverted "Fixed #24607 -- Serialized natural keys in multi-table inheritance models."

This reverts commit 74a575eb7296fb04e1fc2bd4e3f68dee3c66ee0a as it causes
unexpected migrations and doesn't seem to be the best solution.

Backport of 0595bca221825c0c6bd572a32f3bf9eff7069328 from master

comment:15 Changed 9 months ago by Tim Graham

Has patch: unset
Resolution: fixed
Status: closednew

Reopening per discussion on #27742.

comment:16 Changed 7 months ago by Denys Duchier

new PR: https://github.com/django/django/pull/8370

I slightly reworked @jpmelos earlier PR #7231 to hopefully avoid the problem with spurious migrations. When a relation with parent_link=True is used as a pk then, when serializing with natural primary keys, the field must be serialized, even when marked with serialize=False.

Last edited 7 months ago by Denys Duchier (previous) (diff)

comment:17 Changed 4 months ago by Wai Yi Anthony Leung

Has patch: set
Owner: changed from nobody to Wai Yi Anthony Leung
Status: newassigned

I'm reviewing the latest PR on https://github.com/django/django/pull/8370

comment:18 Changed 3 days ago by Wai Yi Anthony Leung

Owner: Wai Yi Anthony Leung deleted
Status: assignednew
Triage Stage: AcceptedReady for checkin
Note: See TracTickets for help on using tickets.
Back to Top