launchpad-reviewers team mailing list archive

[Michael Nelson <michael.nelson@xxxxxxxxxxxxx>]

On Wed, Aug 11, 2010 at 7:54 PM, Edwin Grubbs
<edwin.grubbs@xxxxxxxxxxxxx> wrote:
> Review: Approve code
> Hi Michael,
>
> This is a nice improvement. Since the TeamParticipation table won't be used if the user is anonymous or an admin, it would be a small but easy optimization to not join it into those queries. For example:
>
> origin = [
>  BuildFarmJob,
>  LeftJoin(PackageBuild, ...),
>  LeftJoin(Archive, ...),
>  ]
>
> if ANONYMOUS:
>  ...
> elif ADMIN:
>  ...
> else:
>  origin.append(LeftJoin(TeamParticipation, ...))
>
> filtered_builds = IStore(BuildFarmJob).using(*origin).find(...)
>
> I guess if an admin is running the query, there is no reason to use any tables besides BuildFarmJob.

Yes... good point. I've attached an incremental (pushed as r9651) that
only uses the joins when necessary. Thanks!

-- 
https://code.launchpad.net/~michael.nelson/launchpad/616331-private-builds-in-builder-history/+merge/32355
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~michael.nelson/launchpad/616331-private-builds-in-builder-history into lp:launchpad.

=== modified file 'lib/lp/buildmaster/model/buildfarmjob.py'
--- lib/lp/buildmaster/model/buildfarmjob.py	2010-08-11 17:09:45 +0000
+++ lib/lp/buildmaster/model/buildfarmjob.py	2010-08-11 21:26:07 +0000
@@ -365,15 +365,18 @@
         # Currently only package builds can be private (via their
         # related archive), but not all build farm jobs will have a
         # related package build - hence the left join.
-        left_join_pkg_builds = LeftJoin(
-            PackageBuild, PackageBuild.build_farm_job == BuildFarmJob.id)
-        left_join_archive = LeftJoin(
-            Archive, PackageBuild.archive == Archive.id)
-        left_join_participation = LeftJoin(
-            TeamParticipation, TeamParticipation.teamID == Archive.ownerID)
+        origin = [BuildFarmJob]
+        left_join_archive = [
+            LeftJoin(
+                PackageBuild,
+                PackageBuild.build_farm_job == BuildFarmJob.id),
+            LeftJoin(
+                Archive, PackageBuild.archive == Archive.id),
+            ]
 
         if user is None:
             # Anonymous requests don't get to see private builds at all.
+            origin.extend(left_join_archive)
             extra_clauses.append(Coalesce(Archive.private, False) == False)
 
         elif user.inTeam(getUtility(ILaunchpadCelebrities).admin):
@@ -382,16 +385,15 @@
         else:
             # Everyone else sees all public builds and the
             # specific private builds to which they have access.
+            origin.extend(left_join_archive)
+            origin.append(LeftJoin(
+                TeamParticipation, TeamParticipation.teamID == Archive.ownerID))
             extra_clauses.append(
                 Or(Coalesce(Archive.private, False) == False,
                    TeamParticipation.person == user))
 
-        filtered_builds = IStore(BuildFarmJob).using(
-            BuildFarmJob,
-            left_join_pkg_builds,
-            left_join_archive,
-            left_join_participation,
-            ).find(BuildFarmJob, *extra_clauses)
+        filtered_builds = IStore(BuildFarmJob).using(*origin).find(
+            BuildFarmJob, *extra_clauses)
 
         filtered_builds.order_by(
             Desc(BuildFarmJob.date_finished), BuildFarmJob.id)