Merge lp:~stevenk/launchpad/fixes-bug-553068 into lp:launchpad

[Steve Kowalik]

Slightly refactor findDepCandidateByName to deal with multiple archive dependencies.

This branch fixes bug 553068 by allowing findDepCandidateByName to deal with multiple archive dependencies.

[Gavin Panella (allenap)]

Hi Steven,

This looks good! I have some ideas for making findDepCandidateByName()
faster, but it's good as is.

Gavin.

> === modified file 'lib/lp/soyuz/model/archive.py'
> --- lib/lp/soyuz/model/archive.py 2010-03-18 09:11:33 +0000
> +++ lib/lp/soyuz/model/archive.py 2010-04-07 11:03:27 +0000
> @@ -779,10 +779,8 @@
> if self.is_ppa:
> archives.append(self.distribution.main_archive.id)
> archives.append(self.id)
> - dep = ArchiveDependency.selectOneBy(archive=self)
> - while dep is not None:
> - archives.append(dep.dependency.id)
> - dep = ArchiveDependency.selectOneBy(archive=dep.dependency)
> + archives.extend(
> + [archive_dep.dependency.id for archive_dep in self.dependencies])

I've got a couple of ways to make this more efficient.

First, the list comprehension can be dropped, and dependencyID can be
used instead of loading the dependency Archive (which may result in
another database query). The diff for that is:

{{{
=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py 2010-04-07 06:59:54 +0000
+++ lib/lp/soyuz/model/archive.py 2010-04-07 11:16:29 +0000
@@ -780,7 +780,7 @@
             archives.append(self.distribution.main_archive.id)
         archives.append(self.id)
         archives.extend(
- [archive_dep.dependency.id for archive_dep in self.dependencies])
+ archive_dep.dependencyID for archive_dep in self.dependencies)

         query = """
             binarypackagename = %s AND

}}}

This can be taken even further! Though the above is perfectly okay, we
can avoid loading even the ArchiveDependency objects:

{{{
=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py 2010-04-07 06:59:54 +0000
+++ lib/lp/soyuz/model/archive.py 2010-04-07 11:19:32 +0000
@@ -18,6 +18,7 @@
 from storm.expr import Or, And, Select
 from storm.locals import Count, Join
 from storm.store import Store
+from storm.zope.interfaces import IResultSet
 from zope.component import getUtility
 from zope.event import notify
 from zope.interface import alsoProvides, implements
@@ -780,7 +781,9 @@
             archives.append(self.distribution.main_archive.id)
         archives.append(self.id)
         archives.extend(
- [archive_dep.dependency.id for archive_dep in self.dependencies])
+ IResultSet(self.dependencies).values(
+ ArchiveDependency.dependencyID))
+

         query = """
             binarypackagename = %s AND

}}}

review: Approve

[Gavin Panella (allenap)]

Also, let me know if you need someone to land this branch; I'm happy to do it.

=== modified file 'lib/lp/soyuz/doc/archive.txt'
--- lib/lp/soyuz/doc/archive.txt	2010-03-23 22:15:02 +0000
+++ lib/lp/soyuz/doc/archive.txt	2010-04-08 02:42:27 +0000
@@ -1317,6 +1317,14 @@
 package is not published on the context PPA but is available somewhere
 in the archive dependency domain it will be found.
 
+We also add another archive dependency here to exercise findDepCandidateByName
+a little more.
+
+    >>> joe = factory.makePerson(email='[e-mail]')
+    >>> second_ppa = factory.makeArchive(name="secondppa", owner=joe)
+    >>> second_archive_dep = cprov.archive.addArchiveDependency(
+    ...     second_ppa, release_pocket, main_component)
+
 'at' binary package is not present in Celso's PPA.
 
     >>> cprov_archive.getAllPublishedBinaries(name='at').count()
@@ -1339,6 +1347,9 @@
     >>> primary_candidate == ppa_candidate
     True
 
+And clean-up after ourselves:
+
+    >>> ignore = cprov.archive.removeArchiveDependency(second_ppa)
 
 == Creating a package copy request from an IArchive ==
 
@@ -1347,7 +1358,7 @@
 
     >>> requestor = factory.makePerson(name='me-copy')
     >>> copy_target = factory.makeCopyArchiveLocation(
-    ...     distribution=ubuntu, name='my-copy-archive')
+    ...     distribution=ubuntu, name='my-copy-archive', owner=requestor)
     >>> pcr = ubuntu.main_archive.requestPackageCopy(copy_target, requestor)
     >>> print pcr
     Package copy request
@@ -1518,7 +1529,7 @@
 
     >>> archive_purposes = [archive.purpose.name for archive in archive_set]
     >>> len(archive_purposes)
-    18
+    20
 
     >>> print sorted(set(archive_purposes))
     ['COPY', 'DEBUG', 'PARTNER', 'PPA', 'PRIMARY']
@@ -1919,7 +1930,7 @@
     ...     ubuntu, purposes=ArchivePurpose.COPY)
     >>> print_archive_names(ubuntu_copy_archives)
     Name             Owner         Private  Enabled
-    my-copy-archive  person-name6  False    True
+    my-copy-archive  me-copy       False    True
 
 The method `getArchivesForDistribution` can also be used with multiple
 purposes. First we'll check how many partner archives are in the DB:
@@ -1937,7 +1948,7 @@
     ...     ubuntu, purposes=[ArchivePurpose.COPY, ArchivePurpose.PARTNER])
     >>> print_archive_names(copy_n_partner_archives)
     Name             Owner         Private  Enabled
-    my-copy-archive  person-name6  False    True
+    my-copy-archive  me-copy       False    True
     partner          ubuntu-team   False    True
 
 First we create four copy archives for ubuntu:
@@ -1969,7 +1980,7 @@
     >>> print_archive_names(ubuntu_copy_archives)
     Name             Owner         Private  Enabled
     fine-copy        copy-owner2   False    True
-    my-copy-archive  person-name6  False    True
+    my-copy-archive  me-copy       False    True
     team-archive     t1            False    True
     ultimate-copy    copy-owner1   False    True
 
@@ -2020,7 +2031,7 @@
     >>> print_archive_names(ubuntu_copy_archives)
     Name             Owner         Private  Enabled
     fine-copy        copy-owner2   False    True
-    my-copy-archive  person-name6  True     True
+    my-copy-archive  me-copy       True     True
     ultimate-copy    copy-owner1   False    True
 
 An admin will see all the private and disabled archives in the results:
@@ -2031,7 +2042,7 @@
     >>> print_archive_names(ubuntu_copy_archives)
     Name             Owner         Private  Enabled
     fine-copy        copy-owner2   False    True
-    my-copy-archive  person-name6  True     True
+    my-copy-archive  me-copy       True     True
     team-archive     t1            True     True
     there-we-go      juergen       False    False
     true-copy        copy-owner2   False    False

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2010-03-23 22:15:02 +0000
+++ lib/lp/soyuz/model/archive.py	2010-04-08 02:42:27 +0000
@@ -18,6 +18,7 @@
 from storm.expr import Or, And, Select, Sum
 from storm.locals import Count, Join
 from storm.store import Store
+from storm.zope.interfaces import IResultSet
 from zope.component import getUtility
 from zope.event import notify
 from zope.interface import alsoProvides, implements
@@ -784,10 +785,9 @@
         if self.is_ppa:
             archives.append(self.distribution.main_archive.id)
         archives.append(self.id)
-        dep = ArchiveDependency.selectOneBy(archive=self)
-        while dep is not None:
-            archives.append(dep.dependency.id)
-            dep = ArchiveDependency.selectOneBy(archive=dep.dependency)
+        archives.extend(
+            IResultSet(self.dependencies).values(
+            ArchiveDependency.dependencyID))
 
         query = """
             binarypackagename = %s AND