Opened 10 years ago

Last modified 7 days ago

#21906 new Bug

dumpdata should not use router.allow_migrate

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

Description

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.

Change History (13)

comment:1 by yscumc, 10 years ago

Type: UncategorizedBug

comment:2 by yscumc, 10 years ago

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

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

comment:3 by Aymeric Augustin, 10 years ago

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 by Marc Tamlyn, 10 years ago

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 by Stephane "Twidi" Angel, 10 years ago

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 by Aymeric Augustin, 10 years ago

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

comment:7 by ANUBHAV JOSHI, 10 years ago

Cc: anubhav9042@… added

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

comment:9 by Collin Anderson, 9 years ago

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

comment:10 by Anderson Resende, 8 years ago

Owner: nobody removed

comment:11 by Hasan Ramezani, 4 years ago

Has patch: set
Owner: set to Hasan Ramezani
Status: newassigned

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.

comment:12 by Mariusz Felisiak, 4 years ago

Patch needs improvement: set
Version: 1.5master

comment:13 by Hasan Ramezani, 3 years ago

Owner: Hasan Ramezani removed
Status: assignednew

comment:14 by Ülgen Sarıkavak, 7 days ago

Cc: Ülgen Sarıkavak added
Note: See TracTickets for help on using tickets.
Back to Top