Merge lp:~abentley/launchpad/recipe-errors into lp:launchpad

[Aaron Bentley]

Handle bad branch for recipe editing

= Summary =
Fix bug #683321: Editing recipe oopses if invalid branch used

== Proposed fix ==
Unify error handling into a context manager

== Pre-implementation notes ==
None

== Implementation details ==
With this context manager, two operations (setRecipeText and
ISourcePackageRecipeSource).new) that produce the same exceptions have them
handled the same way. This ensures that adding and editing recipes has uniform
exception handling.

== Tests ==
bin/test -t edit_recipe_bad_base_branch sourcepackagerecipe

== Demo and Q/A ==
Create a recipe. Edit it to set the branch to an invalid value. You should
get an error message, not an OOPS.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/browser/sourcepackagerecipe.py
  lib/lp/code/browser/tests/test_sourcepackagerecipe.py

./lib/lp/code/browser/sourcepackagerecipe.py
     338: Line exceeds 78 characters.
./lib/lp/code/browser/tests/test_sourcepackagerecipe.py
     370: E501 line too long (80 characters)
     752: E501 line too long (85 characters)
     766: E501 line too long (89 characters)
     778: E501 line too long (85 characters)
     793: E501 line too long (89 characters)
     809: E501 line too long (85 characters)
     370: Line exceeds 78 characters.
     752: Line exceeds 78 characters.
     766: Line exceeds 78 characters.
     778: Line exceeds 78 characters.
     793: Line exceeds 78 characters.
     796: Line exceeds 78 characters.
     809: Line exceeds 78 characters.

[Jeroen T. Vermeulen (jtv)]

This is inventive and thus a little shocking. I suppose the real problem is that the textual representations of these exception types are inadequate. Otherwise, all you needed here would be a tuple of exception types that represent input errors!

If you had such a tuple and usable error strings, you'd be able to do this in both use-cases instead of creating a context manager:

    try:

        pass # (or more likely, something useful)

    except recipe_input_errors, e:
        self.setFieldError('recipe_text', str(e))

But I'm sure you would have done that if you had the option of improving the text representations of these exception types.

Still, I can't help feeling that the solution is not the best one you could get here: on the one hand your context manager interacts with the view, which actually fits better in the calling code. On the other it acts as an overlay to a number of exception classes in a "poor man's dynamic dispatch" pattern. And on the gripping hand, it makes a three-way control-flow decision (raise, return, continue) that context managers weren't designed for. If that's one hand too many, well, that's my point!

The context-manager API forces you to let the control-flow decision spill back into the calling code. Which obscures control flow: a "raise" happens in the "with" body, an "except" may happen in the context manager, an exception may be re-raised in the context-manager language support based on the exit handler's return value, and then the caller either returns or continues based on a variable set in the exit handler. That's a lot for a reader to follow when trying to figure out what the view code will do—or what effects a prospective code change will have. Be nice to the future hacker who's in unfamiliar with the code and in a hurry to fix something: it may be you two years from now.

As either an alternative to the context manager or a refinement of it, have you considered extracting just the identification and representation of the input errors?

    try:

        pass # (view work here)

    except Exception, e:
        input_error = represent_recipe_input_error(e)
        if input_error is None:
            # Unexpected problem.
            raise
        else:
            # Known problem with the input.
            self.setFieldError('recipe_text', str(e))
            return

Even in combination with the context manager, separating the representation of the errors would make the control flow more concise.

If you do prefer to continue with the context-manager approach, I would ask for a few changes:
 * Unit-test the context manager.
 * Do exception representation in exception classes where possible.
 * Rename return_now so it doesn't make assumptions about the calling code.
 * Address the XXX one way or another.

Thanks for including "lint" output, by the way. Fixing the lint after review helps keep the diff clean.

review: Needs Fixing (code)

Unmerged revisions

12002. By Aaron Bentley on 2010-11-30

Unify error handling for recipe text

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2010-11-28 23:32:25 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2010-11-30 20:43:10 +0000
@@ -315,6 +315,38 @@
         except RecipeParseError, error:
             self.setFieldError('recipe_text', str(error))
 
+    class HandleRecipeErrors():
+        """Handle errors that may be encountered setting a new recipe text."""
+
+        def __init__(self, view):
+            self.view = view
+            self.return_now = False
+
+        def __enter__(self):
+            return self
+
+        def __exit__(self, exc_type, exc_val, exc_tb):
+            if exc_type is None:
+                return True
+            elif isinstance(exc_val, TooNewRecipeFormat):
+                self.view.setFieldError(
+                    'recipe_text',
+                    'The recipe format version specified is not available.')
+            elif isinstance(exc_val, ForbiddenInstructionError):
+                self.view.setFieldError(
+                    'recipe_text',
+                    'The bzr-builder instruction "run" is not permitted here.')
+            elif isinstance(exc_val, NoSuchBranch):
+                self.view.setFieldError(
+                    'recipe_text', '%s is not a branch on Launchpad.' %
+                    exc_val.name)
+            elif isinstance(exc_val, PrivateBranchRecipe):
+                self.view.setFieldError('recipe_text', str(exc_val))
+            else:
+                return False
+            self.return_now = True
+            return True
+
 
 class SourcePackageRecipeAddView(RecipeTextValidatorMixin, LaunchpadFormView):
     """View for creating Source Package Recipes."""
@@ -344,30 +376,14 @@
 
     @action('Create Recipe', name='create')
     def request_action(self, action, data):
-        try:
+        with self.HandleRecipeErrors(self) as handler:
             source_package_recipe = getUtility(
                 ISourcePackageRecipeSource).new(
                     self.user, data['owner'], data['name'],
                     data['recipe_text'], data['description'], data['distros'],
                     data['daily_build_archive'], data['build_daily'])
             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))
+        if handler.return_now:
             return
 
         self.next_url = canonical_url(source_package_recipe)
@@ -437,25 +453,11 @@
         parser = RecipeParser(recipe_text)
         recipe = parser.parse()
         if self.context.builder_recipe != recipe:
-            try:
+            with self.HandleRecipeErrors(self) as handler:
                 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 handler.return_now:
+                return
 
         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	2010-11-28 22:38:48 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2010-11-30 20:43:10 +0000
@@ -557,6 +557,17 @@
             extract_text(find_tags_by_class(browser.contents, 'message')[1]),
             'The bzr-builder instruction "run" is not permitted here.')
 
+    def test_edit_recipe_bad_base_branch(self):
+        # If a user tries to create source package recipe with a bad base
+        # branch location, they should get an error.
+        recipe = self.factory.makeSourcePackageRecipe(owner=self.chef)
+        browser = self.getViewBrowser(recipe, '+edit')
+        browser.getControl('Recipe text').value = (
+            MINIMAL_RECIPE_TEXT % 'foo')
+        browser.getControl('Update Recipe').click()
+        self.assertEqual(
+            get_message_text(browser, 1), 'foo is not a branch on Launchpad.')
+
     def test_edit_recipe_format_too_new(self):
         # If the recipe's format version is too new, we should notify the
         # user.