Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#25304 closed New feature (fixed)

Allow management commands to check if database migrations are applied

Reported by: Maxime Lorant Owned by: Mounir
Component: Core (Management commands) Version: 1.8
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Mounir)

When creating a new project, you can sometimes forget to run manage.py migrate before creating the initial superuser (especially if you don't execute runserver before, which display a warning about migrations not applied). The resulting error make sense, it can't access to auth_user, since it does not exist yet:


$ django-admin.py startproject sample
$ cd sample/ && python manage.py createsuperuser
Traceback (most recent call last):
  File "manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
[...]
  File "/vagrant/django/django/contrib/auth/management/commands/createsuperuser.py", line 85, in handle
    default_username = get_default_username()
  File "/vagrant/django/django/contrib/auth/management/__init__.py", line 189, in get_default_username
    auth_app.User._default_manager.get(username=default_username)
[...]
  File "/vagrant/django/django/db/backends/sqlite3/base.py", line 323, in execute
    return Database.Cursor.execute(self, query, params)
django.db.utils.OperationalError: no such table: auth_user


... but with a little try/except, it could be nicer and give a more meaningful information:


$ python manage.py createsuperuser
CommandError: You must execute `manage.py migrate` once before creating a super user


I have a patch ready (as I said, it just a try/except, see attachment) but, if accepted, it would need unit tests to be complete.

Attachments (1)

try-except-createsuperuser.diff (1.3 KB ) - added by Maxime Lorant 9 years ago.

Download all attachments as: .zip

Change History (16)

by Maxime Lorant, 9 years ago

comment:1 by Shai Berger, 9 years ago

Patch needs improvement: set

Hi,

Thanks for filing this report and patch. I think the patch is not an improvement, though:

  • The claim that you must run "migrate" is not true, the table could be created in other ways;
  • Since the user model is swappable, many interesting things could be different from default;
  • I suspect other database backends will throw different errors here;
  • I suspect the sqlite backend will throw an OperationalError in different circumstances (e.g. the database file cannot be written to because of permissions or non-existing directory or device full or whatever)

The error message, in general, could be made friendlier, but the source error must not be hidden. I think we could accept a similar modification where the except: clause catches DatabaseError, prints a hint along the lines of "Have you run initial migrations for this project?", and re-raises the exception.

For a test, I think you should try to temporarily swap out the user model; that should create the invalid code-vs-database state to trigger the exception.

comment:2 by Tim Graham, 9 years ago

I don't see much advantage to this. In my opinion, a developer should be able to understand what the lack of a database table means. There are other management commands that access the database -- do they all need to be adjusted too?

comment:3 by Maxime Lorant, 9 years ago

I agree with you, a developer should understand that a missing table on a Django default command means the database has not been created yet. However, I sometimes answer to question on StackOverflow or other programming forums even more... easy *cough*... (like "Why my page is not working and throw name 'url' is not defined in urls.py").

After some thoughts, the idea to raise the exception again with a hint seems more friendly for both usages (new Django users and skilled one), but it is not urgent at all. Also, it needs to establish a list of commands to improve...

Last edited 9 years ago by Maxime Lorant (previous) (diff)

comment:4 by Tim Graham, 9 years ago

Component: contrib.authCore (Management commands)
Easy pickings: unset
Has patch: unset
Patch needs improvement: unset
Summary: Trying to create a super user before first migrate raise a database errorAllow management commands to check if database migrations are applied
Triage Stage: UnreviewedAccepted
Type: Cleanup/optimizationNew feature

I don't think the exception catching solution would work perfectly. For example, there's no guarantee that the database exception is due to missing tables and not some other bad query, etc. A more viable solution might be to allow management commands to check if all database migrations are applied and to output a warning if not, similar to what runserver does. Accepting the ticket for investigation of that idea. #24484 is similar.

comment:5 by Shai Berger, 9 years ago

FWIW, +1

comment:6 by Mounir, 9 years ago

Owner: changed from nobody to Mounir
Status: newassigned

comment:7 by Mounir, 9 years ago

Description: modified (diff)
Has patch: set

comment:8 by Mounir, 9 years ago

Description: modified (diff)

comment:9 by Tim Graham, 9 years ago

Needs documentation: set
Patch needs improvement: set

Tests aren't running on the pull request and docs are needed.

comment:10 by Mounir, 9 years ago

Tests fixed and docs added

comment:11 by Tim Graham, 9 years ago

Needs documentation: unset
Patch needs improvement: unset

comment:12 by Tim Graham, 9 years ago

Needs tests: set

Could use an additional test as noted on the pull request.

comment:13 by Tim Graham, 9 years ago

Needs tests: unset

comment:14 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

In 50931df:

Fixed #25304 -- Allowed management commands to check if migrations are applied.

comment:15 by Tim Graham <timograham@…>, 9 years ago

In 1ac7fdcd:

Refs #25304 -- Added assertion for Command.requires_migrations_checks default.

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