launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/branch-delete-api-702620 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #702620 Can't delete branches from the API that can be deleted from the web UI
  https://bugs.launchpad.net/bugs/702620

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/branch-delete-api-702620/+merge/47449

Summary
=======

Calling a DELETE across the webservice on a branch leads to an OOPS when the branch has linked artifacts (e.g. stacked branches) that cause CannotDeleteBranch to be raised. CannotDeleteBranch should be raised over the webservice instead of the server error.

Preimplementation
=================

Spoke with Curtis Hovey about the reasons for the DELETE failing, and if we should have the DELETE just whack the linked objects. Not having the DELETE hit the linked objects (and providing no way to do so) was a conscious decision, so just exposing the error is correct.

Spoke with Leonard Richardson about various oddities in exposing this particular error, as the exception is raised in such a way that just adding webservice_error was not sufficient.

Proposed Fix
============

Add webservice_error to CannotDeleteBranch.

Implementation
==============

lib/lp/code/errors.py
---------------------

Add webservice_error to CannotDeleteBranch

lib/lp/code/model/branch.py
---------------------------

In deleteSelfBreakReferences, catch the CannotDeleteBranch exception and re-raise with expose.

lib/lp/code/tests/test_branch.py
--------------------------------

Drive by fix; unittest and testsuite boilerplate removed, b/c it's no longer needed.

lib/lp/code/tests/test_branch_webservice.py
-------------------------------------------

Test case added for branch deletion over webservice.

Tests
=====

bin/test -t TestBranchDeletes

Demo & QA
=========

Attempt to delete a branch with branches stacked on it via launchpadlib. You should get a 400, rather than a 500 error.

Lint
====

make lint output:

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/errors.py
  lib/lp/code/model/branch.py
  lib/lp/code/tests/test_branch.py
  lib/lp/code/tests/test_branch_webservice.py



-- 
https://code.launchpad.net/~jcsackett/launchpad/branch-delete-api-702620/+merge/47449
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/branch-delete-api-702620 into lp:launchpad.

=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py	2011-01-19 00:10:48 +0000
+++ lib/lp/code/errors.py	2011-01-25 19:36:11 +0000
@@ -40,6 +40,8 @@
     'WrongBranchMergeProposal',
 ]
 
+import httplib
+
 from lazr.restful.declarations import webservice_error
 
 from lp.app.errors import NameLookupFailed
@@ -87,6 +89,7 @@
 
 class CannotDeleteBranch(Exception):
     """The branch cannot be deleted at this time."""
+    webservice_error(httplib.BAD_REQUEST)
 
 
 class BranchCreationForbidden(BranchCreationException):

=== modified file 'lib/lp/code/tests/test_branch.py'
--- lib/lp/code/tests/test_branch.py	2010-10-04 19:50:45 +0000
+++ lib/lp/code/tests/test_branch.py	2011-01-25 19:36:11 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for methods of Branch and BranchSet."""
@@ -377,7 +377,3 @@
         # not work for private branches.
         branch = self.factory.makeAnyBranch()
         self.assertRaises(AssertionError, branch.composePublicURL, 'https')
-
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)

=== added file 'lib/lp/code/tests/test_branch_webservice.py'
--- lib/lp/code/tests/test_branch_webservice.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/tests/test_branch_webservice.py	2011-01-25 19:36:11 +0000
@@ -0,0 +1,48 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+import httplib
+import transaction
+
+from zope.security.management import endInteraction
+
+from lazr.restfulclient.errors import HTTPError
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.testing import (
+    launchpadlib_for,
+    login_person,
+    logout,
+    TestCaseWithFactory,
+    )
+
+
+class TestBranchDeletes(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestBranchDeletes, self).setUp()
+        self.branch = self.factory.makeBranch(
+            product=self.factory.makeProduct(),
+            name='fraggle')
+        self.lp = launchpadlib_for("test", self.branch.owner.name)
+        transaction.commit()
+
+    def test_delete_branch_without_artifacts(self):
+        # A branch unencumbered by links or stacked branches deletes.
+        target_branch = self.lp.branches.getByUniqueName(unique_name='fraggle')
+        target_branch.delete()
+
+    def test_delete_branch_with_stacked_branch_errors(self):
+        # When trying to delete a branch that cannot be deleted, the
+        # error is raised across the webservice instead of oopsing.
+        stacked_branch = self.factory.makeBranch(stacked_on=self.branch)
+        target_branch = self.lp.branches.getByUniqueName(branch.name)
+        api_error = self.assertRaises(
+            HTTPError,
+            branch.delete)
+        self.assertIn('Cannot delete', api_error.content)
+        self.assertEqual(httplib.FORBIDDEN, api_error.response.status)