Merge lp:~abentley/launchpad/skip-repeat-builds into lp:launchpad/db-devel

[Aaron Bentley]

Avoid creating duplicate pending builds.

= Summary =
Fix bug #595686: duplicate pending builds can be created.

== Proposed fix ==
Raise BuildAlreadyPending SourcePackageRecipe.requestBuild is called and the
arguments match a build already pending. Handle this in the UI as a validation
error. Handle this SourcePackageRecipeBuild.makeDailyBuilds by skipping to the
next distroseries.

== Pre-implementation notes ==
Preimplementation was with thumper

== Implementation details ==
Also added tests for TooManyBuilds on the webservice. Some quota tests had to
be updated so that they did not raise BuildAlreadyPending instead of
TooManyBuilds.

== Tests ==
bin/test -t test_request_builds_rejects_duplicate -t test_requestBuildRejectRepeats -t test_requestBuildRejectOverQuota -t test_makeDailyBuilds_skips_pending

== Demo and Q/A ==
Create a build. Attempt to create a duplicate build. You should get an error
that the build is already pending.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/code/model/tests/test_sourcepackagerecipebuild.py
  lib/lp/code/interfaces/webservice.py
  lib/lp/code/errors.py
  lib/lp/code/model/tests/test_sourcepackagerecipe.py
  lib/lp/code/model/sourcepackagerecipebuild.py
  lib/lp/code/browser/sourcepackagerecipe.py
  lib/lp/code/model/sourcepackagerecipe.py
  lib/lp/code/browser/tests/test_sourcepackagerecipe.py

== Pyflakes notices ==

lib/lp/code/interfaces/webservice.py
    8: 'TooManyBuilds' imported but unused
    8: 'CodeImportAlreadyRunning' imported but unused
    8: 'CodeImportNotInReviewedState' imported but unused
    8: 'BuildAlreadyPending' imported but unused
    8: 'BranchMergeProposalExists' imported but unused
    11: 'IBranchSet' imported but unused
    11: 'BranchCreatorNotOwner' imported but unused
    11: 'IBranch' imported but unused
    11: 'BranchExists' imported but unused
    11: 'BranchCreatorNotMemberOfOwnerTeam' imported but unused
    14: 'IBranchMergeProposal' imported but unused
    15: 'IBranchSubscription' imported but unused
    16: 'ICodeImport' imported but unused
    17: 'ICodeReviewComment' imported but unused
    18: 'ICodeReviewVoteReference' imported but unused
    19: 'IStaticDiff' imported but unused
    19: 'IDiff' imported but unused
    19: 'IPreviewDiff' imported but unused
    20: 'ISourcePackageRecipe' imported but unused
    21: 'ISourcePackageRecipeBuild' imported but unused

== Pylint notices ==

lib/lp/code/interfaces/webservice.py
    19: [W0611] Unused import IDiff
    8: [W0611] Unused import TooManyBuilds
    15: [W0611] Unused import IBranchSubscription
    11: [W0611] Unused import BranchCreatorNotOwner
    8: [W0611] Unused import CodeImportNotInReviewedState
    14: [W0611] Unused import IBranchMergeProposal
    8: [W0611] Unused import CodeImportAlreadyRunning
    11: [W0611] Unused import BranchCreatorNotMemberOfOwnerTeam
    16: [W0611] Unused import ICodeImport
    20: [W0611] Unused import ISourcePackageRecipe
    18: [W0611] Unused import ICodeReviewVoteReference
    11: [W0611] Unused import IBranchSet
    19: [W0611] Unused import IStaticDiff
    8: [W0611] Unused import BuildAlreadyPending
    11: [W0611] Unused import IBranch
    19: [W0611] Unused import IPreviewDiff
    8: [W0611] Unused import BranchMergeProposalExists
    11: [W0611] Unused import BranchExists
    21: [W0611] Unused import ISourcePackageRecipeBuild
    17: [W0611] Unused import ICodeReviewComment

[Brad Crittenden (bac)]

Hi Aaron,

Please add a docstring for test_requestBuildRejectRepeats.

The docstring for test_requestBuildRejectOverQuota is identical to the previous. c-n-p error?

Otherwise this branch looks really straightforward. Thanks.

review: Approve (code)

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2010-06-16 18:47:46 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2010-06-18 20:02:28 +0000
@@ -40,7 +40,7 @@
 from canonical.launchpad.webapp.sorting import sorted_dotted_numbers
 from canonical.widgets.itemswidgets import LabeledMultiCheckBoxWidget
 from lp.buildmaster.interfaces.buildbase import BuildStatus
-from lp.code.errors import ForbiddenInstruction
+from lp.code.errors import BuildAlreadyPending, ForbiddenInstruction
 from lp.code.interfaces.branch import NoSuchBranch
 from lp.code.interfaces.sourcepackagerecipe import (
     ISourcePackageRecipe, ISourcePackageRecipeSource, MINIMAL_RECIPE_TEXT)
@@ -219,11 +219,9 @@
     label = title
 
     @property
-    def next_url(self):
+    def cancel_url(self):
         return canonical_url(self.context)
 
-    cancel_url = next_url
-
     def validate(self, data):
         over_quota_distroseries = []
         for distroseries in data['distros']:
@@ -239,9 +237,18 @@
     def request_action(self, action, data):
         """User action for requesting a number of builds."""
         for distroseries in data['distros']:
-            self.context.requestBuild(
-                data['archive'], self.user, distroseries,
-                PackagePublishingPocket.RELEASE, manual=True)
+            try:
+                self.context.requestBuild(
+                    data['archive'], self.user, distroseries,
+                    PackagePublishingPocket.RELEASE, manual=True)
+            except BuildAlreadyPending, e:
+                self.setFieldError(
+                    'distros',
+                    'An identical build is already pending for %s.' %
+                    e.distroseries)
+                return
+        self.next_url = self.cancel_url
+
 
 
 class SourcePackageRecipeBuildNavigation(Navigation, FileNavigationMixin):

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2010-06-18 07:54:36 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2010-06-18 20:02:28 +0000
@@ -592,9 +592,10 @@
             supports_virtualized=True)
 
         recipe = self.makeRecipe()
-        [recipe.requestBuild(
-            self.ppa, self.chef, woody, PackagePublishingPocket.RELEASE)
-            for x in range(5)]
+        for x in range(5):
+            build = recipe.requestBuild(
+                self.ppa, self.chef, woody, PackagePublishingPocket.RELEASE)
+            removeSecurityProxy(build).buildstate = BuildStatus.FULLYBUILT
 
         browser = self.getViewBrowser(recipe, '+request-builds')
         browser.getControl('Woody').click()
@@ -602,6 +603,26 @@
         self.assertIn("You have exceeded today's quota for ubuntu woody.",
                 extract_text(find_main_content(browser.contents)))
 
+    def test_request_builds_rejects_duplicate(self):
+        """Over-quota build requests cause validation failures."""
+        woody = self.factory.makeDistroSeries(
+            name='woody', displayname='Woody',
+            distribution=self.ppa.distribution)
+        woody.nominatedarchindep = woody.newArch(
+            'i386', ProcessorFamily.get(1), False, self.factory.makePerson(),
+            supports_virtualized=True)
+
+        recipe = self.makeRecipe()
+        recipe.requestBuild(
+            self.ppa, self.chef, woody, PackagePublishingPocket.RELEASE)
+
+        browser = self.getViewBrowser(recipe, '+request-builds')
+        browser.getControl('Woody').click()
+        browser.getControl('Request builds').click()
+        self.assertIn(
+            "An identical build is already pending for ubuntu woody.",
+            extract_text(find_main_content(browser.contents)))
+
 
 class TestSourcePackageRecipeBuildView(BrowserTestCase):
     """Test behaviour of SourcePackageReciptBuildView."""

=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py	2010-06-11 05:05:52 +0000
+++ lib/lp/code/errors.py	2010-06-18 20:02:28 +0000
@@ -7,6 +7,7 @@
 __all__ = [
     'BadBranchMergeProposalSearchContext',
     'BadStateTransition',
+    'BuildAlreadyPending',
     'BranchMergeProposalExists',
     'CodeImportAlreadyRequested',
     'CodeImportAlreadyRunning',
@@ -113,13 +114,33 @@
         self.newest_supported = newest_supported
 
 
-class TooManyBuilds(Exception):
-    """A build was requested that exceeded the quota."""
+class RecipeBuildException(Exception):
 
-    def __init__(self, recipe, distroseries):
+    def __init__(self, recipe, distroseries, template):
         self.recipe = recipe
         self.distroseries = distroseries
-        msg = (
-            'You have exceeded your quota for recipe %s for distroseries %s'
-            % (self.recipe, distroseries))
+        msg = template % {'recipe': recipe, 'distroseries': distroseries}
         Exception.__init__(self, msg)
+
+
+class TooManyBuilds(RecipeBuildException):
+    """A build was requested that exceeded the quota."""
+
+    webservice_error(400)
+
+    def __init__(self, recipe, distroseries):
+        RecipeBuildException.__init__(
+            self, recipe, distroseries,
+            'You have exceeded your quota for recipe %(recipe)s for'
+            ' distroseries %(distroseries)s')
+
+
+class BuildAlreadyPending(RecipeBuildException):
+    """A build was requested when an identical build was already pending."""
+
+    webservice_error(400)
+
+    def __init__(self, recipe, distroseries):
+        RecipeBuildException.__init__(
+            self, recipe, distroseries,
+            'An identical build of this recipe is already pending.')

=== modified file 'lib/lp/code/interfaces/webservice.py'
--- lib/lp/code/interfaces/webservice.py	2010-04-23 02:33:32 +0000
+++ lib/lp/code/interfaces/webservice.py	2010-06-18 20:02:28 +0000
@@ -6,8 +6,8 @@
 # The exceptions are imported so that they can produce the special
 # status code defined by webservice_error when they are raised.
 from lp.code.errors import (
-    BranchMergeProposalExists, CodeImportAlreadyRunning,
-    CodeImportNotInReviewedState)
+    BuildAlreadyPending, BranchMergeProposalExists, CodeImportAlreadyRunning,
+    CodeImportNotInReviewedState, TooManyBuilds)
 from lp.code.interfaces.branch import (
     IBranch, IBranchSet, BranchCreatorNotMemberOfOwnerTeam,
     BranchCreatorNotOwner, BranchExists)
@@ -18,4 +18,5 @@
 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
 from lp.code.interfaces.diff import IDiff, IPreviewDiff, IStaticDiff
 from lp.code.interfaces.sourcepackagerecipe import ISourcePackageRecipe
-from lp.code.interfaces.sourcepackagerecipebuild import ISourcePackageRecipeBuild
+from lp.code.interfaces.sourcepackagerecipebuild import (
+    ISourcePackageRecipeBuild)

=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
--- lib/lp/code/model/sourcepackagerecipe.py	2010-06-16 19:47:27 +0000
+++ lib/lp/code/model/sourcepackagerecipe.py	2010-06-18 20:02:28 +0000
@@ -24,7 +24,8 @@
 from canonical.database.datetimecol import UtcDateTimeCol
 from canonical.launchpad.interfaces.lpstorm import IMasterStore, IStore
 
-from lp.code.errors import TooManyBuilds
+from lp.buildmaster.interfaces.buildbase import BuildStatus
+from lp.code.errors import BuildAlreadyPending, TooManyBuilds
 from lp.code.interfaces.sourcepackagerecipe import (
     ISourcePackageRecipe, ISourcePackageRecipeSource,
     ISourcePackageRecipeData)
@@ -193,6 +194,13 @@
             raise reject_reason
         if self.isOverQuota(requester, distroseries):
             raise TooManyBuilds(self, distroseries)
+        pending = IStore(self).find(SourcePackageRecipeBuild,
+            SourcePackageRecipeBuild.recipe_id == self.id,
+            SourcePackageRecipeBuild.distroseries_id == distroseries.id,
+            SourcePackageRecipeBuild.archive_id == archive.id,
+            SourcePackageRecipeBuild.buildstate == BuildStatus.NEEDSBUILD)
+        if pending.any() is not None:
+            raise BuildAlreadyPending(self, distroseries)
 
         build = getUtility(ISourcePackageRecipeBuildSource).new(distroseries,
             self, requester, archive)

=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2010-06-17 13:38:27 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2010-06-18 20:02:28 +0000
@@ -33,6 +33,7 @@
 from lp.buildmaster.model.buildbase import BuildBase
 from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.buildmaster.model.buildfarmjob import BuildFarmJobOldDerived
+from lp.code.errors import BuildAlreadyPending
 from lp.code.interfaces.sourcepackagerecipebuild import (
     ISourcePackageRecipeBuildJob, ISourcePackageRecipeBuildJobSource,
     ISourcePackageRecipeBuild, ISourcePackageRecipeBuildSource)
@@ -229,6 +230,8 @@
                     build = recipe.requestBuild(
                         recipe.daily_build_archive, recipe.owner,
                         distroseries, PackagePublishingPocket.RELEASE)
+                except BuildAlreadyPending:
+                    continue
                 except:
                     info = sys.exc_info()
                     errorlog.globalErrorUtility.raising(info)

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py	2010-06-17 13:38:27 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py	2010-06-18 20:02:28 +0000
@@ -28,10 +28,12 @@
 from lp.soyuz.interfaces.archive import (
     ArchiveDisabled, ArchivePurpose, CannotUploadToArchive,
     InvalidPocketForPPA)
+from lp.buildmaster.interfaces.buildbase import BuildStatus
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
 from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.code.errors import (
-    ForbiddenInstruction, TooManyBuilds, TooNewRecipeFormat)
+    BuildAlreadyPending, ForbiddenInstruction, TooManyBuilds,
+    TooNewRecipeFormat)
 from lp.code.interfaces.sourcepackagerecipe import (
     ISourcePackageRecipe, ISourcePackageRecipeSource, MINIMAL_RECIPE_TEXT)
 from lp.code.interfaces.sourcepackagerecipebuild import (
@@ -296,14 +298,41 @@
         series = list(recipe.distroseries)[0]
         archive = self.factory.makeArchive(owner=requester)
         def request_build():
-            recipe.requestBuild(archive, requester, series,
+            build = recipe.requestBuild(archive, requester, series,
                     PackagePublishingPocket.RELEASE)
+            removeSecurityProxy(build).buildstate = BuildStatus.FULLYBUILT
         [request_build() for num in range(5)]
         e = self.assertRaises(TooManyBuilds, request_build)
         self.assertIn(
             'You have exceeded your quota for recipe requester/myrecipe',
             str(e))
 
+    def test_requestBuildRejectRepeats(self):
+        """Reject build requests that are identical to pending builds."""
+        recipe = self.factory.makeSourcePackageRecipe()
+        series = list(recipe.distroseries)[0]
+        archive = self.factory.makeArchive(owner=recipe.owner)
+        old_build = recipe.requestBuild(archive, recipe.owner, series,
+                PackagePublishingPocket.RELEASE)
+        self.assertRaises(
+            BuildAlreadyPending, recipe.requestBuild, archive, recipe.owner,
+            series, PackagePublishingPocket.RELEASE)
+        # Varying archive allows build.
+        recipe.requestBuild(
+            self.factory.makeArchive(owner=recipe.owner), recipe.owner,
+            series, PackagePublishingPocket.RELEASE)
+        # Varying distroseries allows build.
+        new_distroseries = self.factory.makeDistroSeries()
+        new_distroseries.nominatedarchindep = new_distroseries.newArch(
+            'i386', ProcessorFamily.get(1), False, recipe.owner,
+            supports_virtualized=True)
+        recipe.requestBuild(archive, recipe.owner,
+            new_distroseries, PackagePublishingPocket.RELEASE)
+        # Changing status of old build allows new build.
+        removeSecurityProxy(old_build).buildstate = BuildStatus.FULLYBUILT
+        recipe.requestBuild(archive, recipe.owner, series,
+                PackagePublishingPocket.RELEASE)
+
     def test_sourcepackagerecipe_description(self):
         """Ensure that the SourcePackageRecipe has a proper description."""
         description = u'The whoozits and whatzits.'
@@ -675,6 +704,51 @@
             archive=archive, distroseries=distroseries,
             pocket=PackagePublishingPocket.RELEASE.title)
 
+    def test_requestBuildRejectRepeat(self):
+        """Build requests are rejected if already pending."""
+        person = self.factory.makePerson()
+        archive = self.factory.makeArchive(owner=person)
+        distroseries = self.factory.makeDistroSeries()
+        distroseries_i386 = distroseries.newArch(
+            'i386', ProcessorFamily.get(1), False, person,
+            supports_virtualized=True)
+        distroseries.nominatedarchindep = distroseries_i386
+
+        recipe, user, launchpad = self.makeRecipe(person)
+        distroseries = ws_object(launchpad, distroseries)
+        archive = ws_object(launchpad, archive)
+        recipe.requestBuild(
+            archive=archive, distroseries=distroseries,
+            pocket=PackagePublishingPocket.RELEASE.title)
+        e = self.assertRaises(Exception, recipe.requestBuild,
+            archive=archive, distroseries=distroseries,
+            pocket=PackagePublishingPocket.RELEASE.title)
+        self.assertIn('BuildAlreadyPending', str(e))
+
+    def test_requestBuildRejectOverQuota(self):
+        """Build requests are rejected if they exceed quota."""
+        person = self.factory.makePerson()
+        archives = [self.factory.makeArchive(owner=person) for x in range(6)]
+        distroseries = self.factory.makeDistroSeries()
+        distroseries_i386 = distroseries.newArch(
+            'i386', ProcessorFamily.get(1), False, person,
+            supports_virtualized=True)
+        distroseries.nominatedarchindep = distroseries_i386
+
+        recipe, user, launchpad = self.makeRecipe(person)
+        distroseries = ws_object(launchpad, distroseries)
+        for archive in archives[:-1]:
+            archive = ws_object(launchpad, archive)
+            recipe.requestBuild(
+                archive=archive, distroseries=distroseries,
+                pocket=PackagePublishingPocket.RELEASE.title)
+
+        archive = ws_object(launchpad, archives[-1])
+        e = self.assertRaises(Exception, recipe.requestBuild,
+            archive=archive, distroseries=distroseries,
+            pocket=PackagePublishingPocket.RELEASE.title)
+        self.assertIn('TooManyBuilds', str(e))
+
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2010-06-16 19:47:27 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2010-06-18 20:02:28 +0000
@@ -236,11 +236,28 @@
         self.assertEqual(list(recipe.distroseries), [build.distroseries])
 
     def test_makeDailyBuilds_clears_is_stale(self):
-        recipe = self.factory.makeSourcePackageRecipe(build_daily=True,
-            is_stale=True)
+        recipe = self.factory.makeSourcePackageRecipe(
+            build_daily=True, is_stale=True)
         SourcePackageRecipeBuild.makeDailyBuilds()[0]
         self.assertFalse(recipe.is_stale)
 
+    def test_makeDailyBuilds_skips_pending(self):
+        """When creating daily builds, skip ones that are already pending."""
+        recipe = self.factory.makeSourcePackageRecipe(
+            build_daily=True, is_stale=True)
+        first_distroseries = list(recipe.distroseries)[0]
+        recipe.requestBuild(
+            recipe.daily_build_archive, recipe.owner, first_distroseries,
+            PackagePublishingPocket.RELEASE)
+        second_distroseries = self.factory.makeDistroSeries()
+        second_distroseries.nominatedarchindep = second_distroseries.newArch(
+            'i386', ProcessorFamily.get(1), False, self.factory.makePerson(),
+            supports_virtualized=True)
+        recipe.distroseries.add(second_distroseries)
+        builds = SourcePackageRecipeBuild.makeDailyBuilds()
+        self.assertEqual(
+            [second_distroseries], [build.distroseries for build in builds])
+
     def test_getRecentBuilds(self):
         """Recent builds match the same person, series and receipe.