Merge lp:~lifeless/launchpad/bug-727560 into lp:launchpad

[Robert Collins]

[r=thumper,wallyworld][ui=none] [r=thumper,wallyworld][bug=727560] Convert Archive.getPublishedSources to storm and use DecoratedResultSet rather than prejoins for eager loading.

Hopefully fix bug 727560:
 - query plan analysis showed prejoins causing pessimistic query plans.
 - stormified to let DecoratedResultSet be used (this caused all the edits all over the place)
 - used DRS to set load just one copy of the related objects
 - also fixed a 1/2 dozen bugs of the form 'if foo: something with foo[0]' which wastes a query for no benefit.

[Ian Booth (wallyworld)]

Hooray for removing a doctest \o/

I have some questions:

1. Use of candidates[0] vs candidates.first()

    try:
        return candidates[0]
    except IndexError:
        pass

return None

Looking at the ResultSet code candidates.first() returns None if there's nothing available. So to me it's preferable to just do

return candidates.first()

and not have the ugly (imho) exception handling. Not having to raise and catch an exception is also perhaps more efficient?

2. This construct:

    try:
        first_source = published_sources[0]
    except IndexError:
        pass
    else:
        sources.append(first_source)

Would it be better as:

    try:
        first_source = published_sources[0]
        sources.append(first_source)
    except IndexError:
        pass

Or even:

    first_source = published_sources.first()
    if first_source:
        sources.append(first_source)

3. This comment:

        # Its not clear that this eager load is necessary or sufficient, it
        # replaces a prejoin that had pathological query plans.

I would prefer that it be established if it is necessary before committing code that would produce unnecessary queries.

review: Needs Information

[Robert Collins (lifeless)]

On Thu, Mar 3, 2011 at 3:07 PM, Ian Booth <email address hidden> wrote:
> I have some questions:
>
> 1. Use of candidates[0] vs candidates.first()
>
>    try:
>        return candidates[0]
>    except IndexError:
>        pass
>
> return None
>
> Looking at the ResultSet code candidates.first() returns None if there's nothing available. So to me it's preferable to just do
>
> return candidates.first()
>
> and not have the ugly (imho) exception handling. Not having to raise and catch an exception is also perhaps more efficient?

The efficiency gain of avoiding exception handling will be lost in the
noise for lp - its µs scale. However, note that returning None in the
example you give wouldn't work. We're in a loop - we need the flow
control.

> 2. This construct:
>
>    try:
>        first_source = published_sources[0]
>    except IndexError:
>        pass
>    else:
>        sources.append(first_source)
>
> Would it be better as:
>
>    try:
>        first_source = published_sources[0]
>        sources.append(first_source)
>    except IndexError:
>        pass

No, because only the thing expected to raise the exception you catch
should be in the try: clause.

> Or even:
>
>    first_source = published_sources.first()
>    if first_source:
>        sources.append(first_source)

Thats nicer, I can certainly update to that for this case.

> 3. This comment:
>
>        # Its not clear that this eager load is necessary or sufficient, it
>        # replaces a prejoin that had pathological query plans.
>
> I would prefer that it be established if it is necessary before committing code that would produce unnecessary queries.

Removing a prior eager load without careful auditing is a serious
performance regression risk. It would be a mistake to just delete the
eager loads and hope that its ok.

If we need them (and we probably do for at least some callers) its
better to migrate and then tune, rather than drop it and put 30 or 40
pages out by causing late evaluation.

-Rob

[Ian Booth (wallyworld)]

> On Thu, Mar 3, 2011 at 3:07 PM, Ian Booth <email address hidden> wrote:
> > I have some questions:
> >
> > 1. Use of candidates[0] vs candidates.first()
> >
> >    try:
> >        return candidates[0]
> >    except IndexError:
> >        pass
> >
> > return None
> >
> > Looking at the ResultSet code candidates.first() returns None if there's
> nothing available. So to me it's preferable to just do
> >
> > return candidates.first()
> >
> > and not have the ugly (imho) exception handling. Not having to raise and
> catch an exception is also perhaps more efficient?
>
> The efficiency gain of avoiding exception handling will be lost in the
> noise for lp - its µs scale. However, note that returning None in the
> example you give wouldn't work. We're in a loop - we need the flow
> control.
>

Sorry, I wasn't clear:

candidate = candidates.first()
if candidate:
    return candidate

Depends on your preference - my personal preference is not to use exceptions where a simple check will suffice.

>
> > Or even:
> >
> >    first_source = published_sources.first()
> >    if first_source:
> >        sources.append(first_source)
>
> Thats nicer, I can certainly update to that for this case.
>

Ok. And any others that may exist :-). I just picked out one example. Removing the use of xxx[0] also ensure better consistency with the other places xxx.first() is used.

> > 3. This comment:
> >
> >        # Its not clear that this eager load is necessary or sufficient, it
> >        # replaces a prejoin that had pathological query plans.
> >
> > I would prefer that it be established if it is necessary before committing
> code that would produce unnecessary queries.
>
> Removing a prior eager load without careful auditing is a serious
> performance regression risk. It would be a mistake to just delete the
> eager loads and hope that its ok.
>
> If we need them (and we probably do for at least some callers) its
> better to migrate and then tune, rather than drop it and put 30 or 40
> pages out by causing late evaluation.
>

My point was that we should not simply do the eager load "just in case" but figure out what's necessary. If it's needed to prevent gobs of late evaluation, fine. If not, we are doing an unnecessary db query. But migrating now and tuning later is ok, so long as it's not forgotten, which often is the case in general :-)

review: Approve (*code)

[Tim Penhey (thumper)]

I agree with Ian's comments. It would have been nicer for this to have been broken into two different pipes, or dependent branches with the mechanical changes separate.

[Robert Collins (lifeless)]

On Thu, Mar 3, 2011 at 4:26 PM, Tim Penhey <email address hidden> wrote:
> I agree with Ian's comments.  It would have been nicer for this to have been broken into two different pipes, or dependent branches with the mechanical changes separate.

We can't do it separately, because without changing to storm we can't
make the mechanical changes, and if we change to storm we need the
mechanical changes.

[Robert Collins (lifeless)]

On Thu, Mar 3, 2011 at 4:22 PM, Ian Booth <email address hidden> wrote:
> Review: Approve *code
>> On Thu, Mar 3, 2011 at 3:07 PM, Ian Booth <email address hidden> wrote:
>> > I have some questions:
>> >
>> > 1. Use of candidates[0] vs candidates.first()
>> >
>> >    try:
>> >        return candidates[0]
>> >    except IndexError:
>> >        pass
>> >
>> > return None
>> >
>> > Looking at the ResultSet code candidates.first() returns None if there's
>> nothing available. So to me it's preferable to just do
>> >
>> > return candidates.first()
>> >
>> > and not have the ugly (imho) exception handling. Not having to raise and
>> catch an exception is also perhaps more efficient?
>>
>> The efficiency gain of avoiding exception handling will be lost in the
>> noise for lp - its µs scale. However, note that returning None in the
>> example you give wouldn't work. We're in a loop - we need the flow
>> control.
>>
>
> Sorry, I wasn't clear:
>
> candidate = candidates.first()
> if candidate:
>    return candidate
>
> Depends on your preference - my personal preference is not to use exceptions where a simple check will suffice.

You were clear, but it won't work in this loop. Note the indentation.
I've updated the other one where it will work.

>> If we need them (and we probably do for at least some callers) its
>> better to migrate and then tune, rather than drop it and put 30 or 40
>> pages out by causing late evaluation.
>>
>
> My point was that we should not simply do the eager load "just in case" but figure out what's necessary. If it's needed to prevent gobs of late evaluation, fine. If not, we are doing an unnecessary db query. But migrating now and tuning later is ok, so long as it's not forgotten, which often is the case in general :-)

This was previously eager loaded. So to figure out if its needed means
trawling through years of bug reports. Thats not useful: if this fix
is insufficient, we'll see in the page performance reports. If it is
sufficient, even if there is a little waste, we can come back once
we're at the performance point where the incremental win is worth
hours of work.

-Rob

[Tim Penhey (thumper)]

You can have separate pipes, review separately, and land the last. We have done this often.

review: Approve (code)

=== modified file 'lib/canonical/launchpad/doc/publishing-security.txt'
--- lib/canonical/launchpad/doc/publishing-security.txt	2010-03-02 22:33:24 +0000
+++ lib/canonical/launchpad/doc/publishing-security.txt	2011-03-06 11:23:52 +0000
@@ -25,8 +25,7 @@
 the public PPA:
 
     >>> login(ANONYMOUS)
-    >>> source_pub = public_ppa.getPublishedSources()[0]
-    >>> print source_pub.displayname
+    >>> print public_ppa.getPublishedSources().first().displayname
     foo 666 in breezy-autotest
 
     >>> binary_pub = public_ppa.getAllPublishedBinaries()[0]
@@ -36,8 +35,7 @@
 A regular user can see them too:
 
     >>> login('[e-mail]')
-    >>> source_pub = public_ppa.getPublishedSources()[0]
-    >>> print source_pub.displayname
+    >>> print public_ppa.getPublishedSources().first().displayname
     foo 666 in breezy-autotest
 
     >>> binary_pub = public_ppa.getAllPublishedBinaries()[0]
@@ -47,7 +45,7 @@
 But when querying the private PPA, anonymous access will be refused:
 
     >>> login(ANONYMOUS)
-    >>> source_pub = private_ppa.getPublishedSources()[0]
+    >>> source_pub = private_ppa.getPublishedSources().first()
     Traceback (most recent call last):
     ...
     Unauthorized:...
@@ -60,7 +58,7 @@
 As is for a regular user.
 
     >>> login('[e-mail]')
-    >>> source_pub = private_ppa.getPublishedSources()[0]
+    >>> source_pub = private_ppa.getPublishedSources().first()
     Traceback (most recent call last):
     ...
     Unauthorized:...
@@ -73,8 +71,7 @@
 But the owner can see them.
 
     >>> login_person(private_ppa.owner)
-    >>> source_pub = private_ppa.getPublishedSources()[0]
-    >>> print source_pub.displayname
+    >>> print public_ppa.getPublishedSources().first().displayname
     foo 666 in breezy-autotest
 
     >>> binary_pub = private_ppa.getAllPublishedBinaries()[0]
@@ -84,8 +81,7 @@
 As can an administrator.
 
     >>> login('[e-mail]')
-    >>> source_pub = private_ppa.getPublishedSources()[0]
-    >>> print source_pub.displayname
+    >>> print public_ppa.getPublishedSources().first().displayname
     foo 666 in breezy-autotest
 
     >>> binary_pub = private_ppa.getAllPublishedBinaries()[0]

=== modified file 'lib/canonical/launchpad/testing/fakepackager.py'
--- lib/canonical/launchpad/testing/fakepackager.py	2010-12-20 03:21:03 +0000
+++ lib/canonical/launchpad/testing/fakepackager.py	2011-03-06 11:23:52 +0000
@@ -421,8 +421,6 @@
         if queue_record.status in needs_acceptance_statuses:
             queue_record.acceptFromUploader(changesfile_path, logger)
 
-        pub_record = queue_record.archive.getPublishedSources(
-            name=self.name, version=version, exact_match=True)[0]
-
-        return pub_record
+        return queue_record.archive.getPublishedSources(
+            name=self.name, version=version, exact_match=True).first()
 

=== modified file 'lib/lp/archivepublisher/tests/archive-signing.txt'
--- lib/lp/archivepublisher/tests/archive-signing.txt	2011-02-17 15:52:05 +0000
+++ lib/lp/archivepublisher/tests/archive-signing.txt	2011-03-06 11:23:52 +0000
@@ -65,7 +65,7 @@
 'No Privileges' PPA will be considered for signing_key creation when
 we copy an arbitrary source into it.
 
-    >>> a_source = cprov.archive.getPublishedSources()[0]
+    >>> a_source = cprov.archive.getPublishedSources().first()
     >>> copied_sources = a_source.copyTo(
     ...     a_source.distroseries, a_source.pocket, no_priv.archive)
 

=== modified file 'lib/lp/archiveuploader/nascentupload.py'
--- lib/lp/archiveuploader/nascentupload.py	2010-11-19 12:47:32 +0000
+++ lib/lp/archiveuploader/nascentupload.py	2011-03-06 11:23:52 +0000
@@ -566,8 +566,10 @@
             candidates = self.policy.distroseries.getPublishedSources(
                 source_name, include_pending=True, pocket=pocket,
                 archive=archive)
-            if candidates:
+            try:
                 return candidates[0]
+            except IndexError:
+                pass
 
         return None
 

=== modified file 'lib/lp/archiveuploader/nascentuploadfile.py'
--- lib/lp/archiveuploader/nascentuploadfile.py	2011-02-17 17:02:54 +0000
+++ lib/lp/archiveuploader/nascentuploadfile.py	2011-03-06 11:23:52 +0000
@@ -796,13 +796,15 @@
         spphs = distroseries.getPublishedSources(
             self.source_name, version=self.source_version,
             include_pending=True, archive=self.policy.archive)
-
-        if spphs.count() == 0:
+        # Workaround storm bug in EmptyResultSet.
+        spphs = list(spphs[:1])
+        try:
+            return spphs[0].sourcepackagerelease
+        except IndexError:
             raise UploadError(
                 "Unable to find source package %s/%s in %s" % (
                 self.source_name, self.source_version, distroseries.name))
 
-        return spphs[0].sourcepackagerelease
 
     def verifySourcePackageRelease(self, sourcepackagerelease):
         """Check if the given ISourcePackageRelease matches the context."""

=== modified file 'lib/lp/archiveuploader/tests/nascentupload-ddebs.txt'
--- lib/lp/archiveuploader/tests/nascentupload-ddebs.txt	2010-10-04 19:50:45 +0000
+++ lib/lp/archiveuploader/tests/nascentupload-ddebs.txt	2011-03-06 11:23:52 +0000
@@ -43,8 +43,8 @@
     >>> print src.queue_root.status.name
     DONE
 
-    >>> [src_pub] = src.queue_root.archive.getPublishedSources(
-    ...     name='debug', version='1.0-1', exact_match=True)
+    >>> src_pub = src.queue_root.archive.getPublishedSources(
+    ...     name='debug', version='1.0-1', exact_match=True).one()
 
     >>> print src_pub.displayname, src_pub.status.name
     debug 1.0-1 in hoary PENDING

=== modified file 'lib/lp/archiveuploader/tests/test_ppauploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_ppauploadprocessor.py	2011-01-26 17:22:39 +0000
+++ lib/lp/archiveuploader/tests/test_ppauploadprocessor.py	2011-03-06 11:23:52 +0000
@@ -209,8 +209,7 @@
         self.assertEqual(pending_ppas.count(), 1)
         self.assertEqual(pending_ppas[0], self.name16.archive)
 
-        pub_sources = self.name16.archive.getPublishedSources(name='bar')
-        [pub_bar] = pub_sources
+        pub_bar = self.name16.archive.getPublishedSources(name='bar').one()
 
         self.assertEqual(pub_bar.sourcepackagerelease.version, u'1.0-1')
         self.assertEqual(pub_bar.status, PackagePublishingStatus.PENDING)
@@ -340,8 +339,7 @@
         _from_addr, _to_addrs, _raw_msg = stub.test_emails.pop()
 
         # The SourcePackageRelease still has a component of universe:
-        pub_sources = self.name16.archive.getPublishedSources(name="bar")
-        [pub_foo] = pub_sources
+        pub_foo = self.name16.archive.getPublishedSources(name="bar").one()
         self.assertEqual(
             pub_foo.sourcepackagerelease.component.name, "universe")
 
@@ -387,8 +385,7 @@
         # Source publication and build record for breezy-i386
         # distroarchseries were created as expected. The source is ready
         # to receive the binary upload.
-        pub_sources = self.name16.archive.getPublishedSources(name='bar')
-        [pub_bar] = pub_sources
+        pub_bar = self.name16.archive.getPublishedSources(name='bar').one()
         self.assertEqual(pub_bar.sourcepackagerelease.version, u'1.0-1')
         self.assertEqual(pub_bar.status, PackagePublishingStatus.PENDING)
         self.assertEqual(pub_bar.component.name, 'main')
@@ -439,8 +436,8 @@
             PackageUploadStatus.DONE)
 
         # Copy source uploaded to name16 PPA to cprov's PPA.
-        pub_sources = self.name16.archive.getPublishedSources(name='bar')
-        [name16_pub_bar] = pub_sources
+        name16_pub_bar = self.name16.archive.getPublishedSources(
+            name='bar').one()
         cprov = getUtility(IPersonSet).getByName("cprov")
         cprov_pub_bar = name16_pub_bar.copyTo(
             self.breezy, PackagePublishingPocket.RELEASE, cprov.archive)
@@ -635,8 +632,8 @@
         # the main archive later where it would be published using the
         # source's component if the standard auto-overrides don't match
         # an existing publication.
-        pub_sources = self.name16.archive.getPublishedSources(name='foocomm')
-        [pub_foocomm] = pub_sources
+        pub_foocomm = self.name16.archive.getPublishedSources(
+            name='foocomm').one()
         self.assertEqual(
             pub_foocomm.sourcepackagerelease.component.name, 'partner')
         self.assertEqual(pub_foocomm.component.name, 'main')
@@ -1125,7 +1122,7 @@
             PackageUploadStatus.DONE)
 
         # Delete the published file.
-        [bar_src] = self.name16.archive.getPublishedSources(name="bar")
+        bar_src = self.name16.archive.getPublishedSources(name="bar").one()
         bar_src.requestDeletion(self.name16)
         bar_src.dateremoved = UTC_NOW
         self.layer.txn.commit()

=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2011-02-01 02:59:05 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2011-03-06 11:23:52 +0000
@@ -351,7 +351,7 @@
         bar = archive.getPublishedSources(
             name='bar', version="1.0-1", exact_match=True)
         changes_lfa = getUtility(IPublishingSet).getChangesFileLFA(
-            bar[0].sourcepackagerelease)
+            bar.first().sourcepackagerelease)
         changes_file = changes_lfa.read()
         self.assertTrue(
             "Format: " in changes_file, "Does not look like a changes file")
@@ -823,8 +823,8 @@
         # Upload 'bar-1.0-1' source and binary to ubuntu/breezy.
         upload_dir = self.queueUpload("bar_1.0-2")
         self.processUpload(uploadprocessor, upload_dir)
-        [bar_source_pub] = self.ubuntu.main_archive.getPublishedSources(
-            name='bar', version='1.0-2', exact_match=True)
+        bar_source_pub = self.ubuntu.main_archive.getPublishedSources(
+            name='bar', version='1.0-2', exact_match=True).one()
         [bar_original_build] = bar_source_pub.getBuilds()
 
         self.options.context = 'buildd'
@@ -1096,8 +1096,8 @@
         # Single source uploads also get their corrsponding builds created
         # at upload-time. 'foocomm' only builds in 'i386', thus only one
         # build gets created.
-        [foocomm_source] = partner_archive.getPublishedSources(
-            name='foocomm', version='1.0-2')
+        foocomm_source = partner_archive.getPublishedSources(
+            name='foocomm', version='1.0-2').one()
         [build] = foocomm_source.sourcepackagerelease.builds
         self.assertEqual(
             build.title,

=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py	2010-10-26 16:22:57 +0000
+++ lib/lp/registry/model/distroseriesdifference.py	2011-03-06 11:23:52 +0000
@@ -157,10 +157,9 @@
                 name=self.source_package_name.name,
                 version=self.base_version,
                 distroseries=self.derived_series)
-            # As we know there is a base version published in the
-            # distroseries' main archive, we don't check (equivalent
-            # of calling .one() for a storm resultset.
-            return pubs[0]
+            # We know there is a base version published in the distroseries'
+            # main archive.
+            return pubs.first()
 
         return None
 
@@ -211,9 +210,9 @@
             self.source_package_name, include_pending=True)
 
         # The most recent published source is the first one.
-        if pubs:
+        try:
             return pubs[0]
-        else:
+        except IndexError:
             return None
 
     def update(self):

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2011-02-21 21:10:45 +0000
+++ lib/lp/soyuz/browser/archive.py	2011-03-06 11:23:52 +0000
@@ -35,6 +35,7 @@
 
 import pytz
 from sqlobject import SQLObjectNotFound
+from storm.expr import Desc
 from storm.zope.interfaces import IResultSet
 from zope.app.form.browser import TextAreaWidget
 from zope.component import getUtility
@@ -150,6 +151,7 @@
     IPublishingSet,
     )
 from lp.soyuz.model.archive import Archive
+from lp.soyuz.model.publishing import SourcePackagePublishingHistory
 from lp.soyuz.scripts.packagecopier import do_copy
 
 
@@ -573,7 +575,7 @@
         the view to determine whether to display "This PPA does not yet
         have any published sources" or "No sources matching 'blah'."
         """
-        return bool(self.context.getPublishedSources())
+        return not self.context.getPublishedSources().is_empty()
 
     @cachedproperty
     def repository_usage(self):
@@ -924,13 +926,8 @@
         """Return the last five published sources for this archive."""
         sources = self.context.getPublishedSources(
             status=PackagePublishingStatus.PUBLISHED)
-
-        # We adapt the ISQLResultSet into a normal storm IResultSet so we
-        # can re-order and limit the results (orderBy is not included on
-        # the ISQLResultSet interface). Because this query contains
-        # pre-joins, the result of the adaption is a set of tuples.
-        result_tuples = IResultSet(sources)
-        result_tuples = result_tuples.order_by('datepublished DESC')[:5]
+        sources.order_by(Desc(SourcePackagePublishingHistory.datepublished))
+        result_tuples = sources[:5]
 
         # We want to return a list of dicts for easy template rendering.
         latest_updates_list = []
@@ -948,7 +945,7 @@
             }
 
         now = datetime.now(tz=pytz.UTC)
-        source_ids = [result_tuple[0].id for result_tuple in result_tuples]
+        source_ids = [result_tuple.id for result_tuple in result_tuples]
         summaries = getUtility(
             IPublishingSet).getBuildStatusSummariesForSourceIdsAndArchive(
                 source_ids, self.context)
@@ -981,11 +978,8 @@
         """Return the number of updates over the past days."""
         now = datetime.now(tz=pytz.UTC)
         created_since = now - timedelta(num_days)
-
-        sources = self.context.getPublishedSources(
-            created_since_date=created_since)
-
-        return sources.count()
+        return self.context.getPublishedSources(
+            created_since_date=created_since).count()
 
     @property
     def num_pkgs_building(self):
@@ -1941,7 +1935,7 @@
 
         if data.get('private') != self.context.private:
             # The privacy is being switched.
-            if bool(self.context.getPublishedSources()):
+            if not self.context.getPublishedSources().is_empty():
                 self.setFieldError(
                     'private',
                     'This archive already has published sources. It is '

=== modified file 'lib/lp/soyuz/browser/tests/publishing-views.txt'
--- lib/lp/soyuz/browser/tests/publishing-views.txt	2010-10-09 16:36:22 +0000
+++ lib/lp/soyuz/browser/tests/publishing-views.txt	2011-03-06 11:23:52 +0000
@@ -78,7 +78,7 @@
     >>> cprov = getUtility(IPersonSet).getByName("cprov")
 
     >>> iceweasel_source_pub = cprov.archive.getPublishedSources(
-    ...     'iceweasel')[0]
+    ...     'iceweasel').first()
 
     >>> ppa_source_view = getMultiAdapter(
     ...     (iceweasel_source_pub, LaunchpadTestRequest()),

=== modified file 'lib/lp/soyuz/browser/tests/test_archive_packages.py'
--- lib/lp/soyuz/browser/tests/test_archive_packages.py	2011-02-24 01:12:02 +0000
+++ lib/lp/soyuz/browser/tests/test_archive_packages.py	2011-03-06 11:23:52 +0000
@@ -187,7 +187,7 @@
         self.assertIs(None, view.specified_name_filter)
 
     def test_source_query_counts(self):
-        query_baseline = 43
+        query_baseline = 47
         # Assess the baseline.
         collector = QueryCollector()
         collector.register()
@@ -228,7 +228,7 @@
         self.assertThat(collector, HasQueryCount(LessThan(expected_count)))
 
     def test_binary_query_counts(self):
-        query_baseline = 40
+        query_baseline = 43
         # Assess the baseline.
         collector = QueryCollector()
         collector.register()

=== modified file 'lib/lp/soyuz/doc/archive.txt'
--- lib/lp/soyuz/doc/archive.txt	2011-02-01 16:41:18 +0000
+++ lib/lp/soyuz/doc/archive.txt	2011-03-06 11:23:52 +0000
@@ -299,11 +299,14 @@
     >>> cprov_archive.getAllPublishedBinaries().count()
     4
 
-This lookup also supports optional filters:
-
- * 'name': as in SQL "LIKE '%%' || NAME || '%%'");
- * 'version': exact version string matching;
- * 'status': a item or a list of PackagePublishingStatus.
+This lookup also supports various filters - see the api docs for more info.
+
+Binary publication lookups
+--------------------------
+
+'getPublishedOnDiskBinaries' returns only unique publications, i.e., it
+excludes architecture-independent duplications which is necessary for
+having correct publication counters and archive size.
 
     >>> from lp.soyuz.enums import PackagePublishingStatus
 
@@ -313,146 +316,10 @@
     >>> inactive_status = [PackagePublishingStatus.SUPERSEDED,
     ...                    PackagePublishingStatus.DELETED]
 
-Let's inspect source publications in Cprov PPA:
-
-    >>> all_sources = cd_lookup = cprov_archive.getPublishedSources()
-    >>> for pub in all_sources:
-    ...     title = pub.sourcepackagerelease.title
-    ...     pub_ds = pub.distroseries.name
-    ...     print "%s -> %s" % (title, pub_ds)
-    cdrkit - 1.0 -> breezy-autotest
-    iceweasel - 1.0 -> warty
-    pmount - 0.1-1 -> warty
-
-Using 'name' filter:
-
-    >>> cprov_archive.getPublishedSources(name='cd').count()
-    1
-
-    >>> cprov_archive.getPublishedSources(name='ice').count()
-    1
-
-Combining 'name' filter and 'exact_match' flag:
-
-    >>> cprov_archive.getPublishedSources(
-    ...     name='iceweasel', exact_match=True).count()
-    1
-    >>> cprov_archive.getPublishedSources(
-    ...     name='ice', exact_match=True).count()
-    0
-
-Using 'version' filter:
-
-    >>> ice_version_lookup = cprov_archive.getPublishedSources(
-    ...     version='1.0')
-    Traceback (most recent call last):
-    ...
-    VersionRequiresName: The 'version' parameter can be used only together
-    with the 'name' parameter.
-
-    >>> ice_version_lookup = cprov_archive.getPublishedSources(
-    ...     name='ice', version='1.0')
-    >>> ice_version_lookup.count()
-    1
-
-    >>> cprov_archive.getPublishedSources(
-    ...     name='ice', version='666').count()
-    0
-
-Using 'status' filter:
-
-    >>> cprov_archive.getPublishedSources(
-    ...     status=PackagePublishingStatus.PUBLISHED).count()
-    3
-
-    >>> cprov_archive.getPublishedSources(
-    ...     status=active_status).count()
-    3
-
-    >>> cprov_archive.getPublishedSources(
-    ...     status=inactive_status).count()
-    0
-
-Using 'distroseries' filter:
-
     >>> warty = cprov_archive.distribution['warty']
     >>> hoary = cprov_archive.distribution['hoary']
     >>> breezy_autotest = cprov_archive.distribution['breezy-autotest']
-
-    >>> cprov_archive.getPublishedSources(
-    ...     distroseries=warty).count()
-    2
-    >>> cprov_archive.getPublishedSources(
-    ...     distroseries=hoary).count()
-    0
-    >>> cprov_archive.getPublishedSources(
-    ...     distroseries=breezy_autotest).count()
-    1
-
-Using 'pocket' filter:
-
     >>> from lp.registry.interfaces.pocket import PackagePublishingPocket
-    >>> cprov_archive.getPublishedSources(
-    ...     distroseries=warty,
-    ...     pocket=PackagePublishingPocket.RELEASE).count()
-    2
-
-    >>> cprov_archive.getPublishedSources(
-    ...     distroseries=warty,
-    ...     pocket=PackagePublishingPocket.UPDATES).count()
-    0
-
-Combining 'name' and 'distroseries' filters:
-
-    >>> cprov_archive.getPublishedSources(
-    ...     name='ice', distroseries=warty).count()
-    1
-    >>> cprov_archive.getPublishedSources(
-    ...     name='ice', distroseries=breezy_autotest).count()
-    0
-
-    >>> cprov_archive.getPublishedSources(
-    ...     name='cd', distroseries=warty).count()
-    0
-    >>> cprov_archive.getPublishedSources(
-    ...     name='cd', distroseries=breezy_autotest).count()
-    1
-
-Using the 'created_since_date' filter. This argument accept both,
-iso-timestamp strings and datetime objects.
-
-    >>> cprov_archive.getPublishedSources(
-    ...     created_since_date='2007-07-09 14:00:00').count()
-    0
-
-    >>> import datetime
-    >>> instant = datetime.datetime(year=2007, month=7, day=9, hour=14)
-
-    >>> cprov_archive.getPublishedSources(
-    ...     created_since_date=instant).count()
-    0
-
-As we move the given 'create_since_date' instant to the past the
-publications in Celso's PPA start to show up.
-
-    >>> one_hour_step = datetime.timedelta(hours=1)
-    >>> one_hour_earlier = instant - one_hour_step
-    >>> cprov_archive.getPublishedSources(
-    ...     created_since_date=one_hour_earlier).count()
-    1
-
-    >>> two_hours_earlier = one_hour_earlier - one_hour_step
-    >>> cprov_archive.getPublishedSources(
-    ...     created_since_date=two_hours_earlier).count()
-    3
-
-
-Binary publication lookups
---------------------------
-
-'getPublishedOnDiskBinaries' returns only unique publications, i.e., it
-excludes architecture-independent duplications which is necessary for
-having correct publication counters and archive size.
 
     >>> def check_bin_pubs(pubs):
     ...     """Print binary publication details."""
@@ -785,8 +652,7 @@
 
     # Grab some source IDs from the archive that we can use for calls to
     # getBuildSummariesForSourceIds():
-    >>> sources = cprov_private_ppa.getPublishedSources()
-    >>> source_ids = [sources[0].id]
+    >>> source_ids = [cprov_private_ppa.getPublishedSources()[0].id]
 
 Then verify that an admin can see the counters and build summaries:
 
@@ -872,8 +738,7 @@
 files as it happens in the archive filesystem (pool/):
 
     >>> def print_published_files(archive):
-    ...     pub_sources = archive.getPublishedSources()
-    ...     for pub_source in pub_sources:
+    ...     for pub_source in archive.getPublishedSources():
     ...         for src_file in pub_source.sourcepackagerelease.files:
     ...             print '%s: %s (%s, %d bytes)' % (
     ...                 src_file.sourcepackagerelease.title,
@@ -896,7 +761,7 @@
     ...     )
     >>> huge_firefox_orig_file = getUtility(ILibraryFileAliasSet)[3]
     >>> cprov_cdrkit_src = cprov_archive.getPublishedSources(
-    ...     name='cdrkit')[0]
+    ...     name='cdrkit').first()
     >>> unused_src_file = cprov_cdrkit_src.sourcepackagerelease.addFile(
     ...     huge_firefox_orig_file)
 
@@ -926,7 +791,7 @@
     >>> cprov_archive.number_of_sources
     3
     >>> cprov_archive.getPublishedSources(
-    ...     name='cdrkit')[0].supersede()
+    ...     name='cdrkit').first().supersede()
 
     >>> cprov_archive.number_of_sources
     2
@@ -987,7 +852,7 @@
 it continues to be considered 'available for deletion'.
 
     >>> removal_candidate = cprov_archive.getPublishedSources(
-    ...     name='ice')[0]
+    ...     name='ice').first()
     >>> len(removal_candidate.getPublishedBinaries())
     1
 
@@ -1820,7 +1685,7 @@
     >>> name12_archive = archive_set.new(
     ...     owner=name12, distribution=None, purpose=ArchivePurpose.PPA)
 
-    >>> a_pub = cprov_archive.getPublishedSources()[0]
+    >>> a_pub = cprov_archive.getPublishedSources().first()
     >>> def create_activity(where, how_many):
     ...     for i in range(how_many):
     ...         a_pub.copyTo(
@@ -2418,10 +2283,10 @@
 
     >>> mark.archive.syncSources(sources, cprov.archive, "release")
 
-    >>> [mark_one] = mark.archive.getPublishedSources(name="package1")
+    >>> mark_one = mark.archive.getPublishedSources(name="package1").one()
     >>> print mark_one.sourcepackagerelease.version
     1.1
-    >>> [mark_two] = mark.archive.getPublishedSources(name="package2")
+    >>> mark_two = mark.archive.getPublishedSources(name="package2").one()
     >>> print mark_two.sourcepackagerelease.version
     1.0
 
@@ -2501,15 +2366,15 @@
 
     >>> login("[e-mail]")
     >>> mark.archive.syncSource("pack", "1.0", cprov.archive, "release")
-    >>> [pack] = mark.archive.getPublishedSources(
-    ...     name="pack", exact_match=True)
+    >>> pack = mark.archive.getPublishedSources(
+    ...     name="pack", exact_match=True).one()
     >>> print pack.sourcepackagerelease.version
     1.0
 
 Copy package3 1.0 explicitly:
 
     >>> mark.archive.syncSource("package3", "1.0", cprov.archive, "release")
-    >>> [mark_three] = mark.archive.getPublishedSources(name="package3")
+    >>> mark_three = mark.archive.getPublishedSources(name="package3").one()
     >>> print mark_three.sourcepackagerelease.version
     1.0
 
@@ -2539,7 +2404,7 @@
 
 Now, Mark's PPA has 'built-source' source and it's 2 binaries.
 
-    >>> [copy] = mark.archive.getPublishedSources(name="built-source")
+    >>> copy = mark.archive.getPublishedSources(name="built-source").one()
     >>> len(copy.getPublishedBinaries())
     2
 
@@ -2580,7 +2445,7 @@
     ...     source_name='overridden', version='1.0',
     ...     from_archive=source_archive, to_pocket='release')
 
-    >>> [copy] = mark.archive.getPublishedSources(name="overridden")
+    >>> copy = mark.archive.getPublishedSources(name="overridden").one()
     >>> print copy.section.name
     python
 

=== modified file 'lib/lp/soyuz/doc/distribution.txt'
--- lib/lp/soyuz/doc/distribution.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/soyuz/doc/distribution.txt	2011-03-06 11:23:52 +0000
@@ -383,7 +383,7 @@
 We can make Celso's PPA pending publication by copying a published
 source to another location within the PPA.
 
-    >>> cprov_src = cprov.archive.getPublishedSources()[0]
+    >>> cprov_src = cprov.archive.getPublishedSources().first()
 
     >>> warty = ubuntu['warty']
     >>> pocket_release = PackagePublishingPocket.RELEASE

=== modified file 'lib/lp/soyuz/doc/publishing.txt'
--- lib/lp/soyuz/doc/publishing.txt	2010-11-10 23:55:07 +0000
+++ lib/lp/soyuz/doc/publishing.txt	2011-03-06 11:23:52 +0000
@@ -112,7 +112,7 @@
 
 The iceweasel source has good data:
 
-    >>> pub = spph.archive.getPublishedSources(name="iceweasel")[0]
+    >>> pub = spph.archive.getPublishedSources(name="iceweasel").first()
     >>> print pub.changesFileUrl()
     http://launchpad.dev/ubuntu/+archive/primary/+files/mozilla-firefox_0.9_i386.changes
 
@@ -1432,9 +1432,9 @@
 Last but not least the publishing set class allows for the bulk deletion
 of publishing history records.
 
-    >>> cprov_sources = sorted(list(
+    >>> cprov_sources = sorted(
     ...     cprov.archive.getPublishedSources(
-    ...     status=PackagePublishingStatus.PUBLISHED)),
+    ...     status=PackagePublishingStatus.PUBLISHED),
     ...     key=operator.attrgetter('id'))
     >>> print len(cprov_sources)
     6

=== modified file 'lib/lp/soyuz/doc/sourcepackagerelease-build-lookup.txt'
--- lib/lp/soyuz/doc/sourcepackagerelease-build-lookup.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/soyuz/doc/sourcepackagerelease-build-lookup.txt	2011-03-06 11:23:52 +0000
@@ -288,7 +288,7 @@
 same source published in hoary to another suite, let's say
 breezy-autotest in his PPA.
 
-    >>> cprov_foo_src = cprov.archive.getPublishedSources(name='foo')[0]
+    >>> cprov_foo_src = cprov.archive.getPublishedSources(name='foo').first()
 
     >>> breezy_autotest = ubuntu.getSeries('breezy-autotest')
     >>> copy = cprov_foo_src.copyTo(
@@ -310,7 +310,7 @@
 suite in Celso's PPA.
 
     >>> cprov_foo_src = cprov.archive.getPublishedSources(
-    ...    name='foo', distroseries=hoary)[0]
+    ...    name='foo', distroseries=hoary).first()
     >>> cprov_foo_bin = cprov_foo_src.getPublishedBinaries()[0]
 
     >>> warty = ubuntu.getSeries('warty')
@@ -340,7 +340,7 @@
 to Mark Shuttleworth's PPA.
 
     >>> cprov_foo_src = cprov.archive.getPublishedSources(
-    ...    name='foo', distroseries=hoary)[0]
+    ...    name='foo', distroseries=hoary).first()
     >>> cprov_foo_bin = cprov_foo_src.getPublishedBinaries()[0]
 
     >>> mark = getUtility(IPersonSet).getByName('mark')

=== modified file 'lib/lp/soyuz/interfaces/publishing.py'
--- lib/lp/soyuz/interfaces/publishing.py	2011-02-22 22:14:32 +0000
+++ lib/lp/soyuz/interfaces/publishing.py	2011-03-06 11:23:52 +0000
@@ -327,6 +327,8 @@
     id = Int(
             title=_('ID'), required=True, readonly=True,
             )
+    sourcepackagereleaseID = Attribute(
+        "The DB id for the sourcepackagerelease.")
     sourcepackagerelease = Int(
             title=_('The source package release being published'),
             required=False, readonly=False,
@@ -338,6 +340,7 @@
             vocabulary=PackagePublishingStatus,
             required=False, readonly=False,
             ))
+    distroseriesID = Attribute("DB ID for distroseries.")
     distroseries = exported(
         Reference(
             IDistroSeries,
@@ -349,6 +352,7 @@
             title=_('The component being published into'),
             required=False, readonly=False,
             )
+    sectionID = Attribute("DB ID for the section")
     section = Int(
             title=_('The section being published into'),
             required=False, readonly=False,

=== modified file 'lib/lp/soyuz/interfaces/sourcepackagerelease.py'
--- lib/lp/soyuz/interfaces/sourcepackagerelease.py	2011-03-03 06:24:47 +0000
+++ lib/lp/soyuz/interfaces/sourcepackagerelease.py	2011-03-06 11:23:52 +0000
@@ -38,6 +38,7 @@
     version = Attribute("A version string")
     dateuploaded = Attribute("Date of Upload")
     urgency = Attribute("Source Package Urgency")
+    dscsigningkeyID = Attribute("DB ID of the DSC Signing Key")
     dscsigningkey = Attribute("DSC Signing Key")
     component = Attribute("Source Package Component")
     format = Attribute("The Source Package Format")

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2011-02-24 15:30:54 +0000
+++ lib/lp/soyuz/model/archive.py	2011-03-06 11:23:52 +0000
@@ -9,6 +9,7 @@
 
 __all__ = ['Archive', 'ArchiveSet']
 
+from operator import attrgetter
 import re
 
 from lazr.lifecycle.event import ObjectCreatedEvent
@@ -25,6 +26,7 @@
     Or,
     Select,
     Sum,
+    SQL,
     )
 from storm.locals import (
     Count,
@@ -50,6 +52,9 @@
     SQLBase,
     sqlvalues,
     )
+from canonical.launchpad.components.decoratedresultset import (
+    DecoratedResultSet,
+    )
 from canonical.launchpad.components.tokens import (
     create_unique_token_for_table,
     )
@@ -81,6 +86,7 @@
 from lp.buildmaster.model.packagebuild import PackageBuild
 from lp.registry.interfaces.distroseries import IDistroSeriesSet
 from lp.registry.interfaces.person import (
+    IPersonSet,
     OPEN_TEAM_POLICY,
     PersonVisibility,
     validate_person,
@@ -88,6 +94,7 @@
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.role import IHasOwner
 from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
+from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.registry.model.teammembership import TeamParticipation
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.propertycache import (
@@ -182,6 +189,7 @@
     PackageUpload,
     PackageUploadSource,
     )
+from lp.soyuz.model.section import Section
 from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
 from lp.soyuz.scripts.packagecopier import do_copy
 
@@ -458,76 +466,94 @@
                             distroseries=None, pocket=None,
                             exact_match=False, created_since_date=None):
         """See `IArchive`."""
-        clauses = ["""
-            SourcePackagePublishingHistory.archive = %s AND
-            SourcePackagePublishingHistory.sourcepackagerelease =
-                SourcePackageRelease.id AND
-            SourcePackageRelease.sourcepackagename =
+        # clauses contains literal sql expressions for things that don't work
+        # easily in storm : this method was migrated from sqlobject but some
+        # callers are problematic. (Migrate them and test to see).
+        clauses = []
+        storm_clauses = [
+            SourcePackagePublishingHistory.archiveID==self.id,
+            SourcePackagePublishingHistory.sourcepackagereleaseID==
+                SourcePackageRelease.id,
+            SourcePackageRelease.sourcepackagenameID==
                 SourcePackageName.id
-            """ % sqlvalues(self)]
-        clauseTables = ['SourcePackageRelease', 'SourcePackageName']
-        orderBy = ['SourcePackageName.name',
-                   '-SourcePackagePublishingHistory.id']
+            ]
+        orderBy = [SourcePackageName.name,
+                   Desc(SourcePackagePublishingHistory.id)]
 
         if name is not None:
             if exact_match:
-                clauses.append("""
-                    SourcePackageName.name=%s
-                """ % sqlvalues(name))
+                storm_clauses.append(SourcePackageName.name==name)
             else:
-                clauses.append("""
-                    SourcePackageName.name LIKE '%%' || %s || '%%'
-                """ % quote_like(name))
+                clauses.append(
+                    "SourcePackageName.name LIKE '%%%%' || %s || '%%%%'"
+                    % quote_like(name))
 
         if version is not None:
             if name is None:
                 raise VersionRequiresName(
                     "The 'version' parameter can be used only together with"
                     " the 'name' parameter.")
-            clauses.append("""
-                SourcePackageRelease.version = %s
-            """ % sqlvalues(version))
+            storm_clauses.append(SourcePackageRelease.version==version)
         else:
-            order_const = "SourcePackageRelease.version"
-            desc_version_order = SQLConstant(order_const+" DESC")
-            orderBy.insert(1, desc_version_order)
+            orderBy.insert(1, Desc(SourcePackageRelease.version))
 
         if status is not None:
             try:
                 status = tuple(status)
             except TypeError:
                 status = (status, )
-            clauses.append("""
-                SourcePackagePublishingHistory.status IN %s
-            """ % sqlvalues(status))
+            clauses.append(
+                "SourcePackagePublishingHistory.status IN %s "
+                % sqlvalues(status))
 
         if distroseries is not None:
-            clauses.append("""
-                SourcePackagePublishingHistory.distroseries = %s
-            """ % sqlvalues(distroseries))
+            storm_clauses.append(
+                SourcePackagePublishingHistory.distroseriesID==distroseries.id)
 
         if pocket is not None:
-            clauses.append("""
-                SourcePackagePublishingHistory.pocket = %s
-            """ % sqlvalues(pocket))
+            storm_clauses.append(
+                SourcePackagePublishingHistory.pocket==pocket)
 
         if created_since_date is not None:
-            clauses.append("""
-                SourcePackagePublishingHistory.datecreated >= %s
-            """ % sqlvalues(created_since_date))
-
-        preJoins = [
-            'sourcepackagerelease.creator',
-            'sourcepackagerelease.dscsigningkey',
-            'distroseries',
-            'section',
-            ]
-
-        sources = SourcePackagePublishingHistory.select(
-            ' AND '.join(clauses), clauseTables=clauseTables, orderBy=orderBy,
-            prejoins=preJoins)
-
-        return sources
+            clauses.append(
+                "SourcePackagePublishingHistory.datecreated >= %s"
+                % sqlvalues(created_since_date))
+
+        store = Store.of(self)
+        if clauses:
+            storm_clauses.append(SQL(' AND '.join(clauses)))
+        resultset = store.find(SourcePackagePublishingHistory,
+            *storm_clauses).order_by(
+            *orderBy)
+        # Its not clear that this eager load is necessary or sufficient, it
+        # replaces a prejoin that had pathological query plans.
+        def eager_load(rows):
+            # \o/ circular imports.
+            from lp.registry.model.distroseries import DistroSeries
+            from lp.registry.model.gpgkey import GPGKey
+            ids = set(map(attrgetter('distroseriesID'), rows))
+            ids.discard(None)
+            if ids:
+                list(store.find(DistroSeries, DistroSeries.id.is_in(ids)))
+            ids = set(map(attrgetter('sectionID'), rows))
+            ids.discard(None)
+            if ids:
+                list(store.find(Section, Section.id.is_in(ids)))
+            ids = set(map(attrgetter('sourcepackagereleaseID'), rows))
+            ids.discard(None)
+            if not ids:
+                return
+            releases = list(store.find(
+                SourcePackageRelease, SourcePackageRelease.id.is_in(ids)))
+            ids = set(map(attrgetter('creatorID'), releases))
+            ids.discard(None)
+            if ids:
+                list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(ids))
+            ids = set(map(attrgetter('dscsigningkeyID'), releases))
+            ids.discard(None)
+            if ids:
+                list(store.find(GPGKey, GPGKey.id.is_in(ids)))
+        return DecoratedResultSet(resultset, pre_iter_hook=eager_load)
 
     def getSourcesForDeletion(self, name=None, status=None,
             distroseries=None):
@@ -1419,7 +1445,7 @@
         getUtility(ISourcePackageNameSet)[source_name]
         # Find and validate the source package version required.
         source = from_archive.getPublishedSources(
-            name=source_name, version=version, exact_match=True)[0]
+            name=source_name, version=version, exact_match=True).first()
 
         self._copySources([source], to_pocket, to_series, include_binaries)
 
@@ -1441,8 +1467,9 @@
             published_sources = from_archive.getPublishedSources(
                 name=name, exact_match=True,
                 status=PackagePublishingStatus.PUBLISHED)
-            if published_sources.count() > 0:
-                sources.append(published_sources[0])
+            first_source = published_sources.first()
+            if first_source is not None:
+                sources.append(first_source)
         return sources
 
     def _copySources(self, sources, to_pocket, to_series=None,

=== modified file 'lib/lp/soyuz/model/packagecloner.py'
--- lib/lp/soyuz/model/packagecloner.py	2011-01-27 22:17:07 +0000
+++ lib/lp/soyuz/model/packagecloner.py	2011-03-06 11:23:52 +0000
@@ -135,10 +135,6 @@
         sources_published = archive.getPublishedSources(
             distroseries=distroseries, status=active_publishing_status)
 
-        def get_spn(pub):
-            """Return the source package name for a publishing record."""
-            return pub.sourcepackagerelease.sourcepackagename.name
-
         for pubrec in sources_published:
             builds = pubrec.createMissingBuilds(
                 architectures_available=architectures)

=== modified file 'lib/lp/soyuz/scripts/add_missing_builds.py'
--- lib/lp/soyuz/scripts/add_missing_builds.py	2011-03-02 10:00:36 +0000
+++ lib/lp/soyuz/scripts/add_missing_builds.py	2011-03-06 11:23:52 +0000
@@ -60,7 +60,8 @@
             distroseries=distroseries,
             pocket=pocket,
             status=PackagePublishingStatus.PUBLISHED)
-        if not bool(sources):
+        sources = list(sources)
+        if not sources:
             self.logger.info("No sources published, nothing to do.")
             return
 

=== modified file 'lib/lp/soyuz/scripts/ftpmasterbase.py'
--- lib/lp/soyuz/scripts/ftpmasterbase.py	2010-08-23 16:51:11 +0000
+++ lib/lp/soyuz/scripts/ftpmasterbase.py	2011-03-06 11:23:52 +0000
@@ -153,12 +153,12 @@
             pocket=self.location.pocket,
             exact_match=True)
 
-        if not published_sources:
+        try:
+            latest_source = published_sources[0]
+        except IndexError:
             raise SoyuzScriptError(
                 "Could not find source '%s/%s' in %s" % (
                 name, self.options.version, self.location))
-
-        latest_source = published_sources[0]
         self._validatePublishing(latest_source)
         return latest_source
 

=== modified file 'lib/lp/soyuz/scripts/packagecopier.py'
--- lib/lp/soyuz/scripts/packagecopier.py	2011-03-03 08:02:29 +0000
+++ lib/lp/soyuz/scripts/packagecopier.py	2011-03-06 11:23:52 +0000
@@ -265,7 +265,7 @@
 
         # If there are no conflicts with the same version, we can skip the
         # rest of the checks, but we still want to check conflicting files
-        if (not bool(destination_archive_conflicts) and
+        if (destination_archive_conflicts.is_empty() and
             len(inventory_conflicts) == 0):
             self._checkConflictingFiles(source)
             return
@@ -556,12 +556,12 @@
         version=source.sourcepackagerelease.version,
         status=active_publishing_status,
         distroseries=series, pocket=pocket)
-    if not bool(source_in_destination):
+    if source_in_destination.is_empty():
         source_copy = source.copyTo(series, pocket, archive)
         close_bugs_for_sourcepublication(source_copy)
         copies.append(source_copy)
     else:
-        source_copy = source_in_destination[0]
+        source_copy = source_in_destination.first()
 
     if not include_binaries:
         source_copy.createMissingBuilds()

=== modified file 'lib/lp/soyuz/scripts/tests/test_copypackage.py'
--- lib/lp/soyuz/scripts/tests/test_copypackage.py	2011-01-25 00:44:23 +0000
+++ lib/lp/soyuz/scripts/tests/test_copypackage.py	2011-03-06 11:23:52 +0000
@@ -1813,8 +1813,8 @@
         target_archive = copy_helper.destination.archive
         self.checkCopies(copied, target_archive, 3)
 
-        [copied_source] = ubuntu.main_archive.getPublishedSources(
-            name='boing')
+        copied_source = ubuntu.main_archive.getPublishedSources(
+            name='boing').one()
         self.assertEqual(copied_source.displayname, 'boing 1.0 in hoary')
         self.assertEqual(len(copied_source.getPublishedBinaries()), 2)
         self.assertEqual(len(copied_source.getBuilds()), 1)
@@ -1891,8 +1891,8 @@
         # The source and the only existing binary were correctly copied.
         # No build was created, but the architecture independent binary
         # was propagated to the new architecture (hoary/amd64).
-        [copied_source] = ubuntu.main_archive.getPublishedSources(
-            name='boing', distroseries=hoary)
+        copied_source = ubuntu.main_archive.getPublishedSources(
+            name='boing', distroseries=hoary).one()
         self.assertEqual(copied_source.displayname, 'boing 1.0 in hoary')
 
         self.assertEqual(len(copied_source.getBuilds()), 0)
@@ -1925,8 +1925,8 @@
 
         # The source and the only existing binary were correctly copied.
         hoary = ubuntu.getSeries('hoary')
-        [copied_source] = ubuntu.main_archive.getPublishedSources(
-            name='boing', distroseries=hoary)
+        copied_source = ubuntu.main_archive.getPublishedSources(
+            name='boing', distroseries=hoary).one()
         self.assertEqual(copied_source.displayname, 'boing 1.0 in hoary')
 
         [copied_binary] = copied_source.getPublishedBinaries()

=== modified file 'lib/lp/soyuz/stories/ppa/xx-delete-packages.txt'
--- lib/lp/soyuz/stories/ppa/xx-delete-packages.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/soyuz/stories/ppa/xx-delete-packages.txt	2011-03-06 11:23:52 +0000
@@ -507,7 +507,7 @@
     >>> login('[e-mail]')
     >>> no_priv = getUtility(IPersonSet).getByName('no-priv')
     >>> deleted_pub = no_priv.archive.getPublishedSources(
-    ...     status=PackagePublishingStatus.DELETED)[0]
+    ...     status=PackagePublishingStatus.DELETED).first()
     >>> deleted_pub.dateremoved = deleted_pub.datecreated
     >>> logout()
 

=== modified file 'lib/lp/soyuz/stories/ppa/xx-ppa-packages.txt'
--- lib/lp/soyuz/stories/ppa/xx-ppa-packages.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/soyuz/stories/ppa/xx-ppa-packages.txt	2011-03-06 11:23:52 +0000
@@ -276,14 +276,14 @@
     >>> login('[e-mail]')
     >>> cprov = getUtility(IPersonSet).getByName('cprov')
     >>> iceweasel_pub = cprov.archive.getPublishedSources(
-    ...     name='iceweasel')[0]
+    ...     name='iceweasel').first()
     >>> bpr = test_publisher.uploadBinaryForBuild(
     ...     iceweasel_pub.getBuilds()[0], 'bar-bin')
     >>> pub_bins = test_publisher.publishBinaryInArchive(
     ...     bpr, cprov.archive, status=PackagePublishingStatus.PUBLISHED)
     >>> iceweasel_pub.status = (
     ...     PackagePublishingStatus.SUPERSEDED)
-    >>> pmount_pub = cprov.archive.getPublishedSources(name='pmount')[0]
+    >>> pmount_pub = cprov.archive.getPublishedSources(name='pmount').first()
     >>> pmount_pub.status = PackagePublishingStatus.DELETED
     >>> pmount_pub.removed_by = cprov
     >>> pmount_pub.removal_comment = 'nhack !'

=== modified file 'lib/lp/soyuz/stories/soyuz/xx-package-diff.txt'
--- lib/lp/soyuz/stories/soyuz/xx-package-diff.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/soyuz/stories/soyuz/xx-package-diff.txt	2011-03-06 11:23:52 +0000
@@ -202,7 +202,7 @@
     >>> anon_browser.open(
     ...     'http://launchpad.dev/~name16/+archive/ppa/+packages')
     >>> login('[e-mail]')
-    >>> biscuit_ppa = name16.archive.getPublishedSources()[0]
+    >>> biscuit_ppa = name16.archive.getPublishedSources().first()
     >>> biscuit_ppa_id = biscuit_ppa.id
     >>> diff_three.date_fulfilled = None
     >>> diff_three.status = PackageDiffStatus.PENDING

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2011-01-20 18:27:00 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2011-03-06 11:23:52 +0000
@@ -3,7 +3,11 @@
 
 """Test Archive features."""
 
-from datetime import date
+from datetime import (
+    date,
+    datetime,
+    timedelta,
+    )
 
 import transaction
 from zope.component import getUtility
@@ -23,7 +27,10 @@
     )
 from lp.app.errors import NotFoundError
 from lp.buildmaster.enums import BuildStatus
-from lp.registry.interfaces.person import TeamSubscriptionPolicy
+from lp.registry.interfaces.person import (
+    IPersonSet,
+    TeamSubscriptionPolicy,
+    )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.job.interfaces.job import JobStatus
@@ -45,6 +52,7 @@
     InvalidPocketForPPA,
     NoRightsForArchive,
     NoRightsForComponent,
+    VersionRequiresName,
     )
 from lp.soyuz.interfaces.archivearch import IArchiveArchSet
 from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
@@ -1635,3 +1643,85 @@
         new_dsc = self.factory.makeLibraryFileAlias(filename=dsc.filename)
         pub.sourcepackagerelease.addFile(new_dsc)
         self.assertEquals(new_dsc, self.archive.getFileByName(dsc.filename))
+
+
+class TestGetPublishedSources(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_getPublishedSources_comprehensive(self):
+        # The doctests for getPublishedSources migrated from a doctest for
+        # better testing.
+        cprov = getUtility(IPersonSet).getByName('cprov')
+        cprov_archive = cprov.archive
+        # There are three published sources by default - no args returns all
+        # publications.
+        self.assertEqual(3, cprov_archive.getPublishedSources().count())
+        # Various filters.
+        active_status = [PackagePublishingStatus.PENDING,
+                         PackagePublishingStatus.PUBLISHED]
+        inactive_status = [PackagePublishingStatus.SUPERSEDED,
+                           PackagePublishingStatus.DELETED]
+        warty = cprov_archive.distribution['warty']
+        hoary = cprov_archive.distribution['hoary']
+        breezy_autotest = cprov_archive.distribution['breezy-autotest']
+        all_sources = cprov_archive.getPublishedSources()
+        expected = [('cdrkit - 1.0', 'breezy-autotest'),
+            ('iceweasel - 1.0', 'warty'),
+            ('pmount - 0.1-1', 'warty'),
+            ]
+        found = []
+        for pub in all_sources:
+            title = pub.sourcepackagerelease.title
+            pub_ds = pub.distroseries.name
+            found.append((title, pub_ds))
+        self.assertEqual(expected, found)
+        self.assertEqual(1,
+            cprov_archive.getPublishedSources(name='cd').count())
+        self.assertEqual(1,
+            cprov_archive.getPublishedSources(name='ice').count())
+        self.assertEqual(1, cprov_archive.getPublishedSources(
+            name='iceweasel', exact_match=True).count())
+        self.assertEqual(0, cprov_archive.getPublishedSources(
+            name='ice', exact_match=True).count())
+        self.assertRaises(VersionRequiresName,
+            cprov_archive.getPublishedSources,
+            version='1.0')
+        self.assertEqual(1, cprov_archive.getPublishedSources(
+            name='ice', version='1.0').count())
+        self.assertEqual(0, cprov_archive.getPublishedSources(
+            name='ice', version='666').count())
+        self.assertEqual(3, cprov_archive.getPublishedSources(
+            status=PackagePublishingStatus.PUBLISHED).count())
+        self.assertEqual(3, cprov_archive.getPublishedSources(
+            status=active_status).count())
+        self.assertEqual(0, cprov_archive.getPublishedSources(
+            status=inactive_status).count())
+        self.assertEqual(2, cprov_archive.getPublishedSources(
+            distroseries=warty).count())
+        self.assertEqual(0, cprov_archive.getPublishedSources(
+            distroseries=hoary).count())
+        self.assertEqual(1, cprov_archive.getPublishedSources(
+            distroseries=breezy_autotest).count())
+        self.assertEqual(2, cprov_archive.getPublishedSources(
+            distroseries=warty,
+            pocket=PackagePublishingPocket.RELEASE).count())
+        self.assertEqual(0, cprov_archive.getPublishedSources(
+            distroseries=warty,
+            pocket=PackagePublishingPocket.UPDATES).count())
+        self.assertEqual(1, cprov_archive.getPublishedSources(
+            name='ice', distroseries=warty).count())
+        self.assertEqual(0, cprov_archive.getPublishedSources(
+            name='ice', distroseries=breezy_autotest).count())
+        self.assertEqual(0, cprov_archive.getPublishedSources(
+            created_since_date='2007-07-09 14:00:00').count())
+        mid_2007 = datetime(year=2007, month=7, day=9, hour=14)
+        self.assertEqual(0, cprov_archive.getPublishedSources(
+            created_since_date=mid_2007).count())
+        one_hour_step = timedelta(hours=1)
+        one_hour_earlier = mid_2007 - one_hour_step
+        self.assertEqual(1, cprov_archive.getPublishedSources(
+             created_since_date=one_hour_earlier).count())
+        two_hours_earlier = one_hour_earlier - one_hour_step
+        self.assertEqual(3, cprov_archive.getPublishedSources(
+            created_since_date=two_hours_earlier).count())

=== modified file 'lib/lp/soyuz/tests/test_processaccepted.py'
--- lib/lp/soyuz/tests/test_processaccepted.py	2010-12-20 03:21:03 +0000
+++ lib/lp/soyuz/tests/test_processaccepted.py	2011-03-06 11:23:52 +0000
@@ -125,8 +125,8 @@
         self.assertEqual(published_main.count(), 0)
 
         # Check the copy archive source was accepted.
-        [published_copy] = copy_archive.getPublishedSources(
-            name=self.test_package_name)
+        published_copy = copy_archive.getPublishedSources(
+            name=self.test_package_name).one()
         self.assertEqual(
             published_copy.status, PackagePublishingStatus.PENDING)
         self.assertEqual(copy_source, published_copy.sourcepackagerelease)

=== modified file 'lib/lp/soyuz/tests/test_syncpackagejob.py'
--- lib/lp/soyuz/tests/test_syncpackagejob.py	2010-11-15 16:25:05 +0000
+++ lib/lp/soyuz/tests/test_syncpackagejob.py	2011-03-06 11:23:52 +0000
@@ -105,8 +105,7 @@
         job.run()
 
         published_sources = archive2.getPublishedSources()
-        self.assertEquals(1, published_sources.count())
-        spr = published_sources[0].sourcepackagerelease
+        spr = published_sources.one().sourcepackagerelease
         self.assertEquals("libc", spr.name)
         self.assertEquals("2.8-1", spr.version)