Code

Opened 7 years ago

Closed 5 years ago

#5066 closed (fixed)

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

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

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@… 7 years ago.
Patch against r5788 that resolves reset crash on apps with no models.
reset_no_app_r6979.patch (499 bytes) - added by gav 6 years ago.
Updates patch to fix resets on empty models to r6979.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 7 years ago by SmileyChris

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

Agreed. Care to attach the simple patch?

Changed 7 years ago by gav@…

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

comment:2 Changed 7 years ago by gav@…

  • Has patch set

Patch now attached.

comment:3 Changed 7 years ago by adrian

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 Changed 7 years ago by George Vilches <gav@…>

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 Changed 7 years ago by George Vilches <gav@…>

  • Owner changed from nobody to gav

Taking ownership of this ticket for the sprint.

Changed 6 years ago by gav

Updates patch to fix resets on empty models to r6979.

comment:6 Changed 6 years ago by gav

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 Changed 6 years ago by SmileyChris

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

comment:8 Changed 5 years ago by kmtracey

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

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.