launchpad-reviewers team mailing list archive

[James Westby <jw+debian@xxxxxxxxxxxxxxx>]

James Westby has proposed merging lp:~james-w/python-oops-tools/weird-statements into lp:python-oops-tools.

Commit message:
Ensure that statements are always strings.

Fixes a crash when adding an oops that doesn't have a string for one of
the statements. The data being sent is bad, but a crash means no further
oopses can be processed.

Requested reviews:
  python-oops-tools reviewers (oops-tools-reviewers)

For more details, see:
https://code.launchpad.net/~james-w/python-oops-tools/weird-statements/+merge/150443

Hi,

This fixes a bug we found in production when a service was generating bad
data for the statement info in the timeline.

This avoids the crash by casting to a string. The data will be nonsense, but
it avoids the crash so that other oopses can be processed.

Thanks,

James
-- 
https://code.launchpad.net/~james-w/python-oops-tools/weird-statements/+merge/150443
Your team python-oops-tools reviewers is requested to review the proposed merge of lp:~james-w/python-oops-tools/weird-statements into lp:python-oops-tools.

=== modified file 'src/oopstools/oops/models.py'
--- src/oopstools/oops/models.py	2012-07-27 04:27:53 +0000
+++ src/oopstools/oops/models.py	2013-02-25 21:46:31 +0000
@@ -387,6 +387,10 @@
     else:
         filler = (0, 0, 'unknown', 'unknown', 'unknown')
         for row, action in enumerate(statements):
+            # Must have a string as the statement
+            if len(action) > 3 and not isinstance(action[3], basestring):
+                action = action[:3] + (str(action[3]),) + action[4:]
+            # Must have length == 5
             statements[row] = tuple(action[:5]) + filler[len(action):]
     duration = oops.get('duration')
     if duration is not None:

=== modified file 'src/oopstools/oops/test/test_dboopsloader.py'
--- src/oopstools/oops/test/test_dboopsloader.py	2012-07-25 14:25:48 +0000
+++ src/oopstools/oops/test/test_dboopsloader.py	2013-02-25 21:46:31 +0000
@@ -232,6 +232,34 @@
             (1, 2, 'foo', 'bar', 'baz'),
             ], oops.statements)
 
+    def test_dict_statement(self):
+        # If one of the statements is a dict for some reason then
+        # convert it to a string
+        report = {
+            'id': 'dict-statement',
+            'timeline': [
+                (1, 2, 'foo', {}, 'baz'),
+                ],
+        }
+        oops = parsed_oops_to_model_oops(report, 'dict-statement')
+        self.assertEqual([
+            (1, 2, 'foo', '{}', 'baz'),
+            ], oops.statements)
+
+    def test_None_statement(self):
+        # If one of the statements is None for some reason then
+        # convert it to a string
+        report = {
+            'id': 'None-statement',
+            'timeline': [
+                (1, 2, 'foo', None, 'baz'),
+                ],
+        }
+        oops = parsed_oops_to_model_oops(report, 'None-statement')
+        self.assertEqual([
+            (1, 2, 'foo', 'None', 'baz'),
+            ], oops.statements)
+
     def test_empty_report_long_id_uses_unknown_bug_889982(self):
         # Some hashes will match the regex that used to extract app server
         # names from oops ids. This is undesirable, because we don't want lots