Merge lp:~jtv/launchpad/bug-798297 into lp:launchpad

[Jeroen T. Vermeulen]

[r=julian-edwards][bug=798297] Provide API access to DistroSeriesDifferenceComments.

= Summary =

The Ubuntu people need API access to DistroSeriesDifferenceComments. Something called Merge-o-Matic will poll for new comments and pull them into its own database.

We need to provide three kinds of filtering: by distroseries (regardless of exact use), by time the comment was made (to support update polling), and by source package name (to produce per-package timelines).

== Proposed fix ==

Provide all these through the DistroSeries devel API, using a single method.

The combination of filtering by age and distroseries can be awkward, since these two items are at opposite ends of the schema sub-graph that we need for this. Luckily, DistroSeriesDifferenceComments are really just thin immutable wrappers around Message. So instead of filtering by Message.datecreated (the real age of a comment) I find the last Message.id prior to the filter date, and look for DistroSeriesDifferenceComments whose message fields are greater than that.

It looks as if this will require a Message table scan; I'm not sure how costly that will be but expect that we'll probably want an index to support this.

== Pre-implementation notes ==

Julian says security is not a concern for this method as yet; we'll trust the security machinery to keep anyone out who isn't allowed to view the DistroSeries.

The use-case is very similar to what we have with feeds, so this may also become the basis for future feed support.

== Implementation details ==

We could have the client remember the last-polled DSDComment id, but we prefer not to expose ids and use dates instead.

== Tests ==

{{{
./bin/test -vvc lp.registry.tests.test_distroseries -t DifferenceComments
./bin/test -vvc lp.registry.tests.test_distroseriesdifferencecomment -t getForDistroSeries
}}}

== Demo and Q/A ==

We'll be able to browse (and to a limited extent) query DSDComments for a DistroSeries. They should be filtered appropriately and ordered by creation time.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/tests/test_distroseriesdifferencecomment.py
  lib/lp/registry/tests/test_distroseries.py
  lib/lp/registry/model/distroseriesdifferencecomment.py
  lib/lp/registry/interfaces/distroseries.py
  lib/lp/registry/interfaces/distroseriesdifferencecomment.py
  lib/lp/registry/model/distroseries.py
  lib/canonical/launchpad/interfaces/_schema_circular_imports.py

[Julian Edwards (julian-edwards)]

76 + :param since: A date. No comments older than this date will be
77 + returned.

We should probably refer to "timestamp" here instead of "date". This string will appear in +apidoc remember!

138 + @property
139 + def source_package_name(self):
140 + """See `IDistroSeriesDifferenceCommentSource`."""
141 + return self.distro_series_difference.source_package_name.name

I'm not sure about the name of this but I can't think of anything better - source_package_name is getting conflated between text and the object everywhere :( However there's precedence in SPPH's exported properties so I guess it doesn't matter here. Just thought I'd mention it anway.

174 + after_msgid = store.find(
175 + Max(Message.id), Message.datecreated < since).one()

If this does cause a scan on Message then we're screwed, Message is a huge table. Looking at the indexes I'm pretty sure it will since there's no index on datecreated. :(

Finally, lovely tests! You just need some more that test the webservice :)

review: Needs Fixing

[Jeroen T. Vermeulen (jtv)]

I've replaced the "date" with "timestamp."

The source_package_name was what I thought you suggested in our pre-imp call; it's sad to see confusion there. Perhaps we ought to stick with sourcepackagename for the SourcePackageName class and source_package_name for the name of a source package.

I will get back to you on IRC with more about the query plan. We may want to slap on an index.

[Julian Edwards (julian-edwards)]

On Tuesday 05 July 2011 11:17:33 you wrote:
> I've replaced the "date" with "timestamp."
>
> The source_package_name was what I thought you suggested in our pre-imp
> call; it's sad to see confusion there.

It's not confusion and yes I did suggest it. I'm merely pointing out that
naming stuff is hard :)

> Perhaps we ought to stick with
> sourcepackagename for the SourcePackageName class and source_package_name
> for the name of a source package.

Maybe, yeah.

> I will get back to you on IRC with more about the query plan. We may want
> to slap on an index.

Righto, cheers.

[Jeroen T. Vermeulen (jtv)]

The query that looks for the last Message.id to be omitted doesn't play well with indexes. Shouldn't usually be a problem, but can cause annoying slowdowns.

We can fix it by indexing Message.datecreated, and changing the query to:

SELECT max(id)
FROM Message
WHERE datecreated = (
    SELECT max(datecreated)
    FROM Message
    WHERE datecreated < %(since)s)

On a hot cache, this will scan back through years in a few milliseconds.

[Robert Collins (lifeless)]

that looks spectacularly awkward; isn't indexing datecreated and doing
select id from message where datecreated < %(since) order by
datecreated desc limit 1;

I get the following:

 explain analyze select id from message where datecreated <
'2008-06-01 10:10:10' order by datecreated desc limit 1;

QUERY PLAN
----------------------------------------------------------------------------------------------------------------------------------------------------
 Limit (cost=0.00..0.08 rows=1 width=12) (actual time=0.027..0.028
rows=1 loops=1)
   -> Index Scan Backward using message_created on message
(cost=0.00..194232.18 rows=2286416 width=12) (actual time=0.026..0.026
rows=1 loops=1)
         Index Cond: (datecreated < '2008-06-01 10:10:10'::timestamp
without time zone)
 Total runtime: 0.067 ms

with a simple
create index message_created on message (datecreated);

-Rob

[Jeroen T. Vermeulen (jtv)]

I think this is ready for landing now. The required index for Message is already on its way in.

[Julian Edwards (julian-edwards)]

review: Approve

=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py	2011-06-23 18:26:26 +0000
+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py	2011-07-06 11:22:26 +0000
@@ -516,6 +516,8 @@
     DistroSeriesDifferenceType)
 patch_collection_return_type(
     IDistroSeries, 'getDifferencesTo', IDistroSeriesDifference)
+patch_collection_return_type(
+    IDistroSeries, 'getDifferenceComments', IDistroSeriesDifferenceComment)
 
 
 # IDistroSeriesDifference

=== modified file 'lib/lp/registry/browser/tests/test_distroseries_webservice.py'
--- lib/lp/registry/browser/tests/test_distroseries_webservice.py	2011-04-27 15:12:08 +0000
+++ lib/lp/registry/browser/tests/test_distroseries_webservice.py	2011-07-06 11:22:26 +0000
@@ -3,6 +3,8 @@
 
 __metaclass__ = type
 
+from datetime import timedelta
+
 from canonical.testing import AppServerLayer
 from lp.registry.enum import (
     DistroSeriesDifferenceStatus,
@@ -42,3 +44,16 @@
             source_package_name_filter=ds_diff.source_package_name.name,
             child_version_higher=False,
             parent_series=self._wsFor(ds_diff.parent_series)))
+
+    def test_getDifferenceComments(self):
+        # getDifferenceComments queries DistroSeriesDifferenceComments
+        # by distroseries, and optionally creation date and/or source
+        # package name.
+        comment = self.factory.makeDistroSeriesDifferenceComment()
+        dsd = comment.distro_series_difference
+        yesterday = comment.comment_date - timedelta(1)
+        package_name = dsd.source_package_name.name
+        ds_ws = self._wsFor(dsd.derived_series)
+        search_results = ds_ws.getDifferenceComments(
+            since=yesterday, source_package_name=package_name)
+        self.assertContentEqual([self._wsFor(comment)], search_results)

=== modified file 'lib/lp/registry/interfaces/distroseries.py'
--- lib/lp/registry/interfaces/distroseries.py	2011-06-20 16:04:15 +0000
+++ lib/lp/registry/interfaces/distroseries.py	2011-07-06 11:22:26 +0000
@@ -903,6 +903,30 @@
     def isInitialized():
         """Has this series been initialized?"""
 
+    @operation_parameters(
+        since=Datetime(
+            title=_("Minimum creation timestamp"),
+            description=_(
+                "Ignore comments that are older than this."),
+            required=False),
+        source_package_name=TextLine(
+            title=_("Name of source package"),
+            description=_("Only return comments for this source package."),
+            required=False))
+    @operation_returns_collection_of(Interface)
+    @export_read_operation()
+    @operation_for_version('devel')
+    def getDifferenceComments(since=None, source_package_name=None):
+        """Get `IDistroSeriesDifferenceComment` items.
+
+        :param since: Ignore comments older than this timestamp.
+        :param source_package_name: Return only comments for a source package
+            with this name.
+        :return: A Storm result set of `IDistroSeriesDifferenceComment`
+            objects for this distroseries, ordered from oldest to newest
+            comment.
+        """
+
 
 class IDistroSeriesEditRestricted(Interface):
     """IDistroSeries properties which require launchpad.Edit."""

=== modified file 'lib/lp/registry/interfaces/distroseriesdifferencecomment.py'
--- lib/lp/registry/interfaces/distroseriesdifferencecomment.py	2011-05-12 21:33:10 +0000
+++ lib/lp/registry/interfaces/distroseriesdifferencecomment.py	2011-07-06 11:22:26 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Distribution series difference messages."""
@@ -20,6 +20,7 @@
     Datetime,
     Int,
     Text,
+    TextLine,
     )
 
 from canonical.launchpad import _
@@ -56,6 +57,11 @@
     comment_date = exported(Datetime(
         title=_('Comment date.'), readonly=True))
 
+    source_package_name = exported(TextLine(
+        title=_("Source package name"), required=True, readonly=True,
+        description=_(
+            "Name of the source package that this comment is for.")))
+
 
 class IDistroSeriesDifferenceCommentSource(Interface):
     """A utility of this interface can be used to create comments."""
@@ -72,3 +78,13 @@
 
     def getForDifference(distro_series_difference, id):
         """Return the `IDistroSeriesDifferenceComment` with the given id."""
+
+    def getForDistroSeries(distroseries, since=None):
+        """Get comments for `distroseries` (since `since` if given).
+
+        :param distroseries: The `DistroSeries` to find comments for.
+        :param since: A timestamp.  No comments older than this will be
+            returned.
+        :return: A result set of `DistroSeriesDifferenceComment`s, ordered
+            from oldest to newest.
+        """

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2011-06-17 14:23:52 +0000
+++ lib/lp/registry/model/distroseries.py	2011-07-06 11:22:26 +0000
@@ -101,6 +101,9 @@
 from lp.registry.interfaces.distroseriesdifference import (
     IDistroSeriesDifferenceSource,
     )
+from lp.registry.interfaces.distroseriesdifferencecomment import (
+    IDistroSeriesDifferenceCommentSource,
+    )
 from lp.registry.interfaces.person import validate_public_person
 from lp.registry.interfaces.pocket import (
     PackagePublishingPocket,
@@ -1895,6 +1898,12 @@
         published = self.main_archive.getPublishedSources(distroseries=self)
         return not published.is_empty()
 
+    def getDifferenceComments(self, since=None, source_package_name=None):
+        """See `IDistroSeries`."""
+        comment_source = getUtility(IDistroSeriesDifferenceCommentSource)
+        return comment_source.getForDistroSeries(
+            self, since=since, source_package_name=source_package_name)
+
 
 class DistroSeriesSet:
     implements(IDistroSeriesSet)

=== modified file 'lib/lp/registry/model/distroseriesdifferencecomment.py'
--- lib/lp/registry/model/distroseriesdifferencecomment.py	2011-05-12 21:33:10 +0000
+++ lib/lp/registry/model/distroseriesdifferencecomment.py	2011-07-06 11:22:26 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """A comment/message for a difference between two distribution series."""
@@ -12,6 +12,7 @@
 from email.Utils import make_msgid
 
 from storm.locals import (
+    Desc,
     Int,
     Reference,
     Storm,
@@ -30,6 +31,7 @@
     IDistroSeriesDifferenceComment,
     IDistroSeriesDifferenceCommentSource,
     )
+from lp.registry.model.sourcepackagename import SourcePackageName
 
 
 class DistroSeriesDifferenceComment(Storm):
@@ -63,6 +65,11 @@
         """See `IDistroSeriesDifferenceComment`."""
         return self.message.datecreated
 
+    @property
+    def source_package_name(self):
+        """See `IDistroSeriesDifferenceCommentSource`."""
+        return self.distro_series_difference.source_package_name.name
+
     @staticmethod
     def new(distro_series_difference, owner, comment):
         """See `IDistroSeriesDifferenceCommentSource`."""
@@ -90,3 +97,36 @@
             DSDComment,
             DSDComment.distro_series_difference == distro_series_difference,
             DSDComment.id == id).one()
+
+    @staticmethod
+    def getForDistroSeries(distroseries, since=None,
+                           source_package_name=None):
+        """See `IDistroSeriesDifferenceCommentSource`."""
+        # Avoid circular imports.
+        from lp.registry.model.distroseriesdifference import (
+            DistroSeriesDifference,
+            )
+        store = IStore(DistroSeriesDifferenceComment)
+        DSD = DistroSeriesDifference
+        DSDComment = DistroSeriesDifferenceComment
+        conditions = [
+            DSDComment.distro_series_difference_id == DSD.id,
+            DSD.derived_series_id == distroseries.id,
+            ]
+
+        if source_package_name is not None:
+            conditions += [
+                SourcePackageName.id == DSD.source_package_name_id,
+                SourcePackageName.name == source_package_name,
+                ]
+
+        if since is not None:
+            older_messages = store.find(
+                Message.id, Message.datecreated < since).order_by(
+                    Desc(Message.datecreated))
+            preceding_message = older_messages.first()
+            if preceding_message is not None:
+                conditions.append(DSDComment.message_id > preceding_message)
+
+        return store.find(DSDComment, *conditions).order_by(
+            DSDComment.message_id)

=== modified file 'lib/lp/registry/tests/test_distroseries.py'
--- lib/lp/registry/tests/test_distroseries.py	2011-06-14 19:49:18 +0000
+++ lib/lp/registry/tests/test_distroseries.py	2011-07-06 11:22:26 +0000
@@ -256,6 +256,14 @@
             distroseries=distroseries, archive=distroseries.main_archive)
         self.assertTrue(distroseries.isInitialized())
 
+    def test_getDifferenceComments_gets_DistroSeriesDifferenceComments(self):
+        distroseries = self.factory.makeDistroSeries()
+        dsd = self.factory.makeDistroSeriesDifference(
+            derived_series=distroseries)
+        comment = self.factory.makeDistroSeriesDifferenceComment(dsd)
+        self.assertContentEqual(
+            [comment], distroseries.getDifferenceComments())
+
 
 class TestDistroSeriesPackaging(TestCaseWithFactory):
 

=== modified file 'lib/lp/registry/tests/test_distroseriesdifferencecomment.py'
--- lib/lp/registry/tests/test_distroseriesdifferencecomment.py	2010-10-06 18:53:53 +0000
+++ lib/lp/registry/tests/test_distroseriesdifferencecomment.py	2011-07-06 11:22:26 +0000
@@ -1,20 +1,41 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Model tests for the DistroSeriesDifferenceComment class."""
 
 __metaclass__ = type
 
+from datetime import timedelta
+from random import randint
 from storm.store import Store
 from zope.component import getUtility
 
 from canonical.launchpad.webapp.testing import verifyObject
-from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.testing.layers import (
+    DatabaseFunctionalLayer,
+    ZopelessDatabaseLayer,
+    )
 from lp.registry.interfaces.distroseriesdifferencecomment import (
     IDistroSeriesDifferenceComment,
     IDistroSeriesDifferenceCommentSource,
     )
 from lp.testing import TestCaseWithFactory
+from lp.testing.matchers import Provides
+
+
+def get_comment_source():
+    """Get `IDistroSeriesDifferemtCommentSource` utility."""
+    return getUtility(IDistroSeriesDifferenceCommentSource)
+
+
+def flip_coin(*args):
+    """Random comparison function.  Returns -1 or 1 randomly."""
+    return 1 - 2 * randint(0, 1)
+
+
+def randomize_list(original_list):
+    """Sort a list (or other iterable) in random order.  Return list."""
+    return sorted(original_list, cmp=flip_coin)
 
 
 class DistroSeriesDifferenceCommentTestCase(TestCaseWithFactory):
@@ -64,7 +85,90 @@
         dsd_comment = self.factory.makeDistroSeriesDifferenceComment()
         Store.of(dsd_comment).flush()
 
-        comment_src = getUtility(IDistroSeriesDifferenceCommentSource)
         self.assertEqual(
-            dsd_comment, comment_src.getForDifference(
+            dsd_comment, get_comment_source().getForDifference(
                 dsd_comment.distro_series_difference, dsd_comment.id))
+
+    def test_source_package_name_returns_package_name(self):
+        # The comment "knows" the name of the source package it's for.
+        package_name = self.factory.getUniqueUnicode()
+        dsd = self.factory.makeDistroSeriesDifference(
+            source_package_name_str=package_name)
+        comment = self.factory.makeDistroSeriesDifferenceComment(dsd)
+        self.assertEqual(package_name, comment.source_package_name)
+
+
+class TestDistroSeriesDifferenceCommentSource(TestCaseWithFactory):
+
+    layer = ZopelessDatabaseLayer
+
+    def test_implements_interface(self):
+        self.assertThat(
+            get_comment_source(),
+            Provides(IDistroSeriesDifferenceCommentSource))
+
+    def test_getForDistroSeries_returns_result_set(self):
+        series = self.factory.makeDistroSeries()
+        source = get_comment_source()
+        self.assertTrue(source.getForDistroSeries(series).is_empty())
+
+    def test_getForDistroSeries_matches_on_distroseries(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        series = dsd.derived_series
+        comment = self.factory.makeDistroSeriesDifferenceComment(dsd)
+        source = get_comment_source()
+        self.assertContentEqual([comment], source.getForDistroSeries(series))
+
+    def test_getForDistroSeries_filters_by_distroseries(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        other_series = self.factory.makeDistroSeries()
+        self.factory.makeDistroSeriesDifferenceComment(dsd)
+        source = get_comment_source()
+        self.assertContentEqual([], source.getForDistroSeries(other_series))
+
+    def test_getForDistroSeries_matches_on_since(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        series = dsd.derived_series
+        comment = self.factory.makeDistroSeriesDifferenceComment(dsd)
+        source = get_comment_source()
+        yesterday = comment.comment_date - timedelta(1)
+        self.assertContentEqual(
+            [comment], source.getForDistroSeries(series, since=yesterday))
+
+    def test_getForDistroSeries_filters_by_since(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        series = dsd.derived_series
+        comment = self.factory.makeDistroSeriesDifferenceComment(dsd)
+        source = get_comment_source()
+        tomorrow = comment.comment_date + timedelta(1)
+        self.assertContentEqual(
+            [], source.getForDistroSeries(series, since=tomorrow))
+
+    def test_getForDistroSeries_orders_by_age(self):
+        series = self.factory.makeDistroSeries()
+        dsds = randomize_list([
+            self.factory.makeDistroSeriesDifference(derived_series=series)
+            for counter in xrange(5)])
+        comments = [
+            self.factory.makeDistroSeriesDifferenceComment(dsd)
+            for dsd in dsds]
+        source = get_comment_source()
+        self.assertEqual(comments, list(source.getForDistroSeries(series)))
+
+    def test_getForDistroSeries_matches_on_package_name(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        series = dsd.derived_series
+        package_name = dsd.source_package_name.name
+        comment = self.factory.makeDistroSeriesDifferenceComment(dsd)
+        source = get_comment_source()
+        self.assertContentEqual([comment], source.getForDistroSeries(
+            series, source_package_name=package_name))
+
+    def test_getForDistroSeries_filters_by_package_name(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        series = dsd.derived_series
+        other_package = self.factory.getUniqueUnicode()
+        self.factory.makeDistroSeriesDifferenceComment(dsd)
+        source = get_comment_source()
+        self.assertContentEqual([], source.getForDistroSeries(
+            series, source_package_name=other_package))