launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04563
[Merge] lp:~lifeless/launchpad/useoops into lp:launchpad
Robert Collins has proposed merging lp:~lifeless/launchpad/useoops into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~lifeless/launchpad/useoops/+merge/71139
This branch migrates use to use the python-oops write method rather than one inside LP itself.
As a result of this, fresh in-memory OOPSes (vs read-from-disk ones) for LP also become python-oops oops dicts, and all the tests that were poking at in-memory OOPSes change to using the dict access syntax.
I've also tweaked the test_testcase test that was comparing the serialised forms: it did that because the LP internal parser wasn't setting the tzinfo correctly, python-oops does not have that bug, and so comparing the dicts works - but the getLastOops function is not migrated yet so I do a small hack there.
Other than that this should be totally noncontroversial :).
--
https://code.launchpad.net/~lifeless/launchpad/useoops/+merge/71139
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/useoops into lp:launchpad.
=== modified file 'lib/canonical/launchpad/webapp/errorlog.py'
--- lib/canonical/launchpad/webapp/errorlog.py 2011-08-10 06:50:52 +0000
+++ lib/canonical/launchpad/webapp/errorlog.py 2011-08-11 03:21:40 +0000
@@ -138,7 +138,8 @@
implements(IErrorReport)
def __init__(self, id, type, value, time, pageid, tb_text, username,
- url, duration, req_vars, db_statements, informational):
+ url, duration, req_vars, db_statements, informational,
+ branch_nick=None, revno=None):
self.id = id
self.type = type
self.value = value
@@ -150,45 +151,13 @@
self.duration = duration
self.req_vars = req_vars
self.db_statements = db_statements
- self.branch_nick = versioninfo.branch_nick
- self.revno = versioninfo.revno
+ self.branch_nick = branch_nick or versioninfo.branch_nick
+ self.revno = revno or versioninfo.revno
self.informational = informational
def __repr__(self):
return '<ErrorReport %s %s: %s>' % (self.id, self.type, self.value)
- def get_chunks(self):
- """Returns a list of bytestrings making up the oops disk content."""
- chunks = []
- chunks.append('Oops-Id: %s\n' % _normalise_whitespace(self.id))
- chunks.append(
- 'Exception-Type: %s\n' % _normalise_whitespace(self.type))
- chunks.append(
- 'Exception-Value: %s\n' % _normalise_whitespace(self.value))
- chunks.append('Date: %s\n' % self.time.isoformat())
- chunks.append('Page-Id: %s\n' % _normalise_whitespace(self.pageid))
- chunks.append('Branch: %s\n' % _safestr(self.branch_nick))
- chunks.append('Revision: %s\n' % self.revno)
- chunks.append('User: %s\n' % _normalise_whitespace(self.username))
- chunks.append('URL: %s\n' % _normalise_whitespace(self.url))
- chunks.append('Duration: %s\n' % self.duration)
- chunks.append('Informational: %s\n' % self.informational)
- chunks.append('\n')
- safe_chars = ';/\\?:@&+$, ()*!'
- for key, value in self.req_vars:
- chunks.append('%s=%s\n' % (urllib.quote(key, safe_chars),
- urllib.quote(value, safe_chars)))
- chunks.append('\n')
- for (start, end, database_id, statement) in self.db_statements:
- chunks.append('%05d-%05d@%s %s\n' % (
- start, end, database_id, _normalise_whitespace(statement)))
- chunks.append('\n')
- chunks.append(self.tb_text)
- return chunks
-
- def write(self, fp):
- fp.writelines(self.get_chunks())
-
@classmethod
def read(cls, fp):
# Deprecated: use the oops module directly now, when possible.
@@ -319,22 +288,21 @@
def _raising(self, info, request=None, now=None, informational=False):
"""Private method used by raising() and handling()."""
- entry = self._makeErrorReport(info, request, now, informational)
+ entry, filename = self._makeErrorReport(info, request, now, informational)
if entry is None:
return
- filename = entry._filename
- entry.write(open(filename, 'wb'))
+ oops.serializer_rfc822.write(entry, open(filename, 'wb'))
# Set file permission to: rw-r--r--
wanted_permission = (
stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH)
os.chmod(filename, wanted_permission)
if request:
- request.oopsid = entry.id
+ request.oopsid = entry['id']
request.oops = entry
if self.copy_to_zlog:
self._do_copy_to_zlog(
- entry.time, entry.type, entry.url, info, entry.id)
+ entry['time'], entry['type'], entry['url'], info, entry['id'])
notify(ErrorReportEvent(entry))
return entry
@@ -387,7 +355,7 @@
strtype = str(getattr(info[0], '__name__', info[0]))
if self._isIgnoredException(strtype, request, info[1]):
- return
+ return None, None
if not isinstance(info[2], basestring):
tb_text = ''.join(format_exception(*info,
@@ -413,7 +381,7 @@
if webservice_error / 100 != 5:
request.oopsid = None
# Return so the OOPS is not generated.
- return
+ return None, None
missing = object()
principal = getattr(request, 'principal', missing)
@@ -427,7 +395,7 @@
login = 'unauthenticated'
if strtype in (
self._ignored_exceptions_for_unauthenticated_users):
- return
+ return None, None
if principal is not None and principal is not missing:
username = _safestr(
@@ -473,12 +441,23 @@
oopsid, filename = self.log_namer.newId(now)
- result = ErrorReport(oopsid, strtype, strv, now, pageid, tb_text,
- username, strurl, duration,
- req_vars, statements,
- informational)
- result._filename = filename
- return result
+ result = {
+ 'id': oopsid,
+ 'type': strtype,
+ 'value': strv,
+ 'time': now,
+ 'pageid': pageid,
+ 'tb_text': tb_text,
+ 'username': username,
+ 'url': strurl,
+ 'duration': duration,
+ 'req_vars': req_vars,
+ 'db_statements': statements,
+ 'informational': informational,
+ 'branch_nick': versioninfo.branch_nick,
+ 'revno': versioninfo.revno,
+ }
+ return result, filename
def handling(self, info, request=None, now=None):
"""Flag ErrorReport as informational only.
=== modified file 'lib/canonical/launchpad/webapp/tests/test_errorlog.py'
--- lib/canonical/launchpad/webapp/tests/test_errorlog.py 2011-08-10 06:50:52 +0000
+++ lib/canonical/launchpad/webapp/tests/test_errorlog.py 2011-08-11 03:21:40 +0000
@@ -102,43 +102,6 @@
entry.db_statements[1],
(5, 10, 'store_b', 'SELECT 2'))
- def test_write(self):
- """Test ErrorReport.write()"""
- entry = ErrorReport('OOPS-A0001', 'NotFound', 'error message',
- datetime.datetime(2005, 04, 01, 00, 00, 00,
- tzinfo=UTC),
- 'IFoo:+foo-template',
- 'traceback-text', 'Sample User',
- 'http://localhost:9000/foo', 42,
- [('HTTP_USER_AGENT', 'Mozilla/5.0'),
- ('HTTP_REFERER', 'http://localhost:9000/'),
- ('name=foo', 'hello\nworld')],
- [(1, 5, 'store_a', 'SELECT 1'),
- (5, 10, 'store_b', 'SELECT\n2')], False)
- fp = StringIO.StringIO()
- entry.write(fp)
- self.assertEqual(fp.getvalue(), dedent("""\
- Oops-Id: OOPS-A0001
- Exception-Type: NotFound
- Exception-Value: error message
- Date: 2005-04-01T00:00:00+00:00
- Page-Id: IFoo:+foo-template
- Branch: %s
- Revision: %s
- User: Sample User
- URL: http://localhost:9000/foo
- Duration: 42
- Informational: False
-
- HTTP_USER_AGENT=Mozilla/5.0
- HTTP_REFERER=http://localhost:9000/
- name%%3Dfoo=hello%%0Aworld
-
- 00001-00005@store_a SELECT 1
- 00005-00010@store_b SELECT 2
-
- traceback-text""" % (versioninfo.branch_nick, versioninfo.revno)))
-
def test_read(self):
"""Test ErrorReport.read()."""
# Note: this exists to test the compatibility thunk only.
@@ -379,7 +342,7 @@
# verify that the oopsid was set on the request
self.assertEqual(request.oopsid, 'OOPS-91T1')
- self.assertEqual(request.oops.id, 'OOPS-91T1')
+ self.assertEqual(request.oops['id'], 'OOPS-91T1')
def test_raising_with_xmlrpc_request(self):
# Test ErrorReportingUtility.raising() with an XML-RPC request.
@@ -823,10 +786,10 @@
raise ArbitraryException('foo')
except ArbitraryException:
info = sys.exc_info()
- oops = utility._makeErrorReport(info)
+ oops, _ = utility._makeErrorReport(info)
self.assertEqual(
[('<oops-message-0>', "{'a': 'b', 'c': 'd'}")],
- oops.req_vars)
+ oops['req_vars'])
def test__makeErrorReport_combines_request_and_error_vars(self):
"""The oops messages should be distinct from real request vars."""
@@ -837,10 +800,10 @@
raise ArbitraryException('foo')
except ArbitraryException:
info = sys.exc_info()
- oops = utility._makeErrorReport(info, request)
+ oops, _ = utility._makeErrorReport(info, request)
self.assertEqual(
[('<oops-message-0>', "{'a': 'b'}"), ('c', 'd')],
- oops.req_vars)
+ oops['req_vars'])
class TestSensitiveRequestVariables(testtools.TestCase):
=== modified file 'lib/launchpad_loggerhead/tests.py'
--- lib/launchpad_loggerhead/tests.py 2011-03-31 03:31:15 +0000
+++ lib/launchpad_loggerhead/tests.py 2011-08-11 03:21:40 +0000
@@ -18,7 +18,6 @@
import zope.event
from canonical.config import config
-from canonical.launchpad.webapp.errorlog import ErrorReport, ErrorReportEvent
from canonical.launchpad.webapp.vhosts import allvhosts
from canonical.testing.layers import DatabaseFunctionalLayer
from launchpad_loggerhead.app import (
@@ -250,11 +249,11 @@
# event
self.assertEqual(1, len(self.oopses))
oops = self.oopses[0]
- self.assertEqual('RuntimeError', oops.type)
+ self.assertEqual('RuntimeError', oops['type'])
# runtime_failing_app doesn't call start_response, but oops_middleware
# does because it tries to send the OOPS information to the user.
self.assertTrue(self.start_response_called)
- self.assertEqual(_oops_html_template % {'oopsid': oops.id},
+ self.assertEqual(_oops_html_template % {'oopsid': oops['id']},
''.join(self._response_chunks))
def test_ignores_socket_exceptions(self):
=== modified file 'lib/lp/code/model/tests/test_diff.py'
--- lib/lp/code/model/tests/test_diff.py 2010-12-02 16:13:51 +0000
+++ lib/lp/code/model/tests/test_diff.py 2011-08-11 03:21:40 +0000
@@ -288,7 +288,7 @@
diff_bytes = "not a real diff"
diff = Diff.fromFile(StringIO(diff_bytes), len(diff_bytes))
oops = self.oopses[0]
- self.assertEqual('MalformedPatchHeader', oops.type)
+ self.assertEqual('MalformedPatchHeader', oops['type'])
self.assertIs(None, diff.diffstat)
self.assertIs(None, diff.added_lines_count)
self.assertIs(None, diff.removed_lines_count)
=== modified file 'lib/lp/services/job/runner.py'
--- lib/lp/services/job/runner.py 2011-06-21 13:24:57 +0000
+++ lib/lp/services/job/runner.py 2011-08-11 03:21:40 +0000
@@ -147,7 +147,7 @@
def notifyOops(self, oops):
"""Report this oops."""
- ctrl = self.getOopsMailController(oops.id)
+ ctrl = self.getOopsMailController(oops['id'])
if ctrl is None:
return
ctrl.send()
@@ -306,7 +306,7 @@
transaction.commit()
oops = self.runJobHandleError(job)
if oops is not None:
- self._logOopsId(oops.id)
+ self._logOopsId(oops['id'])
class RunJobCommand(amp.Command):
@@ -384,7 +384,7 @@
if oops is None:
oops_id = ''
else:
- oops_id = oops.id
+ oops_id = oops['id']
return {'success': len(runner.completed_jobs), 'oops_id': oops_id}
@@ -452,7 +452,7 @@
else:
info = (failure.type, failure.value, failure.tb)
oops = self._doOops(job, info)
- self._logOopsId(oops.id)
+ self._logOopsId(oops['id'])
deferred.addCallbacks(update, job_raised)
return deferred
@@ -461,7 +461,7 @@
raise TimeoutError
except TimeoutError:
oops = self._doOops(job, sys.exc_info())
- self._logOopsId(oops.id)
+ self._logOopsId(oops['id'])
def getTaskSource(self):
"""Return a task source for all jobs in job_source."""
=== modified file 'lib/lp/services/job/tests/test_runner.py'
--- lib/lp/services/job/tests/test_runner.py 2011-07-05 09:01:53 +0000
+++ lib/lp/services/job/tests/test_runner.py 2011-08-11 03:21:40 +0000
@@ -193,9 +193,9 @@
self.assertEqual(JobStatus.FAILED, job_1.job.status)
self.assertEqual(JobStatus.COMPLETED, job_2.job.status)
oops = self.oopses[-1]
- self.assertIn('Fake exception. Foobar, I say!', oops.tb_text)
- self.assertEqual(1, len(oops.req_vars))
- self.assertEqual("{'foo': 'bar'}", oops.req_vars[0][1])
+ self.assertIn('Fake exception. Foobar, I say!', oops['tb_text'])
+ self.assertEqual(1, len(oops['req_vars']))
+ self.assertEqual("{'foo': 'bar'}", oops['req_vars'][0][1])
def test_oops_messages_used_when_handling(self):
"""Oops messages should appear even when exceptions are handled."""
@@ -211,8 +211,8 @@
runner = JobRunner([job_1, job_2])
runner.runAll()
oops = self.oopses[-1]
- self.assertEqual(1, len(oops.req_vars))
- self.assertEqual("{'foo': 'bar'}", oops.req_vars[0][1])
+ self.assertEqual(1, len(oops['req_vars']))
+ self.assertEqual("{'foo': 'bar'}", oops['req_vars'][0][1])
def test_runAll_aborts_transaction_on_error(self):
"""runAll should abort the transaction on oops."""
@@ -250,7 +250,7 @@
self.assertIn(
'Launchpad encountered an internal error during the following'
' operation: appending a string to a list. It was logged with id'
- ' %s. Sorry for the inconvenience.' % oops.id,
+ ' %s. Sorry for the inconvenience.' % oops['id'],
notification.get_payload(decode=True))
self.assertNotIn('Fake exception. Foobar, I say!',
notification.get_payload(decode=True))
=== modified file 'lib/lp/services/profile/profile.py'
--- lib/lp/services/profile/profile.py 2011-08-08 15:24:22 +0000
+++ lib/lp/services/profile/profile.py 2011-08-11 03:21:40 +0000
@@ -16,6 +16,7 @@
from bzrlib import errors
from bzrlib.lsprof import BzrProfiler
+import oops.serializer_rfc822
from zope.pagetemplate.pagetemplatefile import PageTemplateFile
from zope.app.publication.interfaces import IEndRequestEvent
from zope.component import (
@@ -230,10 +231,10 @@
# information.
info = (ProfilingOops, None, None)
error_utility = getUtility(IErrorReportingUtility)
- oops = error_utility.handling(info, request)
- oopsid = oops.id
+ oops_report = error_utility.handling(info, request)
+ oopsid = oops_report['id']
else:
- oops = request.oops
+ oops_report = request.oops
if actions.intersection(('callgrind', 'pstats')):
filename = '%s-%s-%s-%s' % (
timestamp, pageid, oopsid,
@@ -250,10 +251,10 @@
prof_stats.save(dump_path)
template_context['dump_path'] = os.path.abspath(dump_path)
if is_html and 'show' in actions:
- # Generate raw OOPS results.
- f = StringIO.StringIO()
- oops.write(f)
- template_context['oops'] = f.getvalue()
+ # Generate rfc822 OOPS result (might be nice to have an html
+ # serializer..).
+ template_context['oops'] = ''.join(
+ oops.serializer_rfc822.to_chunks(oops_report))
# Generate profile summaries.
for name in ('inlinetime', 'totaltime', 'callcount'):
prof_stats.sort(name)
=== modified file 'lib/lp/services/profile/tests.py'
--- lib/lp/services/profile/tests.py 2011-08-08 15:48:34 +0000
+++ lib/lp/services/profile/tests.py 2011-08-11 03:21:40 +0000
@@ -566,18 +566,16 @@
def test_profiling_oops_is_informational(self):
self.pushProfilingConfig(profiling_allowed='True')
request = self.endRequest('/++profile++show/')
- self.assertIsInstance(request.oops, ErrorReport)
- self.assertTrue(request.oops.informational)
- self.assertEquals(request.oops.type, 'ProfilingOops')
+ self.assertTrue(request.oops['informational'])
+ self.assertEquals(request.oops['type'], 'ProfilingOops')
self.assertCleanProfilerState()
def test_real_oops_trumps_profiling_oops(self):
self.pushProfilingConfig(profiling_allowed='True')
request = self.endRequest('/++profile++show/no-such-file',
KeyError('foo'))
- self.assertIsInstance(request.oops, ErrorReport)
- self.assertFalse(request.oops.informational)
- self.assertEquals(request.oops.type, 'KeyError')
+ self.assertFalse(request.oops['informational'])
+ self.assertEquals(request.oops['type'], 'KeyError')
response = self.getAddedResponse(request)
self.assertIn('Exception-Type: KeyError', response)
self.assertCleanProfilerState()
=== modified file 'lib/lp/services/twistedsupport/tests/test_loggingsupport.py'
--- lib/lp/services/twistedsupport/tests/test_loggingsupport.py 2010-10-30 23:24:28 +0000
+++ lib/lp/services/twistedsupport/tests/test_loggingsupport.py 2011-08-11 03:21:40 +0000
@@ -79,8 +79,8 @@
fail = makeFailure(RuntimeError)
log.err(fail, error_time=error_time)
flush_logged_errors(RuntimeError)
- oops = globalErrorUtility.getOopsReport(error_time)
- self.assertEqual(oops.type, 'RuntimeError')
+ oops = self.oopses[-1]
+ self.assertEqual(oops['type'], 'RuntimeError')
self.assertLogMatches('^Logged OOPS id.*')
=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py 2011-07-29 15:38:00 +0000
+++ lib/lp/testing/__init__.py 2011-08-11 03:21:40 +0000
@@ -58,6 +58,7 @@
timedelta,
)
from fnmatch import fnmatchcase
+from functools import partial
from inspect import (
getargspec,
getmro,
@@ -82,6 +83,7 @@
)
from bzrlib.transport import get_transport
import fixtures
+import oops.serializer_rfc822
import pytz
import simplejson
from storm.expr import Variable
@@ -552,8 +554,9 @@
def attachOopses(self):
if len(self.oopses) > 0:
- for (i, oops) in enumerate(self.oopses):
- content = Content(UTF8_TEXT, oops.get_chunks)
+ for (i, report) in enumerate(self.oopses):
+ content = Content(UTF8_TEXT,
+ partial(oops.serializer_rfc822.to_chunks, report))
self.addDetail("oops-%d" % i, content)
def attachLibrarianLog(self, fixture):
=== modified file 'lib/lp/testing/tests/test_testcase.py'
--- lib/lp/testing/tests/test_testcase.py 2010-09-04 20:18:39 +0000
+++ lib/lp/testing/tests/test_testcase.py 2011-08-11 03:21:40 +0000
@@ -9,6 +9,7 @@
import sys
import unittest
+import oops.serializer_rfc822
from storm.store import Store
from zope.component import getUtility
@@ -86,22 +87,13 @@
self.assertEqual(0, len(self.oopses))
self.trigger_oops()
self.attachOopses()
- oops = errorlog.globalErrorUtility.getLastOopsReport()
- # We have to serialise and read in again, as that is what
- # getLastOopsReport does, and doing so changes whether the
- # timezone is in the timestamp.
content = StringIO()
content.writelines(self.getDetails()['oops-0'].iter_bytes())
content.seek(0)
# Safety net: ensure that no autocasts have occured even on Python 2.6
# which is slightly better.
self.assertIsInstance(content.getvalue(), str)
- from_details = errorlog.ErrorReport.read(content)
- self.assertEqual(
- oops.get_chunks(),
- from_details.get_chunks())
-
-
-def test_suite():
- return unittest.TestLoader().loadTestsFromName(__name__)
+ from_details = oops.serializer_rfc822.read(content)
+ oops_report = errorlog.globalErrorUtility.getLastOopsReport()
+ self.assertEqual(dict(oops_report.__dict__), from_details)
=== modified file 'versions.cfg'
--- versions.cfg 2011-08-10 06:50:52 +0000
+++ versions.cfg 2011-08-11 03:21:40 +0000
@@ -49,7 +49,7 @@
mocker = 0.10.1
mozrunner = 1.3.4
oauth = 1.0
-oops = 0.0.1
+oops = 0.0.2
paramiko = 1.7.4
Paste = 1.7.2
PasteDeploy = 1.3.3
