Merge lp:~jcsackett/launchpad/branch-delete-api-702620 into lp:launchpad

[j.c.sackett]

[r=leonardr][bug=702620] Expose CannotDeleteBranch exception for the webservice on DELETE

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

[Robert Collins (lifeless)]

So this will solve 1/2 the bug: it won't oops. However it won't let
scripts that want to delete branches do so - some of the artifacts
cannot be deleted from the web API or UI except by deleting the branch
in the web UI.

I realise it was a conscious decision, but what was the rationale? Why
does it still make sense?

-Rob

[Leonard Richardson (leonardr)]

review: Approve

[j.c.sackett (jcsackett)]

> So this will solve 1/2 the bug: it won't oops. However it won't let
> scripts that want to delete branches do so - some of the artifacts
> cannot be deleted from the web API or UI except by deleting the branch
> in the web UI.

Are there artifacts that can't be deleted? Which ones? The bug states that it all can be deleted manually, it's just a bit of a pain.

If there are things that can't be deleted, perhaps the appropriate step is to change the webservice so those artifacts can be deleted.

> I realise it was a conscious decision, but what was the rationale? Why
> does it still make sense?

As I understand it, it's because we don't want want someone to delete stacked_on branches &c without being warned, and there's no sane way to provide a method in the service that warns and requires confirmation. We could add a prompt with input() or something, but that strikes me as faintly ridiculous at best.

[Leonard Richardson (leonardr)]

This is very blue-sky, but one thing I've considered in this case is responding to a DELETE request by sending back a list of the documents that would have to be deleted before this document can be deleted. On the client side you could then prompt the user "deleting this object will delete 100 other objects", or go ahead and recursively delete the other documents, or decide not to delete that document after all.

It's difficult to come up with a general solution beyond adding the equivalent of "rm -rf", and I'm open to just making that possible.

=== 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-02-01 15:31:14 +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/model/branch.py'
--- lib/lp/code/model/branch.py	2011-01-20 19:07:26 +0000
+++ lib/lp/code/model/branch.py	2011-02-01 15:31:14 +0000
@@ -15,6 +15,7 @@
 
 from bzrlib import urlutils
 from bzrlib.revision import NULL_REVISION
+from lazr.restful.error import expose
 import pytz
 from sqlobject import (
     BoolCol,
@@ -1025,7 +1026,12 @@
 
     def destroySelfBreakReferences(self):
         """See `IBranch`."""
-        return self.destroySelf(break_references=True)
+        try:
+            return self.destroySelf(break_references=True)
+        except CannotDeleteBranch, e:
+            # Reraise and expose exception here so that the webservice_error
+            # is propogated.
+            raise expose(CannotDeleteBranch(e.message))
 
     def _deleteBranchSubscriptions(self):
         """Delete subscriptions for this branch prior to deleting branch."""

=== 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-02-01 15:31:14 +0000
@@ -1,10 +1,8 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-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."""
 
-import unittest
-
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -377,7 +375,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-02-01 15:31:14 +0000
@@ -0,0 +1,61 @@
+# 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
+
+from zope.component import getUtility
+
+from lazr.restfulclient.errors import HTTPError
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.code.interfaces.branch import IBranchSet
+from lp.testing import (
+    launchpadlib_for,
+    login_person,
+    logout,
+    TestCaseWithFactory,
+    )
+
+
+class TestBranchDeletes(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestBranchDeletes, self).setUp()
+        self.branch_owner = self.factory.makePerson(name='jimhenson')
+        self.branch = self.factory.makeBranch(
+            owner=self.branch_owner,
+            product=self.factory.makeProduct(name='fraggle'),
+            name='rock')
+        self.lp = launchpadlib_for("test", self.branch.owner.name)
+
+    def test_delete_branch_without_artifacts(self):
+        # A branch unencumbered by links or stacked branches deletes.
+        target_branch = self.lp.branches.getByUniqueName(
+            unique_name='~jimhenson/fraggle/rock')
+        target_branch.lp_delete()
+
+        login_person(self.branch_owner)
+        branch_set = getUtility(IBranchSet)
+        self.assertIs(
+            None,
+            branch_set.getByUniqueName('~jimhenson/fraggle/rock'))
+
+    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.
+        login_person(self.branch_owner)
+        stacked_branch = self.factory.makeBranch(
+            stacked_on=self.branch,
+            owner=self.branch_owner)
+        logout()
+        target_branch = self.lp.branches.getByUniqueName(
+            unique_name='~jimhenson/fraggle/rock')
+        api_error = self.assertRaises(
+            HTTPError,
+            target_branch.lp_delete)
+        self.assertIn('Cannot delete', api_error.content)
+        self.assertEqual(httplib.BAD_REQUEST, api_error.response.status)