launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03984
[Merge] lp:~flacoste/launchpad/bug-797917 into lp:launchpad
Francis J. Lacoste has proposed merging lp:~flacoste/launchpad/bug-797917 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~flacoste/launchpad/bug-797917/+merge/64852
= Summary =
This maps NotFound type exception to the 404 status code over the API instead
of the more generic 400 (BadRequest). I also includes a couple of other
exception clean-ups.
== Proposed fix ==
== Pre-implementation notes ==
* This is going to somewhat break backward compatibility as the exception
raised by clients is to change. Both Robert and I thinks this isn't a big
deal, since clients written to recover from these exceptions have to do it
in a very fragile way (by looking at the error message).
== Implementation details ==
* Export all NotFoundError subclass to httplib.NOT_FOUND (404). I could remove
a lot of rendundant webservice_error declaration.
I then search for similar exceptions that were also mapped to 400 and switched
them over to 404:
* ComponentNotFound
* PocketNotFound
* NoSuchBranch
* NoSuchSourcePackageName
As drive-bys, I removed the DistroSeriesNotFound exception defined in
archive.py and use the registry one: NoSuchDistroSeries.
I also substitute a couple of webservice_error to use the new class decorator
@error_status and use the symbolic constants defined in httplib instead of the
raw status code.
== Tests ==
./bin/test -vvt 'webservice/(xx-person.txt|xx-distribtion.txt|xx-packageset.txt)|archive.txt|archivepermission.txt'
== Demo and Q/A ==
>>> from launchpadlib.launchpad import Launchpad
>>> lp = Launchpad.login_with('bug-797917-qa', 'qastaging')
>>> lp.distributions['ubuntu'].getSeries('foobar')
HTTP 1.1 404 Not Found...
= Launchpad lint =
Checking for conflicts and issues in changed files.
I'Ll fix the lint post-review. (As well as probably fix replace all the other
webservice_error() instances in the source tree.)
Linting changed files:
lib/lp/registry/errors.py
lib/lp/soyuz/stories/webservice/xx-archive.txt
lib/lp/code/errors.py
lib/lp/soyuz/stories/webservice/xx-packageset.txt
lib/lp/soyuz/doc/archive.txt
lib/lp/soyuz/interfaces/archive.py
lib/lp/app/errors.py
lib/lp/soyuz/interfaces/packageset.py
lib/lp/soyuz/interfaces/webservice.py
lib/lp/registry/stories/webservice/xx-distribution.txt
lib/lp/soyuz/model/archive.py
lib/lp/soyuz/model/archivepermission.py
lib/lp/registry/stories/webservice/xx-person.txt
lib/lp/soyuz/doc/archivepermission.txt
./lib/lp/soyuz/stories/webservice/xx-archive.txt
20: want exceeds 78 characters.
87: want exceeds 78 characters.
113: source exceeds 78 characters.
120: narrative uses a moin header.
131: source exceeds 78 characters.
140: want exceeds 78 characters.
145: source exceeds 78 characters.
155: want exceeds 78 characters.
160: source exceeds 78 characters.
170: want exceeds 78 characters.
175: source exceeds 78 characters.
185: want exceeds 78 characters.
189: narrative uses a moin header.
272: want exceeds 78 characters.
333: want exceeds 78 characters.
341: narrative has trailing whitespace.
346: source has trailing whitespace.
374: source has trailing whitespace.
464: want exceeds 78 characters.
490: narrative uses a moin header.
497: source exceeds 78 characters.
506: source exceeds 78 characters.
514: source exceeds 78 characters.
520: narrative exceeds 78 characters.
523: source exceeds 78 characters.
530: narrative uses a moin header.
558: narrative uses a moin header.
587: narrative uses a moin header.
766: narrative uses a moin header.
768: narrative exceeds 78 characters.
787: source exceeds 78 characters.
797: narrative uses a moin header.
824: want exceeds 78 characters.
840: want exceeds 78 characters.
854: narrative uses a moin header.
943: narrative uses a moin header.
956: narrative uses a moin header.
1004: narrative uses a moin header.
./lib/lp/soyuz/stories/webservice/xx-packageset.txt
348: source exceeds 78 characters.
509: source exceeds 78 characters.
575: want exceeds 78 characters.
596: want exceeds 78 characters.
605: want exceeds 78 characters.
606: want exceeds 78 characters.
625: want exceeds 78 characters.
652: want exceeds 78 characters.
653: want exceeds 78 characters.
663: want exceeds 78 characters.
664: want exceeds 78 characters.
678: want exceeds 78 characters.
797: want exceeds 78 characters.
798: want exceeds 78 characters.
819: want exceeds 78 characters.
831: want exceeds 78 characters.
832: want exceeds 78 characters.
846: source exceeds 78 characters.
855: source exceeds 78 characters.
863: source exceeds 78 characters.
871: source exceeds 78 characters.
880: source exceeds 78 characters.
./lib/lp/app/errors.py
72: E302 expected 2 blank lines, found 1
./lib/lp/soyuz/model/archive.py
1537: local variable 'error' is assigned to but never used
./lib/lp/soyuz/model/archivepermission.py
197: local variable 'e' is assigned to but never used
./lib/lp/soyuz/doc/archivepermission.txt
1: narrative uses a moin header.
73: source exceeds 78 characters.
263: narrative uses a moin header.
--
https://code.launchpad.net/~flacoste/launchpad/bug-797917/+merge/64852
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~flacoste/launchpad/bug-797917 into lp:launchpad.
=== modified file 'lib/lp/app/errors.py'
--- lib/lp/app/errors.py 2011-03-23 16:28:51 +0000
+++ lib/lp/app/errors.py 2011-06-16 15:27:26 +0000
@@ -14,6 +14,8 @@
'UserCannotUnsubscribePerson',
]
+import httplib
+
from lazr.restful.declarations import error_status
from zope.security.interfaces import (
ForbiddenAttribute,
@@ -25,10 +27,12 @@
"""Translation objects are unavailable."""
+@error_status(httplib.NOT_FOUND)
class NotFoundError(KeyError):
"""Launchpad object not found."""
+@error_status(httplib.GONE)
class GoneError(KeyError):
"""Launchpad object is gone."""
@@ -65,10 +69,10 @@
One example would be a URL containing uppercase letters.
"""
-@error_status(401)
+@error_status(httplib.UNAUTHORIZED)
class UserCannotUnsubscribePerson(Unauthorized):
"""User does not have permission to unsubscribe person or team."""
# Slam a 401 response code onto all ForbiddenAttribute errors.
-error_status(401)(ForbiddenAttribute)
+error_status(httplib.UNAUTHORIZED)(ForbiddenAttribute)
=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py 2011-05-26 15:48:55 +0000
+++ lib/lp/code/errors.py 2011-06-16 15:27:26 +0000
@@ -212,8 +212,6 @@
class NoSuchBranch(NameLookupFailed):
"""Raised when we try to load a branch that does not exist."""
- webservice_error(400)
-
_message_prefix = "No such branch"
=== modified file 'lib/lp/registry/errors.py'
--- lib/lp/registry/errors.py 2011-05-03 06:35:26 +0000
+++ lib/lp/registry/errors.py 2011-06-16 15:27:26 +0000
@@ -45,7 +45,6 @@
class NoSuchDistroSeries(NameLookupFailed):
"""Raised when we try to find a DistroSeries that doesn't exist."""
- webservice_error(httplib.BAD_REQUEST)
_message_prefix = "No such distribution series"
@@ -60,7 +59,6 @@
class NoSuchSourcePackageName(NameLookupFailed):
"""Raised when we can't find a particular sourcepackagename."""
- webservice_error(httplib.BAD_REQUEST)
_message_prefix = "No such source package"
=== modified file 'lib/lp/registry/stories/webservice/xx-distribution.txt'
--- lib/lp/registry/stories/webservice/xx-distribution.txt 2011-04-08 12:04:07 +0000
+++ lib/lp/registry/stories/webservice/xx-distribution.txt 2011-06-16 15:27:26 +0000
@@ -67,12 +67,12 @@
>>> print series['self_link']
http://.../ubuntu/hoary
-Requesting a series that does not exist is a bad request.
+Requesting a series that does not exist is results in a not found error.
>>> print webservice.named_get(
... ubuntu['self_link'], 'getSeries',
... name_or_version='fnord')
- HTTP/1.1 400 Bad Request
+ HTTP/1.1 404 Not Found
...
No such distribution series: 'fnord'.
=== modified file 'lib/lp/registry/stories/webservice/xx-person.txt'
--- lib/lp/registry/stories/webservice/xx-person.txt 2011-05-17 12:59:37 +0000
+++ lib/lp/registry/stories/webservice/xx-person.txt 2011-06-16 15:27:26 +0000
@@ -634,12 +634,12 @@
... name='ppa').jsonBody()['self_link']
http://.../~mark/+archive/ppa
-In cases where a PPA with a given name cannot be found, None is
+In cases where a PPA with a given name cannot be found, a Not Found error is
returned.
>>> print webservice.named_get(
... mark['self_link'], 'getPPAByName', name='boing')
- HTTP/1.1 400 Bad Request
+ HTTP/1.1 404 Not Found
...
No such ppa: 'boing'.
=== modified file 'lib/lp/soyuz/doc/archive.txt'
--- lib/lp/soyuz/doc/archive.txt 2011-06-08 18:55:14 +0000
+++ lib/lp/soyuz/doc/archive.txt 2011-06-16 15:27:26 +0000
@@ -2333,14 +2333,14 @@
... person=mark)
Traceback (most recent call last):
...
- PocketNotFound: 'BADPOCKET'
+ PocketNotFound: No such pocket: 'BADPOCKET'.
>>> mark.archive.syncSources(
... sources, cprov.archive, "release", to_series="badseries",
... person=mark)
Traceback (most recent call last):
...
- DistroSeriesNotFound: badseries
+ NoSuchDistroSeries: No such distribution series: 'badseries'.
We can also specify a single source to be copied with the `syncSource`
call. This allows a version to be specified so older versions can be
=== modified file 'lib/lp/soyuz/doc/archivepermission.txt'
--- lib/lp/soyuz/doc/archivepermission.txt 2011-05-27 19:53:20 +0000
+++ lib/lp/soyuz/doc/archivepermission.txt 2011-06-16 15:27:26 +0000
@@ -146,7 +146,7 @@
... ubuntu.main_archive, "badcomponent")
Traceback (most recent call last):
...
- ComponentNotFound: 'badcomponent'
+ ComponentNotFound: No such component: 'badcomponent'.
If the component argument is not passed, it will return
ArchivePermission records for all matching components:
=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py 2011-06-13 17:41:29 +0000
+++ lib/lp/soyuz/interfaces/archive.py 2011-06-16 15:27:26 +0000
@@ -20,7 +20,6 @@
'CannotUploadToArchive',
'CannotUploadToPPA',
'CannotUploadToPocket',
- 'DistroSeriesNotFound',
'FULL_COMPONENT_SUPPORT',
'IArchive',
'IArchiveAppend',
@@ -47,12 +46,13 @@
'validate_external_dependencies',
]
-
+import httplib
from urlparse import urlparse
from lazr.enum import DBEnumeratedType
from lazr.restful.declarations import (
call_with,
+ error_status,
export_as_webservice_entry,
export_factory_operation,
export_operation_as,
@@ -65,7 +65,6 @@
operation_returns_entry,
rename_parameters_as,
REQUEST_USER,
- webservice_error,
)
from lazr.restful.fields import (
CollectionField,
@@ -104,6 +103,7 @@
from lp.soyuz.interfaces.processor import IProcessorFamily
+@error_status(httplib.BAD_REQUEST)
class ArchiveDependencyError(Exception):
"""Raised when an `IArchiveDependency` does not fit the context archive.
@@ -113,76 +113,68 @@
* It is not a PPA,
* It is already recorded.
"""
- webservice_error(400) # Bad request.
# Exceptions used in the webservice that need to be in this file to get
# picked up therein.
-
+@error_status(httplib.BAD_REQUEST)
class CannotCopy(Exception):
"""Exception raised when a copy cannot be performed."""
- webservice_error(400) # Bad request.
-
-
+
+
+@error_status(httplib.BAD_REQUEST)
class CannotSwitchPrivacy(Exception):
"""Raised when switching the privacy of an archive that has
publishing records."""
- webservice_error(400) # Bad request.
-
-
-class PocketNotFound(Exception):
+
+
+class PocketNotFound(NameLookupFailed):
"""Invalid pocket."""
- webservice_error(400) # Bad request.
-
-
-class DistroSeriesNotFound(Exception):
- """Invalid distroseries."""
- webservice_error(400) # Bad request.
-
-
+ _message_prefix = "No such pocket"
+
+
+@error_status(httplib.BAD_REQUEST)
class AlreadySubscribed(Exception):
"""Raised when creating a subscription for a subscribed person."""
- webservice_error(400) # Bad request.
-
-
+
+
+@error_status(httplib.BAD_REQUEST)
class ArchiveNotPrivate(Exception):
"""Raised when creating an archive subscription for a public archive."""
- webservice_error(400) # Bad request.
-
-
+
+
+@error_status(httplib.BAD_REQUEST)
class NoTokensForTeams(Exception):
"""Raised when creating a token for a team, rather than a person."""
- webservice_error(400) # Bad request.
-
-
-class ComponentNotFound(Exception):
+
+
+class ComponentNotFound(NameLookupFailed):
"""Invalid source name."""
- webservice_error(400) # Bad request.
-
-
+ _message_prefix = 'No such component'
+
+
+@error_status(httplib.BAD_REQUEST)
class InvalidComponent(Exception):
"""Invalid component name."""
- webservice_error(400) # Bad request.
class NoSuchPPA(NameLookupFailed):
"""Raised when we try to look up an PPA that doesn't exist."""
- webservice_error(400) # Bad request.
_message_prefix = "No such ppa"
+@error_status(httplib.BAD_REQUEST)
class VersionRequiresName(Exception):
"""Raised on some queries when version is specified but name is not."""
- webservice_error(400) # Bad request.
class CannotRestrictArchitectures(Exception):
"""The architectures for this archive can not be restricted."""
+@error_status(httplib.FORBIDDEN)
class CannotUploadToArchive(Exception):
"""A reason for not being able to upload to an archive."""
- webservice_error(403) # Forbidden.
_fmt = '%(person)s has no upload rights to %(archive)s.'
@@ -197,9 +189,9 @@
_fmt = "Partner uploads must be for the RELEASE or PROPOSED pocket."
+@error_status(httplib.FORBIDDEN)
class CannotUploadToPocket(Exception):
"""Returned when a pocket is closed for uploads."""
- webservice_error(403) # Forbidden.
def __init__(self, distroseries, pocket):
Exception.__init__(self,
@@ -255,11 +247,10 @@
CannotUploadToArchive.__init__(self, archive_name=archive_name)
+@error_status(httplib.BAD_REQUEST)
class InvalidExternalDependencies(Exception):
"""Tried to set external dependencies to an invalid value."""
- webservice_error(400) # Bad request.
-
def __init__(self, errors):
error_msg = 'Invalid external dependencies:\n%s\n' % '\n'.join(errors)
super(Exception, self).__init__(self, error_msg)
@@ -1267,7 +1258,7 @@
:raises NoSuchSourcePackageName: if the source name is invalid
:raises PocketNotFound: if the pocket name is invalid
- :raises DistroSeriesNotFound: if the distro series name is invalid
+ :raises NoSuchDistroSeries: if the distro series name is invalid
:raises CannotCopy: if there is a problem copying.
"""
@@ -1309,7 +1300,7 @@
:raises NoSuchSourcePackageName: if the source name is invalid
:raises PocketNotFound: if the pocket name is invalid
- :raises DistroSeriesNotFound: if the distro series name is invalid
+ :raises NoSuchDistroSeries: if the distro series name is invalid
:raises CannotCopy: if there is a problem copying.
"""
=== modified file 'lib/lp/soyuz/interfaces/packageset.py'
--- lib/lp/soyuz/interfaces/packageset.py 2011-05-30 15:17:11 +0000
+++ lib/lp/soyuz/interfaces/packageset.py 2011-06-16 15:27:26 +0000
@@ -50,8 +50,6 @@
class NoSuchPackageSet(NameLookupFailed):
"""Raised when we try to look up an PackageSet that doesn't exist."""
- # Bad request.
- webservice_error(400)
_message_prefix = "No such package set (in the specified distro series)"
=== modified file 'lib/lp/soyuz/interfaces/webservice.py'
--- lib/lp/soyuz/interfaces/webservice.py 2010-11-09 23:39:59 +0000
+++ lib/lp/soyuz/interfaces/webservice.py 2011-06-16 15:27:26 +0000
@@ -20,7 +20,6 @@
'CannotUploadToPPA',
'CannotUploadToPocket',
'ComponentNotFound',
- 'DistroSeriesNotFound',
'DuplicatePackagesetName',
'IArchive',
'IArchiveDependency',
@@ -58,7 +57,6 @@
CannotUploadToPPA,
CannotUploadToPocket,
ComponentNotFound,
- DistroSeriesNotFound,
IArchive,
InsufficientUploadRights,
InvalidComponent,
=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py 2011-06-11 00:49:33 +0000
+++ lib/lp/soyuz/model/archive.py 2011-06-16 15:27:26 +0000
@@ -88,6 +88,7 @@
from lp.buildmaster.interfaces.packagebuild import IPackageBuildSet
from lp.buildmaster.model.buildfarmjob import BuildFarmJob
from lp.buildmaster.model.packagebuild import PackageBuild
+from lp.registry.errors import NoSuchDistroSeries
from lp.registry.interfaces.distroseries import IDistroSeriesSet
from lp.registry.interfaces.person import (
IPersonSet,
@@ -128,7 +129,6 @@
CannotUploadToPPA,
ComponentNotFound,
default_name_by_purpose,
- DistroSeriesNotFound,
FULL_COMPONENT_SUPPORT,
IArchive,
IArchiveSet,
@@ -1535,7 +1535,7 @@
try:
pocket = PackagePublishingPocket.items[to_pocket.upper()]
except KeyError, error:
- raise PocketNotFound(error)
+ raise PocketNotFound(to_pocket.upper())
# Fail immediately if the destination pocket is not Release and
# this archive is a PPA.
@@ -1548,7 +1548,7 @@
result = getUtility(IDistroSeriesSet).queryByName(
self.distribution, to_series)
if result is None:
- raise DistroSeriesNotFound(to_series)
+ raise NoSuchDistroSeries(to_series)
series = result
else:
series = None
=== modified file 'lib/lp/soyuz/model/archivepermission.py'
--- lib/lp/soyuz/model/archivepermission.py 2011-01-12 13:27:53 +0000
+++ lib/lp/soyuz/model/archivepermission.py 2011-06-16 15:27:26 +0000
@@ -195,7 +195,7 @@
component = getUtility(IComponentSet)[component]
return component
except NotFoundError, e:
- raise ComponentNotFound(e)
+ raise ComponentNotFound(component)
def _nameToSourcePackageName(self, sourcepackagename):
"""Helper to convert a possible string name to ISourcePackageName."""
=== modified file 'lib/lp/soyuz/stories/webservice/xx-archive.txt'
--- lib/lp/soyuz/stories/webservice/xx-archive.txt 2011-06-06 14:27:47 +0000
+++ lib/lp/soyuz/stories/webservice/xx-archive.txt 2011-06-16 15:27:26 +0000
@@ -233,7 +233,7 @@
>>> print user_webservice.named_get(
... ubuntu['main_archive_link'], 'getUploadersForPackage',
... source_package_name="badpackage")
- HTTP/1.1 400 Bad Request
+ HTTP/1.1 404 Not Found
...
Colin is a valid member of the team who owns the ubuntu primary archive.
@@ -306,7 +306,7 @@
>>> print cjwatson_webservice.named_get(
... ubuntu['main_archive_link'], 'getUploadersForComponent',
... component_name="badcomponent")
- HTTP/1.1 400 Bad Request
+ HTTP/1.1 404 Not Found
...
If you don't specify the component, you get all the uploaders for
=== modified file 'lib/lp/soyuz/stories/webservice/xx-packageset.txt'
--- lib/lp/soyuz/stories/webservice/xx-packageset.txt 2011-03-22 16:30:37 +0000
+++ lib/lp/soyuz/stories/webservice/xx-packageset.txt 2011-06-16 15:27:26 +0000
@@ -72,7 +72,7 @@
>>> response = webservice.named_get(
... '/package-sets', 'getByName', {}, name=u'not-found')
>>> print response
- HTTP/1.1 400 Bad Request
+ HTTP/1.1 404 Not Found
...
No such package set (in the specified distro series): 'not-found'.
@@ -499,7 +499,7 @@
... '/package-sets/', 'setsIncludingSource', {},
... sourcepackagename=u'does-not-exist')
>>> print response
- HTTP/1.1 400 Bad Request
+ HTTP/1.1 404 Not Found
...
No such source package: 'does-not-exist'.
