Merge lp:~james-w/python-oops/record-published-explicitly into lp:python-oops

Proposed by James Westby on 2012-06-07
Status: Rejected
Rejected by: James Westby on 2012-06-08
Proposed branch: lp:~james-w/python-oops/record-published-explicitly
Merge into: lp:python-oops
Diff against target: 125 lines (+39/-11)
4 files modified
oops/config.py (+4/-3)
oops/publishers.py (+1/-1)
oops/tests/test_config.py (+25/-3)
oops/tests/test_publishers.py (+9/-4)
To merge this branch: bzr merge lp:~james-w/python-oops/record-published-explicitly
Reviewer Review Type Date Requested Status
Robert Collins (community) Needs Fixing on 2012-06-08
j.c.sackett (community) 2012-06-07 Approve on 2012-06-07
Review via email: mp+109239@code.launchpad.net

Commit message

Change to explicitly track published state.

If the state is tracked by the presence of the id then a report can't come
with a pre-generated id.

This prevents applications that know an oops will be generated, but don't
have access to the generation, from reporting the oops id to anyone.

Description of the change

Hi,

In discussions around the fact that Django can't easily tell anyone
what the id is for the oops they just encountered Rob said that the
best way to handle it was probably to have django allocate the oops id.

That works great, and the publishers will just use it. The only drawback
is that it means that publish_only_new will never trigger its publisher
any more, because of the double meaning of the id in the report.

Applications that do this could avoid publish_only_new, at the cost of
duplicating all oopses and then de-duplicating them later.

Instead I propose to track the published state more explicitly in the
oops, which this branch does.

Thanks,

James

To post a comment you must log in.
j.c.sackett (jcsackett) wrote :

Looks good.

Thanks, James.

review: Approve
Robert Collins (lifeless) wrote :

So the challenge here is that a second publisher will publish a report *containing* the key 'published'. I think we want the metadata, such as it is, separate.

So, we need to change the API rather than stashing it in the report.

review: Needs Fixing

Unmerged revisions

30. By James Westby on 2012-06-07

Check for the published key in publish_new_only.

29. By James Westby on 2012-06-07

Set a published key in the report to indicate when it has been published.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'oops/config.py'
2--- oops/config.py 2011-11-13 21:24:42 +0000
3+++ oops/config.py 2012-06-07 21:03:22 +0000
4@@ -159,13 +159,13 @@
5 returned to the caller, allowing them to handle the case when multiple
6 publishers allocated different ids. The id from the last publisher is
7 left in the report's id key. This is done for three reasons:
8-
9+
10 * As a convenience for the common case when only one publisher is
11 present.
12-
13+
14 * This allows the next publisher in the chain to reuse that id if it
15 wishes.
16-
17+
18 * Showing users that a report was made only needs one id and this means
19 other code doesn't need to arbitrarily pick one - it is done by
20 publish().
21@@ -189,6 +189,7 @@
22 id = publisher(report)
23 if id:
24 report['id'] = id
25+ report['published'] = True
26 result.append(id)
27 else:
28 result.append(None)
29
30=== modified file 'oops/publishers.py'
31--- oops/publishers.py 2011-11-13 21:24:42 +0000
32+++ oops/publishers.py 2012-06-07 21:03:22 +0000
33@@ -33,7 +33,7 @@
34 >>> config.publishers.append(publish_new_only(datedir_repo.publish))
35 """
36 def result(report):
37- if report.get('id'):
38+ if report.get('published'):
39 return None
40 return publisher(report)
41 return result
42
43=== modified file 'oops/tests/test_config.py'
44--- oops/tests/test_config.py 2011-11-13 21:24:42 +0000
45+++ oops/tests/test_config.py 2012-06-07 21:03:22 +0000
46@@ -79,7 +79,7 @@
47 config.publishers.append(pub_2)
48 report = {}
49 self.assertEqual(['1', '2'], config.publish(report))
50- self.assertEqual({'id': '2'}, report)
51+ self.assertEqual('2', report['id'])
52
53 def test_publish_filters_first(self):
54 calls = []
55@@ -101,7 +101,7 @@
56 self.assertEqual(['ok', 'block'], calls)
57
58 def test_publishers_returning_not_published_no_change_to_report(self):
59- # If a publisher returns non-zero (indicating it did not publish) no
60+ # If a publisher returns a falseish value (indicating it did not publish) no
61 # change is made to the report - this permits chaining publishers as a
62 # primary and fallback: The fallback can choose to do nothing if there
63 # is an id already assigned (and returns None to signal what it did);
64@@ -115,4 +115,26 @@
65 config.publishers.append(pub_noop)
66 report = {}
67 self.assertEqual(['1', None], config.publish(report))
68- self.assertEqual({'id': '1'}, report)
69+ self.assertEqual({'id': '1', 'published': True}, report)
70+
71+ def test_publisher_returning_not_published_doesnt_set_published(self):
72+ # If a publisher indicates that it didn't publish then the
73+ # 'published' key is not set in the report
74+ def pub_noop(report):
75+ return ''
76+ config = Config()
77+ config.publishers.append(pub_noop)
78+ report = {}
79+ config.publish(report)
80+ self.assertEqual({}, report)
81+
82+ def test_publisher_returning_published_sets_published(self):
83+ # If a publisher indicates that it did publish then the
84+ # 'published' key is set in the report
85+ def pub_succeed(report):
86+ return '1'
87+ config = Config()
88+ config.publishers.append(pub_succeed)
89+ report = {}
90+ config.publish(report)
91+ self.assertEqual({'id': '1', 'published': True}, report)
92
93=== modified file 'oops/tests/test_publishers.py'
94--- oops/tests/test_publishers.py 2011-11-13 21:24:42 +0000
95+++ oops/tests/test_publishers.py 2012-06-07 21:03:22 +0000
96@@ -18,20 +18,25 @@
97 __metaclass__ = type
98
99 import testtools
100-from testtools.matchers import Is
101
102 from oops import publish_new_only
103
104 class TestPublishers(testtools.TestCase):
105
106- def test_publish_new_only_id_in_report(self):
107+ def test_publish_new_only_published_in_report(self):
108 def pub_fail(report):
109 self.fail('publication called')
110 publisher = publish_new_only(pub_fail)
111- publisher({'id': 'foo'})
112+ publisher({'published': True})
113
114- def test_publish_new_only_no_id_in_report(self):
115+ def test_publish_new_only_no_published_in_report(self):
116 calls = []
117 publisher = publish_new_only(calls.append)
118 publisher({'foo': 'bar'})
119 self.assertEqual([{'foo': 'bar'}], calls)
120+
121+ def test_publish_new_only_no_published_false_in_report(self):
122+ calls = []
123+ publisher = publish_new_only(calls.append)
124+ publisher({'published': False})
125+ self.assertEqual([{'published': False}], calls)

Subscribers

People subscribed via source and target branches

to all changes: