launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/python-oops-tools/bug-890001 into lp:python-oops-tools.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #890001 in python-oops-tools: "Crashes when importing an OOPS with a corrupt timeline"
  https://bugs.launchpad.net/python-oops-tools/+bug/890001

For more details, see:
https://code.launchpad.net/~lifeless/python-oops-tools/bug-890001/+merge/82334

Handle a crash in oops-tools when a bad report has been generated by LP. This replaces a monkey patch we're currently running.
-- 
https://code.launchpad.net/~lifeless/python-oops-tools/bug-890001/+merge/82334
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/python-oops-tools/bug-890001 into lp:python-oops-tools.

=== modified file 'src/oopstools/NEWS.txt'
--- src/oopstools/NEWS.txt	2011-11-11 04:00:56 +0000
+++ src/oopstools/NEWS.txt	2011-11-16 01:30:28 +0000
@@ -18,6 +18,9 @@
 * The req_vars variable in OOPS reports may now be a dict.
   (Robert Collins, #888866)
 
+* Corrupt timeline objects are handled more gracefully if they are not a list,
+  or the list rows are too short or too long. (Robert Collins, #890001)
+
 0.6.1
 =====
 

=== modified file 'src/oopstools/oops/models.py'
--- src/oopstools/oops/models.py	2011-11-11 04:00:56 +0000
+++ src/oopstools/oops/models.py	2011-11-16 01:30:28 +0000
@@ -345,6 +345,14 @@
         elif key == 'HTTP_USER_AGENT':
             data['user_agent'] = conform(value, 200)
     statements = oops.get('timeline') or []
+    # Sanity check/coerce the statements into what we expect.
+    # Must be a list:
+    if not isinstance(statements, list):
+        statements = [(0, 0, 'badtimeline', str(statements))]
+    else:
+        filler = (0, 0, 'unknown', 'unknown')
+        for row, action in enumerate(statements):
+            statements[row] = tuple(action[:4]) + filler[len(action):]
     duration = oops.get('duration')
     if duration is not None:
         total_time = int(duration * 1000)

=== modified file 'src/oopstools/oops/test/test_dboopsloader.py'
--- src/oopstools/oops/test/test_dboopsloader.py	2011-11-11 04:00:56 +0000
+++ src/oopstools/oops/test/test_dboopsloader.py	2011-11-16 01:30:28 +0000
@@ -186,3 +186,46 @@
         }
         oops = parsed_oops_to_model_oops(report, 'bug-888866')
         self.assertEqual('myuseragent', oops.user_agent)
+
+    def test_nonlist_timeline(self):
+        # A timeline that is nonzero and not a list is turned into a one-entry
+        # timeline starting when the oops starts with no duration and category
+        # badtimeline.
+        report = {
+            'id': 'bug-890001-1',
+            'timeline': 'a'
+        }
+        oops = parsed_oops_to_model_oops(report, 'bug-890001-1')
+        self.assertEqual([(0, 0, 'badtimeline', 'a')], oops.statements)
+        
+    def test_short_timeline_row(self):
+        # A timeline row that is short is padded with 'unknown' or 0's.
+        report = {
+            'id': 'bug-890001-2',
+            'timeline': [
+                (),
+                (1,),
+                (1, 2,),
+                (1, 2, 'foo'),
+                ],
+        }
+        oops = parsed_oops_to_model_oops(report, 'bug-890001-2')
+        self.assertEqual([
+            (0, 0, 'unknown', 'unknown'),
+            (1, 0, 'unknown', 'unknown'),
+            (1, 2, 'unknown', 'unknown'),
+            (1, 2, 'foo', 'unknown'),
+            ], oops.statements)
+
+    def test_long_timeline_row(self):
+        # A timeline row that is long is truncated.
+        report = {
+            'id': 'bug-890001-3',
+            'timeline': [
+                (1, 2, 'foo', 'bar', 'baz'),
+                ],
+        }
+        oops = parsed_oops_to_model_oops(report, 'bug-890001-3')
+        self.assertEqual([
+            (1, 2, 'foo', 'bar'),
+            ], oops.statements)