dumpdata should not use router.allow_migrate

[yscumc]

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 manage.py 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.

[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 change in 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 ​https://docs.djangoproject.com/en/1.5/topics/db/managers/#writing-correct-managers-for-use-in-automatic-manager-instances

[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.

[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.

[Stephane "Twidi" Angel]

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 :)

[Aymeric Augustin]

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 ./manage.py 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.

[ANUBHAV JOSHI]

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

[Hasan Ramezani]

Here is Aymeric Augustin comment:

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.

I created a ​PR to set router.db_for_write() as default value for --database option in dumpdata.

Question: Do we still need ​router.allow_migrate_model(using, model) check?

I am going to create another PR for loaddata if this one will be accepted.