Opened 17 years ago

Closed 15 years ago

#5066 closed (fixed)

Calling "manage.py reset <app>" on an app that has no models causes crash

Reported by: gav@… Owned by: George Vilches
Component: Core (Other) Version: dev
Severity: Keywords: manage.py reset
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

python manage.py --noinput reset <app>, where the specified app does not have any classes that extend models.Model, results in this error:

Traceback (most recent call last):
  File "manage.py", line 12, in <module>
    execute_manager(settings)
  File "/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/site-packages/django/core/management.py", line 1744, in execute_manager
    execute_from_command_line(action_mapping, argv)
  File "/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/site-packages/django/core/management.py", line 1701, in execute_from_command_line
    output = action_mapping[action](mod, options.interactive)
  File "/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/site-packages/django/core/management.py", line 699, in reset
    sql_list = get_sql_reset(app)
  File "/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/site-packages/django/core/management.py", line 384, in get_sql_reset
    return get_sql_delete(app) + get_sql_all(app)
  File "/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/site-packages/django/core/management.py", line 370, in get_sql_delete
    app_label = app_models[0]._meta.app_label
IndexError: list index out of range

app_label on line 370 in django/core/management.py doesn't seem to be used anywhere, it's at the end of the method. Removing that line prevents the crash and appears not to have any side effects.

Even though it doesn't necessarily make sense to call reset on an app with no models, it is a hindrance to have this crash occur if you are, for instance, writing scripts to reset all your apps (since a reset_all isn't provided in django.core.management), or doing anything else where you might reset multiple apps at once without knowing their contents.

Attachments (2)

reset_no_app_label.patch (685 bytes ) - added by gav@… 17 years ago.
Patch against r5788 that resolves reset crash on apps with no models.
reset_no_app_r6979.patch (499 bytes ) - added by George Vilches 16 years ago.
Updates patch to fix resets on empty models to r6979.

Download all attachments as: .zip

Change History (10)

comment:1 by Chris Beaven, 17 years ago

Triage Stage: UnreviewedAccepted

Agreed. Care to attach the simple patch?

by gav@…, 17 years ago

Attachment: reset_no_app_label.patch added

Patch against r5788 that resolves reset crash on apps with no models.

comment:2 by gav@…, 17 years ago

Has patch: set

Patch now attached.

comment:3 by Adrian Holovaty, 17 years ago

I agree that we should fix this, but what an odd patch...Removing that line seems like it would have strange side effects.

Let's explore the repercussions of this change before committing the patch.

comment:4 by George Vilches <gav@…>, 17 years ago

I've run all the unit tests, it does not create any new errors or failures.

As far as side effects, what might not be obvious from the patch is that the variable is not used in that method. In fact, here's what the applicable portion of the function looks like (the ellipsis is just because the function is rather long before it gets here):

def get_sql_delete(app):
    ...
    app_label = app_models[0]._meta.app_label 

    # Close database connection explicitly, in case this output is being piped
    # directly into a database client, to avoid locking issues.
    if cursor:
        cursor.close()
        connection.close()

    return output[::-1] # Reverse it, to deal with table dependencies.
get_sql_delete.help_doc = "Prints the DROP TABLE SQL statements for the given app name(s)."
get_sql_delete.args = APP_ARGS

That's it. That variable is never used anywhere.

As far as the question of maybe it initializes something in the model through a specialized getter, I don't see this being the case. Take a look at django/db/models/base.py, in ModelBase.new (lines 47-51):

        if getattr(new_class._meta, 'app_label', None) is None:
            # Figure out the app_label by looking one level up.
            # For 'django.contrib.sites.models', this would be 'sites'.
            model_module = sys.modules[new_class.__module__]
            new_class._meta.app_label = model_module.__name__.split('.')[-2]

So _meta.app_label is always ensured to exist and be initialized. As far as the usage of _meta, there's nothing special about the _meta dict anywhere that would take advantage of accessing this variable to do more initialization than what happens in ModelBase and Model's init, and this SQL delete management call is far past that point.

So, with all that, I think this change looks pretty safe.

comment:5 by George Vilches <gav@…>, 17 years ago

Owner: changed from nobody to George Vilches

Taking ownership of this ticket for the sprint.

by George Vilches, 16 years ago

Attachment: reset_no_app_r6979.patch added

Updates patch to fix resets on empty models to r6979.

comment:6 by George Vilches, 16 years ago

Updated the patch, although it's still fundamentally the same, and still has the same crash error on an app with no models. I still haven't found any reason for this line to exist, does anyone else have an idea?

comment:7 by Chris Beaven, 16 years ago

All I can suggest is raising it in django-dev again, gav.

comment:8 by Karen Tracey, 15 years ago

Resolution: fixed
Status: newclosed

This appears to have been fixed as a side-effect of some other change prior to 1.0. With 1.0 code, running manage.py reset on an app with no models doesn't produce an error.

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