Merge lp:~al-maisan/launchpad/changes-url-474876 into lp:launchpad/db-devel

[Muharem Hrnjadovic]

[Muharem Hrnjadovic (al-maisan)]

Hello there!

This branch optimizes the database query behind the

    SourcePackagePublishingHistory.changes_file_url

property (bug #474876).

The old query (http://pastebin.ubuntu.com/342524/) joins on the following
tables:

    LibraryFileAlias, LibraryFileContent, PackageUpload, PackageUploadSource,
    SourcePackagePublishingHistory, SourcePackageRelease

In contrast, the new query (http://pastebin.ubuntu.com/342528/) only uses
these tables:

    LibraryFileAlias, PackageUpload, PackageUploadSource

The new query specifically avoids the SourcePackagePublishingHistory and the
SourcePackageRelease tables (with 758673 and 459354 rows respectively).

Please note that I refactored the lib/lp/soyuz/doc/publishing.txt test so
it is easier to read and debug it.
In order to make sure that the changes are proper, I have run the changed
test in a fresh branch that has no changes otherwise.

Tests to run:

    bin/test -vv -t publishing

No pertinent "make lint" errors or warnings.

[Muharem Hrnjadovic (al-maisan)]

Q/A on Soyuz dogfood:

Used an lpapi script (http://pastebin.ubuntu.com/342563/) to obtain a bunch of .changes file URLs and downloaded these using wget.

[Gavin Panella (allenap)]

Looks good, with the addition of the comment and unit test as we
discussed. Thanks!

<allenap> al-maisan_: Line 332, does getPublishedSources() sort the results?
<al-maisan_> allenap: yes, it does.
<allenap> al-maisan_: getChangesFilesForSources() seems to now only be used in lib/lp/soyuz/adapters/archivesourcepublication.py. Is there any way to change that to use your new query?
<al-maisan_> allenap: I am not sure, that method is selecting and pre-joining a lot more data and that probably makes sense for the web UI
<allenap> al-maisan_: Okay. Do you think you could comment in getChangesFilesForSources() that getChangesFileLFA() contains a potentially more efficient query, depending on what you need out of it.
<al-maisan_> allenap: sure, will do that.
<allenap> al-maisan_: There aren't any unit tests for getChangesFileLFA(). I can see that changes_file_url is tested in a few places, but it's not quite the same thing. Unless there's a good reason not to, could you add this?
<al-maisan_> allenap: good point, I'll add unit tests for getChangesFileLFA().

review: Approve

[Muharem Hrnjadovic (al-maisan)]

Hello Gavin,

I have added the comment and the unit test as discussed.

Furthermore, I converted the property into a method in order to avoid it
being computed every time someone requests a SPPH instance on the
Launchpad API.

Can you please take another quick look before I land the branch?

 http://pastebin.ubuntu.com/344031/

Thanks -- Muharem

Gavin Panella wrote:
> Review: Approve
> Looks good, with the addition of the comment and unit test as we
> discussed. Thanks!
>
> <allenap> al-maisan_: Line 332, does getPublishedSources() sort the
> results?
> <al-maisan_> allenap: yes, it does.
> <allenap> al-maisan_: getChangesFilesForSources() seems to now only be
> used in lib/lp/soyuz/adapters/archivesourcepublication.py. Is there
> any way to change that to use your new query?
> <al-maisan_> allenap: I am not sure, that method is selecting and
> pre-joining a lot more data and that probably makes sense for the web
> UI
> <allenap> al-maisan_: Okay. Do you think you could comment in
> getChangesFilesForSources() that getChangesFileLFA() contains a
> potentially more efficient query, depending on what you need out of
> it.
> <al-maisan_> allenap: sure, will do that.
Comment added.

> <allenap> al-maisan_: There aren't any unit tests for
> getChangesFileLFA(). I can see that changes_file_url is tested in a
> few places, but it's not quite the same thing. Unless there's a good
> reason not to, could you add this?
> <al-maisan_> allenap: good point, I'll add unit tests for
> getChangesFileLFA().
Done.

Best regards

--
Muharem Hrnjadovic <email address hidden>
Public key id : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F 5602 219F 6B60 B2BB FCFC

[Gavin Panella (allenap)]

Your post-review changes all look good.

<allenap> al-maisan: Is it very expensive to calculate changes_file_url?
<al-maisan> allenap: it is somewhat expensive and it is calculated for each SourcePackagePublishingHistory on the LP API -- whether you're interested in it or not.
<allenap> al-maisan: By making it a method, Launchpad must field an extra HTTP request for every SPPH. Maybe that's a bigger overhead? Or do you expect that it will be called rarely?
<al-maisan> allenap: it would seem that it's not a datum of general interest .. there are some LP API apps that use it: e.g. code imports or scripts written by kees
<al-maisan> but my feeling is that it's data that's not necessarily needed whenever we access a SPPH instance
<al-maisan> .. and calculating the 'changes_file_url' property now leads the Soyuz OOPS top-10 by a big margin
<allenap> al-maisan: Okay, sounds like you have a handle on it. It can be changed later if it turns out to be otherwise, so I'm not too worried.
<allenap> al-maisan: Timeout or error?
<al-maisan> allenap: time out
<allenap> al-maisan: Wow.
<al-maisan> yeah
<allenap> al-maisan: Okay, one other thing. The test_changesFileLFA method is misnamed; I think it should be test_getChangesFileLFA(). Everything else looks fine.

review: Approve

=== modified file 'lib/lp/soyuz/doc/publishing.txt'
--- lib/lp/soyuz/doc/publishing.txt	2009-12-14 13:49:03 +0000
+++ lib/lp/soyuz/doc/publishing.txt	2009-12-21 11:43:12 +0000
@@ -93,7 +93,7 @@
     >>> print spph.package_signer
     None
 
-There is also a property that returns the changesfile URL.  This is proxied
+There is also a method that returns the .changes file URL. This is proxied
 through the webapp rather than being a librarian URL because the changesfile
 could be private and thus in the restricted librarian.
 
@@ -103,13 +103,13 @@
 
 The pmount source has no packageupload in the sampledata:
 
-    >>> print spph.changes_file_url
+    >>> print spph.changesFileUrl()
     None
 
 The iceweasel source has good data:
 
     >>> pub = spph.archive.getPublishedSources(name="iceweasel")[0]
-    >>> print pub.changes_file_url
+    >>> print pub.changesFileUrl()
     http://launchpad.dev/ubuntu/+archive/primary/+files/mozilla-firefox_0.9_i386.changes
 
 There is also a helper property to determine whether the current release for
@@ -140,7 +140,8 @@
 'pending' state when builds are fully built but not yet published.
 
     >>> from lp.soyuz.interfaces.build import BuildStatus
-    >>> spph = test_publisher.getPubSource(architecturehintlist='any')
+    >>> spph = test_publisher.getPubSource(
+    ...     sourcename='abc', architecturehintlist='any')
     >>> builds = spph.createMissingBuilds()
     >>> for build in builds:
     ...     build.buildstate = BuildStatus.FULLYBUILT
@@ -154,15 +155,15 @@
     >>> build_status_summary = spph.getStatusSummaryForBuilds()
     >>> print_build_status_summary(build_status_summary)
     FULLYBUILT_PENDING
-    hppa build of foo 666 in ubuntutest breezy-autotest RELEASE
-    i386 build of foo 666 in ubuntutest breezy-autotest RELEASE
+    hppa build of abc 666 in ubuntutest breezy-autotest RELEASE
+    i386 build of abc 666 in ubuntutest breezy-autotest RELEASE
 
 The underlying method being used here is getUnpublishedBuilds():
 
     >>> for build in spph.getUnpublishedBuilds():
     ...     print build.title
-    hppa build of foo 666 in ubuntutest breezy-autotest RELEASE
-    i386 build of foo 666 in ubuntutest breezy-autotest RELEASE
+    hppa build of abc 666 in ubuntutest breezy-autotest RELEASE
+    i386 build of abc 666 in ubuntutest breezy-autotest RELEASE
 
 Note: if the related archive for this source package publishing is
 a rebuild archive then the status summary will always display
@@ -173,8 +174,8 @@
     >>> build_status_summary = spph.getStatusSummaryForBuilds()
     >>> print_build_status_summary(build_status_summary)
     FULLYBUILT
-    hppa build of foo 666 in ubuntutest breezy-autotest RELEASE
-    i386 build of foo 666 in ubuntutest breezy-autotest RELEASE
+    hppa build of abc 666 in ubuntutest breezy-autotest RELEASE
+    i386 build of abc 666 in ubuntutest breezy-autotest RELEASE
 
     # Just set the purpose back before continuing on.
     >>> spph.archive.purpose = ArchivePurpose.PRIMARY
@@ -183,18 +184,18 @@
 
     >>> from lp.soyuz.interfaces.publishing import (
     ...     PackagePublishingStatus)
-    >>> bpr = test_publisher.uploadBinaryForBuild(builds[0], 'foo-bin')
+    >>> bpr = test_publisher.uploadBinaryForBuild(builds[0], 'abc-bin')
     >>> bpph = test_publisher.publishBinaryInArchive(bpr, spph.archive,
     ...     status=PackagePublishingStatus.PUBLISHED)
     >>> print_build_status_summary(spph.getStatusSummaryForBuilds())
     FULLYBUILT_PENDING
-    i386 build of foo 666 in ubuntutest breezy-autotest RELEASE
+    i386 build of abc 666 in ubuntutest breezy-autotest RELEASE
 
 Nor will it be included in the unpublished builds:
 
     >>> for build in spph.getUnpublishedBuilds():
     ...     print build.title
-    i386 build of foo 666 in ubuntutest breezy-autotest RELEASE
+    i386 build of abc 666 in ubuntutest breezy-autotest RELEASE
 
 By default, only FULLYBUILT builds are included in the returned
 unpublished builds:
@@ -210,7 +211,7 @@
     ...     BuildStatus.SUPERSEDED
     ...     ]):
     ...     print build.title
-    i386 build of foo 666 in ubuntutest breezy-autotest RELEASE
+    i386 build of abc 666 in ubuntutest breezy-autotest RELEASE
 
 Just switch it back to FULLYBUILT before continuing:
 
@@ -219,14 +220,14 @@
 After publishing the second binary, the status changes to FULLYBUILT as
 per normal:
 
-    >>> bpr = test_publisher.uploadBinaryForBuild(builds[1], 'foo-bin')
+    >>> bpr = test_publisher.uploadBinaryForBuild(builds[1], 'abc-bin')
     >>> bpph = test_publisher.publishBinaryInArchive(
     ...     bpr, spph.archive,
     ...     status=PackagePublishingStatus.PUBLISHED)
     >>> print_build_status_summary(spph.getStatusSummaryForBuilds())
     FULLYBUILT
-    hppa build of foo 666 in ubuntutest breezy-autotest RELEASE
-    i386 build of foo 666 in ubuntutest breezy-autotest RELEASE
+    hppa build of abc 666 in ubuntutest breezy-autotest RELEASE
+    i386 build of abc 666 in ubuntutest breezy-autotest RELEASE
 
 There are no longer any unpublished builds for the source package:
 
@@ -242,41 +243,42 @@
     >>> transaction.commit()
     >>> print_build_status_summary(spph.getStatusSummaryForBuilds())
     FULLYBUILT
-    hppa build of foo 666 in ubuntutest breezy-autotest RELEASE
-    i386 build of foo 666 in ubuntutest breezy-autotest RELEASE
+    hppa build of abc 666 in ubuntutest breezy-autotest RELEASE
+    i386 build of abc 666 in ubuntutest breezy-autotest RELEASE
 
 If a build of a SourcePackagePublishingHistory is manually set to
 superseded (just to cancel the build) even though the SPPH is itself
 not marked as superseded, the status summary will not include
 that build:
 
-    >>> spph = test_publisher.getPubSource(architecturehintlist='any')
+    >>> spph = test_publisher.getPubSource(
+    ...     sourcename='def', architecturehintlist='any')
     >>> builds = spph.createMissingBuilds()
     >>> builds[0].buildstate = BuildStatus.SUPERSEDED
     >>> builds[1].buildstate = BuildStatus.FULLYBUILT
     >>> build_status_summary = spph.getStatusSummaryForBuilds()
     >>> print_build_status_summary(build_status_summary)
     FULLYBUILT_PENDING
-    i386 build of foo 666 in ubuntutest breezy-autotest RELEASE
+    i386 build of def 666 in ubuntutest breezy-autotest RELEASE
 
 And after publishing the other build, the normal FULLY_BUILT status
 is achieved (without the 'canceled' build):
 
-    >>> bpr = test_publisher.uploadBinaryForBuild(builds[1], 'foo-bin')
+    >>> bpr = test_publisher.uploadBinaryForBuild(builds[1], 'def-bin')
     >>> bpph = test_publisher.publishBinaryInArchive(bpr, spph.archive,
     ...     status=PackagePublishingStatus.PUBLISHED)
     >>> print_build_status_summary(spph.getStatusSummaryForBuilds())
     FULLYBUILT
-    i386 build of foo 666 in ubuntutest breezy-autotest RELEASE
+    i386 build of def 666 in ubuntutest breezy-autotest RELEASE
 
 IBinaryPackagePublishingHistory also contains similar API conveniences.
 
-    >>> bpph = test_publisher.getPubBinaries()[0]
+    >>> bpph = test_publisher.getPubBinaries(binaryname='def-bin')[0]
     >>> verifyObject(IBinaryPackagePublishingHistory, bpph)
     True
 
     >>> print bpph.binary_package_name
-    foo-bin
+    def-bin
 
     >>> print bpph.binary_package_version
     666
@@ -409,23 +411,25 @@
 two architecture-specific binaries in ubuntu/breezy-autotest.
 
     >>> from lp.registry.interfaces.pocket import PackagePublishingPocket
-    >>> source = test_publisher.getPubSource(
+    >>> source = test_publisher.getPubSource( 
+    ...     sourcename='ghi',
     ...     architecturehintlist='any',
     ...     status=PackagePublishingStatus.PUBLISHED,
     ...     pocket=PackagePublishingPocket.PROPOSED)
 
     >>> binaries = test_publisher.getPubBinaries(
+    ...     binaryname='ghi-bin',
     ...     pub_source=source,
     ...     status=PackagePublishingStatus.PUBLISHED,
     ...     pocket=PackagePublishingPocket.PROPOSED)
 
     >>> print source.displayname
-    foo 666 in breezy-autotest
+    ghi 666 in breezy-autotest
 
     >>> for bin in binaries:
     ...     print bin.displayname
-    foo-bin 666 in breezy-autotest i386
-    foo-bin 666 in breezy-autotest hppa
+    ghi-bin 666 in breezy-autotest i386
+    ghi-bin 666 in breezy-autotest hppa
 
 Using the source publication, ISecureSourcePackagePublishingHistory, we
 can obtain the published binaries.
@@ -440,8 +444,8 @@
 
     >>> for build in source.getBuilds():
     ...     print build.title
-    hppa build of foo 666 in ubuntutest breezy-autotest PROPOSED
-    i386 build of foo 666 in ubuntutest breezy-autotest PROPOSED
+    hppa build of ghi 666 in ubuntutest breezy-autotest PROPOSED
+    i386 build of ghi 666 in ubuntutest breezy-autotest PROPOSED
 
 Now that we know how to retrieve generated binary publication related
 to a source publication we can exercise the API provided to copy
@@ -513,13 +517,13 @@
 
     >>> for bin in source.getPublishedBinaries():
     ...     print bin.displayname, bin.pocket.name, bin.status.name
-    foo-bin 666 in breezy-autotest hppa PROPOSED PUBLISHED
-    foo-bin 666 in breezy-autotest i386 PROPOSED PUBLISHED
+    ghi-bin 666 in breezy-autotest hppa PROPOSED PUBLISHED
+    ghi-bin 666 in breezy-autotest i386 PROPOSED PUBLISHED
 
     >>> for bin in copied_source.getPublishedBinaries():
     ...     print bin.displayname, bin.pocket.name, bin.status.name
-    foo-bin 666 in breezy-autotest hppa UPDATES PENDING
-    foo-bin 666 in breezy-autotest i386 UPDATES PENDING
+    ghi-bin 666 in breezy-autotest hppa UPDATES PENDING
+    ghi-bin 666 in breezy-autotest i386 UPDATES PENDING
 
 Note that even PENDING binary publications are returned by
 getPublishedBinaries(), it considers both PENDING and PUBLISHED status
@@ -609,9 +613,9 @@
 
     >>> for pub in copied_source.getPublishedBinaries():
     ...     print pub.displayname, pub.component.name
-    foo-bin 666 in breezy-autotest hppa universe
-    foo-bin 666 in breezy-autotest hppa main
-    foo-bin 666 in breezy-autotest i386 main
+    ghi-bin 666 in breezy-autotest hppa universe
+    ghi-bin 666 in breezy-autotest hppa main
+    ghi-bin 666 in breezy-autotest i386 main
 
 The publication duplication is solved in the publishing pipeline,
 specifically in the 'domination' state. See
@@ -622,8 +626,8 @@
 
     >>> for pub in copied_source.getBuiltBinaries():
     ...     print pub.displayname, pub.component.name
-    foo-bin 666 in breezy-autotest hppa universe
-    foo-bin 666 in breezy-autotest i386 main
+    ghi-bin 666 in breezy-autotest hppa universe
+    ghi-bin 666 in breezy-autotest i386 main
 
 We have to re-publish the superseded and the deleted publications above
 because it's used below.
@@ -725,19 +729,21 @@
     >>> cprov = getUtility(IPersonSet).getByName('cprov')
 
     >>> ppa_source = test_publisher.getPubSource(
+    ...     sourcename='jkl',
     ...     archive=cprov.archive,
     ...     status=PackagePublishingStatus.PUBLISHED)
     >>> ppa_binaries = test_publisher.getPubBinaries(
+    ...     binaryname='jkl-bin',
     ...     pub_source=ppa_source,
     ...     status=PackagePublishingStatus.PUBLISHED)
 
     >>> print ppa_source.displayname, ppa_source.archive.displayname
-    foo 666 in breezy-autotest PPA for Celso Providelo
+    jkl 666 in breezy-autotest PPA for Celso Providelo
 
     >>> for bin in ppa_binaries:
     ...     print bin.displayname, bin.archive.displayname
-    foo-bin 666 in breezy-autotest i386 PPA for Celso Providelo
-    foo-bin 666 in breezy-autotest hppa PPA for Celso Providelo
+    jkl-bin 666 in breezy-autotest i386 PPA for Celso Providelo
+    jkl-bin 666 in breezy-autotest hppa PPA for Celso Providelo
 
 Now we will copy only the source from Celso's PPA breezy-autotest to
 hoary-test.
@@ -764,7 +770,7 @@
 
     >>> for build in builds:
     ...     print build.title
-    i386 build of foo 666 in ubuntutest hoary-test RELEASE
+    i386 build of jkl 666 in ubuntutest hoary-test RELEASE
 
 If createMissingBuilds get called again on either sources no builds
 will get created.
@@ -808,10 +814,12 @@
 architecture-independent sources.
 
     >>> ppa_source = test_publisher.getPubSource(
+    ...     sourcename='mno',
     ...     archive=cprov.archive, version="999",
     ...     status=PackagePublishingStatus.PUBLISHED)
 
     >>> ppa_binaries = test_publisher.getPubBinaries(
+    ...     binaryname='mno-bin',
     ...     pub_source=ppa_source,
     ...     status=PackagePublishingStatus.PUBLISHED)
 
@@ -825,7 +833,7 @@
 
     >>> ppa_binary_i386 = ppa_binaries[0]
     >>> print ppa_binary_i386.displayname
-    foo-bin 999 in breezy-autotest i386
+    mno-bin 999 in breezy-autotest i386
 
     >>> copied_binary = ppa_binary_i386.copyTo(series, pocket, archive)
 
@@ -833,12 +841,12 @@
 
     >>> copied_source = SourcePackagePublishingHistory.get(copied_source.id)
     >>> print copied_source.displayname
-    foo 999 in hoary-test
+    mno 999 in hoary-test
 
     >>> for bin in copied_source.getPublishedBinaries():
     ...     print bin.displayname
-    foo-bin 999 in hoary-test amd64
-    foo-bin 999 in hoary-test i386
+    mno-bin 999 in hoary-test amd64
+    mno-bin 999 in hoary-test i386
 
 So, no builds are created.
 
@@ -856,23 +864,25 @@
 
     >>> for file in source.getSourceAndBinaryLibraryFiles():
     ...     print file.filename
-    foo-bin_666_hppa.deb
-    foo-bin_666_i386.deb
-    foo_666.dsc
+    ghi-bin_666_hppa.deb
+    ghi-bin_666_i386.deb
+    ghi_666.dsc
 
 We can also publish a package in a PPA and query on its files:
 
     >>> ppa_source = test_publisher.getPubSource(
+    ...     sourcename='pqr',
     ...     status=PackagePublishingStatus.PUBLISHED,
     ...     archive=cprov.archive)
     >>> ppa_binaries= test_publisher.getPubBinaries(
+    ...     binaryname='pqr-bin',
     ...     pub_source=ppa_source,
     ...     status=PackagePublishingStatus.PUBLISHED)
 
     >>> for file in ppa_source.getSourceAndBinaryLibraryFiles():
     ...     print file.filename
-    foo-bin_666_all.deb
-    foo_666.dsc
+    pqr-bin_666_all.deb
+    pqr_666.dsc
 
 
 Publishing records age
@@ -1064,6 +1074,16 @@
     >>> cprov_sources = list(cprov.archive.getPublishedSources())
     >>> len(cprov_sources)
     8
+    >>> for spph in cprov_sources:
+    ...     print spph.displayname
+    cdrkit 1.0 in breezy-autotest
+    iceweasel 1.0 in warty
+    jkl 666 in hoary-test
+    jkl 666 in breezy-autotest
+    mno 999 in hoary-test
+    mno 999 in breezy-autotest
+    pmount 0.1-1 in warty
+    pqr 666 in breezy-autotest
 
 Now that we have a set of source publications let's get the builds in
 its context.
@@ -1138,7 +1158,8 @@
     >>> sources = []
     >>> builds = []
     >>> for count in range(2):
-    ...     spph = test_publisher.getPubSource(architecturehintlist='any')
+    ...     spph = test_publisher.getPubSource(
+    ...     sourcename='stu', architecturehintlist='any')
     ...     missing_builds = spph.createMissingBuilds()
     ...     for build in missing_builds:
     ...         build.buildstate = BuildStatus.FULLYBUILT
@@ -1184,16 +1205,16 @@
     ...  arch) = cprov_binaries.last()
 
     >>> print source_pub.displayname
-    foo 666 in breezy-autotest
+    pqr 666 in breezy-autotest
 
     >>> print binary_pub.displayname
-    foo-bin 666 in breezy-autotest i386
+    pqr-bin 666 in breezy-autotest i386
 
     >>> print binary.title
-    foo-bin-666
+    pqr-bin-666
 
     >>> print binary_name.name
-    foo-bin
+    pqr-bin
 
     >>> print arch.displayname
     ubuntutest Breezy Badger Autotest i386
@@ -1237,14 +1258,15 @@
     >>> file_ids = [file.id for source, file, content in binary_files]
     >>> file_ids == sorted(file_ids)
     True
+
     >>> for source, file, content in binary_files:
     ...     print file.filename
     mozilla-firefox_0.9_i386.deb
-    foo-bin_666_all.deb
-    foo-bin_666_all.deb
-    foo-bin_999_all.deb
-    foo-bin_999_all.deb
-    foo-bin_666_all.deb
+    jkl-bin_666_all.deb
+    jkl-bin_666_all.deb
+    mno-bin_999_all.deb
+    mno-bin_999_all.deb
+    pqr-bin_666_all.deb
 
 getPackageDiffsForSources() is also provided by `IPublishingSet`, it
 allows callsites to retrieve all related `PackageDiff` records based
@@ -1262,11 +1284,11 @@
 For testing purposes we will create a `PackageDiff` for Celso's
 'pmount' and another for Celso's 'iceweasel' sources.
 
-    >>> a_source = cprov_sources[-2]
+    >>> a_source = cprov_sources[1]
     >>> print a_source.displayname
     iceweasel 1.0 in warty
 
-    >>> to_source = cprov_sources[-1]
+    >>> to_source = cprov_sources[-2]
     >>> print to_source.displayname
     pmount 0.1-1 in warty
 
@@ -1346,7 +1368,7 @@
     None
 
 getChangesFilesForSources(), provided by IPublishingSet, allows
-call sites to retrieve all changesfiles related to a set of source
+call sites to retrieve all .changes files related to a set of source
 publications.
 
     >>> cprov_changes = publishing_set.getChangesFilesForSources(
@@ -1460,6 +1482,12 @@
     >>> cprov = getUtility(IPersonSet).getByName('cprov')
     >>> cprov_published_sources = cprov.archive.getPublishedSources(
     ...     status=PackagePublishingStatus.PUBLISHED)
+    >>> for spph in cprov_published_sources:
+    ...     print spph.displayname
+    jkl 666 in breezy-autotest
+    mno 999 in breezy-autotest
+    pmount 0.1-1 in warty
+    pqr 666 in breezy-autotest
 
 We use the source publications to initialise
 `ArchiveSourcePublications`.
@@ -1541,7 +1569,7 @@
 a dictionary, keyed by `SourcePackagePublishingHistory `, in way they
 can be quickly looked up when building `ArchiveSourcePublications`.
 
-    >>> real_pub = cprov_published_sources[0]
+    >>> real_pub = cprov_published_sources[1]
 
 Builds by source.
 
@@ -1562,15 +1590,26 @@
     Matches
 
 getChangesFileBySources() returns a dictionary mapping each individual
-source package publication to its corresponding changesfile (as a
+source package publication to its corresponding .changes file (as a
 LibraryFileAlias).
 
     >>> all_cprov_sources = cprov.archive.getPublishedSources()
+    >>> for spph in all_cprov_sources:
+    ...     print spph.displayname
+    cdrkit 1.0 in breezy-autotest
+    foo 666 in breezy-autotest
+    iceweasel 1.0 in warty
+    jkl 666 in hoary-test
+    jkl 666 in breezy-autotest
+    mno 999 in hoary-test
+    mno 999 in breezy-autotest
+    pmount 0.1-1 in warty
+    pqr 666 in breezy-autotest
 
 We select the only available publication in Celso's PPA with a valid
-changesfile in the sampledata.
+.changes file in the sampledata.
 
-    >>> pub_with_changes = all_cprov_sources[-2]
+    >>> pub_with_changes = all_cprov_sources[2]
     >>> the_source = pub_with_changes.sourcepackagerelease
     >>> the_change = the_source.upload_changesfile
     >>> print the_change.filename
@@ -1592,7 +1631,7 @@
 whole difference when rendering PPA pages with many source
 publications.
 
-    >>> decorated_pub = list(decorated_set)[0]
+    >>> decorated_pub = list(decorated_set)[1]
 
     >>> print decorated_pub
     <...ArchiveSourcePublication ...>
@@ -1625,17 +1664,17 @@
 `PackageUpload.changesfile` in both, the real and the decorated
 objects.
 
-    >>> pub_with_changes = cprov_published_sources[0]
+    >>> pub_with_changes = cprov_published_sources[1]
     >>> the_source = pub_with_changes.sourcepackagerelease
     >>> changesfile = the_source.upload_changesfile
     >>> print '%s (%s)' % (changesfile.filename, changesfile.content.md5)
-    foo_999_source.changes (6168e17ba012fc3db6dc77e255243bd1)
+    mno_999_source.changes (6168e17ba012fc3db6dc77e255243bd1)
 
-    >>> decorated_pub_with_changes = list(decorated_set)[0]
+    >>> decorated_pub_with_changes = list(decorated_set)[1]
     >>> decorated_source = decorated_pub_with_changes.sourcepackagerelease
     >>> changesfile = decorated_source.upload_changesfile
     >>> print '%s (%s)' % (changesfile.filename, changesfile.content.md5)
-    foo_999_source.changes (6168e17ba012fc3db6dc77e255243bd1)
+    mno_999_source.changes (6168e17ba012fc3db6dc77e255243bd1)
 
 `ArchiveSourcePublication` also has a decorated version of the
 getStatusSummaryForBuilds() method.
@@ -1643,7 +1682,7 @@
     >>> print_build_status_summary(
     ...     decorated_pub.getStatusSummaryForBuilds())
     FULLYBUILT
-    i386 build of foo 999 in ubuntutest breezy-autotest RELEASE
+    i386 build of mno 999 in ubuntutest breezy-autotest RELEASE
 
 We can verify that this decorated version is in fact using the cached
 builds by explicitly modifying the cached builds on the delegate. We'll
@@ -1653,8 +1692,8 @@
     >>> print_build_status_summary(
     ...     decorated_pub.getStatusSummaryForBuilds())
     FULLYBUILT
-    i386 build of foo 999 in ubuntutest breezy-autotest RELEASE
-    i386 build of foo 999 in ubuntutest breezy-autotest RELEASE
+    i386 build of mno 999 in ubuntutest breezy-autotest RELEASE
+    i386 build of mno 999 in ubuntutest breezy-autotest RELEASE
 
 
 IPublishingSet.getBuildStatusSummariesForSourceIdsAndArchive()

=== modified file 'lib/lp/soyuz/interfaces/publishing.py'
--- lib/lp/soyuz/interfaces/publishing.py	2009-12-12 03:25:27 +0000
+++ lib/lp/soyuz/interfaces/publishing.py	2009-12-21 11:43:12 +0000
@@ -527,12 +527,6 @@
             title=_("Source Package Version"),
             required=False, readonly=True))
 
-    changes_file_url = exported(
-        Text(
-            title=_("Changes File URL"),
-            description=_("A URL for this source publication's changes file "
-                          "for the source upload.")))
-
     source_file_urls = exported(
         List(
             value_type=Text(),
@@ -612,6 +606,13 @@
         :return: a list of `IBuilds`.
         """
 
+    @export_read_operation()
+    def changesFileUrl():
+        """The .changes file URL for this source publication.
+
+        :return: the .changes file URL for this source (a string).
+        """
+
     def getUnpublishedBuilds(build_states=None):
         """Return a resultset of `IBuild` objects in this context that are
         not published.
@@ -1089,6 +1090,15 @@
              `SourcePackageRelease`, `LibraryFileAlias`, `LibraryFileContent`)
         """
 
+    def getChangesFileLFA(spr):
+        """The changes file for the given `SourcePackageRelease`.
+
+        :param spr: the `SourcePackageRelease` for which to return the
+            changes file `LibraryFileAlias`.
+
+        :return: a `LibraryFileAlias` instance or None
+        """
+
     def requestDeletion(sources, removed_by, removal_comment=None):
         """Delete the source and binary publications specified.
 

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2009-12-14 13:49:03 +0000
+++ lib/lp/soyuz/model/publishing.py	2009-12-21 11:43:12 +0000
@@ -593,29 +593,28 @@
 
         return DecoratedResultSet(result_set, result_to_build)
 
-    @property
-    def changes_file_url(self):
+    def changesFileUrl(self):
         """See `ISourcePackagePublishingHistory`."""
-        results = getUtility(IPublishingSet).getChangesFilesForSources(
-            self)
+        # We use getChangesFileLFA() as opposed to getChangesFilesForSources()
+        # because the latter is more geared towards the web UI and taxes the
+        # db much more in terms of the join width and the pre-joined data.
+        #
+        # This method is accessed overwhelmingly via the LP API and calling
+        # getChangesFileLFA() which is much lighter on the db has the
+        # potential of performing significantly better.
+        changes_lfa = getUtility(IPublishingSet).getChangesFileLFA(
+            self.sourcepackagerelease)
 
-        result = results.one()
-        if result is None:
+        if changes_lfa is None:
             # This should not happen in practice, but the code should
             # not blow up because of bad data.
             return None
-        source, packageupload, spr, changesfile, lfc = result
 
         # Return a webapp-proxied LibraryFileAlias so that restricted
         # librarian files are accessible.  Non-restricted files will get
         # a 302 so that webapp threads are not tied up.
-
-        # Avoid circular imports.
-        from canonical.launchpad.browser.librarian import (
-            ProxiedLibraryFileAlias)
-
-        proxied_file = ProxiedLibraryFileAlias(changesfile, self.archive)
-        return proxied_file.http_url
+        the_url = self._proxied_urls((changes_lfa,), self.archive)[0]
+        return the_url
 
     def createMissingBuilds(self, architectures_available=None,
                             pas_verify=None, logger=None):
@@ -1584,6 +1583,23 @@
         result_set.order_by(SourcePackagePublishingHistory.id)
         return result_set
 
+    def getChangesFileLFA(self, spr):
+        """See `IPublishingSet`."""
+        # Import PackageUpload and PackageUploadSource locally to avoid
+        # circular imports.
+        from lp.soyuz.model.queue import PackageUpload, PackageUploadSource
+
+        store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
+        result_set = store.find(
+            LibraryFileAlias,
+            LibraryFileAlias.id == PackageUpload.changesfileID,
+            PackageUpload.status == PackageUploadStatus.DONE,
+            PackageUpload.distroseriesID == spr.upload_distroseriesID,
+            PackageUpload.archiveID == spr.upload_archiveID,
+            PackageUpload.id == PackageUploadSource.packageuploadID,
+            PackageUploadSource.sourcepackagereleaseID == spr.id)
+        return result_set.one()
+
     def getBuildStatusSummariesForSourceIdsAndArchive(self,
                                                       source_ids,
                                                       archive):

=== modified file 'lib/lp/soyuz/stories/webservice/xx-source-package-publishing.txt'
--- lib/lp/soyuz/stories/webservice/xx-source-package-publishing.txt	2009-12-10 13:21:06 +0000
+++ lib/lp/soyuz/stories/webservice/xx-source-package-publishing.txt	2009-12-21 11:43:12 +0000
@@ -112,7 +112,6 @@
     >>> pprint_entry(pubs['entries'][0])
     archive_link: u'http://.../~cprov/+archive/ppa'
     binary_file_urls: []
-    changes_file_url: u'http://launchpad.dev/~cprov/+archive/ppa/+files/testwebservice_666_source.changes'
     component_name: u'main'
     date_created: ...
     date_made_pending: None
@@ -136,6 +135,10 @@
     source_package_version: u'666'
     status: u'Pending'
 
+    >>> webservice.named_get(
+    ...     pubs['entries'][0]['self_link'], 'changesFileUrl').jsonBody()
+    u'http://launchpad.dev/~cprov/+archive/ppa/+files/testwebservice_666_source.changes'
+
 
 Unsigned sources
 ================

=== modified file 'lib/lp/soyuz/tests/test_publishing_models.py'
--- lib/lp/soyuz/tests/test_publishing_models.py	2009-07-17 02:25:09 +0000
+++ lib/lp/soyuz/tests/test_publishing_models.py	2009-12-21 11:43:12 +0000
@@ -76,5 +76,18 @@
         # even though it is no longer published.
         self.assertContentEqual(self.builds[1:3], unpublished_builds)
 
+    def test_getChangesFileLFA(self):
+        # The getChangesFileLFA() method finds the right LFAs.
+        lfas = (
+            self.publishing_set.getChangesFileLFA(hist.sourcepackagerelease)
+            for hist in self.sources)
+        urls = [lfa.http_url for lfa in lfas]
+        self.assertEqual(urls, [
+            'http://localhost:58000/94/gedit_666_source.changes', 
+            'http://localhost:58000/96/firefox_666_source.changes', 
+            'http://localhost:58000/98/getting-things-gnome_666_source.changes'
+            ])
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)