python-oops-tools

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

  1. dbsummaries_utf8_encode_891186
  2. Merge into trunk
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)
lp:~gz/python-oops-tools/dbsummaries_utf8_encode_891186 updated
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.

lp:~gz/python-oops-tools/dbsummaries_utf8_encode_891186 updated
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
=== modified file 'src/oopstools/oops/dbsummaries.py'
--- src/oopstools/oops/dbsummaries.py 2011-10-24 05:21:01 +0000
+++ src/oopstools/oops/dbsummaries.py 2011-12-15 12:48:48 +0000
@@ -42,6 +42,12 @@
4242
43def _escape(value):43def _escape(value):
44 if value is not None:44 if value is not None:
45 if not isinstance(value, unicode):
46 # If a random byte string gets here, ensure it's ascii. Latin-1 is
47 # used to decode so the escaped form matches the original bytes.
48 value = value.decode("latin1").encode("ascii", "backslashreplace")
49 else:
50 value = value.encode("utf-8")
45 value = cgi.escape(value)51 value = cgi.escape(value)
46 return value52 return value
4753
@@ -656,6 +662,7 @@
656 fp.write('<html>\n'662 fp.write('<html>\n'
657 '<head>\n'663 '<head>\n'
658 '<title>Oops Report Summary</title>\n'664 '<title>Oops Report Summary</title>\n'
665 '<meta http-equiv="Content-Type" content="text/html;charset=UTF-8" />\n'
659 '<link rel="stylesheet" type="text/css" href="%s/oops/static/oops.css" />\n'666 '<link rel="stylesheet" type="text/css" href="%s/oops/static/oops.css" />\n'
660 '</head>\n'667 '</head>\n'
661 '<body>\n'668 '<body>\n'
662669
=== added file 'src/oopstools/oops/test/test_dbsummaries.py'
--- src/oopstools/oops/test/test_dbsummaries.py 1970-01-01 00:00:00 +0000
+++ src/oopstools/oops/test/test_dbsummaries.py 2011-12-15 12:48:48 +0000
@@ -0,0 +1,55 @@
1# Copyright 2005-2011 Canonical Ltd. All rights reserved.
2#
3# This program is free software: you can redistribute it and/or modify
4# it under the terms of the GNU Affero General Public License as published by
5# the Free Software Foundation, either version 3 of the License, or
6# (at your option) any later version.
7#
8# This program is distributed in the hope that it will be useful,
9# but WITHOUT ANY WARRANTY; without even the implied warranty of
10# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11# GNU Affero General Public License for more details.
12#
13# You should have received a copy of the GNU Affero General Public License
14# along with this program. If not, see <http://www.gnu.org/licenses/>.
15
16
17from datetime import (
18 datetime,
19 )
20from cStringIO import StringIO
21
22from pytz import utc
23from testtools import TestCase
24from testtools.matchers import Contains
25
26from oopstools.oops.dbsummaries import WebAppErrorSummary
27from oopstools.oops.models import parsed_oops_to_model_oops
28
29
30class TestWebAppErrorSummary(TestCase):
31
32 def _createOops(self):
33 python_oops = {
34 'id': 'OOPS-1234S101',
35 'reporter': 'edge',
36 'type': 'Exception',
37 'value': u'a unicode char (\xa7)',
38 'time': datetime(2008, 1, 13, 23, 14, 23, 00, utc),
39 }
40 ignored = parsed_oops_to_model_oops(
41 python_oops, 'test_unicode_handling')
42
43 def setUp(self):
44 super(TestWebAppErrorSummary, self).setUp()
45 self._createOops()
46 start = end = datetime(2008, 1, 13)
47 prefixes = ['EDGE']
48 self.summary = WebAppErrorSummary(start, end, prefixes)
49
50 def test_renderHTML_with_unicode_data(self):
51 # Summarising an oops with a unicode exception value should output
52 # a UTF-8 encoded html representation.
53 fp = StringIO()
54 self.summary.renderHTML(fp)
55 self.assertThat(fp.getvalue(), Contains('a unicode char (\xc2\xa7)'))
056
=== modified file 'src/oopstools/scripts/analyse_error_reports.py'
--- src/oopstools/scripts/analyse_error_reports.py 2011-10-13 20:18:51 +0000
+++ src/oopstools/scripts/analyse_error_reports.py 2011-12-15 12:48:48 +0000
@@ -26,14 +26,7 @@
26 Prefix,26 Prefix,
27 Report,27 Report,
28 )28 )
29from oopstools.oops.oopsstore import OopsStore
30from oopstools.oops import dbsummaries29from oopstools.oops import dbsummaries
31from oopstools.oops.summaries import (
32 WebAppErrorSummary,
33 CheckwatchesErrorSummary,
34 CodeHostingWithRemoteSectionSummary,
35 GenericErrorSummary,
36)
3730
3831
39def main(argv=None):32def main(argv=None):

Subscribers

People subscribed via source and target branches

to all changes: