Merge lp:~lifeless/python-oops-datedir-repo/extraction into lp:python-oops-datedir-repo

[Robert Collins]

This handles the new topic and reporter keys (mapping pageid to topic on in-out for compat with oops-tools (until oops-tools uses this library :P)), and takes more encoding responsibility so that callers can relax a little (given the definition we're aiming at of 'bson serializable is all we need).

[Steve Kowalik (stevenk)]

review: Approve (code)

=== modified file 'oops_datedir_repo/__init__.py'
--- oops_datedir_repo/__init__.py	2011-08-16 02:21:20 +0000
+++ oops_datedir_repo/__init__.py	2011-08-17 01:17:24 +0000
@@ -25,7 +25,7 @@
 # established at this point, and setup.py will use a version of next-$(revno).
 # If the releaselevel is 'final', then the tarball will be major.minor.micro.
 # Otherwise it is major.minor.micro~$(revno).
-__version__ = (0, 0, 2, 'beta', 0)
+__version__ = (0, 0, 3, 'beta', 0)
 
 __all__ = [
     'DateDirRepo',

=== modified file 'oops_datedir_repo/serializer_rfc822.py'
--- oops_datedir_repo/serializer_rfc822.py	2011-08-15 05:30:39 +0000
+++ oops_datedir_repo/serializer_rfc822.py	2011-08-17 01:17:24 +0000
@@ -19,13 +19,16 @@
 This style of OOPS format is very web server specific, not extensible - it
 should be considered deprecated.
 
-The reports this serializer handles always have the following variables:
+The reports this serializer handles always have the following variables (See
+the python-oops api docs for more information about these 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.
+* reporter: The reporting program.
+* topic: The identifier for the template/script that oopsed.
+  [this is written as Page-Id for compatibility with as yet unported tools.]
 * branch_nick: The branch nickname.
 * revno: The revision number of the branch.
 * tb_text: A text version of the traceback.
@@ -37,6 +40,9 @@
 * revno: The revision that the branch was at.
 * Informational: A flag, True if the error wasn't fatal- if it was
   'informational'.
+  [Deprecated - this is no longer part of the oops report conventions. Existing
+   reports with it set are still read, but the key is only present if it was
+   truely in the report.]
 """
 
 
@@ -67,13 +73,16 @@
         date =iso8601.parse_date(msg.getheader('date'))
     else:
         date = None
-    pageid = msg.getheader('page-id')
+    topic = msg.getheader('topic')
+    if topic is None:
+        topic = msg.getheader('page-id')
     username = msg.getheader('user')
     url = msg.getheader('url')
     duration = int(float(msg.getheader('duration', '-1')))
     informational = msg.getheader('informational')
     branch_nick = msg.getheader('branch')
     revno = msg.getheader('revision')
+    reporter = msg.getheader('oops-reporter')
 
     # Explicitly use an iterator so we can process the file
     # sequentially. In most instances the iterator will actually
@@ -109,10 +118,15 @@
     # 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,
+    result = dict(id=id, type=exc_type, value=exc_value, time=date,
+            topic=topic, tb_text=tb_text, username=username, url=url,
             duration=duration, req_vars=req_vars, db_statements=statements,
-            informational=informational, branch_nick=branch_nick, revno=revno)
+            branch_nick=branch_nick, revno=revno)
+    if informational is not None:
+        result['informational'] = informational
+    if reporter is not None:
+        result['reporter'] = reporter
+    return result
 
 
 def _normalise_whitespace(s):
@@ -151,45 +165,41 @@
 def to_chunks(report):
     """Returns a list of bytestrings making up the serialized oops."""
     chunks = []
-    chunks.append('Oops-Id: %s\n' % _normalise_whitespace(report['id']))
-    if 'type' in report:
-        chunks.append(
-            'Exception-Type: %s\n' % _normalise_whitespace(report['type']))
-    if 'value' in report:
-        chunks.append(
-            'Exception-Value: %s\n' % _normalise_whitespace(report['value']))
+    def header(label, key, optional=True):
+        if optional and key not in report:
+            return
+        value = _safestr(report[key])
+        value = _normalise_whitespace(value)
+        chunks.append('%s: %s\n' % (label, value))
+    header('Oops-Id', 'id', optional=False)
+    header('Exception-Type', 'type')
+    header('Exception-Value', 'value')
     if 'time' in report:
         chunks.append('Date: %s\n' % report['time'].isoformat())
-    if 'pageid' in report:
-        chunks.append(
-                'Page-Id: %s\n' % _normalise_whitespace(report['pageid']))
-    if 'branch_nick' in report:
-        chunks.append('Branch: %s\n' % _safestr(report['branch_nick']))
-    if 'revno' in report:
-        chunks.append('Revision: %s\n' % report['revno'])
-    if 'username' in report:
-        chunks.append(
-                'User: %s\n' % _normalise_whitespace(report['username']))
-    if 'url' in report:
-        chunks.append('URL: %s\n' % _normalise_whitespace(report['url']))
-    if 'duration' in report:
-        chunks.append('Duration: %s\n' % report['duration'])
-    if 'informational' in report:
-        chunks.append('Informational: %s\n' % report['informational'])
+    header('Page-Id', 'topic')
+    header('Branch', 'branch_nick')
+    header('Revision', 'revno')
+    header('User', 'username')
+    header('URL', 'url')
+    header('Duration', 'duration')
+    header('Informational', 'informational')
+    header('Oops-Reporter', 'reporter')
     chunks.append('\n')
     safe_chars = ';/\\?:@&+$, ()*!'
     if 'req_vars' in report:
         for key, value in report['req_vars']:
-            chunks.append('%s=%s\n' % (urllib.quote(key, safe_chars),
-                                  urllib.quote(value, safe_chars)))
+            chunks.append('%s=%s\n' % (
+                    urllib.quote(_safestr(key), safe_chars),
+                    urllib.quote(_safestr(value), safe_chars)))
         chunks.append('\n')
     if 'db_statements' in report:
         for (start, end, database_id, statement) in report['db_statements']:
             chunks.append('%05d-%05d@%s %s\n' % (
-                start, end, database_id, _normalise_whitespace(statement)))
+                start, end, _safestr(database_id),
+                _safestr(_normalise_whitespace(statement))))
         chunks.append('\n')
     if 'tb_text' in report:
-        chunks.append(report['tb_text'])
+        chunks.append(_safestr(report['tb_text']))
     return chunks
 
 

=== modified file 'oops_datedir_repo/tests/test_serializer_rfc822.py'
--- oops_datedir_repo/tests/test_serializer_rfc822.py	2011-08-15 05:30:39 +0000
+++ oops_datedir_repo/tests/test_serializer_rfc822.py	2011-08-17 01:17:24 +0000
@@ -41,7 +41,7 @@
             Exception-Type: NotFound
             Exception-Value: error message
             Date: 2005-04-01T00:00:00+00:00
-            Page-Id: IFoo:+foo-template
+            Topic: IFoo:+foo-template
             User: Sample User
             URL: http://localhost:9000/foo
             Duration: 42
@@ -60,7 +60,7 @@
         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['topic'], 'IFoo:+foo-template')
         self.assertEqual(report['tb_text'], 'traceback-text')
         self.assertEqual(report['username'], 'Sample User')
         self.assertEqual(report['url'], 'http://localhost:9000/foo')
@@ -86,7 +86,7 @@
             Exception-Type: NotFound
             Exception-Value: error message
             Date: 2005-04-01T00:00:00+00:00
-            Page-Id: IFoo:+foo-template
+            Topic: IFoo:+foo-template
             User: Sample User
             URL: http://localhost:9000/foo
             Duration: 42
@@ -105,7 +105,7 @@
         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['topic'], 'IFoo:+foo-template')
         self.assertEqual(report['tb_text'], 'traceback-text')
         self.assertEqual(report['username'], 'Sample User')
         self.assertEqual(report['url'], 'http://localhost:9000/foo')
@@ -119,7 +119,6 @@
         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'))
-        self.assertFalse(report['informational'])
 
     def test_read_branch_nick_revno(self):
         """Test ErrorReport.read()."""
@@ -128,7 +127,6 @@
             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
@@ -147,6 +145,45 @@
         self.assertEqual(report['branch_nick'], 'mybranch')
         self.assertEqual(report['revno'], '45')
 
+    def test_read_reporter(self):
+        """Test ErrorReport.read()."""
+        fp = StringIO.StringIO(dedent("""\
+            Oops-Id: OOPS-A0001
+            Oops-Reporter: foo/bar
+
+            """))
+        report = read(fp)
+        self.assertEqual(report['reporter'], 'foo/bar')
+
+    def test_read_pageid_to_topic(self):
+        """Test ErrorReport.read()."""
+        fp = StringIO.StringIO(dedent("""\
+            Oops-Id: OOPS-A0001
+            Page-Id: IFoo:+foo-template
+
+            """))
+        report = read(fp)
+        self.assertEqual(report['topic'], 'IFoo:+foo-template')
+
+    def test_read_informational_read(self):
+        """Test ErrorReport.read()."""
+        fp = StringIO.StringIO(dedent("""\
+            Oops-Id: OOPS-A0001
+            Informational: True
+
+            """))
+        report = read(fp)
+        self.assertEqual('True', report['informational'])
+
+    def test_read_no_informational_no_key(self):
+        """Test ErrorReport.read()."""
+        fp = StringIO.StringIO(dedent("""\
+            Oops-Id: OOPS-A0001
+
+            """))
+        report = read(fp)
+        self.assertFalse('informational' in report)
+
     def test_minimal_oops(self):
         # If we get a crazy-small oops, we can read it sensibly.  Because there
         # is existing legacy code, all keys are filled in with None, [] rather
@@ -159,14 +196,13 @@
         self.assertEqual(report['type'], None)
         self.assertEqual(report['value'], None)
         self.assertEqual(report['time'], None)
-        self.assertEqual(report['pageid'], None)
+        self.assertEqual(report['topic'], None)
         self.assertEqual(report['tb_text'], '')
         self.assertEqual(report['username'], None)
         self.assertEqual(report['url'], None)
         self.assertEqual(report['duration'], -1)
         self.assertEqual(len(report['req_vars']), 0)
         self.assertEqual(len(report['db_statements']), 0)
-        self.assertFalse(report['informational'])
         self.assertEqual(report['branch_nick'], None)
         self.assertEqual(report['revno'], None)
 
@@ -180,7 +216,7 @@
             'type': 'NotFound',
             'value': 'error message',
             'time': datetime.datetime(2005, 04, 01, 00, 00, 00, tzinfo=utc),
-            'pageid': 'IFoo:+foo-template',
+            'topic': 'IFoo:+foo-template',
             'tb_text': 'traceback-text',
             'username': 'Sample User',
             'url': 'http://localhost:9000/foo',
@@ -223,7 +259,7 @@
             'type': 'NotFound',
             'value': 'error message',
             'time': datetime.datetime(2005, 04, 01, 00, 00, 00, tzinfo=utc),
-            'pageid': 'IFoo:+foo-template',
+            'topic': 'IFoo:+foo-template',
             'tb_text': 'traceback-text',
             'username': 'Sample User',
             'url': 'http://localhost:9000/foo',
@@ -269,3 +305,26 @@
             "Oops-Id: OOPS-1234\n",
             "\n"
             ], to_chunks(report))
+
+    def test_reporter(self):
+        report = {'reporter': 'foo', 'id': 'bar'}
+        self.assertEqual([
+            "Oops-Id: bar\n",
+            "Oops-Reporter: foo\n",
+            "\n",
+            ], to_chunks(report))
+
+    def test_bad_strings(self):
+        # Because of the rfc822 limitations, not all strings can be supported
+        # in this format - particularly in headers... so all header strings are
+        # passed through an escape process.
+        report = {'id': u'\xeafoo'}
+        self.assertEqual([
+            "Oops-Id: \\xeafoo\n",
+            "\n",
+            ], to_chunks(report))
+        report = {'id': '\xeafoo'}
+        self.assertEqual([
+            "Oops-Id: \\xeafoo\n",
+            "\n",
+            ], to_chunks(report))

=== modified file 'setup.py'
--- setup.py	2011-08-16 02:21:20 +0000
+++ setup.py	2011-08-17 01:17:24 +0000
@@ -22,7 +22,7 @@
 description = file(os.path.join(os.path.dirname(__file__), 'README'), 'rb').read()
 
 setup(name="oops_datedir_repo",
-      version="0.0.2",
+      version="0.0.3",
       description="OOPS disk serialisation and repository management.",
       long_description=description,
       maintainer="Launchpad Developers",