launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #889982 in python-oops-tools: "crash using hashed oops id as appinstance"
  https://bugs.launchpad.net/python-oops-tools/+bug/889982

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

Stop applying the old embedded-oops-prefix heuristic when the data looks inapplicable. Should let all staging oopses through properly. Also shows LP has at least one bug where an empty reporter is used. I'm filing that separately.
-- 
https://code.launchpad.net/~lifeless/python-oops-tools/bug-889982/+merge/82337
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/python-oops-tools/bug-889982 into lp:python-oops-tools.

=== modified file 'src/oopstools/NEWS.txt'
--- src/oopstools/NEWS.txt	2011-11-16 01:27:58 +0000
+++ src/oopstools/NEWS.txt	2011-11-16 02:24:24 +0000
@@ -5,9 +5,16 @@
 NEXT
 ====
 
+* 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)
+
 * Flush stdout when logging an OOPS receipt in amqp2disk.
   (Robert Collins, #884569)
 
+* Long oops ids - which are likely hashes - no longer have the old appserver id
+  heuristic applied to them: if they have a missing or empty reporter, they are
+  associated with the instance 'unknown'. (Robert Collins, #889982)
+
 * Mixed case OOPS reports can now be looked up in the web UI without their
   OOPS-prefix (if they had one). (Robert Collins, #884571)
 
@@ -18,9 +25,6 @@
 * 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-16 01:27:58 +0000
+++ src/oopstools/oops/models.py	2011-11-16 02:24:24 +0000
@@ -281,7 +281,8 @@
     if not prefix:
         # Legacy support for pre-reporter using OOPSes.
         prefix_match = oops_re.match(oopsid)
-        if prefix_match is not None:
+        # 37 is len (OOPS-<hash>)
+        if prefix_match is not None and len(oopsid) < 37:
             prefix = prefix_match.group('oopsprefix')
         else:
             prefix = 'UNKNOWN'

=== modified file 'src/oopstools/oops/test/test_dboopsloader.py'
--- src/oopstools/oops/test/test_dboopsloader.py	2011-11-16 01:27:58 +0000
+++ src/oopstools/oops/test/test_dboopsloader.py	2011-11-16 02:24:24 +0000
@@ -229,3 +229,15 @@
         self.assertEqual([
             (1, 2, 'foo', 'bar'),
             ], 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
+        # of one-off instance ids. Rather we should use 'unknown' if its
+        # unknown.
+        report = {
+            'id': 'OOPS-68383510f5054932567230492013ce90',
+            'reporter': ''
+        }
+        oops = parsed_oops_to_model_oops(report, 'bug-889982')
+        self.assertEqual('UNKNOWN', oops.appinstance.title)