Merge lp:~benji/launchpad/bug-781600 into lp:launchpad

[Benji York]

[r=flacoste][bug=781600] Provide a web service API to retrieve the branch tip revisions (optionally since a given date) and official series information for a given distribution.

Bug 781600 is about providing a web service API to retrieve the branch
tip revisions (optionally since a given date) and official series
information for a given distribution. This branch adds a getBranchTips
named operation to distributions to provide that functionality.

Preimplemenation discussion was had with Francis.

The implementation is a relatively straight-forward SQL query who's
results are lightly post-processed to provide a slightly nicer (and
requested) structure for the return value.

Tests were added to lib/lp/registry/tests/test_distro_webservice.py.
Specifically the TestGetBranchTips class.

Lint: some pre-existing lint fixed, now lint-free.

[Francis J. Lacoste (flacoste)]

> === modified file 'lib/lp/registry/interfaces/distribution.py'

> @operation_parameters(
> + since=Datetime(
> + title=_("Time of last change"),
> + description=_(
> + "Return branches that have new tips since this timestamp."),
> + required=False))
> + @export_operation_as(name="getBranchTips")
> + @export_read_operation()
> + @operation_for_version('devel')
> + def getBranchTips(since):
> + """Return a collection of branches which have new tips since a date.
> + """

I'd suggest improving the docstring. Something along the lines of:

    Return a list of branches information which have new tips since a date.

    Each branch information is a tuple of (branch_unique_name, tip_revision,
    (official_series*)).

    So for each branch in the distribution, you'll get the branch unique name,
    the revision id of tip, and if the branch is official for some series, the
    list of series name.

    :param since: If specified, only returns branch modified since that date
        time.

You probably need to make since=None since it's not a required parameter.

> === modified file 'lib/lp/registry/model/distribution.py'

> """See `IBugTarget`."""
> return get_bug_tags("BugTask.distribution = %s" % sqlvalues(self))
>
> + def getBranchTips(self, since=None):
> + """See `IDistribution`."""
> + query = """
> + SELECT unique_name, last_scanned_id,
> + SeriesSourcePackageBranch.distroseries FROM Branch
> + JOIN DistroSeries ON Branch.distroseries = DistroSeries.id
> + JOIN Distribution ON DistroSeries.distribution = Distribution.id
> + JOIN SeriesSourcePackageBranch ON
> + Branch.id = SeriesSourcePackageBranch.branch
> + WHERE Distribution.name = %s""" % sqlvalues(self.name)

Not all branches will be official, so you want to use a LEFT OUTER JOIN on
both SeriesSourcePackageBranch and DistroSeries.

Distro series id is pretty meangingless, we should return their name.
DistroSeries.name instead of SeriesSourcePackageBranch.distroseries

> +
> + if since is not None:
> + query += (
> + ' AND branch.last_scanned > %s' % sqlvalues(since))
> +
> + query += ' ORDER BY unique_name, last_scanned_id;'
> +
> + data = list(Store.of(self).execute(query))
> +
> + # Group on location (unique_name) and revision (last_scanned_id).
> + results = []
> + for key, group in itertools.groupby(data, itemgetter(0, 1)):
> + results.append(list(key))
> + # Pull out all the official series IDs and append them as a list
> + # to the end of the current record.
> + results[-1].append(map(itemgetter(-1), group))
> + return results
> +

That last grouping will need to be modified to handle the NULL case.

It's kind of a shame that we have to materialize the whole results set here,
since for huge size, it will be batched anyway by the web service code. Would
it make sense to use an generator here? Maybe not, since we'd still retrieve
all results for later batche...

review: Needs Fixing

[Benji York (benji)]

On Tue, Jul 12, 2011 at 3:34 PM, Francis J. Lacoste
<email address hidden> wrote:
> Review: Needs Fixing

Thanks very much for the review. I'm certainly not knowledgeable of
this area of LP yet and can use the help.

>> === modified file 'lib/lp/registry/interfaces/distribution.py'
>
>>      @operation_parameters(
>> +        since=Datetime(
>> +            title=_("Time of last change"),
>> +            description=_(
>> +                "Return branches that have new tips since this timestamp."),
>> +            required=False))
>> +    @export_operation_as(name="getBranchTips")
>> +    @export_read_operation()
>> +    @operation_for_version('devel')
>> +    def getBranchTips(since):
>> +        """Return a collection of branches which have new tips since a date.
>> +        """
>
> I'd suggest improving the docstring. Something along the lines of:

Looks good. Done.

> You probably need to make since=None since it's not a required parameter.

It seems odd to me to put the default in the interface in addition to in
the implementation. It has no effect and I worry that the DRY violation
could result in a loss of synchronization with the real default and
result in confusion. Also, the "required=False" bit is what indicates
that "since" is optional, the interface reader shouldn't care what
internal value we use to signify that it wasn't provided.

That being said, I'm only -0 so I'll add it given a bit of a push.

>> === modified file 'lib/lp/registry/model/distribution.py'
>
>>          """See `IBugTarget`."""
>>          return get_bug_tags("BugTask.distribution = %s" % sqlvalues(self))
>>
>> +    def getBranchTips(self, since=None):
>> +        """See `IDistribution`."""
>> +        query = """
>> +            SELECT unique_name, last_scanned_id,
>> +                SeriesSourcePackageBranch.distroseries FROM Branch
>> +            JOIN DistroSeries ON Branch.distroseries = DistroSeries.id
>> +            JOIN Distribution ON DistroSeries.distribution = Distribution.id
>> +            JOIN SeriesSourcePackageBranch ON
>> +                Branch.id = SeriesSourcePackageBranch.branch
>> +            WHERE Distribution.name = %s""" % sqlvalues(self.name)
>
> Not all branches will be official, so you want to use a LEFT OUTER JOIN on
> both SeriesSourcePackageBranch and DistroSeries.

I fixed that and added a test as mentioned below.

> Distro series id is pretty meangingless, we should return their name.
> DistroSeries.name instead of SeriesSourcePackageBranch.distroseries

Done. I had to tweak the query more than just substituting
DistroSeries.name but not too much.

>> +
>> +        if since is not None:
>> +            query += (
>> +                ' AND branch.last_scanned > %s' % sqlvalues(since))
>> +
>> +        query += ' ORDER BY unique_name, last_scanned_id;'
>> +
>> +        data = list(Store.of(self).execute(query))
>> +
>> +        # Group on location (unique_name) and revision (last_scanned_id).
>> +        results = []
>> +        for key, group in itertools.groupby(data, itemgetter(0, 1)):
>> +            results.append(list(key))
>> +            # Pull out all the official series IDs and append them as a list
>> +  ...

[Robert Collins (lifeless)]

On Thu, Jul 14, 2011 at 8:48 AM, Benji York <email address hidden> wrote:
>
>> It's kind of a shame that we have to materialize the whole results set here,
>> since for huge size, it will be batched anyway by the web service code. Would
>> it make sense to use an generator here? Maybe not, since we'd still retrieve
>> all results for later batches.
>
> Given that we use ORDER BY, I suspect the only savings would be not
> transmitting the non-batched results from the DB server to the app
> server.  However, making the method a generator might still be a win
> because even though they will likely come back to get the rest, that
> request may well hit a different app server.  (I don't think we do any
> sort of request affinity but would like to know if we do.)
>
> Later... Nope, that didn't work.  lazr.restful exploded because it
> couldn't adapt the generator to IFiniteSequence.  I would have expected
> returning a generator to work, but I can't find any other instances in
> the code base.  I've left it as a non-generator for the time being but
> would appreciate any knowledge that can be shared on the topic.

So, order by does not on its own mean that the whole result set is
materialised; in fact, if there is an index that can satisfy the
ordering, and its limited, pg seems to prioritise it very highly.

It looks like the grouping could be done in the DB ?
If so then returning the result set may make sense.

Another alternative is to use the new batchnavigator 1.2.5 facilities
to do batch memos rather than slicing. However that may require some
glue into the lazr stuff.. OTOH you're pretty well placed for that
sort of surgery :)

-Rob

[Francis J. Lacoste (flacoste)]

On 11-07-13 04:48 PM, Benji York wrote:
> On Tue, Jul 12, 2011 at 3:34 PM, Francis J. Lacoste

>
>> You probably need to make since=None since it's not a required parameter.
>
> It seems odd to me to put the default in the interface in addition to in
> the implementation. It has no effect and I worry that the DRY violation
> could result in a loss of synchronization with the real default and
> result in confusion. Also, the "required=False" bit is what indicates
> that "since" is optional, the interface reader shouldn't care what
> internal value we use to signify that it wasn't provided.
>
> That being said, I'm only -0 so I'll add it given a bit of a push.

Well, interfaces and implementation are usually not on the side of DRY
:-) To me, if the interface doesn't have a default parameter, it usually
means that client should expect to have to provide a parameter. Again,
the fact that this is mainly for webservice consumption makes this a
little less important. Anyway, Do as you will!

>
>>> === modified file 'lib/lp/registry/model/distribution.py'
>>
>>> """See `IBugTarget`."""
>>> return get_bug_tags("BugTask.distribution = %s" % sqlvalues(self))
>>>
>>> + def getBranchTips(self, since=None):
>>> + """See `IDistribution`."""
>>> + query = """
>>> + SELECT unique_name, last_scanned_id,
>>> + SeriesSourcePackageBranch.distroseries FROM Branch
>>> + JOIN DistroSeries ON Branch.distroseries = DistroSeries.id
>>> + JOIN Distribution ON DistroSeries.distribution = Distribution.id
>>> + JOIN SeriesSourcePackageBranch ON
>>> + Branch.id = SeriesSourcePackageBranch.branch
>>> + WHERE Distribution.name = %s""" % sqlvalues(self.name)
>>
>> Not all branches will be official, so you want to use a LEFT OUTER JOIN on
>> both SeriesSourcePackageBranch and DistroSeries.
>
> I fixed that and added a test as mentioned below.
>
>> Distro series id is pretty meangingless, we should return their name.
>> DistroSeries.name instead of SeriesSourcePackageBranch.distroseries
>
> Done. I had to tweak the query more than just substituting
> DistroSeries.name but not too much.

Some comment on your new query:

> + query = """
> + SELECT unique_name, last_scanned_id, SPBDS.name FROM Branch
> + LEFT OUTER JOIN DistroSeries
> + ON Branch.distroseries = DistroSeries.id

You probably want a normal join here as you don't expect any branch here
without a distroseries. (And this join is just to get at the
distribution right?)

> + LEFT OUTER JOIN SeriesSourcePackageBranch
> + ON Branch.id = SeriesSourcePackageBranch.branch
> + JOIN Distribution
> + ON DistroSeries.distribution = Distribution.id
> + LEFT OUTER JOIN DistroSeries SPBDS -- (SourcePackageBranchDistroSeries)
> + ON SeriesSourcePackageBranch.distroseries = SPBDS.id
> + WHERE Distribution.name = %s""" % sqlvalues(self.name)

You don't have to join the distribution here.

You could use

DistroSeries.distribution = self.id

The SPBDDS left outer join seems right....

review: Approve

[Benji York (benji)]

On Wed, Jul 13, 2011 at 6:13 PM, Francis J. Lacoste
<email address hidden> wrote:
> Review: Approve
> On 11-07-13 04:48 PM, Benji York wrote:
>> On Tue, Jul 12, 2011 at 3:34 PM, Francis J. Lacoste

> Some comment on your new query:
>
>> +        query = """
>> +            SELECT unique_name, last_scanned_id, SPBDS.name FROM Branch
>> +            LEFT OUTER JOIN DistroSeries
>> +                ON Branch.distroseries = DistroSeries.id
>
> You probably want a normal join here as you don't expect any branch here
> without a distroseries. (And this join is just to get at the
> distribution right?)

Good catch. Fixed.

>> +            LEFT OUTER JOIN SeriesSourcePackageBranch
>> +                ON Branch.id = SeriesSourcePackageBranch.branch
>> +            JOIN Distribution
>> +                ON DistroSeries.distribution = Distribution.id
>> +            LEFT OUTER JOIN DistroSeries SPBDS -- (SourcePackageBranchDistroSeries)
>> +                ON SeriesSourcePackageBranch.distroseries = SPBDS.id
>> +            WHERE Distribution.name = %s""" % sqlvalues(self.name)
>
> You don't have to join the distribution here.
>
> You could use
>
> DistroSeries.distribution = self.id

Changed.

>>> It's kind of a shame that we have to materialize the whole results set here,
>>> since for huge size, it will be batched anyway by the web service code. Would
>>> it make sense to use an generator here? Maybe not, since we'd still retrieve
>>> all results for later batches.
>>
>> Given that we use ORDER BY, I suspect the only savings would be not
>> transmitting the non-batched results from the DB server to the app
>> server.  However, making the method a generator might still be a win
>> because even though they will likely come back to get the rest, that
>> request may well hit a different app server.  (I don't think we do any
>> sort of request affinity but would like to know if we do.)
>
> We don't and wouldn't really matter since each API request results in a
> different DB transaction.

Indeed. My history with ZODB is showing here. Where's the object cache
when you need it. ;)

>> Later... Nope, that didn't work.  lazr.restful exploded because it
>> couldn't adapt the generator to IFiniteSequence.  I would have expected
>> returning a generator to work, but I can't find any other instances in
>> the code base.  I've left it as a non-generator for the time being but
>> would appreciate any knowledge that can be shared on the topic.
>
> Yeah, that's fine. The direction Robert suggested might work, but not
> sure how easy/hard it would be.

Given the number of WIP items I have at the moment I really need to move
on so I'm not going to investigate further.

>>>> === modified file 'lib/lp/registry/tests/test_distro_webservice.py'
>>>
>
>>>
>>> You can use makeRelatedBranches(reference_branch=self.branch) to make it an
>>> official source package. Or use
>>> makeRelatedBranchesForSourcePackage(source_package) to create the branch and
>>> make it official in one call.
>>
>> I tried to use makeRelatedBranchesForSourcePackage and
>> makeRelatedBranches but the need to have two different distro series
>> releases (series_1 and series_2 in the original cod...

=== modified file 'lib/lp/registry/interfaces/distribution.py'
--- lib/lp/registry/interfaces/distribution.py	2011-06-16 21:17:37 +0000
+++ lib/lp/registry/interfaces/distribution.py	2011-07-14 14:11:23 +0000
@@ -28,6 +28,7 @@
     export_operation_as,
     export_read_operation,
     exported,
+    operation_for_version,
     operation_parameters,
     operation_returns_collection_of,
     operation_returns_entry,
@@ -392,6 +393,29 @@
         """
 
     @operation_parameters(
+        since=Datetime(
+            title=_("Time of last change"),
+            description=_(
+                "Return branches that have new tips since this timestamp."),
+            required=False))
+    @export_operation_as(name="getBranchTips")
+    @export_read_operation()
+    @operation_for_version('devel')
+    def getBranchTips(since):
+        """Return a list of branches which have new tips since a date.
+
+        Each branch information is a tuple of (branch_unique_name,
+        tip_revision, (official_series*)).
+
+        So for each branch in the distribution, you'll get the branch unique
+        name, the revision id of tip, and if the branch is official for some
+        series, the list of series name.
+
+        :param since: If specified, limits results to branches modified since
+            that date and time.
+        """
+
+    @operation_parameters(
         name=TextLine(title=_("Name"), required=True))
     @operation_returns_entry(IDistributionMirror)
     @export_read_operation()
 
=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2011-06-24 05:26:37 +0000
+++ lib/lp/registry/model/distribution.py	2011-07-14 14:11:23 +0000
@@ -10,7 +10,8 @@
     'DistributionSet',
     ]
 
-from operator import attrgetter
+from operator import attrgetter, itemgetter
+import itertools
 
 from sqlobject import (
     BoolCol,
@@ -654,6 +655,36 @@
         """See `IBugTarget`."""
         return get_bug_tags("BugTask.distribution = %s" % sqlvalues(self))
 
+    def getBranchTips(self, since=None):
+        """See `IDistribution`."""
+        query = """
+            SELECT unique_name, last_scanned_id, SPBDS.name FROM Branch
+            JOIN DistroSeries
+                ON Branch.distroseries = DistroSeries.id
+            LEFT OUTER JOIN SeriesSourcePackageBranch
+                ON Branch.id = SeriesSourcePackageBranch.branch
+            LEFT OUTER JOIN DistroSeries SPBDS
+                -- (SPDBS stands for Source Package Branch Distro Series)
+                ON SeriesSourcePackageBranch.distroseries = SPBDS.id
+            WHERE DistroSeries.distribution = %s""" % sqlvalues(self.id)
+
+        if since is not None:
+            query += (
+                ' AND branch.last_scanned > %s' % sqlvalues(since))
+
+        query += ' ORDER BY unique_name, last_scanned_id;'
+
+        data = Store.of(self).execute(query)
+
+        result = []
+        # Group on location (unique_name) and revision (last_scanned_id).
+        for key, group in itertools.groupby(data, itemgetter(0, 1)):
+            result.append(list(key))
+            # Pull out all the official series names and append them as a list
+            # to the end of the current record, removing Nones from the list.
+            result[-1].append(filter(None, map(itemgetter(-1), group)))
+        return result
+
     def getMirrorByName(self, name):
         """See `IDistribution`."""
         return Store.of(self).find(
@@ -1422,7 +1453,7 @@
             # the sourcepackagename from that.
             bpph = IStore(BinaryPackagePublishingHistory).find(
                 BinaryPackagePublishingHistory,
-                # See comment above for rationale for using an extra query 
+                # See comment above for rationale for using an extra query
                 # instead of an inner join. (Bottom line, it would time out
                 # otherwise.)
                 BinaryPackagePublishingHistory.archiveID.is_in(
 
=== modified file 'lib/lp/registry/tests/test_distro_webservice.py'
--- lib/lp/registry/tests/test_distro_webservice.py	2011-03-23 16:28:51 +0000
+++ lib/lp/registry/tests/test_distro_webservice.py	2011-07-14 14:11:23 +0000
@@ -3,12 +3,21 @@
 
 __metaclass__ = type
 
+from datetime import datetime
+
+import pytz
 from launchpadlib.errors import Unauthorized
 
-from zope.security.management import endInteraction
-from zope.security.proxy import removeSecurityProxy
+from zope.security.management import (
+    endInteraction,
+    newInteraction,
+    )
 
 from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.code.model.seriessourcepackagebranch import (
+    SeriesSourcePackageBranchSet,
+    )
+from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.testing import (
     api_url,
     launchpadlib_for,
@@ -21,10 +30,88 @@
 
     layer = DatabaseFunctionalLayer
 
-    def test_attempt_to_write_data_without_permission_gives_Unauthorized(self):
+    def test_write_without_permission_gives_Unauthorized(self):
         distro = self.factory.makeDistribution()
         endInteraction()
         lp = launchpadlib_for("anonymous-access")
         lp_distro = lp.load(api_url(distro))
         lp_distro.active = False
         self.assertRaises(Unauthorized, lp_distro.lp_save)
+
+
+class TestGetBranchTips(TestCaseWithFactory):
+    """Test the getBranchTips method and it's exposure to the web service."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestGetBranchTips, self).setUp()
+        self.distro = self.factory.makeDistribution()
+        series_1 = self.series_1 = self.factory.makeDistroRelease(self.distro)
+        series_2 = self.series_2 = self.factory.makeDistroRelease(self.distro)
+        source_package = self.factory.makeSourcePackage(distroseries=series_1)
+        branch = self.factory.makeBranch(sourcepackage=source_package)
+        unofficial_branch =  self.factory.makeBranch(sourcepackage=source_package)
+        registrant = self.factory.makePerson()
+        now = datetime.now(pytz.UTC)
+        sourcepackagename = self.factory.makeSourcePackageName()
+        SeriesSourcePackageBranchSet.new(
+            series_1, PackagePublishingPocket.RELEASE, sourcepackagename,
+            branch, registrant, now)
+        SeriesSourcePackageBranchSet.new(
+            series_2, PackagePublishingPocket.RELEASE, sourcepackagename,
+            branch, registrant, now)
+        self.factory.makeRevisionsForBranch(branch)
+        self.branch_name = branch.unique_name
+        self.unofficial_branch_name = unofficial_branch.unique_name
+        self.branch_last_scanned_id = branch.last_scanned_id
+        endInteraction()
+        self.lp = launchpadlib_for("anonymous-access")
+        self.lp_distro = self.lp.distributions[self.distro.name]
+
+    def test_structure(self):
+        """The structure of the results is what we expect."""
+        # The results should be structured as a list of
+        # (location, tip revision ID, [official series, official series, ...])
+        item = self.lp_distro.getBranchTips()[0]
+        self.assertEqual(item[0], self.branch_name)
+        self.assertTrue(item[1], self.branch_last_scanned_id)
+        self.assertEqual(
+            sorted(item[2]),
+            [self.series_1.name, self.series_2.name])
+
+    def test_same_results(self):
+        """Calling getBranchTips directly matches calling it via the API."""
+        # The web service transmutes tuples into lists, so we have to do the
+        # same to the results of directly calling getBranchTips.
+        listified = [list(x) for x in self.distro.getBranchTips()]
+        self.assertEqual(listified, self.lp_distro.getBranchTips())
+
+    def test_revisions(self):
+        """If a branch has revisions then the most recent one is returned."""
+        revision = self.lp_distro.getBranchTips()[0][1]
+        self.assertNotEqual(None, revision)
+
+    def test_since(self):
+        """If "since" is given, return branches with new tips since then."""
+        # There is at least one branch with a tip since the year 2000.
+        self.assertNotEqual(0, len(self.lp_distro.getBranchTips(
+            since=datetime(2000, 1, 1))))
+        # There are no branches with a tip since the year 3000.
+        self.assertEqual(0, len(self.lp_distro.getBranchTips(
+            since=datetime(3000, 1, 1))))
+
+    def test_series(self):
+        """The official series are included in the data."""
+        actual_series_names = sorted([self.series_1.name, self.series_2.name])
+        returned_series_names = sorted(self.lp_distro.getBranchTips()[0][-1])
+        self.assertEqual(actual_series_names, returned_series_names)
+
+    def test_unofficial_branch(self):
+        """Not all branches are official."""
+        # If a branch isn't official, the last skanned ID will be None and the
+        # official distro series list will be empty.
+        tips = self.lp_distro.getBranchTips()[1]
+        self.assertEqual(tips[0], self.unofficial_branch_name)
+        self.assertEqual(tips[1], None)
+        self.assertEqual(tips[2], [])