Merge lp:~james-w/python-oops-tools/weird-statements into lp:python-oops-tools

Proposed by James Westby on 2013-02-25
Status: Merged
Approved by: James Westby on 2013-02-25
Approved revision: 54
Merged at revision: 54
Proposed branch: lp:~james-w/python-oops-tools/weird-statements
Merge into: lp:python-oops-tools
Diff against target: 53 lines (+32/-0)
2 files modified
src/oopstools/oops/models.py (+4/-0)
src/oopstools/oops/test/test_dboopsloader.py (+28/-0)
To merge this branch: bzr merge lp:~james-w/python-oops-tools/weird-statements
Reviewer Review Type Date Requested Status
Sidnei da Silva (community) 2013-02-25 Approve on 2013-02-25
Review via email: mp+150443@code.launchpad.net

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.

Description of the change

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

To post a comment you must log in.
Sidnei da Silva (sidnei) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/oopstools/oops/models.py'
2--- src/oopstools/oops/models.py 2012-07-27 04:27:53 +0000
3+++ src/oopstools/oops/models.py 2013-02-25 21:46:31 +0000
4@@ -387,6 +387,10 @@
5 else:
6 filler = (0, 0, 'unknown', 'unknown', 'unknown')
7 for row, action in enumerate(statements):
8+ # Must have a string as the statement
9+ if len(action) > 3 and not isinstance(action[3], basestring):
10+ action = action[:3] + (str(action[3]),) + action[4:]
11+ # Must have length == 5
12 statements[row] = tuple(action[:5]) + filler[len(action):]
13 duration = oops.get('duration')
14 if duration is not None:
15
16=== modified file 'src/oopstools/oops/test/test_dboopsloader.py'
17--- src/oopstools/oops/test/test_dboopsloader.py 2012-07-25 14:25:48 +0000
18+++ src/oopstools/oops/test/test_dboopsloader.py 2013-02-25 21:46:31 +0000
19@@ -232,6 +232,34 @@
20 (1, 2, 'foo', 'bar', 'baz'),
21 ], oops.statements)
22
23+ def test_dict_statement(self):
24+ # If one of the statements is a dict for some reason then
25+ # convert it to a string
26+ report = {
27+ 'id': 'dict-statement',
28+ 'timeline': [
29+ (1, 2, 'foo', {}, 'baz'),
30+ ],
31+ }
32+ oops = parsed_oops_to_model_oops(report, 'dict-statement')
33+ self.assertEqual([
34+ (1, 2, 'foo', '{}', 'baz'),
35+ ], oops.statements)
36+
37+ def test_None_statement(self):
38+ # If one of the statements is None for some reason then
39+ # convert it to a string
40+ report = {
41+ 'id': 'None-statement',
42+ 'timeline': [
43+ (1, 2, 'foo', None, 'baz'),
44+ ],
45+ }
46+ oops = parsed_oops_to_model_oops(report, 'None-statement')
47+ self.assertEqual([
48+ (1, 2, 'foo', 'None', 'baz'),
49+ ], oops.statements)
50+
51 def test_empty_report_long_id_uses_unknown_bug_889982(self):
52 # Some hashes will match the regex that used to extract app server
53 # names from oops ids. This is undesirable, because we don't want lots

Subscribers

People subscribed via source and target branches

to all changes: