Opened 6 years ago

Last modified 14 months ago

#21906 new Bug

dumpdata should not use router.allow_migrate

Reported by: yscumc Owned by:
Component: Core (Management commands) Version: 1.5
Severity: Normal Keywords:
Cc: Stephane "Twidi" Angel, anubhav9042@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


I have two databases with a router in place to prevent syncdb from automatically creating tables on the 2nd database (referred to as: second_database).

When dumpdata --database=second_database is executed, an empty JSON array is printed: []

Upon a lot of fumbling around, it seems that this behavior is caused by ticket #13308 in this commit.

                if not model._meta.proxy and router.allow_syncdb(using, model):

For the dumpdata command to be dependent on the router.allow_syncdb() was totally unexpected because it is not documented anywhere and the name of the router method is allow_syncdb() which gives the impression that it only influences the syncdb command.

The allow_syncdb should ONLY affect syncdb and a separate method should be created for the dumpdata/loaddata commands. This would allow dumpdata to be run on a non-syncdb-able database.

Note: I do know there's a Managed = False meta option on each model and I could remove the allow_syncdb on my router as a workaround for my specific use case, but I still feel the current behavior is extremely counter-intuitive. Either the doc should be updated, or the alternative methods added to the router to determine the behavior for the other two non-syncdb commands.

Change History (9)

comment:1 Changed 6 years ago by yscumc

Type: UncategorizedBug

comment:2 Changed 6 years ago by yscumc

After doing more digging, it seems that allow_syncdb is meant to define which models are are available on which particular database, and not just to prevent syncdb from acting upon a particular database.

I guess there should be no change in the logic and I believe the best resolution is to rename allow_syncdb or document this in the doc just like how the historic naming for use_for_related_fields is explained in

Last edited 6 years ago by yscumc (previous) (diff)

comment:3 Changed 6 years ago by Aymeric Augustin

Indeed, if allow_syncdb returns False for a given database and model, Django expects that this database will not contain a table storing instances of this model.

I'm not particularly fond of the idea of renaming this method. Everyone would have to update its routers for a fairly small benefit.

comment:4 Changed 6 years ago by Marc Tamlyn

This method is actually being renamed in 1.7 anyway to be allow_migrate so if we want to change things, now is a good time.

comment:5 Changed 6 years ago by Stephane "Twidi" Angel

Cc: Stephane "Twidi" Angel added

As dumpdata is about reading data from the database, shouldn't the db_for_read method be called ?

By the way, by having the exact same problem as described in this ticket, yes, this method's name is not relevant :)

comment:6 Changed 6 years ago by Aymeric Augustin

Triage Stage: UnreviewedAccepted

Currently, the documentation for allow_syncdb says:

This method can be used to determine the availability of a model on a given database.

I don't think that's entirely correct. allow_syncdb / allow_migrate controls on which databases syncdb / migrate will create the tables. In a master - slave setup where the replication mechanism propagates DDL statements, allow_syncdb should return True for the master and False for the slave; however, the model is available on both databases. I believe the documentation of allow_syncdb / allow_migrate should be changed.

Furthermore, the fix for #13308 was a bit simplistic. The goal is to prevent ./ dumpdata --database=... to fail when only a few models don't exist in that database. There must be a better way to achieve that.

With the current database router API, all dumpdata and loaddata can do with router information is check db_for_read and db_for_write to determine which database to use by default for each model. Most likely the ORM already does this automatically.

comment:7 Changed 5 years ago by ANUBHAV JOSHI

Cc: anubhav9042@… added

Is the decision for this issue to use db_for_read in place of allow_migrate or else?

comment:9 Changed 5 years ago by Collin Anderson

Summary: dumpdata should not use router.allow_syncdbdumpdata should not use router.allow_migrate

comment:10 Changed 4 years ago by Anderson Resende

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