launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04581
[Merge] lp:~abentley/launchpad/push-creates-package into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/push-creates-package into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #386596 in Launchpad itself: "pushing to a packaging branch can't create a new package"
https://bugs.launchpad.net/launchpad/+bug/386596
For more details, see:
https://code.launchpad.net/~abentley/launchpad/push-creates-package/+merge/71366
= 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
--
https://code.launchpad.net/~abentley/launchpad/push-creates-package/+merge/71366
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/push-creates-package into lp:launchpad.
=== 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-12 14:22:31 +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-12 14:22:31 +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-12 14:22:31 +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-12 14:22:31 +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-12 14:22:31 +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-12 14:22:31 +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-01-31 06:33:01 +0000
+++ lib/lp/codehosting/tests/test_acceptance.py 2011-08-12 14:22:31 +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-12 14:22:31 +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-04-18 23:05:14 +0000
+++ lib/lp/codehosting/vfs/tests/test_branchfs.py 2011-08-12 14:22:31 +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-12 14:22:31 +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-12 14:22:31 +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-12 14:22:31 +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-12 14:22:31 +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-12 14:22:31 +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-12 14:22:31 +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')
