Opened 18 months ago

Last modified 7 months ago

#21906 assigned Bug

dumpdata should not use router.allow_migrate

Reported by: yscumc Owned by: andersonresende
Component: Core (Management commands) Version: 1.5
Severity: Normal Keywords:
Cc: Twidi, anubhav9042@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
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 (9)

comment:1 Changed 18 months ago by yscumc

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Type changed from Uncategorized to Bug

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

Version 1, edited 18 months ago by yscumc (previous) (next) (diff)

comment:3 Changed 18 months ago by aaugustin

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 18 months ago by mjtamlyn

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 17 months ago by Twidi

  • Cc Twidi 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 17 months ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

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 Changed 13 months ago by anubhav9042

  • Cc anubhav9042@… added

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

comment:8 Changed 9 months ago by andersonresende

  • Owner changed from nobody to andersonresende
  • Status changed from new to assigned

comment:9 Changed 7 months ago by collinanderson

  • Summary changed from dumpdata should not use router.allow_syncdb to dumpdata should not use router.allow_migrate
Note: See TracTickets for help on using tickets.
Back to Top