Opened 4 years ago

Closed 21 months ago

#16568 closed Bug (fixed)

require_debug_false does not work as intended (backward incompatible)

Reported by: andreas_pelme Owned by: nobody
Component: Core (Other) Version: master
Severity: Release blocker Keywords:
Cc: djangoproject.com@…, kulala.venu@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Snip from django/conf/global_settings.py:

LOGGING = {
    ...
    'filters': {
        'require_debug_false': {
            '()': 'django.utils.log.CallbackFilter',
            'callback': lambda r: not DEBUG
        }
    },
    ...
}

This does not work as intended. The callback uses the DEBUG setting that is defined in the same file -- django/conf/global_settings.py -- not in the project setting file. This configuration works when used in a project settings file - but not in the global one. The result is that callback will always return True, since DEBUG is "always" False in global_settings.py.

I hit this problem by not having defined LOGGING in my project settings file (since I just want everything to keep working the way it's been before). The trivial work-around is to copy/paste this into my project settings file.

Steps to reproduce:

  • Create a fresh project
  • Remove LOGGING from the generated settings.py file (to simulate an old project where LOGGING is not added)
  • Define ADMINS
  • Make sure DEBUG = True
  • Trigger an internal error
  • An email 500 is sent to ADMINS

I see a couple of ways to solve this:

  1. Remove require_debug_false from global_settings.py (since it does not work) and force everyone to copy/paste the default LOGGING snippet to their settings
  2. Fix the callback function to read the actual settings instead of global_settings, for instance like this:


def _require_debug_false(request):
    from django.conf import settings
    return not settings.DEBUG

LOGGING = {
    ....
    'filters': {
        'require_debug_false': {
            '()': 'django.utils.log.CallbackFilter',
            'callback': _require_debug_false
        }
    },
    ....
}

I would provide a proper patch, but I have no idea for how to provide a valid test case for this.

Attachments (2)

ticket_16568.patch (7.6 KB) - added by andreas_pelme 4 years ago.
ticket_16568_v2.patch (7.6 KB) - added by andreas_pelme 3 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 4 years ago by andreas_pelme

  • Cc djangoproject.com@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 4 years ago by russellm

  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

I'm not wild about having a method definition in global_settings.py like that, but not sure it's avoidable, either.

comment:3 Changed 4 years ago by andreas_pelme

  • Has patch set

I created another filter, django.utils.log.RequireDebugFalse which checks django.conf.settings.DEBUG directly and does not have the same problem. This also avoids the need to introduce callables into the settings file.

That leaves the CallbackFilter unused, but I have left it as is.

The attached patch contains tests and updates the docs accordingly.

The patch is also available here: https://github.com/pelme/django/tree/t16568 / git://github.com/pelme/django.git

Changed 4 years ago by andreas_pelme

comment:4 Changed 3 years ago by carljm

Just spent a while trying to write a proper end-to-end regression test for this failure case, and there really is no reliable way to do it, since we can't enforce that tests are run with no LOGGING setting. We'd need to able to save-modify-and-restore the entire global state of the logging module, which AFAICT there is no way to do.

The test_require_debug_no_logging_setting test in the patch does test the added RequireDebugFalse filter, but the setUp and tearDown methods in that test case are pointless: temporarily overriding settings.LOGGING has no impact, because the setting will already long ago have been parsed and passed to logging.dictConfig.

So I think we'll have to be satisfied with testing the filter and testing that global_settings.LOGGING has that filter in the config. This should be adequate.

comment:5 follow-up: Changed 3 years ago by andreas_pelme

I had a hard time figuring out how to properly write a test for this too. I will update the patch to current trunk with the changes you suggest soonish!

comment:6 in reply to: ↑ 5 Changed 3 years ago by carljm

  • Patch needs improvement set

Replying to andreas_pelme:

I had a hard time figuring out how to properly write a test for this too. I will update the patch to current trunk with the changes you suggest soonish!

I think the updates are pretty minor, I might just make them today and commit. If I don't get to it, feel free to update the patch, that'd be great.

Changed 3 years ago by andreas_pelme

comment:7 Changed 3 years ago by andreas_pelme

Yeah, it was a trivial change and a small merge conflict to update to latest trunk, anyways, here is a patch that applies cleanly on latest trunk.

You can also pull branch t16568 from git://github.com/pelme/django.git if you like!

comment:8 Changed 3 years ago by andreas_pelme

  • Patch needs improvement unset

comment:9 Changed 3 years ago by carljm

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

In [16840]:

Fixed #16568 -- Added RequireDebugFalse filter to prevent sending 500 error emails when DEBUG is True in projects with no explicit LOGGING setting. Thanks to Andreas Pelme for report and patch.

comment:10 Changed 3 years ago by tyre77@…

  • Resolution fixed deleted
  • Status changed from closed to reopened

I don't think this is actually fixed. With 1.3.1 installed, a simple python manage.py shell" produces the following:

ValueError: Unable to configure filter 'require_debug_false': Cannot resolve 'django.utils.log.CallbackFilter': No module named CallbackFilter

comment:12 Changed 3 years ago by andreas_pelme

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

I am not sure what your problem is exactly, but I do not think it is related to this ticket. The CallbackFilter was not added until after 1.3, so it should not be import:able in a Django 1.3.x install. And this ticket actually removes CallbackFilter from the default settings. You probably have your settings files mixed up, your project is probably using a django-trunk-settings-file when the CallbackFilter was added to the settings template.

You should probably add your settings file to the stackoverflow question. I have apparently not enough stackoverflow karma to post a comment, but that would be my advice to be able to help out!

Im closing this ticket as I cannot see how your problem is possibly related to this bug, please reopen it with proper instructions for how to reproduce this if I am wrong!

Please try the same commands as shown here (requires pip, virtualenv and virtualenvwrapper):

~ $ mkvirtualenv t16568-regression
New python executable in t16568-regression/bin/python
Installing setuptools............done.
Installing pip...............done.
virtualenvwrapper.user_scripts creating /Users/andreas/.virtualenvs/t16568-regression/bin/predeactivate
virtualenvwrapper.user_scripts creating /Users/andreas/.virtualenvs/t16568-regression/bin/postdeactivate
virtualenvwrapper.user_scripts creating /Users/andreas/.virtualenvs/t16568-regression/bin/preactivate
virtualenvwrapper.user_scripts creating /Users/andreas/.virtualenvs/t16568-regression/bin/postactivate
virtualenvwrapper.user_scripts creating /Users/andreas/.virtualenvs/t16568-regression/bin/get_env_details
[t16568-regression] ~ $ pip install django==1.3.1
Downloading/unpacking django==1.3.1
  Downloading Django-1.3.1.tar.gz (6.5Mb): 6.5Mb downloaded
  Running setup.py egg_info for package django
    
Installing collected packages: django
  Running setup.py install for django
    changing mode of build/scripts-2.7/django-admin.py from 644 to 755
    
    changing mode of /Users/andreas/.virtualenvs/t16568-regression/bin/django-admin.py to 755
Successfully installed django
Cleaning up...
[t16568-regression] ~ $ cd code/
[t16568-regression] ~ $ django-admin.py startproject foo
[t16568-regression] ~ $ cd foo/
[t16568-regression] ~/foo $ python manage.py shell

In [1]: import django; django.get_version()
Out[1]: '1.3.1'

In [2]: exit()

comment:13 Changed 3 years ago by anonymous

Answer updated on stackoverflow question too.

comment:14 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:15 Changed 21 months ago by venu <kulala.venu@…>

  • Resolution fixed deleted
  • Status changed from closed to new

comment:16 Changed 21 months ago by venu <kulala.venu@…>

  • Cc kulala.venu@… added

comment:17 Changed 21 months ago by charettes

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

Please don't reopen closed/fixed ticket with no apparent reason.

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