Opened 13 years ago
Closed 11 years 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: | dev |
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:
- 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
- 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)
Change History (19)
comment:1 by , 13 years ago
Cc: | added |
---|
comment:2 by , 13 years ago
Severity: | Normal → Release blocker |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:3 by , 13 years ago
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
by , 13 years ago
Attachment: | ticket_16568.patch added |
---|
comment:4 by , 13 years ago
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.
follow-up: 6 comment:5 by , 13 years ago
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 by , 13 years ago
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.
by , 13 years ago
Attachment: | ticket_16568_v2.patch added |
---|
comment:7 by , 13 years ago
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 by , 13 years ago
Patch needs improvement: | unset |
---|
comment:10 by , 13 years ago
Resolution: | fixed |
---|---|
Status: | closed → 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:11 by , 13 years ago
Reference to the bug on stackoverflow.com: http://stackoverflow.com/questions/7410151/valueerror-unable-to-configure-filter-require-debug-false-cannot-resolve-dja
comment:12 by , 13 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → 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:15 by , 11 years ago
Resolution: | fixed |
---|---|
Status: | closed → new |
comment:16 by , 11 years ago
Cc: | added |
---|
comment:17 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Please don't reopen closed/fixed ticket with no apparent reason.
I'm not wild about having a method definition in global_settings.py like that, but not sure it's avoidable, either.