-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed #18773 -- Add logging for template variable resolving #2552
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,11 @@ | |
'level': 'ERROR', | ||
'propagate': False, | ||
}, | ||
'django.template':{ | ||
'handlers':['null'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing space after : (check code with flake8) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might consider using the console logger by default for this to be more useful. I don't think most people will configuration logging in development to enable it otherwise. On the other hand, I don't know if this would clutter up the runserver logs. |
||
'level': 'WARNING', | ||
'propagate': False, | ||
}, | ||
'django.security': { | ||
'handlers': ['mail_admins'], | ||
'level': 'ERROR', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -413,7 +413,7 @@ requirements of logging in Web server environment. | |
Loggers | ||
------- | ||
|
||
Django provides four built-in loggers. | ||
Django provides five built-in loggers. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer to say "several" so we don't need to update it again. |
||
|
||
``django`` | ||
~~~~~~~~~~ | ||
|
@@ -438,6 +438,15 @@ Messages to this logger have the following extra context: | |
* ``request``: The request object that generated the logging | ||
message. | ||
|
||
.. _django-template-logger: | ||
|
||
``django.template`` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need |
||
~~~~~~~~~~~~~~~~~~ | ||
|
||
Log messages related to the rendering of templates. missing | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing (capitalize) |
||
variables are raised as ``WARNING`` messages. By default this logger uses | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logged as |
||
the null handler. | ||
|
||
``django.db.backends`` | ||
~~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -179,6 +179,11 @@ def safe_remove(path): | |
|
||
return self.run_test('./manage.py', args, settings_file) | ||
|
||
def assertTemplateLogHandlerMissingOutput(self, stream): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm confused with this happens if we assign it the null handler... is that not considered a handler? |
||
message = 'No handlers could be found for logger "django.template"' | ||
stream = force_text(stream) | ||
self.assertTrue(message in stream, "'%s' does not match actual output text '%s'" % (message, stream)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. assertTrue -> assertIn (although hopefully we can avoid a need for the changes in this file in the first place) |
||
|
||
def assertNoOutput(self, stream): | ||
"Utility assertion: assert that the given stream is empty" | ||
self.assertEqual(len(stream), 0, "Stream should be empty: actually contains '%s'" % stream) | ||
|
@@ -1698,7 +1703,7 @@ def test_custom_project_template(self): | |
self.addCleanup(shutil.rmtree, testproject_dir, True) | ||
|
||
out, err = self.run_django_admin(args) | ||
self.assertNoOutput(err) | ||
self.assertTemplateLogHandlerMissingOutput(err) | ||
self.assertTrue(os.path.isdir(testproject_dir)) | ||
self.assertTrue(os.path.exists(os.path.join(testproject_dir, 'additional_dir'))) | ||
|
||
|
@@ -1710,7 +1715,7 @@ def test_template_dir_with_trailing_slash(self): | |
self.addCleanup(shutil.rmtree, testproject_dir, True) | ||
|
||
out, err = self.run_django_admin(args) | ||
self.assertNoOutput(err) | ||
self.assertTemplateLogHandlerMissingOutput(err) | ||
self.assertTrue(os.path.isdir(testproject_dir)) | ||
self.assertTrue(os.path.exists(os.path.join(testproject_dir, 'additional_dir'))) | ||
|
||
|
@@ -1773,7 +1778,7 @@ def test_file_without_extension(self): | |
self.addCleanup(shutil.rmtree, testproject_dir, True) | ||
|
||
out, err = self.run_django_admin(args) | ||
self.assertNoOutput(err) | ||
self.assertTemplateLogHandlerMissingOutput(err) | ||
self.assertTrue(os.path.isdir(testproject_dir)) | ||
self.assertTrue(os.path.exists(os.path.join(testproject_dir, 'additional_dir'))) | ||
base_path = os.path.join(testproject_dir, 'additional_dir') | ||
|
@@ -1791,7 +1796,7 @@ def test_custom_project_template_context_variables(self): | |
os.mkdir(testproject_dir) | ||
self.addCleanup(shutil.rmtree, testproject_dir) | ||
out, err = self.run_django_admin(args) | ||
self.assertNoOutput(err) | ||
self.assertTemplateLogHandlerMissingOutput(err) | ||
test_manage_py = os.path.join(testproject_dir, 'manage.py') | ||
with open(test_manage_py, 'r') as fp: | ||
content = force_text(fp.read()) | ||
|
@@ -1836,7 +1841,7 @@ def test_custom_project_template_with_non_ascii_templates(self): | |
self.addCleanup(shutil.rmtree, testproject_dir, True) | ||
|
||
out, err = self.run_django_admin(args) | ||
self.assertNoOutput(err) | ||
self.assertTemplateLogHandlerMissingOutput(err) | ||
self.assertTrue(os.path.isdir(testproject_dir)) | ||
path = os.path.join(testproject_dir, 'ticket-18091-non-ascii-template.txt') | ||
with codecs.open(path, 'r', encoding='utf-8') as f: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,7 @@ | |
from django.conf import settings | ||
from django.core import urlresolvers | ||
from django.template import (base as template_base, loader, Context, | ||
RequestContext, Template, TemplateSyntaxError) | ||
RequestContext, Template, TemplateSyntaxError, Variable, VariableDoesNotExist) | ||
from django.template.loaders import app_directories, filesystem, cached | ||
from django.test import RequestFactory, TestCase | ||
from django.test.utils import (setup_test_template_loader, | ||
|
@@ -1962,3 +1962,70 @@ def test_relative_include_exploit(self): | |
with override_settings(ALLOWED_INCLUDE_ROOTS=(self.ssi_dir,)): | ||
for path in disallowed_paths: | ||
self.assertEqual(self.render_ssi(path), '') | ||
|
||
|
||
class VariableResolveLoggingTests(unittest.TestCase): | ||
def setUp(self): | ||
import logging | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please put imports at the top of the file |
||
from logging import Handler | ||
|
||
class TestHandler(Handler): | ||
def __init__(self): | ||
super(TestHandler, self).__init__() | ||
from django.utils.six import StringIO | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unused? |
||
self.log_record = None | ||
|
||
def emit(self, record): | ||
self.log_record = record | ||
|
||
self.test_handler = TestHandler() | ||
self.logger = logging.getLogger('django.template') | ||
self.original_level = self.logger.level | ||
self.logger.addHandler(self.test_handler) | ||
self.logger.setLevel(logging.WARNING) | ||
|
||
def tearDown(self): | ||
self.logger.removeHandler(self.test_handler) | ||
self.logger.level = self.original_level | ||
|
||
def test_log_on_variable_does_not_exist_silent(self): | ||
class TestObject(object): | ||
class SilentDoesNotExist(Exception): | ||
silent_variable_failure = True | ||
|
||
@property | ||
def template_name(self): | ||
return "template" | ||
|
||
@property | ||
def article(self): | ||
raise TestObject.SilentDoesNotExist("Attribute does not exist.") | ||
|
||
def __iter__(self): | ||
return iter([attr for attr in dir(TestObject) if attr[:2] != "__"]) | ||
|
||
c = TestObject() | ||
Variable('article').resolve(c) | ||
self.assertEqual(self.test_handler.log_record.msg, | ||
'template - Attribute does not exist.') | ||
|
||
def test_log_on_variable_does_not_exist_not_silent(self): | ||
c = {'article': {'section':u'News'}} | ||
|
||
with self.assertRaises(VariableDoesNotExist): | ||
Variable('article.author').resolve(c) | ||
|
||
self.assertEqual(self.test_handler.log_record.msg, | ||
'Unknown - Failed lookup for key [author] in u"{u\'section\': u\'News\'}"') | ||
|
||
def test_no_log_when_variable_exists(self): | ||
c = {'article': {'section':u'News'}} | ||
Variable('article.section').resolve(c) | ||
self.assertIsNone(self.test_handler.log_record) | ||
|
||
def test_default_null_handler_for_template_logging(self): | ||
from logging import NullHandler | ||
self.logger.removeHandler(self.test_handler) | ||
|
||
self.assertEqual(len(self.logger.handlers), 1) | ||
self.assertEqual(type(self.logger.handlers[0]), NullHandler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
getLogger()
calls typically go at the top of a file.