Merge lp:~abentley/launchpad/push-creates-package into lp:launchpad

[Aaron Bentley]

Push creates source package names.

= Summary =
Fix bug #386596: pushing to a packaging branch can't create a new package

== Proposed fix ==
Support creating new source package names in createBranch

== Pre-implementation notes ==
None

== Implementation details ==
In the xmlrpc-backed version, catch NoSuchSourcePackageName and handle it by creating the name and doing a recursive call to createBranch.

In the memory-backed version, create the sourcepackagename if not already present.

In both cases, SourcePackageNameSet.new raises InvalidName if valid_name returns False. Accordingly, InvalidName moved from person.py to errors.py.

createBranch translates InvalidName to the new InvalidSourcePackageName fault.

The vfs transport converts InvalidSourcePackageName into PermissionDenied.

== Tests ==
bin/test -t test_createBranch_invalid_package_name -t test_createBranch_missing_sourcepackagename -t test_push_new_branch_of_non_existant_source_package_name -t t test_createBranch_invalid_package_name.

== Demo and Q/A ==

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/xmlrpc/faults.py
  lib/lp/registry/errors.py
  lib/lp/code/xmlrpc/codehosting.py
  lib/lp/code/xmlrpc/tests/test_codehosting.py
  lib/lp/code/tests/helpers.py
  lib/canonical/launchpad/xmlrpc/configure.zcml
  lib/lp/codehosting/vfs/branchfs.py
  lib/lp/registry/model/sourcepackagename.py
  lib/lp/codehosting/inmemory.py
  lib/lp/registry/tests/test_sourcepackagename.py
  lib/lp/codehosting/vfs/tests/test_branchfs.py
  lib/lp/codehosting/tests/test_acceptance.py
  lib/lp/registry/interfaces/person.py
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_person.py

[Abel Deuring (adeuring)]

review: Approve (code)

=== modified file 'lib/canonical/launchpad/xmlrpc/configure.zcml'
--- lib/canonical/launchpad/xmlrpc/configure.zcml	2010-11-08 14:16:17 +0000
+++ lib/canonical/launchpad/xmlrpc/configure.zcml	2011-08-15 13:18:36 +0000
@@ -207,4 +207,7 @@
   <class class="canonical.launchpad.xmlrpc.faults.AccountSuspended">
     <require like_class="xmlrpclib.Fault" />
   </class>
+  <class class="canonical.launchpad.xmlrpc.faults.InvalidSourcePackageName">
+    <require like_class="xmlrpclib.Fault" />
+  </class>
 </configure>

=== modified file 'lib/canonical/launchpad/xmlrpc/faults.py'
--- lib/canonical/launchpad/xmlrpc/faults.py	2010-12-12 23:09:36 +0000
+++ lib/canonical/launchpad/xmlrpc/faults.py	2011-08-15 13:18:36 +0000
@@ -22,6 +22,7 @@
     'InvalidBranchUniqueName',
     'InvalidProductIdentifier',
     'InvalidBranchUrl',
+    'InvalidSourcePackageName',
     'OopsOccurred',
     'NoBranchWithID',
     'NoLinkedBranch',
@@ -476,3 +477,13 @@
     def __init__(self, server_op, oopsid):
         LaunchpadFault.__init__(self, server_op=server_op, oopsid=oopsid)
         self.oopsid = oopsid
+
+
+class InvalidSourcePackageName(LaunchpadFault):
+
+    error_code = 390
+    msg_template = ("%(name)s is not a valid source package name.")
+
+    def __init__(self, name):
+        self.name = name
+        LaunchpadFault.__init__(self, name=name)

=== modified file 'lib/lp/code/tests/helpers.py'
--- lib/lp/code/tests/helpers.py	2011-08-03 11:00:11 +0000
+++ lib/lp/code/tests/helpers.py	2011-08-15 13:18:36 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 __all__ = [
     'add_revision_to_branch',
+    'get_non_existant_source_package_branch_unique_name',
     'make_erics_fooix_project',
     'make_linked_package_branch',
     'make_merge_proposal_without_reviewers',
@@ -313,3 +314,16 @@
     for vote in proposal.votes:
         removeSecurityProxy(vote).destroySelf()
     return proposal
+
+
+def get_non_existant_source_package_branch_unique_name(owner, factory):
+    """Return the unique name for a non-existanct source package branch.
+
+    Neither the branch nor the source package name will exist.
+    """
+    distroseries = factory.makeDistroSeries()
+    source_package = factory.getUniqueString('source-package')
+    branch = factory.getUniqueString('branch')
+    return '~%s/%s/%s/%s/%s' % (
+        owner, distroseries.distribution.name, distroseries.name,
+        source_package, branch)

=== modified file 'lib/lp/code/xmlrpc/codehosting.py'
--- lib/lp/code/xmlrpc/codehosting.py	2011-05-25 20:16:58 +0000
+++ lib/lp/code/xmlrpc/codehosting.py	2011-08-15 13:18:36 +0000
@@ -64,6 +64,10 @@
     LAUNCHPAD_SERVICES,
     )
 from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
+from lp.registry.errors import (
+    InvalidName,
+    NoSuchSourcePackageName,
+    )
 from lp.registry.interfaces.person import (
     IPersonSet,
     NoSuchPerson,
@@ -72,6 +76,7 @@
     InvalidProductName,
     NoSuchProduct,
     )
+from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
 from lp.services.scripts.interfaces.scriptactivity import IScriptActivitySet
 from lp.services.utils import iter_split
 
@@ -220,6 +225,12 @@
             except NoSuchProduct, e:
                 return faults.NotFound(
                     "Project '%s' does not exist." % e.name)
+            except NoSuchSourcePackageName as e:
+                try:
+                    getUtility(ISourcePackageNameSet).new(e.name)
+                except InvalidName:
+                    return faults.InvalidSourcePackageName(e.name)
+                return self.createBranch(login_id, branch_path)
             except NameLookupFailed, e:
                 return faults.NotFound(str(e))
             try:

=== modified file 'lib/lp/code/xmlrpc/tests/test_codehosting.py'
--- lib/lp/code/xmlrpc/tests/test_codehosting.py	2011-08-03 11:00:11 +0000
+++ lib/lp/code/xmlrpc/tests/test_codehosting.py	2011-08-15 13:18:36 +0000
@@ -418,19 +418,39 @@
         message = "No such distribution series: 'ningnangnong'."
         self.assertEqual(faults.NotFound(message), fault)
 
+    def test_createBranch_missing_sourcepackagename(self):
+        # If createBranch is called with the path to a missing source
+        # package, it will create the source package.
+        owner = self.factory.makePerson()
+        distroseries = self.factory.makeDistroSeries()
+        branch_name = self.factory.getUniqueString()
+        unique_name = '/~%s/%s/%s/ningnangnong/%s' % (
+            owner.name, distroseries.distribution.name, distroseries.name,
+            branch_name)
+        branch_id = self.codehosting_api.createBranch(
+            owner.id, escape(unique_name))
+        login(ANONYMOUS)
+        branch = self.branch_lookup.get(branch_id)
+        self.assertEqual(owner, branch.owner)
+        self.assertEqual(distroseries, branch.distroseries)
+        self.assertEqual(
+            'ningnangnong', branch.sourcepackagename.name)
+        self.assertEqual(branch_name, branch.name)
+        self.assertEqual(owner, branch.registrant)
+        self.assertEqual(BranchType.HOSTED, branch.branch_type)
+
     def test_createBranch_invalid_sourcepackagename(self):
-        # If createBranch is called with the path to an invalid source
-        # package, it will return a Fault saying so.
+        # If createBranch is called with an invalid path, it will fault.
         owner = self.factory.makePerson()
         distroseries = self.factory.makeDistroSeries()
         branch_name = self.factory.getUniqueString()
-        unique_name = '/~%s/%s/%s/ningnangnong/%s' % (
+        unique_name = '/~%s/%s/%s/ningn%%20angnong/%s' % (
             owner.name, distroseries.distribution.name, distroseries.name,
             branch_name)
         fault = self.codehosting_api.createBranch(
             owner.id, escape(unique_name))
-        message = "No such source package: 'ningnangnong'."
-        self.assertEqual(faults.NotFound(message), fault)
+        self.assertEqual(
+            faults.InvalidSourcePackageName('ningn%20angnong'), fault)
 
     def test_createBranch_using_branch_alias(self):
         # Branches can be created using the branch alias and the full unique

=== modified file 'lib/lp/codehosting/inmemory.py'
--- lib/lp/codehosting/inmemory.py	2011-08-03 11:00:11 +0000
+++ lib/lp/codehosting/inmemory.py	2011-08-15 13:18:36 +0000
@@ -25,7 +25,10 @@
 
 from canonical.database.constants import UTC_NOW
 from canonical.launchpad.xmlrpc import faults
-from lp.app.validators import LaunchpadValidationError
+from lp.app.validators import (
+    LaunchpadValidationError,
+    )
+from lp.app.validators.name import valid_name
 from lp.code.bzr import (
     BranchFormat,
     ControlFormat,
@@ -51,6 +54,7 @@
     ProductBranchTarget,
     )
 from lp.code.xmlrpc.codehosting import datetime_from_tuple
+from lp.registry.errors import InvalidName
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.utils import iter_split
 from lp.services.xmlrpc import LaunchpadFault
@@ -194,6 +198,14 @@
         self.distroseries._linked_branches[self, pocket] = branch
 
 
+class SourcePackageNameSet(ObjectSet):
+
+    def new(self, name_string):
+        if not valid_name(name_string):
+            raise InvalidName(name_string)
+        return self._add(FakeSourcePackageName(name_string))
+
+
 @adapter(FakeSourcePackage)
 @implementer(IBranchTarget)
 def fake_source_package_to_branch_target(fake_package):
@@ -454,9 +466,7 @@
         return distroseries
 
     def makeSourcePackageName(self):
-        sourcepackagename = FakeSourcePackageName(self.getUniqueString())
-        self._sourcepackagename_set._add(sourcepackagename)
-        return sourcepackagename
+        return self._sourcepackagename_set.new(self.getUniqueString())
 
     def makeSourcePackage(self, distroseries=None, sourcepackagename=None):
         if distroseries is None:
@@ -659,9 +669,12 @@
             sourcepackagename = self._sourcepackagename_set.getByName(
                 data['sourcepackagename'])
             if sourcepackagename is None:
-                raise faults.NotFound(
-                    "No such source package: '%s'."
-                    % (data['sourcepackagename'],))
+                try:
+                    sourcepackagename = self._sourcepackagename_set.new(
+                        data['sourcepackagename'])
+                except InvalidName:
+                    raise faults.InvalidSourcePackageName(
+                        data['sourcepackagename'])
             sourcepackage = self._factory.makeSourcePackage(
                 distroseries, sourcepackagename)
         else:
@@ -890,7 +903,7 @@
         self._product_set = ObjectSet()
         self._distribution_set = ObjectSet()
         self._distroseries_set = ObjectSet()
-        self._sourcepackagename_set = ObjectSet()
+        self._sourcepackagename_set = SourcePackageNameSet()
         self._factory = FakeObjectFactory(
             self._branch_set, self._person_set, self._product_set,
             self._distribution_set, self._distroseries_set,

=== modified file 'lib/lp/codehosting/tests/test_acceptance.py'
--- lib/lp/codehosting/tests/test_acceptance.py	2011-08-12 14:57:27 +0000
+++ lib/lp/codehosting/tests/test_acceptance.py	2011-08-15 13:18:36 +0000
@@ -35,6 +35,9 @@
 from lp.code.enums import BranchType
 from lp.code.interfaces.branch import IBranchSet
 from lp.code.interfaces.branchnamespace import get_branch_namespace
+from lp.code.tests.helpers import (
+    get_non_existant_source_package_branch_unique_name,
+    )
 from lp.codehosting import (
     get_bzr_path,
     get_BZR_PLUGIN_PATH_for_subprocess,
@@ -56,7 +59,7 @@
 
 
 class ForkingServerForTests(object):
-    """Map starting/stopping a LPForkingService with setUp() and tearDown()."""
+    """Map starting/stopping a LPForkingService to setUp() and tearDown()."""
 
     def __init__(self):
         self.process = None
@@ -68,8 +71,8 @@
         env = os.environ.copy()
         env['BZR_PLUGIN_PATH'] = BZR_PLUGIN_PATH
         # TODO: We probably want to use a random disk path for
-        #       forking_daemon_socket, but we need to update config so that the
-        #       CodeHosting service can find it.
+        #       forking_daemon_socket, but we need to update config so that
+        #       the CodeHosting service can find it.
         #       The main problem is that CodeHostingTac seems to start a tac
         #       server directly from the disk configs, and doesn't use the
         #       in-memory config. So we can't just override the memory
@@ -83,14 +86,14 @@
         self.process = process
         # Wait for it to indicate it is running
         # The first line should be "Preloading" indicating it is ready
-        preloading_line = process.stderr.readline()
+        process.stderr.readline()
         # The next line is the "Listening on socket" line
-        socket_line = process.stderr.readline()
+        process.stderr.readline()
         # Now it is ready
 
     def tearDown(self):
-        # SIGTERM is the graceful exit request, potentially we could wait a bit
-        # and send something stronger?
+        # SIGTERM is the graceful exit request, potentially we could wait a
+        # bit and send something stronger?
         if self.process is not None and self.process.poll() is None:
             os.kill(self.process.pid, signal.SIGTERM)
             self.process.wait()
@@ -102,7 +105,6 @@
             os.remove(self.socket_path)
 
 
-
 class SSHServerLayer(ZopelessAppServerLayer):
 
     _tac_handler = None
@@ -611,6 +613,15 @@
             self.local_branch_path, remote_url,
             ['Permission denied:', 'Transport operation not possible:'])
 
+    def test_push_new_branch_of_non_existant_source_package_name(self):
+        ZopelessAppServerLayer.txn.begin()
+        unique_name = get_non_existant_source_package_branch_unique_name(
+            'testuser', self.factory)
+        ZopelessAppServerLayer.txn.commit()
+        remote_url = self.getTransportURL(unique_name)
+        self.push(self.local_branch_path, remote_url)
+        self.assertBranchesMatch(self.local_branch_path, remote_url)
+
     def test_can_push_loom_branch(self):
         # We can push and pull a loom branch.
         self.makeLoomBranchAndTree('loom')
@@ -696,7 +707,6 @@
         urllib2.urlopen(web_status_url)
 
 
-
 def make_server_tests(base_suite, servers):
     from lp.codehosting.tests.helpers import (
         CodeHostingTestProviderAdapter)

=== modified file 'lib/lp/codehosting/vfs/branchfs.py'
--- lib/lp/codehosting/vfs/branchfs.py	2011-08-06 17:37:47 +0000
+++ lib/lp/codehosting/vfs/branchfs.py	2011-08-15 13:18:36 +0000
@@ -46,9 +46,6 @@
 __metaclass__ = type
 __all__ = [
     'AsyncLaunchpadTransport',
-    'BadUrlLaunchpad',
-    'BadUrlScheme',
-    'BadUrlSsh',
     'branch_id_to_path',
     'DirectDatabaseLaunchpadServer',
     'get_lp_server',
@@ -599,7 +596,8 @@
             # exist. You may supply --create-prefix to create all leading
             # parent directories", which is just misleading.
             fault = trap_fault(
-                fail, faults.NotFound, faults.PermissionDenied)
+                fail, faults.NotFound, faults.PermissionDenied,
+                faults.InvalidSourcePackageName)
             faultString = fault.faultString
             if isinstance(faultString, unicode):
                 faultString = faultString.encode('utf-8')
@@ -747,4 +745,3 @@
         DeferredBlockingProxy(codehosting_client), user_id, branch_transport,
         seen_new_branch_hook)
     return lp_server
-

=== modified file 'lib/lp/codehosting/vfs/tests/test_branchfs.py'
--- lib/lp/codehosting/vfs/tests/test_branchfs.py	2011-08-12 05:03:42 +0000
+++ lib/lp/codehosting/vfs/tests/test_branchfs.py	2011-08-15 13:18:36 +0000
@@ -49,7 +49,6 @@
 from twisted.internet import defer
 
 from canonical.launchpad.webapp import errorlog
-from canonical.launchpad.webapp.errorlog import ErrorReportingUtility
 from canonical.testing.layers import (
     ZopelessDatabaseLayer,
     )
@@ -798,6 +797,17 @@
             errors.PermissionDenied, message,
             transport.mkdir, '~%s/%s/some-name' % (person.name, product.name))
 
+    def test_createBranch_invalid_package_name(self):
+        # When createBranch raises faults.InvalidSourcePackageName, the
+        # transport should translate this to a PermissionDenied exception
+        transport = self.getTransport()
+        series = self.factory.makeDistroSeries()
+        unique_name = '~%s/%s/%s/spaced%%20name/branch' % (
+            self.requester.name, series.distribution.name, series.name)
+        return self.assertFiresFailureWithSubstring(
+            errors.PermissionDenied, "is not a valid source package name",
+            transport.mkdir, unique_name)
+
     def test_rmdir(self):
         transport = self.getTransport()
         self.assertFiresFailure(
@@ -1155,7 +1165,8 @@
         self.addCleanup(memory_server.stop_server)
         return memory_server
 
-    def _setUpLaunchpadServer(self, user_id, codehosting_api, backing_transport):
+    def _setUpLaunchpadServer(self, user_id, codehosting_api,
+                              backing_transport):
         server = LaunchpadServer(
             XMLRPCWrapper(codehosting_api), user_id, backing_transport)
         server.start_server()

=== modified file 'lib/lp/registry/errors.py'
--- lib/lp/registry/errors.py	2011-07-27 08:13:18 +0000
+++ lib/lp/registry/errors.py	2011-08-15 13:18:36 +0000
@@ -8,6 +8,7 @@
     'CannotTransitionToCountryMirror',
     'CountryMirrorAlreadySet',
     'DeleteSubscriptionError',
+    'InvalidName',
     'JoinNotAllowed',
     'MirrorNotOfficial',
     'MirrorHasNoHTTPURL',
@@ -42,6 +43,10 @@
     """The name given for a person is already in use by other person."""
 
 
+class InvalidName(Exception):
+    """The name given for a person is not valid."""
+
+
 class NoSuchDistroSeries(NameLookupFailed):
     """Raised when we try to find a DistroSeries that doesn't exist."""
     _message_prefix = "No such distribution series"

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2011-07-29 06:08:15 +0000
+++ lib/lp/registry/interfaces/person.py	2011-08-15 13:18:36 +0000
@@ -27,7 +27,6 @@
     'ITeamCreation',
     'ITeamReassignment',
     'ImmutableVisibilityError',
-    'InvalidName',
     'NoSuchPerson',
     'OPEN_TEAM_POLICY',
     'PersonCreationRationale',
@@ -1405,7 +1404,8 @@
         )
     @export_factory_operation(Interface, [])  # Really IArchive.
     @operation_for_version("beta")
-    def createPPA(name=None, displayname=None, description=None, private=False):
+    def createPPA(name=None, displayname=None, description=None,
+                  private=False):
         """Create a PPA.
 
         :param name: A string with the name of the new PPA to create. If
@@ -2255,8 +2255,8 @@
         addresses associated with.
 
         When merging teams, from_person must have no IMailingLists
-        associated with it. If it has active members they will be deactivated -
-        and reviewer must be supplied.
+        associated with it. If it has active members they will be deactivated
+        - and reviewer must be supplied.
 
         :param from_person: An IPerson or ITeam that is a duplicate.
         :param to_person: An IPerson or ITeam that is a master.
@@ -2447,10 +2447,6 @@
     """A change in team membership visibility is not allowed."""
 
 
-class InvalidName(Exception):
-    """The name given for a person is not valid."""
-
-
 class NoSuchPerson(NameLookupFailed):
     """Raised when we try to look up an IPerson that doesn't exist."""
 

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-08-01 15:28:09 +0000
+++ lib/lp/registry/model/person.py	2011-08-15 13:18:36 +0000
@@ -199,6 +199,7 @@
     HasRequestedReviewsMixin,
     )
 from lp.registry.errors import (
+    InvalidName,
     JoinNotAllowed,
     NameAlreadyTaken,
     PPACreationError,
@@ -225,7 +226,6 @@
     )
 from lp.registry.interfaces.person import (
     ImmutableVisibilityError,
-    InvalidName,
     IPerson,
     IPersonSet,
     IPersonSettings,

=== modified file 'lib/lp/registry/model/sourcepackagename.py'
--- lib/lp/registry/model/sourcepackagename.py	2010-11-09 08:38:23 +0000
+++ lib/lp/registry/model/sourcepackagename.py	2011-08-15 13:18:36 +0000
@@ -25,7 +25,11 @@
     )
 from canonical.launchpad.helpers import ensure_unicode
 from lp.app.errors import NotFoundError
-from lp.registry.errors import NoSuchSourcePackageName
+from lp.app.validators.name import valid_name
+from lp.registry.errors import (
+    InvalidName,
+    NoSuchSourcePackageName,
+    )
 from lp.registry.interfaces.sourcepackagename import (
     ISourcePackageName,
     ISourcePackageNameSet,
@@ -90,6 +94,9 @@
         return SourcePackageName.selectOneBy(name=name)
 
     def new(self, name):
+        if not valid_name(name):
+            raise InvalidName(
+                "%s is not a valid name for a source package." % name)
         return SourcePackageName(name=name)
 
     def getOrCreateByName(self, name):

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2011-08-03 11:00:11 +0000
+++ lib/lp/registry/tests/test_person.py	2011-08-15 13:18:36 +0000
@@ -47,6 +47,7 @@
 from lp.bugs.model.bug import Bug
 from lp.bugs.model.bugtask import get_related_bugtasks_search_params
 from lp.registry.errors import (
+    InvalidName,
     NameAlreadyTaken,
     PrivatePersonLinkageError,
     )
@@ -55,7 +56,6 @@
 from lp.registry.interfaces.nameblacklist import INameBlacklistSet
 from lp.registry.interfaces.person import (
     ImmutableVisibilityError,
-    InvalidName,
     IPersonSet,
     PersonCreationRationale,
     PersonVisibility,

=== added file 'lib/lp/registry/tests/test_sourcepackagename.py'
--- lib/lp/registry/tests/test_sourcepackagename.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/tests/test_sourcepackagename.py	2011-08-15 13:18:36 +0000
@@ -0,0 +1,24 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for SourcePackageName"""
+
+__metaclass__ = type
+
+from testtools.testcase import ExpectedException
+
+from canonical.testing.layers import DatabaseLayer
+from lp.registry.errors import InvalidName
+from lp.registry.model.sourcepackagename import SourcePackageNameSet
+from lp.testing import TestCase
+
+
+class TestSourcePackageNameSet(TestCase):
+
+    layer = DatabaseLayer
+
+    def test_invalid_name(self):
+        with ExpectedException(
+            InvalidName,
+            'invalid%20name is not a valid name for a source package.'):
+            SourcePackageNameSet().new('invalid%20name')