Launchpad itself

Code review comment for lp:~michael.nelson/launchpad/616331-private-builds-in-builder-history

616331-private-builds-in-builder-history
Merge into db-devel

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2010-08-11:

On Wed, Aug 11, 2010 at 7:54 PM, Edwin Grubbs
<email address hidden> 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!

review_edwin.diff

« Back to merge proposal

 === 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)