Merge lp:~jelmer/launchpad/can-upload-webcall into lp:launchpad

[Jelmer Vernooij]

Expose IArchive.checkUpload() over the web service API.

This exposes the IArchive.checkUpload() call over the web service API.

[Michael Nelson (michael.nelson)]

Hi Jelmer,

As discussed, my main questions were regarding the return value... we should never be raising 500's... there are a bunch of descriptive webservice_error exceptions in interfaces/archive.py that you might be able to re-user (or add new ones).

I'd suggest returning True if they can upload (as returning nothing is kinda weird), and raising the a descriptive webservice error otherwise.

Regarding the wrapping of the method for the API, I can see why you've done so (rather than changing the signature of checkUpload to use strings and updating all the call-sites), but prefixing with an '_' seems strange to me... if there are other precedents for this fine, but otherwise, I'd expect something like checkUploadAPI() or similar to be (1) a bit more descriptive, and (2) not imply that it should be treated as protected? What do you think? Let me know if you find any other examples of methods being wrapped for API arguments.

-Michael

{{{
16:15 < noodles775> jelmer_: are there other examples of wrapping a method for api exposure that prefix with an _?
16:15 < noodles775> (in ref. to your checkUpload branch).
16:16 < sinzui> noodles775, I think that means you want to rename the method to be public now
16:16 < sinzui> noodles775, I will refrain from suggesting a uses the rename hack since exposing a method explicitly makes it public
16:17 -!- NCommander [~mcasadeva@ubuntu/member/ncommander] has joined #launchpad-reviews
16:17 < jelmer_> noodles775: Maybe, I haven't actually checked...
16:17 < jelmer_> noodles775: I'll have a look now.
16:18 < noodles775> sinzui: in this case it seems it's just to use string args instead of the normal IFaces... it seems strange to prefix with _ imo. (sinzui: https://code.edge.launchpad.net/~jelmer/launchpad/can-upload-webcall/+merge/26141 for reference :) ).
16:22 < noodles775> jelmer_: also, why are you not returning True/False? It also seems strange to not return *anything* to mean yes, and raise an exception otherwise?
16:24 < jelmer_> noodles775: Originally I had it return the exception string and None when uploading was allowed.
16:25 < jelmer_> noodles775: I guess it really depends on whether we would want to expose the reason why the upload would not be possible to the user
16:25 < jelmer_> noodles775: there are different exception classes so in theory you could have different behaviour based on that.
16:27 < noodles775> jelmer_: we should never be expecting a 500, afaik (if you do raise an exception, it needs to use webservice_error(xxx) as in the other examples in interfaces/archive.py)
16:29 < jelmer_> noodles775: ok
16:29 < noodles775> jelmer_: in fact, there's already an InsufficientUploadRights exception defined there - it might be what you're after.
16:30 < noodles775> jelmer_: but I'm still unsure why you're not returning True/False.
16:30 * noodles775 checks for other examples.
16:30 < jelmer_> noodles775: With True/False the user doesn't get an idea why the upload wouldn't be allowed (user not in packageset, pocket is release and distroseries is closed, etc)
16:31 < jelmer_> noodles775: The feature request doesn't require that though, so I'm happy to change it to a boolean.
16:32 < noodles775...

review: Needs Fixing (code)

[Jelmer Vernooij (jelmer)]

The only two other examples I can find are Bug.linkCVE (API call is named Bug.linkCVEAndReturnNothing) and Branch._createMergeProposal() (which is the web API for Branch.createMergeProposal and consistent with Archive.checkUpload).

[Curtis Hovey (sinzui)]

Thanks for asking me to review this before the deadline. Thanks to for explaining the naming situation for me. I think this branch is fine to land.

review: Approve (code and rc)

[Michael Nelson (michael.nelson)]

Thanks for finding the precedent Jelmer, and for updating the exception to use webservice_error.

review: Approve

=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py	2010-05-27 17:20:07 +0000
+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py	2010-06-04 15:03:25 +0000
@@ -267,6 +267,10 @@
 patch_plain_parameter_type(
     IArchive, 'isSourceUploadAllowed', 'distroseries', IDistroSeries)
 patch_plain_parameter_type(
+    IArchive, '_checkUpload', 'distroseries', IDistroSeries)
+patch_choice_parameter_type(
+    IArchive, '_checkUpload', 'pocket', PackagePublishingPocket)
+patch_plain_parameter_type(
     IArchive, 'newPackagesetUploader', 'packageset', IPackageset)
 patch_plain_parameter_type(
     IArchive, 'getUploadersForPackageset', 'packageset', IPackageset)
 
=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2010-05-19 10:28:25 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2010-06-04 15:03:25 +0000
@@ -143,6 +143,7 @@
 
 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.'
 
@@ -492,6 +493,31 @@
         :return: Reason why uploading is not possible or None
         """
 
+    @operation_parameters(
+        person=Reference(schema=IPerson),
+        distroseries=Reference(
+            # Really IDistroSeries, avoiding a circular import here.
+            Interface,
+            title=_("The distro series"), required=True),
+        sourcepackagename=TextLine(
+            title=_("Source package name"), required=True),
+        component=TextLine(
+            title=_("Component"), required=True),
+        pocket=Choice(
+            title=_("Pocket"),
+            description=_("The pocket into which this entry is published"),
+            # Really PackagePublishingPocket, circular import fixed below.
+            vocabulary=DBEnumeratedType,
+            required=True),
+        strict_component=Bool(
+            title=_("Strict component"), required=False)
+        )
+    @export_operation_as("checkUpload")
+    @export_read_operation()
+    def _checkUpload(person, distroseries, sourcepackagename, component, 
+            pocket, strict_component=True):
+        """Wrapper around checkUpload for the web service API."""
+
     def checkUpload(person, distroseries, sourcepackagename, component, 
                     pocket, strict_component=True):
         """Check if 'person' upload 'suitesourcepackage' to 'archive'.
 
=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2010-05-26 08:54:27 +0000
+++ lib/lp/soyuz/model/archive.py	2010-06-04 15:03:25 +0000
@@ -1019,6 +1019,20 @@
             if not distroseries.canUploadToPocket(pocket):
                 return CannotUploadToPocket(distroseries, pocket)
 
+    def _checkUpload(self, person, distroseries, sourcepackagename, component, 
+                    pocket, strict_component=True):
+        """See `IArchive`."""
+        if isinstance(component, basestring):
+            component = getUtility(IComponentSet)[component]
+        if isinstance(sourcepackagename, basestring):
+            sourcepackagename = getUtility(
+                ISourcePackageNameSet)[sourcepackagename]
+        reason = self.checkUpload(person, distroseries, sourcepackagename, 
+            component, pocket, strict_component)
+        if reason is not None:
+            raise reason
+        return True
+
     def checkUpload(self, person, distroseries, sourcepackagename, component,
                     pocket, strict_component=True):
         """See `IArchive`."""
 
=== modified file 'lib/lp/soyuz/stories/webservice/xx-archive.txt'
--- lib/lp/soyuz/stories/webservice/xx-archive.txt	2010-03-17 15:57:17 +0000
+++ lib/lp/soyuz/stories/webservice/xx-archive.txt	2010-06-04 15:03:25 +0000
@@ -335,6 +335,19 @@
     Archive Upload Rights ...~ubuntu-team restricted None
     Archive Upload Rights ...~ubuntu-team universe None
 
+We can use ``checkUpload`` to verify that a person can upload a 
+sourcepackage.
+
+    >>> grumpy = user_webservice.get("/ubuntu/grumpy").jsonBody()
+    >>> response = user_webservice.named_get(
+    ...     ubuntu['main_archive_link'], 'checkUpload', 
+    ...     distroseries=grumpy['self_link'],
+    ...     sourcepackagename='mozilla-firefox', pocket='Release',
+    ...     component='restricted', person=name12['self_link'])
+    >>> print(response)
+    HTTP/1.1 200 Ok
+    ...
+
 deleteComponentUploader() removes that permission:
 
     >>> print cjwatson_webservice.named_post(
@@ -351,6 +364,21 @@
     Archive Upload Rights ...~ubuntu-team restricted None
     Archive Upload Rights ...~ubuntu-team universe None
 
+And ``checkUpload`` now also no longer passes:
+
+    >>> grumpy = user_webservice.get("/ubuntu/grumpy").jsonBody()
+    >>> response = user_webservice.named_get(
+    ...     ubuntu['main_archive_link'], 'checkUpload', 
+    ...     distroseries=grumpy['self_link'],
+    ...     sourcepackagename='mozilla-firefox', pocket='Release',
+    ...     component='main', person=name12['self_link'])
+    >>> print(response)
+    HTTP/1.1 403 Forbidden
+    ...
+	InsufficientUploadRights: The signer of this package is lacking the upload rights for the source package, component or package set in question.
+
+
+
 Only the archive owners can add or remove component-uploaders.
 
     >>> no_priv = webservice.get("/~no-priv").jsonBody()
@@ -457,7 +485,6 @@
     Queue Administration Rights ...~name12 restricted None
     Queue Administration Rights ...~name12 universe None
 
-
 == Malformed archive permission URLs ==
 
 Malformed URLs are handled reasonably well.