launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-builds-batching into lp:launchpad.

Commit message:
Fix builds_for_snap to avoid iterating over an unsliced DecoratedResultSet.

builds_for_snap iterates over snap.pending_builds and snap.completed_builds
doing visibility checks, and terminates the loop over snap.completed_builds
if it reaches ten builds.  Unfortunately, the first call to
snap.completed_builds.__iter__ runs the pre_iter_hook on the whole
DecoratedResultSet, since we aren't slicing it, which does a great deal of
work if the snap has many builds.

As it happens, the model already does the necessary visibility checks, as
proven by the existing tests for this still passing.  So we can just drop
this excess complication, slice snap.completed_builds as necessary, and be
done.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1671134 in Launchpad itself: "OOPS when trying to access certain snap build pages"
  https://bugs.launchpad.net/launchpad/+bug/1671134

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-builds-batching/+merge/319380

I couldn't see an obvious way to test this, since the symptom was larger queries rather than more queries.  Suggestions welcome.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-builds-batching into lp:launchpad.

=== modified file 'lib/lp/snappy/browser/snap.py'
--- lib/lp/snappy/browser/snap.py	2017-02-06 14:34:35 +0000
+++ lib/lp/snappy/browser/snap.py	2017-03-08 23:50:24 +0000
@@ -75,7 +75,6 @@
     stepthrough,
     structured,
     )
-from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.breadcrumb import (
     Breadcrumb,
     NameBreadcrumb,
@@ -211,17 +210,12 @@
     but unfinished builds to show up in the view but be discarded as more
     recent builds become available.
 
-    Builds that the user does not have permission to see are excluded.
+    Builds that the user does not have permission to see are excluded (by
+    the model code).
     """
-    builds = [
-        build for build in snap.pending_builds
-        if check_permission('launchpad.View', build)]
-    for build in snap.completed_builds:
-        if not check_permission('launchpad.View', build):
-            continue
-        builds.append(build)
-        if len(builds) >= 10:
-            break
+    builds = list(snap.pending_builds)
+    if len(builds) < 10:
+        builds.extend(snap.completed_builds[:10 - len(builds)])
     return builds
 
 

=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py	2017-02-27 18:46:38 +0000
+++ lib/lp/snappy/model/snap.py	2017-03-08 23:50:24 +0000
@@ -504,7 +504,7 @@
 
         def eager_load(rows):
             getUtility(ISnapBuildSet).preloadBuildsData(rows)
-            getUtility(IBuildQueueSet).preloadForBuildFarmJobs(result)
+            getUtility(IBuildQueueSet).preloadForBuildFarmJobs(rows)
 
         return DecoratedResultSet(result, pre_iter_hook=eager_load)