← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~lifeless/launchpad/bug-888375 into lp:launchpad

 

Robert Collins has proposed merging lp:~lifeless/launchpad/bug-888375 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~lifeless/launchpad/bug-888375/+merge/81928

Delete oops-prune.py. Replace with an API that permits out-of-launchpad pruning.

I sanity checked the 'return all references' aspect with wgrant; we agree the risk is minimal - and the regex we're using is narrow it doesn't even permit line wrapping breaks, to reduce the risk.

The benefit of changing to an API rather than our internal script is that U1, landscape and others that choose to use oopses can prune their reports while keeping the ones referenced in LP, just like LP itself does. It also gets all the disk format specific stuff out of LP and able to be consumed externally - e.g. by/within python-oops-tools.
-- 
https://code.launchpad.net/~lifeless/launchpad/bug-888375/+merge/81928
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/bug-888375 into lp:launchpad.
=== removed file 'cronscripts/oops-prune.py'
--- cronscripts/oops-prune.py	2011-09-26 07:23:44 +0000
+++ cronscripts/oops-prune.py	1970-01-01 00:00:00 +0000
@@ -1,62 +0,0 @@
-#!/usr/bin/python -S
-#
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-# pylint: disable-msg=C0103,W0403
-
-"""Cronscript to prune old and unreferenced OOPS reports from the archive."""
-
-__metaclass__ = type
-
-import os
-
-import _pythonpath
-
-from canonical.config import config
-from canonical.launchpad.scripts.oops import (
-    prune_empty_oops_directories,
-    unwanted_oops_files,
-    )
-from lp.services.scripts.base import (
-    LaunchpadCronScript,
-    LaunchpadScriptFailure,
-    )
-
-
-default_lock_filename = '/var/lock/oops-prune.lock'
-
-
-class OOPSPruner(LaunchpadCronScript):
-    def add_my_options(self):
-        self.parser.add_option(
-                '-n', '--dry-run', default=False, action='store_true',
-                dest="dry_run", help="Do a test run. No files are removed."
-                )
-
-    def main(self):
-        # Default to using the OOPS directory in config file.
-        if not self.args:
-            self.args = [config.error_reports.error_dir]
-
-        oops_directories = []
-        for oops_dir in self.args:
-            if not os.path.isdir(oops_dir):
-                raise LaunchpadScriptFailure(
-                    "%s is not a directory" % oops_dir)
-
-            oops_directories.append(oops_dir)
-
-        for oops_directory in oops_directories:
-            for oops_path in unwanted_oops_files(oops_directory,
-                                                 40, self.logger):
-                self.logger.info("Removing %s", oops_path)
-                if not self.options.dry_run:
-                    os.unlink(oops_path)
-
-            prune_empty_oops_directories(oops_directory)
-
-
-if __name__ == '__main__':
-    script = OOPSPruner('oops-prune', dbuser='oopsprune')
-    script.lock_and_run(isolation='autocommit')

=== modified file 'lib/lp/registry/interfaces/distribution.py'
--- lib/lp/registry/interfaces/distribution.py	2011-08-29 00:13:22 +0000
+++ lib/lp/registry/interfaces/distribution.py	2011-11-11 02:17:27 +0000
@@ -83,6 +83,7 @@
     ICanGetMilestonesDirectly,
     IHasMilestones,
     )
+from lp.registry.interfaces.oopsreferences import IHasOOPSReferences
 from lp.registry.interfaces.pillar import IPillar
 from lp.registry.interfaces.role import (
     IHasAppointedDriver,
@@ -134,9 +135,9 @@
 class IDistributionPublic(
     IBugTarget, ICanGetMilestonesDirectly, IHasAppointedDriver,
     IHasBuildRecords, IHasDrivers, IHasMilestones,
-    IHasOwner, IHasSecurityContact, IHasSprints, IHasTranslationImports,
-    ITranslationPolicy, IKarmaContext, ILaunchpadUsage, IMakesAnnouncements,
-    IOfficialBugTagTargetPublic, IPillar, IServiceUsage,
+    IHasOOPSReferences, IHasOwner, IHasSecurityContact, IHasSprints,
+    IHasTranslationImports, ITranslationPolicy, IKarmaContext, ILaunchpadUsage,
+    IMakesAnnouncements, IOfficialBugTagTargetPublic, IPillar, IServiceUsage,
     ISpecificationTarget):
     """Public IDistribution properties."""
 

=== added file 'lib/lp/registry/interfaces/oopsreferences.py'
--- lib/lp/registry/interfaces/oopsreferences.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/interfaces/oopsreferences.py	2011-11-11 02:17:27 +0000
@@ -0,0 +1,45 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Interfaces for querying OOPS references."""
+
+__metaclass__ = type
+
+__all__ = [
+    'IHasOOPSReferences',
+    ]
+
+
+from lazr.restful.declarations import (
+    export_read_operation,
+    operation_for_version,
+    operation_parameters,
+    )
+from zope.interface import (
+    Interface,
+    )
+from zope.schema import (
+    Datetime
+    )
+
+from canonical.launchpad import _
+
+
+class IHasOOPSReferences(Interface):
+    """Has references to OOPSes that can be queried."""
+
+    @operation_parameters(
+        start_date=Datetime(title=_("Modified after date")),
+        end_date=Datetime(title=_("Modified before date")),
+        )
+    @export_read_operation()
+    @operation_for_version('devel')
+    def findReferencedOOPS(start_date, end_date):
+        """Find OOPS reports between start_date and end_date.
+
+        :param start_date: Do not look in objects whose last modification time
+            is before this date.
+        :param end_date: Do not look in objects whose last modification time
+            is after this date.
+        :return: A set of OOPS id's - strings of the form 'OOPS-\w+'.
+        """

=== modified file 'lib/lp/registry/interfaces/product.py'
--- lib/lp/registry/interfaces/product.py	2011-10-29 18:23:06 +0000
+++ lib/lp/registry/interfaces/product.py	2011-11-11 02:17:27 +0000
@@ -114,6 +114,7 @@
     ICanGetMilestonesDirectly,
     IHasMilestones,
     )
+from lp.registry.interfaces.oopsreferences import IHasOOPSReferences
 from lp.registry.interfaces.pillar import IPillar
 from lp.registry.interfaces.productrelease import IProductRelease
 from lp.registry.interfaces.productseries import IProductSeries
@@ -414,8 +415,8 @@
     IHasMugshot, IHasOwner, IHasSecurityContact, IHasSprints,
     IHasTranslationImports, ITranslationPolicy, IKarmaContext,
     ILaunchpadUsage, IMakesAnnouncements, IOfficialBugTagTargetPublic,
-    IPillar, ISpecificationTarget, IHasRecipes, IHasCodeImports,
-    IServiceUsage):
+    IHasOOPSReferences, IPillar, ISpecificationTarget, IHasRecipes,
+    IHasCodeImports, IServiceUsage):
     """Public IProduct properties."""
 
     id = Int(title=_('The Project ID'))

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2011-10-07 16:20:20 +0000
+++ lib/lp/registry/model/distribution.py	2011-11-11 02:17:27 +0000
@@ -129,6 +129,7 @@
     MirrorFreshness,
     MirrorStatus,
     )
+from lp.registry.interfaces.oopsreferences import IHasOOPSReferences
 from lp.registry.interfaces.packaging import PackagingType
 from lp.registry.interfaces.person import (
     validate_person,
@@ -155,6 +156,7 @@
     HasMilestonesMixin,
     Milestone,
     )
+from lp.registry.model.oopsreferences import referenced_oops
 from lp.registry.model.pillar import HasAliasMixin
 from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.services.propertycache import (
@@ -209,7 +211,7 @@
     implements(
         IBugSummaryDimension, IDistribution, IFAQTarget, IHasBugHeat,
         IHasBugSupervisor, IHasBuildRecords, IHasIcon, IHasLogo, IHasMugshot,
-        ILaunchpadUsage, IServiceUsage)
+        IHasOOPSReferences, ILaunchpadUsage, IServiceUsage)
 
     _table = 'Distribution'
     _defaultOrder = 'name'
@@ -1013,6 +1015,12 @@
             owner=owner, title=title, content=content, keywords=keywords,
             date_created=date_created, distribution=self)
 
+    def findReferencedOOPS(self, start_date, end_date):
+        """See `IHasOOPSReferences`."""
+        return list(referenced_oops(
+            start_date, end_date, "distribution=%(distribution)s",
+            {'distribution': self.id}))
+
     def findSimilarFAQs(self, summary):
         """See `IFAQTarget`."""
         return FAQ.findSimilar(summary, distribution=self)

=== renamed file 'lib/canonical/launchpad/scripts/oops.py' => 'lib/lp/registry/model/oopsreferences.py'
--- lib/canonical/launchpad/scripts/oops.py	2011-09-13 01:17:31 +0000
+++ lib/lp/registry/model/oopsreferences.py	2011-11-11 02:17:27 +0000
@@ -1,143 +1,81 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Module docstring goes here."""
-
-from __future__ import absolute_import
+"""Find OOPS References within the LP database."""
 
 __metaclass__ = type
 
 __all__ = [
-    'referenced_oops', 'unwanted_oops_files', 'path_to_oopsid',
-    'prune_empty_oops_directories',
+    'referenced_oops',
     ]
 
-from datetime import (
-    date,
-    datetime,
-    timedelta,
+import re
+
+from canonical.database.sqlbase import (
+    cursor,
+    sqlvalues,
     )
-import os
-import re
-
-from oops_datedir_repo import uniquefileallocator
-from pytz import utc
-
-from canonical.database.sqlbase import cursor
-from canonical.launchpad.webapp.dbpolicy import SlaveOnlyDatabasePolicy
-from lp.app.browser.stringformatter import FormattersAPI
-
-
-def referenced_oops():
-    '''Return a set of OOPS codes that are referenced somewhere in the
-    Launchpad database.
-
-    We currently check the entire Message store, Bugs, BugTasks and Question
+
+
+def referenced_oops(start_date, end_date, context_clause, context_params):
+    '''Find OOPS codes that are referenced somewhere in Launchpad.
+
+    This returns OOPS references from:
+     - any message, message chunk or bug.
+     - any question that passes context_clause.
+
+    Privacy and access controls are ignored: the maximum disclosure is a single
+    word immediately after the word 'OOPS'. Future iterations may tighten the
+    returned references up.
+
+    :param start_date: The earliest modification date to consider.
+    :param end_date: The last modification date to consider.
+    :param context_clause: A filter to restrict the question clause against.
+        For instance: 'product=%(product)s'.
+    :param context_params: Parameters needed to evaluate context_clause.
+        For instance: {'product': 12}
+    :return: A set of the found OOPS ids.
     '''
     # Note that the POSIX regexp syntax is subtly different to the Python,
     # and that we need to escape all \ characters to keep the SQL interpreter
     # happy.
-    posix_oops_match = (r"~* '^(oops\\s*-?\\s*\\d*[a-z]+\\d+)"
-        "|[^=]+(\\moops\\s*-?\\s*\\d*[a-z]+\\d+)'")
-    query = """
+    posix_oops_match = (r"~* '^(oops-\\w+)|(\\moops-\\w+)'")
+    params = dict(start_date=start_date, end_date=end_date)
+    params.update(context_params)
+    sql_params = sqlvalues(**params)
+    sql_params['posix_oops_match'] = posix_oops_match
+    query = ("""
+        WITH recent_messages AS
+            (SELECT id FROM Message WHERE
+             datecreated BETWEEN %(start_date)s AND %(end_date)s)
         SELECT DISTINCT subject FROM Message
         WHERE subject %(posix_oops_match)s AND subject IS NOT NULL
+            AND id IN (SELECT id FROM recent_messages)
         UNION ALL
         SELECT content FROM MessageChunk WHERE content %(posix_oops_match)s
+            AND message IN (SELECT id FROM recent_messages)
         UNION ALL
         SELECT title || ' ' || description
-        FROM Bug WHERE title %(posix_oops_match)s
-            OR description %(posix_oops_match)s
+        FROM Bug WHERE
+            date_last_updated BETWEEN %(start_date)s AND %(end_date)s AND
+            (title %(posix_oops_match)s OR description %(posix_oops_match)s)
         UNION ALL
         SELECT title || ' ' || description || ' ' || COALESCE(whiteboard,'')
-        FROM Question WHERE title %(posix_oops_match)s
-            OR description %(posix_oops_match)s
-            OR whiteboard %(posix_oops_match)s
-        """ % {'posix_oops_match': posix_oops_match}
+        FROM Question WHERE """ + context_clause + """
+            AND (datelastquery BETWEEN %(start_date)s AND %(end_date)s
+                OR datelastresponse BETWEEN %(start_date)s AND %(end_date)s)
+            AND (title %(posix_oops_match)s
+                OR description %(posix_oops_match)s
+                OR whiteboard %(posix_oops_match)s)
+        """) % sql_params
 
     referenced_codes = set()
+    oops_re = re.compile(r'(?i)(?P<oops>\boops-\w+)')
 
-    with SlaveOnlyDatabasePolicy():
-        cur = cursor()
-        cur.execute(query)
-        for content in (row[0] for row in cur.fetchall()):
-            for match in FormattersAPI._re_linkify.finditer(content):
-                if match.group('oops') is not None:
-                    code_string = match.group('oopscode')
-                    referenced_codes.add(code_string.upper())
+    cur = cursor()
+    cur.execute(query)
+    for content in (row[0] for row in cur.fetchall()):
+        for oops in oops_re.findall(content):
+            referenced_codes.add(oops.upper())
 
     return referenced_codes
-
-
-def path_to_oopsid(path):
-    '''Extract the OOPS id from a path to an OOPS file'''
-    date_str = os.path.basename(os.path.dirname(path))
-    match = re.search('^(\d\d\d\d)-(\d\d+)-(\d\d+)$', date_str)
-    year, month, day = (int(bit) for bit in match.groups())
-    oops_id = os.path.basename(path).split('.')[1]
-    # Should be making API calls not directly calculating.
-    day = (datetime(year, month, day, tzinfo=utc) - \
-        uniquefileallocator.epoch).days + 1
-    return '%d%s' % (day, oops_id)
-
-
-def unwanted_oops_files(root_path, days, log=None):
-    '''Generate a list of OOPS files that are older than 'days' and are
-       not referenced in the Launchpad database.
-    '''
-    wanted_oops = referenced_oops()
-
-    for oops_path in old_oops_files(root_path, days):
-        oopsid = path_to_oopsid(oops_path)
-        if oopsid.upper() not in wanted_oops:
-            yield oops_path
-        elif log is not None:
-            log.debug("%s (%s) is wanted" % (oops_path, oopsid))
-
-
-def old_oops_files(root_path, days):
-    """Generate a list of all OOPS files older than 'days' days old.
-
-    :param root_path: The directory that contains the OOPS files. The
-        cronscript will pass config.error_reports.error_dir if the root_path
-        was not specified on the command line.
-    :param days: The age the OOPS files must exceed to be considered old.
-    """
-    now = date.today()
-    for (dirpath, dirnames, filenames) in os.walk(root_path):
-
-        # Only recurse into correctly named OOPS report directories that
-        # are more than 'days' days old.
-        all_dirnames = dirnames[:]
-        del dirnames[:]
-        for subdir in all_dirnames:
-            date_match = re.search(r'^(\d\d\d\d)-(\d\d)-(\d\d)$', subdir)
-            if date_match is None:
-                continue
-
-            # Skip if he directory is too new.
-            year, month, day = (int(bit) for bit in date_match.groups())
-            oops_date = date(year, month, day)
-            if now - oops_date <= timedelta(days=days):
-                continue
-
-            dirnames.append(subdir)
-
-        # Yield out OOPS filenames
-        for filename in filenames:
-            if re.search(
-                    r'^\d+\.[a-zA-Z]+\d+(?:\.gz|\.bz2)?$', filename
-                    ) is not None:
-                yield os.path.join(dirpath, filename)
-
-
-def prune_empty_oops_directories(root_path):
-    for filename in os.listdir(root_path):
-        if re.search(r'^\d\d\d\d-\d\d-\d\d$', filename) is None:
-            continue
-        path = os.path.join(root_path, filename)
-        if not os.path.isdir(path):
-            continue
-        if os.listdir(path):
-            continue
-        os.rmdir(path)

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2011-10-29 18:23:06 +0000
+++ lib/lp/registry/model/product.py	2011-11-11 02:17:27 +0000
@@ -138,6 +138,7 @@
     )
 from lp.code.model.sourcepackagerecipe import SourcePackageRecipe
 from lp.code.model.sourcepackagerecipedata import SourcePackageRecipeData
+from lp.registry.interfaces.oopsreferences import IHasOOPSReferences
 from lp.registry.interfaces.person import (
     IPersonSet,
     validate_person,
@@ -160,6 +161,7 @@
     HasMilestonesMixin,
     Milestone,
     )
+from lp.registry.model.oopsreferences import referenced_oops
 from lp.registry.model.packaging import Packaging
 from lp.registry.model.person import Person
 from lp.registry.model.pillar import HasAliasMixin
@@ -309,7 +311,7 @@
     implements(
         IBugSummaryDimension, IFAQTarget, IHasBugHeat, IHasBugSupervisor,
         IHasCustomLanguageCodes, IHasIcon, IHasLogo, IHasMugshot,
-        ILaunchpadUsage, IProduct, IServiceUsage)
+        IHasOOPSReferences, ILaunchpadUsage, IProduct, IServiceUsage)
 
     _table = 'Product'
 
@@ -975,6 +977,12 @@
             owner=owner, title=title, content=content, keywords=keywords,
             date_created=date_created, product=self)
 
+    def findReferencedOOPS(self, start_date, end_date):
+        """See `IHasOOPSReferences`."""
+        return list(referenced_oops(
+            start_date, end_date, "product=%(product)s", {'product': self.id}
+            ))
+
     def findSimilarFAQs(self, summary):
         """See `IFAQTarget`."""
         return FAQ.findSimilar(summary, product=self)

=== modified file 'lib/lp/registry/tests/test_distribution.py'
--- lib/lp/registry/tests/test_distribution.py	2011-10-12 11:10:39 +0000
+++ lib/lp/registry/tests/test_distribution.py	2011-11-11 02:17:27 +0000
@@ -5,14 +5,19 @@
 
 __metaclass__ = type
 
+import datetime
+
 from lazr.lifecycle.snapshot import Snapshot
+import pytz
 import soupmatchers
 from storm.store import Store
 from testtools import ExpectedException
 from testtools.matchers import (
+    MatchesAll,
     MatchesAny,
     Not,
     )
+import transaction
 from zope.component import getUtility
 from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
@@ -32,6 +37,7 @@
     IDistribution,
     IDistributionSet,
     )
+from lp.registry.interfaces.oopsreferences import IHasOOPSReferences
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.tests.test_distroseries import (
@@ -46,6 +52,7 @@
     login_person,
     person_logged_in,
     TestCaseWithFactory,
+    WebServiceTestCase,
     )
 from lp.testing.matchers import Provides
 from lp.testing.views import create_initialized_view
@@ -214,6 +221,15 @@
             Unauthorized,
             setattr, distro, "package_derivatives_email", "foo")
 
+    def test_implements_interfaces(self):
+        # Distribution fully implements its interfaces.
+        distro = removeSecurityProxy(self.factory.makeDistribution())
+        expected_interfaces = [
+            IHasOOPSReferences,
+            ]
+        provides_all = MatchesAll(*map(Provides, expected_interfaces))
+        self.assertThat(distro, provides_all)
+
 
 class TestDistributionCurrentSourceReleases(
     TestDistroSeriesCurrentSourceReleases):
@@ -342,7 +358,7 @@
         self.distribution = self.factory.makeDistribution(name="boobuntu")
 
     def test_snapshot(self):
-        """Snapshots of products should not include marked attribues.
+        """Snapshots of distributions should not include marked attribues.
 
         Wrap an export with 'doNotSnapshot' to force the snapshot to not
         include that attribute.
@@ -528,3 +544,39 @@
             distro.translation_focus = new_series
             distro.translationgroup = new_group
             distro.translationpermission = TranslationPermission.CLOSED
+
+
+class TestWebService(WebServiceTestCase):
+
+    def test_oops_references_matching_distro(self):
+        # The distro layer provides the context restriction, so we need to
+        # check we can access context filtered references - e.g. on question.
+        oopsid = "OOPS-abcdef1234"
+        distro = self.factory.makeDistribution()
+        question = self.factory.makeQuestion(
+            title="Crash with %s" % oopsid, target=distro)
+        transaction.commit()
+        ws_distro = self.wsObject(distro, distro.owner)
+        now = datetime.datetime.now(tz=pytz.utc)
+        day = datetime.timedelta(days=1)
+        self.failUnlessEqual(
+            [oopsid.upper()],
+            ws_distro.findReferencedOOPS(start_date=now - day, end_date=now))
+        self.failUnlessEqual(
+            [],
+            ws_distro.findReferencedOOPS(
+                start_date=now + day, end_date=now + day))
+
+    def test_oops_references_different_distro(self):
+        # The distro layer provides the context restriction, so we need to
+        # check the filter is tight enough - other contexts should not work.
+        oopsid = "OOPS-abcdef1234"
+        self.factory.makeQuestion(title="Crash with %s" % oopsid)
+        distro = self.factory.makeDistribution()
+        transaction.commit()
+        ws_distro = self.wsObject(distro, distro.owner)
+        now = datetime.datetime.now(tz=pytz.utc)
+        day = datetime.timedelta(days=1)
+        self.failUnlessEqual(
+            [],
+            ws_distro.findReferencedOOPS(start_date=now - day, end_date=now))

=== renamed file 'lib/canonical/launchpad/scripts/ftests/test_oops_prune.py' => 'lib/lp/registry/tests/test_oopsreferences.py'
--- lib/canonical/launchpad/scripts/ftests/test_oops_prune.py	2011-09-13 01:17:31 +0000
+++ lib/lp/registry/tests/test_oopsreferences.py	2011-11-11 02:17:27 +0000
@@ -1,9 +1,7 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Test the oops-prune.py cronscript and methods in the
-   canonical.launchpad.scripts.oops module.
-"""
+"""Tests of the oopsreferences core."""
 
 __metaclass__ = type
 
@@ -11,290 +9,179 @@
     datetime,
     timedelta,
     )
-import os
-import re
-import shutil
-from subprocess import (
-    PIPE,
-    Popen,
-    STDOUT,
-    )
-import sys
-import tempfile
-import unittest
-
-from oops_datedir_repo import uniquefileallocator
-from pytz import UTC
-import transaction
-
-from canonical.config import config
-from canonical.database.sqlbase import cursor
-from canonical.launchpad.scripts.oops import (
-    old_oops_files,
-    path_to_oopsid,
-    prune_empty_oops_directories,
-    referenced_oops,
-    unwanted_oops_files,
-    )
-from canonical.testing.layers import LaunchpadZopelessLayer
-
-
-class TestOopsPrune(unittest.TestCase):
-    layer = LaunchpadZopelessLayer
+
+from pytz import utc
+
+from canonical.launchpad.interfaces.lpstorm import IStore
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.registry.model.oopsreferences import referenced_oops
+from lp.services.messages.model.message import (
+    Message,
+    MessageSet,
+    )
+from lp.testing import TestCaseWithFactory, person_logged_in
+
+
+class TestOopsReferences(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
 
     def setUp(self):
-        # The dots in the directory name are here because we had a bug
-        # where this situation would break due to using split('.') on the
-        # whole path rather than the path's basename.
-        self.oops_dir = tempfile.mkdtemp('.directory.with.dots')
-
-        # TODO: This should be in the errorlog tests, and calling into
-        # errorlog methods.
-        # Create some fake OOPS files
-        self.today = datetime.now(tz=UTC)
-        self.ages_ago = uniquefileallocator.epoch + timedelta(days=1)
-        self.awhile_ago = self.ages_ago + timedelta(days=1)
-
-        for some_date in [self.today, self.ages_ago, self.awhile_ago]:
-            date_dir = os.path.join(
-                    self.oops_dir, some_date.strftime('%Y-%m-%d')
-                    )
-            os.mkdir(date_dir)
-            # Note - one of these is lowercase to demonstrate case handling
-            for oops_id in ['A666', 'A1234.gz', 'A5678.bz2', 'a666']:
-                oops_filename = os.path.join(date_dir, '000.%s' % oops_id)
-                open(oops_filename, 'w').write('Fnord')
-
-        # Create a reference to one of the old OOPS reports in the DB
-        self.referenced_oops_code = '2A666'
-        cur = cursor()
-        cur.execute("""
-            INSERT INTO MessageChunk(message, sequence, content)
-            VALUES (1, 99, 'OOPS-%s')
-            """ % self.referenced_oops_code)
-        if not os.path.exists(config.error_reports.error_dir):
-            os.mkdir(config.error_reports.error_dir)
-
-        # Need to commit or the changes are not visible on the slave.
-        transaction.commit()
-
-    def tearDown(self):
-        shutil.rmtree(self.oops_dir)
-        shutil.rmtree(config.error_reports.error_dir)
-
-    def test_referenced_oops(self):
-        self.failUnlessEqual(
-                set([self.referenced_oops_code]),
-                referenced_oops()
-                )
-
-        # We also check in other places besides MessageChunk for oops ids
-        cur = cursor()
-        cur.execute("UPDATE Message SET subject='OOPS-1MessageSubject666'")
-        cur.execute("""
-            UPDATE Bug SET
-                title='OOPS-1BugTitle666',
-                description='OOPS-1BugDescription666'
-            """)
-        cur.execute("""
-            UPDATE Question SET
-                title='OOPS - 1TicketTitle666 bar',
-                description='http://foo.com OOPS-1TicketDescription666',
-                whiteboard='OOPS-1TicketWhiteboard666'
-                WHERE id=1
-            """)
-        # Add a question entry with a NULL whiteboard to ensure the SQL query
-        # copes.
-        cur.execute("""
-            UPDATE Question SET
-                title='OOPS - 1TicketTitle666 bar',
-                description='http://foo.com OOPS-1TicketDescription666',
-                whiteboard=NULL
-                WHERE id=2
-            """)
-
-        # Need to commit or the changes are not visible on the slave.
-        transaction.commit()
-
-        self.failUnlessEqual(
-                set([
-                    self.referenced_oops_code,
-                    '1MESSAGESUBJECT666',
-                    '1BUGTITLE666',
-                    '1BUGDESCRIPTION666',
-                    '1TICKETTITLE666',
-                    '1TICKETDESCRIPTION666',
-                    '1TICKETWHITEBOARD666',
-                    ]),
-                referenced_oops()
-                )
-
-    def test_old_oops_files(self):
-        old = set(old_oops_files(self.oops_dir, 90))
-        self.failUnlessEqual(len(old), 8)
-        # Make sure the paths are valid
-        for old_path in old:
-            self.failUnless(os.path.exists(old_path))
-            self.failUnless(
-                    '2006-01' in old_path, '%s not in old area' % old_path
-                    )
-
-    def test_unwanted_oops_files(self):
-        unwanted = set(unwanted_oops_files(self.oops_dir, 90))
-        # Make sure that 2A666 and 2a666 are wanted.
-        unwanted_ids = set(path_to_oopsid(path) for path in unwanted)
-        self.failUnlessEqual(
-            unwanted_ids,
-            set(['2A1234', '2A5678', '3A666', '3a666', '3A1234', '3A5678'])
-            )
-        # Make sure the paths are valid
-        for unwanted_path in unwanted:
-            self.failUnless(os.path.exists(unwanted_path))
-            self.failUnless(
-                    '2006-01' in unwanted_path,
-                    'New OOPS %s unwanted' % unwanted_path
-                    )
-
-    def test_referenced_oops_in_urls(self):
-        # Sometimes OOPS ids appears as part of an URL. We don't want the
-        # POSIX regexp matching on those OOPS ids since the FormattersAPI
-        # doesn't match them.
-        cur = cursor()
-        cur.execute("""
-        UPDATE Bug SET
-            title='Some title',
-            description=
-                'https://lp-oops.canonical.com/oops.py/?oopsid=OOPS-1Foo666'
-        """)
-        self.failUnlessEqual(
-                set([self.referenced_oops_code]),
-                referenced_oops())
-
-    def test_script(self):
-        unwanted_oops_files(self.oops_dir, 90)
-        # Commit so our script can see changes made by the setUp method
-        LaunchpadZopelessLayer.commit()
-        process = Popen([
-                sys.executable,
-                os.path.join(config.root, 'cronscripts', 'oops-prune.py'),
-                '-q', self.oops_dir,
-                ], stdin=PIPE, stdout=PIPE, stderr=STDOUT)
-        (out, err) = process.communicate()
-        self.failUnlessEqual(out, '')
-
-        # Check out OOPS directory to ensure that there are now only
-        # three OOPS reports - 1 in the old directory and 3 in the new one.
-        today_dir = os.path.join(
-                self.oops_dir, self.today.strftime('%Y-%m-%d')
-                )
-        found_oops_files = set()
-        for dirpath, dirnames, filenames in os.walk(today_dir):
-            for filename in filenames:
-                found_oops_files.add(
-                        path_to_oopsid(os.path.join(dirpath, filename))
-                        )
-        today_day_count = (self.today - uniquefileallocator.epoch).days + 1
-        self.failUnlessEqual(
-                found_oops_files,
-                set([
-                    '%dA666' % today_day_count,
-                    '%da666' % today_day_count,
-                    '%dA1234' % today_day_count,
-                    '%dA5678' % today_day_count,
-                    ])
-                )
-
-        old_dir = os.path.join(
-                self.oops_dir, self.ages_ago.strftime('%Y-%m-%d')
-                )
-        found_oops_files = set()
-        for dirpath, dirnames, filenames in os.walk(old_dir):
-            for filename in filenames:
-                found_oops_files.add(
-                        path_to_oopsid(os.path.join(dirpath, filename))
-                        )
-        # Note that we err on the side of caution when we find files
-        # differing by case.
-        self.failUnlessEqual(
-                found_oops_files,
-                set(['2A666', '2a666'])
-                )
-
-        # The directory containing only old, unreferenced OOPS files should
-        # have been removed.
-        self.failIf(
-                os.path.isdir(os.path.join(self.oops_dir, '2006-01-03')),
-                'Script failed to remove 2006-01-03 directory'
-                )
-
-    def test_script_dryrun(self):
-        unwanted_oops_files(self.oops_dir, 90)
-        # Commit so our script can see changes made by the setUp method
-        LaunchpadZopelessLayer.commit()
-
-        # Count how many OOPS reports there currently are
-        orig_count = 0
-        for dirpath, dirnames, filenames in os.walk(self.oops_dir):
-            for filename in filenames:
-                if re.search(
-                    r'^\d+\.\d+[a-zA-Z]\d+(?:\.gz|\.bz2)?$', filename):
-                    orig_count += 1
-
-        # Run the script, which should make no changes with the --dry-run
-        # option.
-        process = Popen([
-                sys.executable,
-                os.path.join(config.root, 'cronscripts', 'oops-prune.py'),
-                '-q', '--dry-run', self.oops_dir,
-                ], stdin=PIPE, stdout=PIPE, stderr=STDOUT)
-        (out, err) = process.communicate()
-        self.failUnlessEqual(out, '')
-
-        # Check out OOPS directory to ensure that no OOPS reports have
-        # been removed.
-        new_count = 0
-        for dirpath, dirnames, filenames in os.walk(self.oops_dir):
-            for filename in filenames:
-                if re.search(
-                    r'^\d+\.\d+[a-zA-Z]\d+(?:\.gz|\.bz2)?$', filename):
-                    new_count += 1
-
-        self.failUnlessEqual(orig_count, new_count)
-
-    def test_script_default_error_dir(self):
-        # Verify that the script runs without the error_dir argument.
-        default_error_dir = config.error_reports.error_dir
-        unwanted_oops_files(default_error_dir, 90)
-        # Commit so our script can see changes made by the setUp method.
-        LaunchpadZopelessLayer.commit()
-        process = Popen([
-            sys.executable,
-            os.path.join(config.root, 'cronscripts', 'oops-prune.py'), '-q'],
-            stdin=PIPE, stdout=PIPE, stderr=STDOUT)
-        (out, err) = process.communicate()
-        self.failUnlessEqual(out, '')
-
-    def test_prune_empty_oops_directories(self):
-        # And a directory empty of OOPS reports
-        os.mkdir(os.path.join(self.oops_dir, '2038-12-01'))
-        os.mkdir(os.path.join(self.oops_dir, '2001-12-01'))
-
-        # And a directory empty of OOPS reports, but with some rubbish
-        os.mkdir(os.path.join(self.oops_dir, '2001-12-02'))
-        open(
-                os.path.join(self.oops_dir, '2001-12-02', 'foo'), 'w'
-                ).write('foo')
-
-        prune_empty_oops_directories(self.oops_dir)
-
-        # Most directories should still be there
-        for date in ['2006-01-02', '2006-01-03', '2001-12-02']:
-            self.failUnless(
-                os.path.isdir(os.path.join(self.oops_dir, date))
-                )
-
-        # But the empty ones should be gone
-        for date in ['2038-12-01', '2001-12-01']:
-            self.failIf(
-                os.path.isdir(os.path.join(self.oops_dir, date))
-                )
+        super(TestOopsReferences, self).setUp()
+        self.store = IStore(Message)
+
+    def test_oops_in_messagechunk(self):
+        oopsid = "OOPS-abcdef1234"
+        MessageSet().fromText('foo', "foo %s bar" % oopsid)
+        self.store.flush()
+        now = datetime.now(tz=utc)
+        day = timedelta(days=1)
+        self.failUnlessEqual(
+            set([oopsid.upper()]),
+            referenced_oops(now - day, now, "product=1", {}))
+        self.failUnlessEqual(
+            set(),
+            referenced_oops(now + day, now + day, "product=1", {}))
+
+    def test_oops_in_messagesubject(self):
+        oopsid = "OOPS-abcdef1234"
+        self.factory.makeEmailMessage()
+        MessageSet().fromText("Crash with %s" % oopsid, "body")
+        self.store.flush()
+        now = datetime.now(tz=utc)
+        day = timedelta(days=1)
+        self.failUnlessEqual(
+            set([oopsid.upper()]),
+            referenced_oops(now - day, now, "product=1", {}))
+        self.failUnlessEqual(
+            set(),
+            referenced_oops(now + day, now + day, "product=1", {}))
+
+    def test_oops_in_bug_title(self):
+        oopsid = "OOPS-abcdef1234"
+        bug = self.factory.makeBug()
+        with person_logged_in(bug.owner):
+            bug.title = "Crash with %s" % oopsid
+        self.store.flush()
+        now = datetime.now(tz=utc)
+        day = timedelta(days=1)
+        self.failUnlessEqual(
+            set([oopsid.upper()]),
+            referenced_oops(now - day, now, "product=1", {}))
+        self.failUnlessEqual(
+            set(),
+            referenced_oops(now + day, now + day, "product=1", {}))
+
+    def test_oops_in_bug_description(self):
+        oopsid = "OOPS-abcdef1234"
+        bug = self.factory.makeBug()
+        with person_logged_in(bug.owner):
+            bug.description = "Crash with %s" % oopsid
+        self.store.flush()
+        now = datetime.now(tz=utc)
+        day = timedelta(days=1)
+        self.failUnlessEqual(
+            set([oopsid.upper()]),
+            referenced_oops(now - day, now, "product=1", {}))
+        self.failUnlessEqual(
+            set(),
+            referenced_oops(now + day, now + day, "product=1", {}))
+
+    def test_oops_in_question_title(self):
+        oopsid = "OOPS-abcdef1234"
+        question = self.factory.makeQuestion(title="Crash with %s" % oopsid)
+        self.store.flush()
+        now = datetime.now(tz=utc)
+        day = timedelta(days=1)
+        self.failUnlessEqual(
+            set([oopsid.upper()]),
+            referenced_oops(now - day, now, "product=%(product)s",
+            {'product': question.product.id}))
+        self.failUnlessEqual(
+            set([]),
+            referenced_oops(now + day, now + day, "product=%(product)s",
+            {'product': question.product.id}))
+
+    def test_oops_in_question_wrong_context(self):
+        oopsid = "OOPS-abcdef1234"
+        question = self.factory.makeQuestion(title="Crash with %s" % oopsid)
+        self.store.flush()
+        now = datetime.now(tz=utc)
+        day = timedelta(days=1)
+        self.store.flush()
+        self.failUnlessEqual(
+            set(),
+            referenced_oops(now - day, now, "product=%(product)s",
+            {'product': question.product.id + 1}))
+
+    def test_oops_in_question_description(self):
+        oopsid = "OOPS-abcdef1234"
+        question = self.factory.makeQuestion(
+            description="Crash with %s" % oopsid)
+        self.store.flush()
+        now = datetime.now(tz=utc)
+        day = timedelta(days=1)
+        self.failUnlessEqual(
+            set([oopsid.upper()]),
+            referenced_oops(now - day, now, "product=%(product)s",
+            {'product': question.product.id}))
+        self.failUnlessEqual(
+            set([]),
+            referenced_oops(now + day, now + day, "product=%(product)s",
+            {'product': question.product.id}))
+
+    def test_oops_in_question_whiteboard(self):
+        oopsid = "OOPS-abcdef1234"
+        question = self.factory.makeQuestion()
+        with person_logged_in(question.owner):
+            question.whiteboard = "Crash with %s" % oopsid
+            self.store.flush()
+        now = datetime.now(tz=utc)
+        day = timedelta(days=1)
+        self.failUnlessEqual(
+            set([oopsid.upper()]),
+            referenced_oops(now - day, now, "product=%(product)s",
+            {'product': question.product.id}))
+        self.failUnlessEqual(
+            set([]),
+            referenced_oops(now + day, now + day, "product=%(product)s",
+            {'product': question.product.id}))
+
+    def test_oops_in_question_distribution(self):
+        oopsid = "OOPS-abcdef1234"
+        distro = self.factory.makeDistribution()
+        question = self.factory.makeQuestion(target=distro)
+        with person_logged_in(question.owner):
+            question.whiteboard = "Crash with %s" % oopsid
+            self.store.flush()
+        now = datetime.now(tz=utc)
+        day = timedelta(days=1)
+        self.failUnlessEqual(
+            set([oopsid.upper()]),
+            referenced_oops(now - day, now, "distribution=%(distribution)s",
+            {'distribution': distro.id}))
+        self.failUnlessEqual(
+            set([]),
+            referenced_oops(now + day, now + day,
+            "distribution=%(distribution)s", {'distribution': distro.id}))
+
+    def test_referenced_oops_in_urls_bug_663249(self):
+        # Sometimes OOPS ids appears as part of an URL. These should could as
+        # a reference even though they are not formatted specially - this
+        # requires somewhat special handling in the reference calculation
+        # function.
+        oopsid = "OOPS-abcdef1234"
+        bug = self.factory.makeBug()
+        with person_logged_in(bug.owner):
+            bug.description = (
+                "foo https://lp-oops.canonical.com/oops.py?oopsid=%s bar"
+                % oopsid)
+            self.store.flush()
+        now = datetime.now(tz=utc)
+        day = timedelta(days=1)
+        self.failUnlessEqual(
+            set([oopsid.upper()]),
+            referenced_oops(now - day, now, "product=1", {}))
+        self.failUnlessEqual(
+            set([]),
+            referenced_oops(now + day, now + day, "product=1", {}))

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2011-10-18 12:57:33 +0000
+++ lib/lp/registry/tests/test_product.py	2011-11-11 02:17:27 +0000
@@ -7,6 +7,7 @@
 import datetime
 
 import pytz
+from testtools.matchers import MatchesAll
 import transaction
 from zope.security.proxy import removeSecurityProxy
 
@@ -15,6 +16,7 @@
     IHasLogo,
     IHasMugshot,
     )
+from canonical.launchpad.interfaces.lpstorm import IStore
 from canonical.launchpad.testing.pages import (
     find_main_content,
     get_feedback_messages,
@@ -34,6 +36,7 @@
 from lp.bugs.interfaces.bugsummary import IBugSummaryDimension
 from lp.bugs.interfaces.bugsupervisor import IHasBugSupervisor
 from lp.bugs.interfaces.bugtarget import IHasBugHeat
+from lp.registry.interfaces.oopsreferences import IHasOOPSReferences
 from lp.registry.interfaces.product import (
     IProduct,
     License,
@@ -76,17 +79,22 @@
     def test_implements_interfaces(self):
         # Product fully implements its interfaces.
         product = removeSecurityProxy(self.factory.makeProduct())
-        self.assertThat(product, Provides(IProduct))
-        self.assertThat(product, Provides(IBugSummaryDimension))
-        self.assertThat(product, Provides(IFAQTarget))
-        self.assertThat(product, Provides(IHasBugHeat))
-        self.assertThat(product, Provides(IHasBugSupervisor))
-        self.assertThat(product, Provides(IHasCustomLanguageCodes))
-        self.assertThat(product, Provides(IHasIcon))
-        self.assertThat(product, Provides(IHasLogo))
-        self.assertThat(product, Provides(IHasMugshot))
-        self.assertThat(product, Provides(ILaunchpadUsage))
-        self.assertThat(product, Provides(IServiceUsage))
+        expected_interfaces = [
+            IProduct,
+            IBugSummaryDimension,
+            IFAQTarget,
+            IHasBugHeat,
+            IHasBugSupervisor,
+            IHasCustomLanguageCodes,
+            IHasIcon,
+            IHasLogo,
+            IHasMugshot,
+            IHasOOPSReferences,
+            ILaunchpadUsage,
+            IServiceUsage,
+            ]
+        provides_all = MatchesAll(*map(Provides, expected_interfaces))
+        self.assertThat(product, provides_all)
 
     def test_deactivation_failure(self):
         # Ensure that a product cannot be deactivated if
@@ -438,3 +446,35 @@
         ws_group = self.wsObject(group)
         ws_product.translationgroup = ws_group
         ws_product.lp_save()
+
+    def test_oops_references_matching_product(self):
+        # The product layer provides the context restriction, so we need to
+        # check we can access context filtered references - e.g. on question.
+        oopsid = "OOPS-abcdef1234"
+        question = self.factory.makeQuestion(title="Crash with %s" % oopsid)
+        product = question.product
+        transaction.commit()
+        ws_product = self.wsObject(product, product.owner)
+        now = datetime.datetime.now(tz=pytz.utc)
+        day = datetime.timedelta(days=1)
+        self.failUnlessEqual(
+            [oopsid.upper()],
+            ws_product.findReferencedOOPS(start_date=now - day, end_date=now))
+        self.failUnlessEqual(
+            [],
+            ws_product.findReferencedOOPS(
+                start_date=now + day, end_date=now + day))
+
+    def test_oops_references_different_product(self):
+        # The product layer provides the context restriction, so we need to
+        # check the filter is tight enough - other contexts should not work.
+        oopsid = "OOPS-abcdef1234"
+        self.factory.makeQuestion(title="Crash with %s" % oopsid)
+        product = self.factory.makeProduct()
+        transaction.commit()
+        ws_product = self.wsObject(product, product.owner)
+        now = datetime.datetime.now(tz=pytz.utc)
+        day = datetime.timedelta(days=1)
+        self.failUnlessEqual(
+            [],
+            ws_product.findReferencedOOPS(start_date=now - day, end_date=now))