launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/set-recipe-text-bad-data into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #592513 Instructions don't have any information on the actual instruction text
  https://bugs.launchpad.net/bugs/592513
  #683321 Editing recipe oopses if invalid branch used
  https://bugs.launchpad.net/bugs/683321

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/set-recipe-text-bad-data/+merge/48853

Fix bug 683321, 'Editing recipe oopses if invalid branch used'. The real error was that the request_action for creating recipes dealt with four exceptions, but request_action for editing recipes only dealt with three. This means if the branch passed in was invalid, the exception wouldn't be caught and would leak up the stack, causing the OOPS.

I have fixed this by refactoring the exception handler to a parent class of both AddView and EditView, RecipeTextValidatorMixin. While performing this refactoring, I fixed bug 592513 by accident and removed the XXX.

I added a test that checks the error is raised correctly when a recipe is edited and is submitted with a branch that doesn't exist.
-- 
https://code.launchpad.net/~stevenk/launchpad/set-recipe-text-bad-data/+merge/48853
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/set-recipe-text-bad-data into lp:launchpad.

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2011-02-03 05:23:55 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2011-02-07 22:13:16 +0000
@@ -402,6 +402,29 @@
         except RecipeParseError, error:
             self.setFieldError('recipe_text', str(error))
 
+    def error_handler(self, callable, *args):
+        ret = None
+        try:
+            ret = callable(*args)
+            # So we can tell the difference between an error condition, and
+            # a callable that happens to return nothing.
+            if ret is None:
+                ret = ''
+        except TooNewRecipeFormat:
+            self.setFieldError(
+                'recipe_text',
+                'The recipe format version specified is not available.')
+        except ForbiddenInstructionError, e:
+            self.setFieldError(
+                'recipe_text',
+                'The bzr-builder instruction "%s" is not permitted '
+                'here.' % e.instruction_name)
+        except NoSuchBranch, e:
+            self.setFieldError(
+                'recipe_text', '%s is not a branch on Launchpad.' % e.name)
+        except PrivateBranchRecipe, e:
+            self.setFieldError('recipe_text', str(e))
+        return ret
 
 class RelatedBranchesWidget(Widget):
     """A widget to render the related branches for a recipe."""
@@ -512,37 +535,22 @@
 
     @action('Create Recipe', name='create')
     def request_action(self, action, data):
-        try:
-            owner = data['owner']
-            if data['use_ppa'] == CREATE_NEW:
-                ppa_name = data.get('ppa_name', None)
-                ppa = owner.createPPA(ppa_name)
-            else:
-                ppa = data['daily_build_archive']
-            source_package_recipe = getUtility(
-                ISourcePackageRecipeSource).new(
-                    self.user, owner, data['name'],
-                    data['recipe_text'], data['description'], data['distros'],
-                    ppa, data['build_daily'])
+        owner = data['owner']
+        if data['use_ppa'] == CREATE_NEW:
+            ppa_name = data.get('ppa_name', None)
+            ppa = owner.createPPA(ppa_name)
+        else:
+            ppa = data['daily_build_archive']
+        source_package_recipe = super(
+            SourcePackageRecipeAddView, self).error_handler(
+                getUtility(ISourcePackageRecipeSource).new, 
+                self.user, owner, data['name'],
+                data['recipe_text'], data['description'], data['distros'],
+                ppa, data['build_daily'])
+        if source_package_recipe is None:
+            return
+        else:
             Store.of(source_package_recipe).flush()
-        except TooNewRecipeFormat:
-            self.setFieldError(
-                'recipe_text',
-                'The recipe format version specified is not available.')
-            return
-        except ForbiddenInstructionError:
-            # XXX: bug=592513 We shouldn't be hardcoding "run" here.
-            self.setFieldError(
-                'recipe_text',
-                'The bzr-builder instruction "run" is not permitted here.')
-            return
-        except NoSuchBranch, e:
-            self.setFieldError(
-                'recipe_text', '%s is not a branch on Launchpad.' % e.name)
-            return
-        except PrivateBranchRecipe, e:
-            self.setFieldError('recipe_text', str(e))
-            return
 
         self.next_url = canonical_url(source_package_recipe)
 
@@ -625,25 +633,10 @@
         parser = RecipeParser(recipe_text)
         recipe = parser.parse()
         if self.context.builder_recipe != recipe:
-            try:
-                self.context.setRecipeText(recipe_text)
-                changed = True
-            except TooNewRecipeFormat:
-                self.setFieldError(
-                    'recipe_text',
-                    'The recipe format version specified is not available.')
-                return
-            except ForbiddenInstructionError:
-                # XXX: bug=592513 We shouldn't be hardcoding "run" here.
-                self.setFieldError(
-                    'recipe_text',
-                    'The bzr-builder instruction "run" is not permitted'
-                    ' here.')
-                return
-            except PrivateBranchRecipe, e:
-                self.setFieldError('recipe_text', str(e))
-                return
-
+            if super(SourcePackageRecipeEditView, self).error_handler(
+                self.context.setRecipeText, recipe_text) is None:
+                return
+            changed = True
 
         distros = data.pop('distros')
         if distros != self.context.distroseries:

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2011-02-03 05:23:55 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2011-02-07 22:13:16 +0000
@@ -986,6 +986,19 @@
             get_message_text(browser, 1),
             'Recipe may not refer to private branch: %s' % bzr_identity)
 
+    def test_edit_recipe_no_branch(self):
+        # If a user tries to set a source package recipe to use a branch
+        # that isn't registred, they will get an error.
+        recipe = self.factory.makeSourcePackageRecipe(owner=self.user)
+        no_branch_recipe_text = recipe.recipe_text[:-4]
+        expected_name = recipe.base_branch.unique_name[:-3]
+        browser = self.getViewBrowser(recipe, '+edit')
+        browser.getControl('Recipe text').value = no_branch_recipe_text
+        browser.getControl('Update Recipe').click()
+        self.assertEqual(
+            get_message_text(browser, 1),
+            'lp://dev/%s is not a branch on Launchpad.' % expected_name)
+
     def _test_edit_recipe_with_no_related_branches(self, recipe):
         # The Related Branches section should not appear if there are no
         # related branches.