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

Proposed by Jelmer Vernooij
Status: Merged
Approved by: Curtis Hovey
Approved revision: no longer in the source branch.
Merged at revision: 10949
Proposed branch: lp:~jelmer/launchpad/can-upload-webcall
Merge into: lp:launchpad
Diff against target: 137 lines (+72/-1)
4 files modified
lib/canonical/launchpad/interfaces/_schema_circular_imports.py (+4/-0)
lib/lp/soyuz/interfaces/archive.py (+26/-0)
lib/lp/soyuz/model/archive.py (+14/-0)
lib/lp/soyuz/stories/webservice/xx-archive.txt (+28/-1)
To merge this branch: bzr merge lp:~jelmer/launchpad/can-upload-webcall
Reviewer Review Type Date Requested Status
Michael Nelson (community) Approve
Curtis Hovey (community) code and rc Approve
Review via email: mp+26141@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Michael Nelson (michael.nelson) wrote :
Download full text (3.5 KiB)

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...

Read more...

review: Needs Fixing (code)
Jelmer Vernooij (jelmer) wrote :

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) wrote :

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) wrote :

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
2--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2010-05-27 17:20:07 +0000
3+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2010-06-04 15:03:25 +0000
4@@ -267,6 +267,10 @@
5 patch_plain_parameter_type(
6 IArchive, 'isSourceUploadAllowed', 'distroseries', IDistroSeries)
7 patch_plain_parameter_type(
8+ IArchive, '_checkUpload', 'distroseries', IDistroSeries)
9+patch_choice_parameter_type(
10+ IArchive, '_checkUpload', 'pocket', PackagePublishingPocket)
11+patch_plain_parameter_type(
12 IArchive, 'newPackagesetUploader', 'packageset', IPackageset)
13 patch_plain_parameter_type(
14 IArchive, 'getUploadersForPackageset', 'packageset', IPackageset)
15
16=== modified file 'lib/lp/soyuz/interfaces/archive.py'
17--- lib/lp/soyuz/interfaces/archive.py 2010-05-19 10:28:25 +0000
18+++ lib/lp/soyuz/interfaces/archive.py 2010-06-04 15:03:25 +0000
19@@ -143,6 +143,7 @@
20
21 class CannotUploadToArchive(Exception):
22 """A reason for not being able to upload to an archive."""
23+ webservice_error(403) # Forbidden.
24
25 _fmt = '%(person)s has no upload rights to %(archive)s.'
26
27@@ -492,6 +493,31 @@
28 :return: Reason why uploading is not possible or None
29 """
30
31+ @operation_parameters(
32+ person=Reference(schema=IPerson),
33+ distroseries=Reference(
34+ # Really IDistroSeries, avoiding a circular import here.
35+ Interface,
36+ title=_("The distro series"), required=True),
37+ sourcepackagename=TextLine(
38+ title=_("Source package name"), required=True),
39+ component=TextLine(
40+ title=_("Component"), required=True),
41+ pocket=Choice(
42+ title=_("Pocket"),
43+ description=_("The pocket into which this entry is published"),
44+ # Really PackagePublishingPocket, circular import fixed below.
45+ vocabulary=DBEnumeratedType,
46+ required=True),
47+ strict_component=Bool(
48+ title=_("Strict component"), required=False)
49+ )
50+ @export_operation_as("checkUpload")
51+ @export_read_operation()
52+ def _checkUpload(person, distroseries, sourcepackagename, component,
53+ pocket, strict_component=True):
54+ """Wrapper around checkUpload for the web service API."""
55+
56 def checkUpload(person, distroseries, sourcepackagename, component,
57 pocket, strict_component=True):
58 """Check if 'person' upload 'suitesourcepackage' to 'archive'.
59
60=== modified file 'lib/lp/soyuz/model/archive.py'
61--- lib/lp/soyuz/model/archive.py 2010-05-26 08:54:27 +0000
62+++ lib/lp/soyuz/model/archive.py 2010-06-04 15:03:25 +0000
63@@ -1019,6 +1019,20 @@
64 if not distroseries.canUploadToPocket(pocket):
65 return CannotUploadToPocket(distroseries, pocket)
66
67+ def _checkUpload(self, person, distroseries, sourcepackagename, component,
68+ pocket, strict_component=True):
69+ """See `IArchive`."""
70+ if isinstance(component, basestring):
71+ component = getUtility(IComponentSet)[component]
72+ if isinstance(sourcepackagename, basestring):
73+ sourcepackagename = getUtility(
74+ ISourcePackageNameSet)[sourcepackagename]
75+ reason = self.checkUpload(person, distroseries, sourcepackagename,
76+ component, pocket, strict_component)
77+ if reason is not None:
78+ raise reason
79+ return True
80+
81 def checkUpload(self, person, distroseries, sourcepackagename, component,
82 pocket, strict_component=True):
83 """See `IArchive`."""
84
85=== modified file 'lib/lp/soyuz/stories/webservice/xx-archive.txt'
86--- lib/lp/soyuz/stories/webservice/xx-archive.txt 2010-03-17 15:57:17 +0000
87+++ lib/lp/soyuz/stories/webservice/xx-archive.txt 2010-06-04 15:03:25 +0000
88@@ -335,6 +335,19 @@
89 Archive Upload Rights ...~ubuntu-team restricted None
90 Archive Upload Rights ...~ubuntu-team universe None
91
92+We can use ``checkUpload`` to verify that a person can upload a
93+sourcepackage.
94+
95+ >>> grumpy = user_webservice.get("/ubuntu/grumpy").jsonBody()
96+ >>> response = user_webservice.named_get(
97+ ... ubuntu['main_archive_link'], 'checkUpload',
98+ ... distroseries=grumpy['self_link'],
99+ ... sourcepackagename='mozilla-firefox', pocket='Release',
100+ ... component='restricted', person=name12['self_link'])
101+ >>> print(response)
102+ HTTP/1.1 200 Ok
103+ ...
104+
105 deleteComponentUploader() removes that permission:
106
107 >>> print cjwatson_webservice.named_post(
108@@ -351,6 +364,21 @@
109 Archive Upload Rights ...~ubuntu-team restricted None
110 Archive Upload Rights ...~ubuntu-team universe None
111
112+And ``checkUpload`` now also no longer passes:
113+
114+ >>> grumpy = user_webservice.get("/ubuntu/grumpy").jsonBody()
115+ >>> response = user_webservice.named_get(
116+ ... ubuntu['main_archive_link'], 'checkUpload',
117+ ... distroseries=grumpy['self_link'],
118+ ... sourcepackagename='mozilla-firefox', pocket='Release',
119+ ... component='main', person=name12['self_link'])
120+ >>> print(response)
121+ HTTP/1.1 403 Forbidden
122+ ...
123+ InsufficientUploadRights: The signer of this package is lacking the upload rights for the source package, component or package set in question.
124+
125+
126+
127 Only the archive owners can add or remove component-uploaders.
128
129 >>> no_priv = webservice.get("/~no-priv").jsonBody()
130@@ -457,7 +485,6 @@
131 Queue Administration Rights ...~name12 restricted None
132 Queue Administration Rights ...~name12 universe None
133
134-
135 == Malformed archive permission URLs ==
136
137 Malformed URLs are handled reasonably well.