Opened 14 years ago

Closed 10 years ago

#14226 closed Bug (fixed)

Bug in dumpdata dependency calculation involving ManyToManyFields

Reported by: aneil Owned by: Rainer Koirikivi
Component: Core (Serialization) Version: 1.2
Severity: Normal Keywords: easy-pickings
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The manage.py dumpdata command incorrectly interprets ManyToMany relationships as dependencies of the model that declares them (rather than the other way around).

In the example below are 5 models - User, Tag and Posting, where both User and Posting have M2M relationships to Tag via UserTag and PostingTag, respectively. This should be serializable.

Here are the actual dependencies:

User:  None
Tag: None
Posting:  User
PostingTag: Posting, Tag
UserTag:   User, Tag

However, dumpdata fails with this error:
Error: Can't resolve dependencies for main.Posting, main.PostingTag, main.Tag, main.User, main.UserTag in serialized app list.

from django.db.models import Model,CharField,ForeignKey,ManyToManyField,TextField,DateTimeField
class User(Model):
    username  = CharField(max_length=20)
    password = CharField(max_length=20)
    topics = ManyToManyField("Tag",through="UserTag")
    
    def natural_key(self):
        return (self.username,)
    
class Posting(Model):
    user = ForeignKey(User)
    text = TextField()
    time = DateTimeField()

    def natural_key(self):
        return (self.user.username,self.time)

    natural_key.dependencies=['main.User']

class Tag(Model):
    name      = CharField(max_length=20)
    postings = ManyToManyField(Posting,through="PostingTag")

    def natural_key(self):
        return (self.name,)

class PostingTag(Model):
    tag = ForeignKey(Tag)
    posting = ForeignKey(Posting)
    
    def natural_key(self):
        return (self.tag.natural_key(),self.posting.natural_key())

class UserTag(Model):
    user = ForeignKey(User)
    tag = ForeignKey(Tag)
    
    def natural_key(self):
        return (self.tag.natural_key(),self.user.natural_key())

The reason this occurs is invalid logic in django/core/management/commands/dumpdata.py in lines 152-155. Here is the relevant code & context:

145            # Now add a dependency for any FK or M2M relation with
146           # a model that defines a natural key
147            for field in model._meta.fields:
148                if hasattr(field.rel, 'to'):
149                    rel_model = field.rel.to
150                    if hasattr(rel_model, 'natural_key'):
151                        deps.append(rel_model)
152            for field in model._meta.many_to_many:
153                rel_model = field.rel.to
154                if hasattr(rel_model, 'natural_key'):
155                    deps.append(rel_model)
156            model_dependencies.append((model, deps))

Lines 152-155 treat M2M relations like FK relations. This is incorrect. A Model named by an FK is a dependency, however, the model named by an M2M is not.

The fix requires adding the M2M *table* to the model_list, and processing its dependencies accordingly.

I've attached a simple test project that demonstrates the problem.

Attachments (3)

dumpdata_m2m_problem.tar.gz (3.2 KB ) - added by aneil 14 years ago.
test project demonstrating dumpdata problem
dumpdata.patch (1.3 KB ) - added by aneil 14 years ago.
Patch processes M2M tables correctly
test.diff (3.5 KB ) - added by Martin Chase 14 years ago.
Patch to add/fix tests under fixtures_regress

Download all attachments as: .zip

Change History (19)

by aneil, 14 years ago

Attachment: dumpdata_m2m_problem.tar.gz added

test project demonstrating dumpdata problem

by aneil, 14 years ago

Attachment: dumpdata.patch added

Patch processes M2M tables correctly

comment:1 by Russell Keith-Magee, 14 years ago

Cc: james@… andrew@… added
Has patch: set
Keywords: database postgres schema added
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

This needs tests; fixtures_regress already has tests for the sort_dependencies utility method, which is what is being modified here.

Also - I'm not completely convinced the patch is correct. On first inspection, I'm fairly certain the "add the through model to the dependency chain" will result in objects being added to the fixture that aren't required for the simple case. The simple case (normal m2m) can be satisfied by simply removing m2m checks from the dependency chain. In the complex case (manually specified m2m model), checks aren't required either, because the manually specified m2m model will be processed as a standalone model.

Tests would help to validate this :-)

comment:2 by Russell Keith-Magee, 14 years ago

Cc: james@… andrew@… removed
Keywords: easy-pickings added; database postgres schema removed

Not sure how the CC and keywords got modified. Sorry James and Andrew.

comment:3 by Martin Chase, 14 years ago

Owner: changed from nobody to Martin Chase
Status: newassigned

comment:4 by Martin Chase, 14 years ago

the patch to add tests will only be usefully applied after the genocide of doc tests has been merged into master.

comment:5 by Martin Chase, 14 years ago

also, this test does not account for russelm's concerns.

comment:6 by Martin Chase, 14 years ago

The proposed code patch appeared to break fixtures_regress:test_dependency_sorting_dangling, but on closer inspection, should not be considered to be the case. That test explicitly expected the dangling, un-related model to have a position in the resultant dependencies, but such an expectation is invalid.

by Martin Chase, 14 years ago

Attachment: test.diff added

Patch to add/fix tests under fixtures_regress

comment:7 by Martin Chase, 14 years ago

Needs tests: unset

active development of this at http://github.com/outofculture/django

comment:8 by Russell Keith-Magee, 14 years ago

The test cases look good, but running the full test suite reveals some additional breakages -- most notably in the fixtures tests. Some of these breakages are output ordering problems, but some appear to be more than that.

comment:9 by Graham King, 14 years ago

Severity: Normal
Type: Bug

comment:10 by Jacob, 14 years ago

Easy pickings: set

comment:11 by Aymeric Augustin, 13 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 by Rainer Koirikivi, 11 years ago

Owner: changed from Martin Chase to Rainer Koirikivi

comment:13 by Rainer Koirikivi, 11 years ago

I think the correct way to fix this is to remove models referenced by complex (with explicit intermediate models i.e. through=...) M2M relations from the dependency chain, but keep simple (with automatic intermediate models) in the dependency chain.

This is also the check done by django.core.serializers.python.Serializer.handle_m2m_field. In serialized data, simple M2M relations are shown inline with the model that defines them, which makes dependencies of the referenced models. For complex M2M relations, however, the intermediate models should be serialized along with the other models, and should be included as models in the serialized data. The model defining the M2M relation thus has no dependency to the other model, but the intermediate model will have a dependency to both of the M2M models.

Development branch at https://github.com/koirikivi/django/tree/ticket_14226
Pull request at https://github.com/django/django/pull/1495

All tests pass under sqlite and postgres.

I created quite a few tests to first learn about the issue and then to be sure that everything works. The test_dump_and_load_m2m_complex_* tests are most likely redundant with the other tests in the PR, and can be removed as seemed fit.

FWIW I also tried the patch posted by aneil, which broke a couple of tests, and removing the M2M checks altogether, which didn't break any tests but resulted in a regression that's caught in the tests in the PR (namely test_dependency_sorting_m2m_simple)

comment:14 by Rainer Koirikivi, 11 years ago

Patch needs improvement: unset

comment:15 by Tim Graham, 11 years ago

Easy pickings: unset

comment:16 by Ramiro Morales <cramm0@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In a75324c6544d728d3bd8f678b1b8021fdff18332:

Fixed #14226 -- Dependency calculation for complex M2M relations.

sort_dependencies incorrectly interpreted 'complex' M2M relations
(with explicit through models) as dependencies for a model. This caused
circular complex M2M relations to be unserializable by dumpdata.

Thanks to aneil for the report and outofculture for initial tests.

Note: See TracTickets for help on using tickets.
Back to Top