Merge lp:~gz/python-oops-tools/dbsummaries_utf8_encode_891186 into lp:python-oops-tools

Proposed by Martin Packman
Status: Merged
Approved by: Graham Binns
Approved revision: 31
Merged at revision: 29
Proposed branch: lp:~gz/python-oops-tools/dbsummaries_utf8_encode_891186
Merge into: lp:python-oops-tools
Diff against target: 103 lines (+62/-7)
3 files modified
src/oopstools/oops/dbsummaries.py (+7/-0)
src/oopstools/oops/test/test_dbsummaries.py (+55/-0)
src/oopstools/scripts/analyse_error_reports.py (+0/-7)
To merge this branch: bzr merge lp:~gz/python-oops-tools/dbsummaries_utf8_encode_891186
Reviewer Review Type Date Requested Status
Graham Binns (community) code Approve
Review via email: mp+85180@code.launchpad.net

Commit message

Encode output of analyse_error_reports as UTF-8.

Description of the change

There doesn't seem any need to hold off on this bug, so this branch merges Diogo's test and gets it working then applies a version of the fix I hope is acceptable to everyone. As we're not convinced that non-ascii byte strings can't be passed to the _escape function, cope with them by doing some gentle mangling to ascii. This ensures the output is well encoded but should give enough feedback if there are issues.

Is there anything else that's required? It's possible that some more _escape() calls need adding around some of the output, but this should be enough to resolve the bug as filed.

To post a comment you must log in.
Revision history for this message
Graham Binns (gmb) wrote :

Hi Martin,

This change looks good to me. A couple of cosmetic comments, though. The branch is good to land once you've fixed these:

[1]

61 + python_oops = {'id': 'OOPS-1234S101', 'reporter': 'edge',
62 + 'type': 'Exception', 'value': u'a unicode char (\xa7)',
63 + 'time': datetime(2008, 1, 13, 23, 14, 23, 00, utc)}

The formatting here's a bit dense. Dicts should be one element per line:

        python_oops = {
            'id': 'OOPS-1234S101',
            'reporter': 'edge',
            'type': 'Exception',
            'value': u'a unicode char (\xa7)',
            'time': datetime(2008, 1, 13, 23, 14, 23, 00, utc),
            }

74 + def test_renderHTML_with_unicode_data(self):
75 + # Using cStringIO here to simulate a file object open without an
76 + # encoding specified.

This comment should state the expected behaviour rather than saying what
the test does. (Saves lazy developers from reading the code unless the
thing breaks.)

review: Approve (code)
32. By Martin Packman

Address review comments from gmb

Revision history for this message
Martin Packman (gz) wrote :

> This change looks good to me. A couple of cosmetic comments, though. The
> branch is good to land once you've fixed these:

Thanks Graham, I've made those changes.

33. By Martin Packman

Comment addition suggested by allenap

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/oopstools/oops/dbsummaries.py'
2--- src/oopstools/oops/dbsummaries.py 2011-10-24 05:21:01 +0000
3+++ src/oopstools/oops/dbsummaries.py 2011-12-15 12:48:48 +0000
4@@ -42,6 +42,12 @@
5
6 def _escape(value):
7 if value is not None:
8+ if not isinstance(value, unicode):
9+ # If a random byte string gets here, ensure it's ascii. Latin-1 is
10+ # used to decode so the escaped form matches the original bytes.
11+ value = value.decode("latin1").encode("ascii", "backslashreplace")
12+ else:
13+ value = value.encode("utf-8")
14 value = cgi.escape(value)
15 return value
16
17@@ -656,6 +662,7 @@
18 fp.write('<html>\n'
19 '<head>\n'
20 '<title>Oops Report Summary</title>\n'
21+ '<meta http-equiv="Content-Type" content="text/html;charset=UTF-8" />\n'
22 '<link rel="stylesheet" type="text/css" href="%s/oops/static/oops.css" />\n'
23 '</head>\n'
24 '<body>\n'
25
26=== added file 'src/oopstools/oops/test/test_dbsummaries.py'
27--- src/oopstools/oops/test/test_dbsummaries.py 1970-01-01 00:00:00 +0000
28+++ src/oopstools/oops/test/test_dbsummaries.py 2011-12-15 12:48:48 +0000
29@@ -0,0 +1,55 @@
30+# Copyright 2005-2011 Canonical Ltd. All rights reserved.
31+#
32+# This program is free software: you can redistribute it and/or modify
33+# it under the terms of the GNU Affero General Public License as published by
34+# the Free Software Foundation, either version 3 of the License, or
35+# (at your option) any later version.
36+#
37+# This program is distributed in the hope that it will be useful,
38+# but WITHOUT ANY WARRANTY; without even the implied warranty of
39+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
40+# GNU Affero General Public License for more details.
41+#
42+# You should have received a copy of the GNU Affero General Public License
43+# along with this program. If not, see <http://www.gnu.org/licenses/>.
44+
45+
46+from datetime import (
47+ datetime,
48+ )
49+from cStringIO import StringIO
50+
51+from pytz import utc
52+from testtools import TestCase
53+from testtools.matchers import Contains
54+
55+from oopstools.oops.dbsummaries import WebAppErrorSummary
56+from oopstools.oops.models import parsed_oops_to_model_oops
57+
58+
59+class TestWebAppErrorSummary(TestCase):
60+
61+ def _createOops(self):
62+ python_oops = {
63+ 'id': 'OOPS-1234S101',
64+ 'reporter': 'edge',
65+ 'type': 'Exception',
66+ 'value': u'a unicode char (\xa7)',
67+ 'time': datetime(2008, 1, 13, 23, 14, 23, 00, utc),
68+ }
69+ ignored = parsed_oops_to_model_oops(
70+ python_oops, 'test_unicode_handling')
71+
72+ def setUp(self):
73+ super(TestWebAppErrorSummary, self).setUp()
74+ self._createOops()
75+ start = end = datetime(2008, 1, 13)
76+ prefixes = ['EDGE']
77+ self.summary = WebAppErrorSummary(start, end, prefixes)
78+
79+ def test_renderHTML_with_unicode_data(self):
80+ # Summarising an oops with a unicode exception value should output
81+ # a UTF-8 encoded html representation.
82+ fp = StringIO()
83+ self.summary.renderHTML(fp)
84+ self.assertThat(fp.getvalue(), Contains('a unicode char (\xc2\xa7)'))
85
86=== modified file 'src/oopstools/scripts/analyse_error_reports.py'
87--- src/oopstools/scripts/analyse_error_reports.py 2011-10-13 20:18:51 +0000
88+++ src/oopstools/scripts/analyse_error_reports.py 2011-12-15 12:48:48 +0000
89@@ -26,14 +26,7 @@
90 Prefix,
91 Report,
92 )
93-from oopstools.oops.oopsstore import OopsStore
94 from oopstools.oops import dbsummaries
95-from oopstools.oops.summaries import (
96- WebAppErrorSummary,
97- CheckwatchesErrorSummary,
98- CodeHostingWithRemoteSectionSummary,
99- GenericErrorSummary,
100-)
101
102
103 def main(argv=None):

Subscribers

People subscribed via source and target branches

to all changes: