launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/python-oops/extraction into lp:python-oops.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~lifeless/python-oops/extraction/+merge/70993

This rfc822 oops parsing logic is adapted from the LP code with the following changes:
 - no fixed type, just a dict (for greater flexability)
 - no custom iso parser, rather use the iso8601 module (and no, datetime.strptime is insufficient).

Would appreciate review of those changes, not the copy-pasted code :P
-- 
https://code.launchpad.net/~lifeless/python-oops/extraction/+merge/70993
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/python-oops/extraction into lp:python-oops.

=== modified file 'README'
--- README	2011-08-10 05:06:50 +0000
+++ README	2011-08-10 06:18:06 +0000
@@ -40,7 +40,14 @@
 Usage
 =====
 
-Coming soon.
+In Python, OOPS reports are dicts with some well known keys, but extensible
+simply by adding as many additional keys asneeded. The only constraint is that
+the resulting dict must be bson serializable : this is the standard to which
+new serializers are held. Some existing serializers cannot handle this degree
+of extensability and will ignore additional keys, and/or raise an error on keys
+they expect but which contain unexpected data.
+
+More coming soon.
 
 Installation
 ============

=== added file 'oops/serializer_rfc822.py'
--- oops/serializer_rfc822.py	1970-01-01 00:00:00 +0000
+++ oops/serializer_rfc822.py	2011-08-10 06:18:06 +0000
@@ -0,0 +1,100 @@
+# Copyright (c) 2010, 2011, Canonical Ltd
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Read / Write an OOPS dict as an rfc822 formatted message.
+
+This style of OOPS format is very web server specific, not extensible - it
+should be considered deprecated.
+
+It always has the following variables:
+
+* id: The name of this error report.
+* type: The type of the exception that occurred.
+* value: The value of the exception that occurred.
+* time: The time at which the exception occurred.
+* pageid: The identifier for the template/script that oopsed.
+* branch_nick: The branch nickname.
+* revno: The revision number of the branch.
+* tb_text: A text version of the traceback.
+* username: The user associated with the request.
+* url: The URL for the failed request.
+* req_vars: The request variables.
+"""
+
+
+__all__ = ['read']
+
+__metaclass__ = type
+
+import datetime
+import rfc822
+import re
+import urllib
+
+import iso8601
+
+
+def read(fp):
+    """Deserialize an OOPS from an RFC822 format message."""
+    msg = rfc822.Message(fp)
+    id = msg.getheader('oops-id')
+    exc_type = msg.getheader('exception-type')
+    exc_value = msg.getheader('exception-value')
+    date = iso8601.parse_date(msg.getheader('date'))
+    pageid = msg.getheader('page-id')
+    username = msg.getheader('user')
+    url = msg.getheader('url')
+    duration = int(float(msg.getheader('duration', '-1')))
+    informational = msg.getheader('informational')
+
+    # Explicitly use an iterator so we can process the file
+    # sequentially. In most instances the iterator will actually
+    # be the file object passed in because file objects should
+    # support iteration.
+    lines = iter(msg.fp)
+
+    # Request variables until the first blank line.
+    req_vars = []
+    for line in lines:
+        line = line.strip()
+        if line == '':
+            break
+        key, value = line.split('=', 1)
+        req_vars.append((urllib.unquote(key), urllib.unquote(value)))
+
+    # Statements until the next blank line.
+    statements = []
+    for line in lines:
+        line = line.strip()
+        if line == '':
+            break
+        match = re.match(
+            r'^(\d+)-(\d+)(?:@([\w-]+))?\s+(.*)', line)
+        assert match is not None, (
+            "Unable to interpret oops line: %s" % line)
+        start, end, db_id, statement = match.groups()
+        if db_id is not None:
+            db_id = intern(db_id)  # This string is repeated lots.
+        statements.append(
+            (int(start), int(end), db_id, statement))
+
+    # The rest is traceback.
+    tb_text = ''.join(lines)
+
+    return dict(id=id, type=exc_type, value=exc_value, time=date,
+            pageid=pageid, tb_text=tb_text, username=username, url=url,
+            duration=duration, req_vars=req_vars, db_statements=statements,
+            information=informational)

=== modified file 'oops/tests/__init__.py'
--- oops/tests/__init__.py	2011-08-10 05:08:58 +0000
+++ oops/tests/__init__.py	2011-08-10 06:18:06 +0000
@@ -22,6 +22,7 @@
 def test_suite():
     test_mod_names = [
         'uniquefileallocator',
+        'serializer_rfc822',
         ]
     return TestLoader().loadTestsFromNames(
         ['oops.tests.test_' + name for name in test_mod_names])

=== added file 'oops/tests/test_serializer_rfc822.py'
--- oops/tests/test_serializer_rfc822.py	1970-01-01 00:00:00 +0000
+++ oops/tests/test_serializer_rfc822.py	2011-08-10 06:18:06 +0000
@@ -0,0 +1,117 @@
+# Copyright (c) 2010, 2011, Canonical Ltd
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the legacy rfc822 based [de]serializer."""
+
+__metaclass__ = type
+
+import datetime
+import StringIO
+from textwrap import dedent
+
+from pytz import utc
+import testtools
+
+from oops.serializer_rfc822 import read
+
+
+class TestParsing(testtools.TestCase):
+
+    def test_read(self):
+        """Test ErrorReport.read()."""
+        fp = StringIO.StringIO(dedent("""\
+            Oops-Id: OOPS-A0001
+            Exception-Type: NotFound
+            Exception-Value: error message
+            Date: 2005-04-01T00:00:00+00:00
+            Page-Id: IFoo:+foo-template
+            User: Sample User
+            URL: http://localhost:9000/foo
+            Duration: 42
+
+            HTTP_USER_AGENT=Mozilla/5.0
+            HTTP_REFERER=http://localhost:9000/
+            name%3Dfoo=hello%0Aworld
+
+            00001-00005@store_a SELECT 1
+            00005-00010@store_b SELECT 2
+
+            traceback-text"""))
+        report = read(fp)
+        self.assertEqual(report['id'], 'OOPS-A0001')
+        self.assertEqual(report['type'], 'NotFound')
+        self.assertEqual(report['value'], 'error message')
+        self.assertEqual(
+                report['time'], datetime.datetime(2005, 4, 1, tzinfo=utc))
+        self.assertEqual(report['pageid'], 'IFoo:+foo-template')
+        self.assertEqual(report['tb_text'], 'traceback-text')
+        self.assertEqual(report['username'], 'Sample User')
+        self.assertEqual(report['url'], 'http://localhost:9000/foo')
+        self.assertEqual(report['duration'], 42)
+        self.assertEqual(len(report['req_vars']), 3)
+        self.assertEqual(report['req_vars'][0], ('HTTP_USER_AGENT',
+                                             'Mozilla/5.0'))
+        self.assertEqual(report['req_vars'][1], ('HTTP_REFERER',
+                                             'http://localhost:9000/'))
+        self.assertEqual(report['req_vars'][2], ('name=foo', 'hello\nworld'))
+        self.assertEqual(len(report['db_statements']), 2)
+        self.assertEqual(
+            report['db_statements'][0],
+            (1, 5, 'store_a', 'SELECT 1'))
+        self.assertEqual(
+            report['db_statements'][1],
+            (5, 10, 'store_b', 'SELECT 2'))
+
+    def test_read_no_store_id(self):
+        """Test ErrorReport.read() for old logs with no store_id."""
+        fp = StringIO.StringIO(dedent("""\
+            Oops-Id: OOPS-A0001
+            Exception-Type: NotFound
+            Exception-Value: error message
+            Date: 2005-04-01T00:00:00+00:00
+            Page-Id: IFoo:+foo-template
+            User: Sample User
+            URL: http://localhost:9000/foo
+            Duration: 42
+
+            HTTP_USER_AGENT=Mozilla/5.0
+            HTTP_REFERER=http://localhost:9000/
+            name%3Dfoo=hello%0Aworld
+
+            00001-00005 SELECT 1
+            00005-00010 SELECT 2
+
+            traceback-text"""))
+        report = read(fp)
+        self.assertEqual(report['id'], 'OOPS-A0001')
+        self.assertEqual(report['type'], 'NotFound')
+        self.assertEqual(report['value'], 'error message')
+        self.assertEqual(
+                report['time'], datetime.datetime(2005, 4, 1, tzinfo=utc))
+        self.assertEqual(report['pageid'], 'IFoo:+foo-template')
+        self.assertEqual(report['tb_text'], 'traceback-text')
+        self.assertEqual(report['username'], 'Sample User')
+        self.assertEqual(report['url'], 'http://localhost:9000/foo')
+        self.assertEqual(report['duration'], 42)
+        self.assertEqual(len(report['req_vars']), 3)
+        self.assertEqual(report['req_vars'][0], ('HTTP_USER_AGENT',
+                                             'Mozilla/5.0'))
+        self.assertEqual(report['req_vars'][1], ('HTTP_REFERER',
+                                             'http://localhost:9000/'))
+        self.assertEqual(report['req_vars'][2], ('name=foo', 'hello\nworld'))
+        self.assertEqual(len(report['db_statements']), 2)
+        self.assertEqual(report['db_statements'][0], (1, 5, None, 'SELECT 1'))
+        self.assertEqual(report['db_statements'][1], (5, 10, None, 'SELECT 2'))

=== modified file 'setup.py'
--- setup.py	2011-08-10 05:08:58 +0000
+++ setup.py	2011-08-10 06:18:06 +0000
@@ -38,6 +38,7 @@
           'Programming Language :: Python',
           ],
       install_requires = [
+          'iso8601',
           'pytz',
           ],
       extras_require = dict(

=== modified file 'versions.cfg'
--- versions.cfg	2011-08-10 05:08:58 +0000
+++ versions.cfg	2011-08-10 06:18:06 +0000
@@ -3,6 +3,7 @@
 
 [versions]
 fixtures = 0.3.6
+iso8601 = 0.1.4
 pytz = 2010o
 setuptools = 0.6c11
 testtools = 0.9.11