Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#9717 closed (fixed)

manage.py flush raises error if there are unsynchronized applications

Reported by: russellm Owned by: nobody
Component: Core (Management commands) Version: 1.0
Severity: Keywords:
Cc: oliver@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

If an application has been added to INSTALLED_APPS, but has not yet been synchronized, ./manage.py flush will raise a "no such table:" error.

How to reproduce:

  1. Create a new project with a bunch of apps
  2. Call manage.py syncdb
  3. Add some new apps to INSTALLED_APPS (e.g., add contrib.admin to the list)
  4. Call manage.py flush

This will raise an error because the tables for contrib.admin haven't been created, but the flush command will try to delete those tables.

The solution here will be to check the tables that already exist in the database before attempting to destroy them. Syncdb already does a version of this, checking the tables that already exist before trying to create; the same logic needs to be applied to flush.

Attachments (1)

9717.flush_command.diff (2.0 KB) - added by julien 6 years ago.
Patch, but without regression tests yet…

Download all attachments as: .zip

Change History (8)

comment:1 Changed 6 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 follow-up: Changed 6 years ago by julien

The proposed patch is a bit simpler than replicating syncdb's behaviour. It works when tested locally with my apps, but unfortunately I could not find a way to write regression tests for it. Here's a first sketch below:

>>> _old_installed_apps = settings.INSTALLED_APPS
>>> settings.INSTALLED_APPS += ['?????']
>>> management.call_command('flush', verbosity=0, interactive=False)
>>> settings.INSTALLED_APPS = _old_installed_apps

The problem is: what should we put in place of the "?????". All contrib apps with models are included by default in the test suite, so we can't play with those. So, I guess that to properly test this, one would need to be able to include apps dynamically in the test suite, a problem which #7835 is trying to solve. Looks like a "chicken or the egg" puzzle :)

Any suggestion?

Changed 6 years ago by julien

Patch, but without regression tests yet...

comment:3 Changed 6 years ago by julien

In the case where no regression tests could be written for this, here's a suggestion, based on the assumption that this patch is trivial enough and does not break anything in the Django test suite:

  • Check this patch in.
  • Open a ticket T saying that there should be a regression test for this.
  • Create new test API and close #7835.
  • Write regression tests thanks to the new API and fix T.

comment:4 in reply to: ↑ 2 Changed 6 years ago by russellm

Replying to julien:

So, I guess that to properly test this, one would need to be able to include apps dynamically in the test suite, a problem which #7835 is trying to solve. Looks like a "chicken or the egg" puzzle :)

This is one of the rare occasions where it is acceptable to not include a unit test with a patch. There are a number of areas of Django that aren't unit tested - for example, most of the behaviour of syncdb is not formally tested. This isn't ideal, but sometimes it is unavoidable - usually because the cost of setting up the infrastructure required for testing these edge cases vastly exceeds the benefit that will be returned, especially given the frequency with which those areas of code are modified is taken in to consideration.

I'll take a look at this patch tonight.

comment:5 Changed 6 years ago by russellm

  • Resolution set to fixed
  • Status changed from new to closed

(In [9535]) Fixed #9717 -- Corrected a problem where django-admin.py flush would attempt to flush database tables that had not yet been created. This occurred when an application had been added to INSTALLED_APPS, but had not yet been synchronized. Thanks to Julien Phalip for the patch.

comment:6 Changed 6 years ago by russellm

(In [9536]) [1.0.X] Fixed #9717 -- Corrected a problem where django-admin.py flush would attempt to flush database tables that had not yet been created. This occurred when an application had been added to INSTALLED_APPS, but had not yet been synchronized. Thanks to Julien Phalip for the patch.

Merge of [9535] from trunk.

comment:7 Changed 6 years ago by anonymous

  • Cc oliver@… added
Note: See TracTickets for help on using tickets.
Back to Top