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

Proposed by James Westby
Status: Rejected
Rejected by: James Westby
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
j.c.sackett (community) Approve
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.
Revision history for this message
j.c.sackett (jcsackett) wrote :

Looks good.

Thanks, James.

review: Approve
Revision history for this message
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

Check for the published key in publish_new_only.

29. By James Westby

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
=== modified file 'oops/config.py'
--- oops/config.py 2011-11-13 21:24:42 +0000
+++ oops/config.py 2012-06-07 21:03:22 +0000
@@ -159,13 +159,13 @@
159 returned to the caller, allowing them to handle the case when multiple159 returned to the caller, allowing them to handle the case when multiple
160 publishers allocated different ids. The id from the last publisher is160 publishers allocated different ids. The id from the last publisher is
161 left in the report's id key. This is done for three reasons:161 left in the report's id key. This is done for three reasons:
162 162
163 * As a convenience for the common case when only one publisher is163 * As a convenience for the common case when only one publisher is
164 present.164 present.
165 165
166 * This allows the next publisher in the chain to reuse that id if it166 * This allows the next publisher in the chain to reuse that id if it
167 wishes.167 wishes.
168 168
169 * Showing users that a report was made only needs one id and this means169 * Showing users that a report was made only needs one id and this means
170 other code doesn't need to arbitrarily pick one - it is done by170 other code doesn't need to arbitrarily pick one - it is done by
171 publish().171 publish().
@@ -189,6 +189,7 @@
189 id = publisher(report)189 id = publisher(report)
190 if id:190 if id:
191 report['id'] = id191 report['id'] = id
192 report['published'] = True
192 result.append(id)193 result.append(id)
193 else:194 else:
194 result.append(None)195 result.append(None)
195196
=== modified file 'oops/publishers.py'
--- oops/publishers.py 2011-11-13 21:24:42 +0000
+++ oops/publishers.py 2012-06-07 21:03:22 +0000
@@ -33,7 +33,7 @@
33 >>> config.publishers.append(publish_new_only(datedir_repo.publish))33 >>> config.publishers.append(publish_new_only(datedir_repo.publish))
34 """34 """
35 def result(report):35 def result(report):
36 if report.get('id'):36 if report.get('published'):
37 return None37 return None
38 return publisher(report)38 return publisher(report)
39 return result39 return result
4040
=== modified file 'oops/tests/test_config.py'
--- oops/tests/test_config.py 2011-11-13 21:24:42 +0000
+++ oops/tests/test_config.py 2012-06-07 21:03:22 +0000
@@ -79,7 +79,7 @@
79 config.publishers.append(pub_2)79 config.publishers.append(pub_2)
80 report = {}80 report = {}
81 self.assertEqual(['1', '2'], config.publish(report))81 self.assertEqual(['1', '2'], config.publish(report))
82 self.assertEqual({'id': '2'}, report)82 self.assertEqual('2', report['id'])
8383
84 def test_publish_filters_first(self):84 def test_publish_filters_first(self):
85 calls = []85 calls = []
@@ -101,7 +101,7 @@
101 self.assertEqual(['ok', 'block'], calls)101 self.assertEqual(['ok', 'block'], calls)
102102
103 def test_publishers_returning_not_published_no_change_to_report(self):103 def test_publishers_returning_not_published_no_change_to_report(self):
104 # If a publisher returns non-zero (indicating it did not publish) no104 # If a publisher returns a falseish value (indicating it did not publish) no
105 # change is made to the report - this permits chaining publishers as a105 # change is made to the report - this permits chaining publishers as a
106 # primary and fallback: The fallback can choose to do nothing if there106 # primary and fallback: The fallback can choose to do nothing if there
107 # is an id already assigned (and returns None to signal what it did);107 # is an id already assigned (and returns None to signal what it did);
@@ -115,4 +115,26 @@
115 config.publishers.append(pub_noop)115 config.publishers.append(pub_noop)
116 report = {}116 report = {}
117 self.assertEqual(['1', None], config.publish(report))117 self.assertEqual(['1', None], config.publish(report))
118 self.assertEqual({'id': '1'}, report)118 self.assertEqual({'id': '1', 'published': True}, report)
119
120 def test_publisher_returning_not_published_doesnt_set_published(self):
121 # If a publisher indicates that it didn't publish then the
122 # 'published' key is not set in the report
123 def pub_noop(report):
124 return ''
125 config = Config()
126 config.publishers.append(pub_noop)
127 report = {}
128 config.publish(report)
129 self.assertEqual({}, report)
130
131 def test_publisher_returning_published_sets_published(self):
132 # If a publisher indicates that it did publish then the
133 # 'published' key is set in the report
134 def pub_succeed(report):
135 return '1'
136 config = Config()
137 config.publishers.append(pub_succeed)
138 report = {}
139 config.publish(report)
140 self.assertEqual({'id': '1', 'published': True}, report)
119141
=== modified file 'oops/tests/test_publishers.py'
--- oops/tests/test_publishers.py 2011-11-13 21:24:42 +0000
+++ oops/tests/test_publishers.py 2012-06-07 21:03:22 +0000
@@ -18,20 +18,25 @@
18__metaclass__ = type18__metaclass__ = type
1919
20import testtools20import testtools
21from testtools.matchers import Is
2221
23from oops import publish_new_only22from oops import publish_new_only
2423
25class TestPublishers(testtools.TestCase):24class TestPublishers(testtools.TestCase):
2625
27 def test_publish_new_only_id_in_report(self):26 def test_publish_new_only_published_in_report(self):
28 def pub_fail(report):27 def pub_fail(report):
29 self.fail('publication called')28 self.fail('publication called')
30 publisher = publish_new_only(pub_fail)29 publisher = publish_new_only(pub_fail)
31 publisher({'id': 'foo'})30 publisher({'published': True})
3231
33 def test_publish_new_only_no_id_in_report(self):32 def test_publish_new_only_no_published_in_report(self):
34 calls = []33 calls = []
35 publisher = publish_new_only(calls.append)34 publisher = publish_new_only(calls.append)
36 publisher({'foo': 'bar'})35 publisher({'foo': 'bar'})
37 self.assertEqual([{'foo': 'bar'}], calls)36 self.assertEqual([{'foo': 'bar'}], calls)
37
38 def test_publish_new_only_no_published_false_in_report(self):
39 calls = []
40 publisher = publish_new_only(calls.append)
41 publisher({'published': False})
42 self.assertEqual([{'published': False}], calls)

Subscribers

People subscribed via source and target branches

to all changes: