Bugzilla should allow access to data via REST API similar to XMLRPC and JSONRPC. The current code base can be extended to allow REST API calls using the same backend code to retrieve the data as well as submit data. This patch is my initial pass at adding this functionality. dkl

Comment on attachment 743287 [details] [diff] [review] Patch to enhance WebService API to allow REST style API calls (v1) >=== modified file '.htaccess' >+RewriteEngine On >+RewriteRule ^rest/(.*)$ ./rest.cgi/$1 [NE] Are ReweriteEngine and RewriteRule allowed in .htaccess with the default Apache configuration? >=== modified file 'Bugzilla/Mailer.pm' >+ return if $email->header('to') eq ''; This change has nothing to do with this patch. >=== modified file 'Bugzilla/WebService.pm' >+use Tie::IxHash; Tie::IxHash is not part of the core Perl distribution. If we are going to use it, then it must be listed as an optional module in Install/Requirements.pm. >+sub rest_resources { I'm not happy with all these rest_resources() methods in all the WS files. These files are supposed to be independent of the method used to access the data. Method-specific stuff should go into REST.pm. >+ Bugzilla::Hook::process('webservice_rest_resources', >+ { rest_resources => \%_rest_resources }); Please add hooks in a separate patch. The code is already large enough, and you would have to document them in Hook.pm + Example/Extension.pm. >=== modified file 'Bugzilla/WebService/Bug.pm' >+use Bugzilla::WebService::Util qw(filter filter_wants validate ref_urlbase); Where is ref_urlbase() used? >+use Bugzilla::Util qw(trick_taint trim diff_arrays validate_email_syntax); Same question here about validate_email_syntax(). >+sub rest_resources { Same comment as above. Is there really no way to not fill these files with REST-specific code? >=== added file 'Bugzilla/WebService/Server/REST.pm' >+# This Source Code Form is "Incompatible With Secondary Licenses", as >+# defined by the Mozilla Public License, v. 2.0. >+package Bugzilla::WebService::Server::REST; Nit: add an empty line before |package...|. >=== modified file 'Bugzilla/WebService/User.pm' >+use Bugzilla::WebService::Util qw(filter validate translate params_to_objects ref_urlbase); Where is ref_urlbase() used? >=== added file 'rest.cgi' >+# The contents of this file are subject to the Mozilla Public >+# License Version 1.1 (the "License"); you may not use this file >+# except in compliance with the License. You may obtain a copy of >+# the License at http://www.mozilla.org/MPL/ Wrong version of the license. We now use MPL2. I didn't look at your patch carefully nor tested it. But above are some obvious things to fix first. It would be good to also ask glob for review.

New patch addressing suggested changes. Re: mod_rewrite, you're right in that it will require custom configuration for the admin in order to get the cleaner /rest/ type path. For now rest.cgi works just fine for testing purposes. dkl

Yeah, sorry, I haven't got to this yet :-| Gerv

Comment on attachment 744299 [details] [diff] [review] Patch to enhance WebService API to allow REST style API calls (v2) Review of attachment 744299 [details] [diff] [review]: ----------------------------------------------------------------- * This patch delegates a lot of work to the JSON-RPC implementation. That's good design in a way, but it makes it hard for a code review to assess the level of API compatibility with the existing implementation. I don't see any code for ironing out quirks or adapting parameter names or that kind of thing. Are you aiming for full compatibility (for the methods you've implemented)? Or are you just reflecting the way the existing API does it into REST, which may or may not turn out to be compatible? Or some mixture of both? * This patch is missing several files which were patched in the first version; is that intentional? E.g. .htaccess, Mailer.pm, Classification.pm. * Where are all the tests? ::: Bugzilla/Error.pm @@ +124,5 @@ > my $server = Bugzilla->_json_server; > + > + my $status_code = 0; > + if (Bugzilla->error_mode == ERROR_MODE_REST) { > + $status_code = STATUS_BAD_REQUEST; Is it right to use the same HTTP error code for all errors in a REST-based service? ::: Bugzilla/WebService/Constants.pm @@ +196,5 @@ > ); > > +use constant REST_CONTENT_TYPE_WHITELIST => qw( > + text/html > + application/json-rpc Under what conditions would we want to return data of type application/json-rpc over the REST interface? Is text/html just for debugging? @@ +219,5 @@ > return $dispatch; > }; > > +use constant STATUS_OK => 200; > +use constant STATUS_CREATED => 201; Isn't there a module you could import (or which we already use) which contains these? DRY and all that... ::: Bugzilla/WebService/Server/REST.pm @@ +86,5 @@ > + return $self->response( > + $self->response_header($self->bz_response_code, $result)); > + } > + > + $self->response($self->error_response_header); This section seems the wrong way round; I would check for error and return that, and do success in the mainline. @@ +121,5 @@ > + stringify_json_objects($result); > + my $html_content = $self->json->pretty->encode($result); > + $content = "<html><title>Bugzilla::REST::API</title><body>" . > + "<pre>$html_content</pre></body></html>"; > + } Does this feature break handy addons like JSONView, which do pretty display of returned JSON? Are you sure this is correctly _HTML_ escaped? @@ +150,5 @@ > + # XXX There's no particularly good way for us to get a parameter > + # to Bugzilla->login at this point, so we pass this information > + # around using request_cache, which is a bit of a hack. The > + # implementation of it is in Bugzilla::Auth::Login::Stack. > + Bugzilla->request_cache->{auth_no_automatic_login} = 1; I don't see a patch to B::A::L::S in your set...? Is one necessary? @@ +366,5 @@ > + } > +} > + > +sub _best_content_type { > + my ($self, @types) = @_; Isn't all of this content negotiation stuff implemented in a module somewhere? @@ +415,5 @@ > + > + # Sort the types by score, subscore by order, and pull out just the name > + @prefs = map {$_->{name}} sort {$b->{score} <=> $a->{score} || > + $a->{order} <=> $b->{order}} @prefs; > + return @prefs, '*/*'; # Allows allow for */* ITYM "Always". :-) ::: Bugzilla/WebService/Server/REST/Resources.pm @@ +9,5 @@ > +BEGIN { > + *Bugzilla::WebService::Bug::rest_resources = \&_rest_resources; > +}; > + > +sub _rest_resources { It seems like you have only implemented a subset of the calls the current REST API supports. Is that correct? If so, it would be good to list which ones you are not supporting. Ones I see missing: /count GET, /attachment/<id> PUT, and /configuration GET. @@ +48,5 @@ > + return { id => $_[0] }; > + } > + } > + }, > + qr{/bug/comment/([^/]+)$} => { This doesn't match any existing REST API call, and seems to duplicate the function of the call two entries above; what's the story? If it's for returning comments by ID, it should be /comment/<id>, not /bug/comment/<id> (see /attachment for an example of this pattern). Gerv @@ +115,5 @@ > + > +sub _rest_resources { > + my %rest_resources; > + tie(%rest_resources, "Tie::IxHash", > + qr{/version$} => { You have reflected a lot of existing JSON-RPC/XML-RPC API calls directly into REST. We should make sure we consider whether the API design of those calls fits well with the existing design of the REST API before we ship them and have to support them. Has any work been done on that yet? @@ +278,5 @@ > + > +sub _rest_resources { > + my %rest_resources; > + tie(%rest_resources, "Tie::IxHash", > + qr{/login$} => { As far as I can see, it makes no sense to reflect the login and logout methods in the REST API, because the API is stateless...

Thanks Gerv. Still working through your comments and will spin a new patch where needed. Just wanted to re-emphasize that this is not a patch to make BMO compatible with BzAPI. This first patch is to simply allow the same data that is currently available from Bugzilla's WebServices to be available over REST. BzAPI compatibility (extension) and any other changes to the data structure that is passed in or returned will be discussed in separate bugs. Thanks dkl

I think that Bugzilla's REST API should be BzAPI-compatible by default. Is that a discussion which has already happened? Is the plan for this patch that it will be BMO-only for now, or is this an upstream patch? If the latter, we run the risk of shipping a REST API which will then change when/if the above question gets resolved. Gerv

(In reply to Gervase Markham [:gerv] from comment #6) > I think that Bugzilla's REST API should be BzAPI-compatible by default. Is > that a discussion which has already happened? Yes, when we discussed this in person when I was in London :) I outlined that we would have a REST API that was compatible with the current WebService data format used by upstream. Then we would create an extension that manipulated the data coming in and leaving that matched what the BzAPI currently uses to help people in transition. It will involve using a different REST path to choose which you want to use. > Is the plan for this patch that it will be BMO-only for now, or is this an > upstream patch? If the latter, we run the risk of shipping a REST API which > will then change when/if the above question gets resolved. The goal of this patch is to be considered for upstream. As for your last statement, we already have a lot of XMLRPC/JSONRPC methods that are marked as STABLE in the documentation and would require very good reason to change those at this point. The ones marked UNSTABLE or EXPERIMENTAL still could possibly still be changed if a better format is determined. The BzAPI extension on the other hand can and will be different than the normal upstream format and we will either continue to maintain it indefinitely and/or try to convince people to transition to the upstream version when possible. There are some features missing from the upstream WebServices that would need to be added to fully convince people to move, mainly searching capability, dkl

(In reply to David Lawrence [:dkl] from comment #7) > Yes, when we discussed this in person when I was in London :) I outlined > that we would have > a REST API that was compatible with the current WebService data format used > by upstream. > > Then we would create an extension that manipulated the data coming in and > leaving that matched what the BzAPI currently uses to help people in > transition. It will involve using a different REST path to choose which you > want to use. This plan basically says "we will create a new REST API and transition people to it." What is the value to our users in doing that? Unlike Bugzilla's other APIs, the REST API was designed all at once to be consistent. It has stood the test of time very well, and people are used to it. What benefit is it to our users to get them to change all their code (over time) to use a different API with the same capabilities? I think that given then choice of having the official REST API work exactly like the other two and so making all the current REST API users migrate, and having it that there are some differences between the REST API and the other two APIs (which may be more inconvenient for Bugzilla developers to maintain those differences), we should choose user ease over developer ease. > The goal of this patch is to be considered for upstream. As for your last > statement, we already have a lot of XMLRPC/JSONRPC methods that are marked > as STABLE in the documentation and would require very good reason to change > those at this point. The ones marked UNSTABLE or EXPERIMENTAL still could > possibly still be changed if a better format is determined. When we talked in London, I think you said that you would produce a document explaining what differences there are between BzAPI and a current-API-to-REST-straight-conversion. Does that document yet exist? Gerv

(In reply to Gervase Markham [:gerv] from comment #8) > This plan basically says "we will create a new REST API and transition > people to it." What is the value to our users in doing that? > [...] What benefit is it to our users to get them to change all their code > (over time) to use a different API with the same capabilities? It is not uncommon to have API changes between major releases (Firefox does it too, forcing extensions to be updated from time to time). And the unofficial BzAPI vs the officially supported by upstream REST API is a good enough reason to ask people to update their code. > I think that given then choice of having the official REST API work exactly > like the other two and so making all the current REST API users migrate, and > having it that there are some differences between the REST API and the other > two APIs (which may be more inconvenient for Bugzilla developers to maintain > those differences), we should choose user ease over developer ease. I disagree. It would quickly become a mess if the REST API was different from other APIs. Consistency is important, IMO. And from a security point of view, it's much safer if all APIs behave the same way. For now, BzAPI only works with bmo. And because bmo use BzAPI, everyone else should follow the same API? I don't think so.

(In reply to Frédéric Buclin from comment #9) > (In reply to Gervase Markham [:gerv] from comment #8) > > This plan basically says "we will create a new REST API and transition > > people to it." What is the value to our users in doing that? > > [...] What benefit is it to our users to get them to change all their code > > (over time) to use a different API with the same capabilities? > > It is not uncommon to have API changes between major releases (Firefox does > it too, forcing extensions to be updated from time to time). I agree. But if we are happy to make API changes between major releases, we should, at the very least, analyse the design choices made by BzAPI where they differ from those made by the existing API, and decide which is better and which needs breaking - rather than assuming the existing API is right all the time. > And the > unofficial BzAPI vs the officially supported by upstream REST API is a good > enough reason to ask people to update their code. Only if they are different, which they do not need to be. > > I think that given then choice of having the official REST API work exactly > > like the other two and so making all the current REST API users migrate, and > > having it that there are some differences between the REST API and the other > > two APIs (which may be more inconvenient for Bugzilla developers to maintain > > those differences), we should choose user ease over developer ease. > > I disagree. It would quickly become a mess if the REST API was different > from other APIs. It depends in part on the level of difference. dkl promised to produce a document which listed the differences; I think that would be important input into this discussion. > Consistency is important, IMO. And from a security point of > view, it's much safer if all APIs behave the same way. I think it's highly unlikely that someone is going to be using one form of the API for half their calls, and another form for the other half. Also, they cannot work exactly the same way, because the REST API, by its nature, is stateless and loginless, whereas the RPC APIs, by their nature, are not. > For now, BzAPI only works with bmo. And because bmo use BzAPI, everyone else > should follow the same API? I don't think so. BzAPI does not work only with BMO; it has proved a popular piece of software. I regularly get support requests from people using it with their own installations of Bugzilla. What is more, I think there's a pretty good chance that BzAPI alone has more bits of client software than the existing XML-RPC and JSON-RPC APIs put together. Have you not seen the explosion in Bugzilla-integration innovation which it has unleashed? Full capabilities, a REST model, good documentation and a low barrier to entry has led to lots and lots of people building stuff which uses it. We currently have an existing REST API used by a lot of people, and a new proposed REST API which is not very different but different enough to require code to be updated, and which is used by no-one (because it hasn't shipped yet). It is not of any value to the many people writing tools using the existing REST API to change it to make it consistent with an API they have never used or seen, forcing them to update their code for no increase in function. They will not thank us for doing that. Gerv

well... I agree with both of you. a) we should have all of the APIs consistent with each other b) we should use the one with the highest adoption as the base for what everything is consistent with, and there is a lot of evidence that bzAPI is the most well-used. All of the Android apps that interface with Bugzilla that I'm familiar with use it, for example. Most well-designed APIs have a version number as part of the base URI used to access them. It appears this was left out of the existing webservice APIs. Breaking a "stable" api call would be fine as long as such a version number got bumped when we did so (and the old version remained working, so you have to opt-in to get the new syntax). We could always add a version number to the URI for version 2, and treat the lack of a version number as version 1, so we can do this going forward. All of that said, having that list of differences between the two would be really useful before making any kind of decision here.

(In reply to Dave Miller [:justdave] from comment #11) > All of that said, having that list of differences between the two would be > really useful before making any kind of decision here. I've started creating that detailed documentation here: https://wiki.mozilla.org/Bugzilla:API_Comparison Gerv

Comment on attachment 744299 [details] [diff] [review] Patch to enhance WebService API to allow REST style API calls (v2) Review of attachment 744299 [details] [diff] [review]: ----------------------------------------------------------------- i'm still working through poking at it, but here's a preliminary review. this patch doesn't pass all tests (goodperl due to missing require 5.10.1, and pod coverage). html errors are sometimes returned instead of json faults. eg. hit rest.cgi/user/1 in your browser. some other faults are application/json, even when hit in the browser. eg. hit rest.cgi in your browser. i think it's worth putting the rewrite back into .htaccess by default, wrapped in <IfModule> some of the objects returned always contain empty values. eg. rest.cgi/bug/39/comment returns the comments under 'bugs'.'39'.'comments' but also returns an empty 'comments' hash. rest.cgi/bug/comment/81 returns the comment under 'comments'.'81' but also returns an empty 'bugs' hash. ::: Bugzilla/WebService/Server/REST.pm @@ +118,5 @@ > + > + # If accessing through web browser, then display in readable format > + if ($self->content_type eq 'text/html') { > + stringify_json_objects($result); > + my $html_content = $self->json->pretty->encode($result); you should include canonical in that mix (..->json->pretty->canonical->..), to sort the json keys. @@ +121,5 @@ > + stringify_json_objects($result); > + my $html_content = $self->json->pretty->encode($result); > + $content = "<html><title>Bugzilla::REST::API</title><body>" . > + "<pre>$html_content</pre></body></html>"; > + } gerv's correct, html escaping is missing and is required. you're also missing a doctype declaration. @@ +297,5 @@ > + my $extra_params = {}; > + my $json = delete $params->{'POSTDATA'} || delete $params->{'PUTDATA'}; > + print STDERR $json; > + if ($json) { > + $extra_params = eval q| $self->json->decode($json) |; shouldn't that be eval { $self->json->decode($jons) }; ? i don't like the idea of passing the json data as a string to eval. ::: Bugzilla/WebService/Server/REST/Resources.pm @@ +11,5 @@ > +}; > + > +sub _rest_resources { > + my %rest_resources; > + tie(%rest_resources, "Tie::IxHash", this could probably be done using an array instead pulling in an extra dependency. @@ +88,5 @@ > + } > + }, > + qr{/bug/([^/]+)/flag$} => { > + GET => { > + method => 'flags', "message" : "No such a method : 'flags'." @@ +100,5 @@ > +} > + > +1; > + > +package Bugzilla::WebService::Server::REST::Resources::Bugzilla; please split these packages into individual files. @@ +201,5 @@ > + POST => { > + method => 'create' > + } > + }, > + qr{/user/([^/]+)$} => { s/user/group/ ? ::: Bugzilla/WebService/Util.pm @@ +163,5 @@ > +} > + > +# stringify all objects in data hash: > +sub stringify_json_objects { > + for my $val (@_) { s/for/foreach ::: rest.cgi @@ +26,5 @@ > +local @INC = (bz_locations()->{extensionsdir}, @INC); > +my $server = new Bugzilla::WebService::Server::REST; > +$server->version('1.1'); > +$server->handle(); > +exit; no need to exit at the end of a script.

New patch with suggested changes from your review and gerv's review. dkl

Comment on attachment 758165 [details] [diff] [review] Patch to enhance WebService API to allow REST style API calls (v3) Missing some files. New patch coming up.

Includes some missing files, etc.

Comment on attachment 759948 [details] [diff] [review] Patch to enhance WebService API to allow REST style API calls (v4) Review of attachment 759948 [details] [diff] [review]: ----------------------------------------------------------------- all your resource regexs need to be anchored at the start of the string in order to avoid conflicts. for example, you have /bug/field/([^/]+)/values$ /classification/([^/]+)$ both of these match /bug/field/classification/values in this case you get an error that "there is no classification named 'values'" due to it matches the regex in Bugzilla::WebService::Server::REST::Resources::Classification.pm. ::: Bugzilla/WebService/Server/REST.pm @@ +73,5 @@ > + > + # Fix includes/excludes for each call > + rest_include_exclude($params); > + > + #z Set callback name if exists nit: a stray z appears @@ +84,5 @@ > + my $obj = {}; > + $obj->{'version'} = "1.1"; > + $obj->{'id'} = correct_urlbase(); > + $obj->{'method'} = $self->bz_method_name; > + $obj->{'params'} = $params; do this with a single assignment, rather than 5: my $obj = { version => '1.1', ... @@ +501,5 @@ > +=item bz_rest_options > + > +=item bz_success_code > + > +=back all of these need POD documentation. ::: Bugzilla/WebService/Server/REST/Resources/Bug.pm @@ +21,5 @@ > + my $rest_resources = [ > + qr{/bug$}, { > + GET => { > + method => 'search', > + }, nit: trailing space @@ +95,5 @@ > + return { attachment_ids => [ $_[0] ] }; > + } > + } > + }, > + qr{/bug/field$}, { this means we can't access a bug with an alias 'field' by name via the rest api. perhaps it would be better to make this /field/bug ? ::: Bugzilla/WebService/Server/REST/Resources/Group.pm @@ +18,5 @@ > +}; > + > +sub _rest_resources { > + my $rest_resources = [ > + qr{/group$}, { A REST API resource was not found for 'GET /group'. ::: Bugzilla/WebService/Server/REST/Resources/Product.pm @@ +27,5 @@ > + || Bugzilla->input_params->{type} eq 'accessible'; > + return 'get_enterable_products' > + if Bugzilla->input_params->{type} eq 'enterable'; > + return 'get_selectable_products' > + if Bugzilla->input_params->{type} eq 'selectable'; passing in an invalid type (eg /product?type=cheese) returns "method is nothing.". it should return a more informative error. @@ +41,5 @@ > + method => 'get', > + params => sub { > + my $param = $_[0] =~ /^\d+$/ ? 'ids' : 'names'; > + return { $param => [ $_[0] ] }; > + } passing an invalid name returns an empty array instead of throwing an error. this is inconsistent with other methods (bug, classification, etc, all throw errors) ::: Bugzilla/WebService/Server/REST/Resources/User.pm @@ +20,5 @@ > +sub _rest_resources { > + my $rest_resources = [ > + qr{/user/login$}, { > + GET => { > + method => 'login' as this doesn't make any sense in the context of rest, login and logout should both throw errors (perhaps add a "rest_not_supported" error). ::: Bugzilla/WebService/Util.pm @@ +140,4 @@ > return \@objects; > } > > +sub rest_include_exclude { as this is specific to rest, please move it to the utility section of the Bugzilla/WebService/Server/REST package. @@ +162,5 @@ > + return $params; > +} > + > +# stringify all objects in data hash: > +sub stringify_json_objects { this method name and comment is misleading - all this method does is convert JSON boolean objects to 1 or 0 (integers, not strings), and leaves all other values untouched. given it's only used by the text/html display, where it'll be serialised to "true"/"false" without this method, i suggest deleting this. ::: template/en/default/rest.html.tmpl @@ +9,5 @@ > + "http://www.w3.org/TR/html4/loose.dtd"> > +<html> > + <head> > + <title>Bugzilla::REST::API</title> > + <link href="[% 'skins/standard/global.css' FILTER mtime %]" when accessing via a rewritten url (eg. ../rest/user/1), this stylesheet isn't loaded because it resolves to "rest/user/skins/standard/global.css".

(In reply to Byron Jones ‹:glob› from comment #17) > @@ +501,5 @@ > > +=item bz_rest_options > > + > > +=item bz_success_code > > + > > +=back > > all of these need POD documentation. Agreed. But I want to make sure the code is sane and then POD will be done as the last review. > ::: Bugzilla/WebService/Server/REST/Resources/Group.pm > @@ +18,5 @@ > > +}; > > + > > +sub _rest_resources { > > + my $rest_resources = [ > > + qr{/group$}, { > > A REST API resource was not found for 'GET /group'. > Currently Bugzilla/WebService/Group.pm only supports create() and update() so there is no method yet for getting information about a group. This can be addressed separately and the REST resources updated at the same time. > @@ +41,5 @@ > > + method => 'get', > > + params => sub { > > + my $param = $_[0] =~ /^\d+$/ ? 'ids' : 'names'; > > + return { $param => [ $_[0] ] }; > > + } > > passing an invalid name returns an empty array instead of throwing an error. > this is inconsistent with other methods (bug, classification, etc, all throw > errors) This happens in the normal Product.get call so the REST version just does the same. I will file a separate bug about this and when it is fixed, the REST API will follow. All other suggested changes have been done. dkl

Comment on attachment 766926 [details] [diff] [review] Patch to enhance WebService API to allow REST style API calls (v5) Review of attachment 766926 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to David Lawrence [:dkl] from comment #18) > > all of these need POD documentation. > Agreed. But I want to make sure the code is sane and then POD will be done > as the last review. now would be the time to do that :) we'll also need the qa testsuite updated to extend webservice coverage to rest (raise a bug for this if one doesn't already exist). ::: Bugzilla/WebService/Server/REST.pm @@ +28,5 @@ > +use Bugzilla::WebService::Server::REST::Resources::User; > + > +use Scalar::Util qw(blessed reftype); > +use MIME::Base64 qw(decode_base64); > +use Data::Dumper; data::dumper is unused @@ +274,5 @@ > + > +sub bz_success_code { > + my ($self, $value) = @_; > + $self->{_bz_success_code} = $value if $value; > + return $self->{_bz_success_code} || STATUS_OK; that's an unusual place for a default value. it would be clearer to set it to STATUS_OK while processing the request in handle(). @@ +415,5 @@ > + > +sub _fix_credentials { > + my ($self, $params) = @_; > + # Allow user to pass in &username=foo&password=bar > + if ($params->{'username'} && $params->{'password'}) { these tests should use 'exists' @@ +421,5 @@ > + $params->{'Bugzilla_password'} = delete $params->{'password'}; > + } > + > + # Allow a user to pass a valid cookie token in as parameters > + if ($params->{'userid'} && $params->{'cookie'}) { same here for 'exists' @@ +447,5 @@ > + my $score = scalar(@accept_types); > + for my $accept_type (@accept_types) { > + return $score if $type eq $accept_type; > + my $pat; > + ($pat = $accept_type) =~ s/([^\w*])/\\$1/g; # escape meta characters my $pat = quotemeta($accept_type);

Patch with updated pod documentation and other fixes. I have left ETag support and Cookie based auth for another bug once the basic support is in place. dkl

Comment on attachment 773055 [details] [diff] [review] Patch to enhance WebService API to allow REST style API calls (v6) Review of attachment 773055 [details] [diff] [review]: ----------------------------------------------------------------- r=glob the following points need to be addressed at commit time. ::: Bugzilla/WebService/Server/REST.pm @@ +23,5 @@ > +use Bugzilla::WebService::Server::REST::Resources::Bug; > +use Bugzilla::WebService::Server::REST::Resources::Bugzilla; > +use Bugzilla::WebService::Server::REST::Resources::Group; > +use Bugzilla::WebService::Server::REST::Resources::Product; > +use Bugzilla::WebService::Server::REST::Resources::User; Bugzilla::WebService::Server::REST::Resources::Classification is missing, which means /rest/classification/$name no longer works. @@ +494,5 @@ > +The endpoint for the REST interface is the C<rest.cgi> script in > +your Bugzilla installation. You can possibly configure C<.htaccess> > +to have a path based end point using mod_rewrite. For example, if your > +Bugzilla is at C<bugzilla.yourdomain.com>, then your REST client would > +access the API via: C<http://bugzilla.yourdomain.com/rest/bug/35>. updating .htaccess is already part of this patch, so you don't need to mention that here. instead you should say that if mod_rewrite is installed and enabled, you can also use /rest/.. as an endpoint. @@ +555,5 @@ > +=head1 SEE ALSO > + > +L<Bugzilla::WebService> > + > +=head1 B<Methods in need of POD> all of these methods need to be documented, even minimally - new methods shouldn't be added without POD.

Comment on attachment 773055 [details] [diff] [review] Patch to enhance WebService API to allow REST style API calls (v6) Review of attachment 773055 [details] [diff] [review]: ----------------------------------------------------------------- I'm a little bit sad that we haven't found time to discuss the suggestions in the API Comparison document and whether they are worth making https://wiki.mozilla.org/Bugzilla:API_Comparison . If this patch is barrelling towards being checked in, I hope we will have time to do so soon after it is, and that discussion won't be closed off because we've deployed an initial implementation. How can we best indicate to potential users of this API that it's currently experimental? ::: .htaccess @@ +28,5 @@ > </IfModule> > + > +<IfModule mod_rewrite.c> > + RewriteEngine On > + RewriteRule ^rest/(.*)$ rest.cgi/$1 [NE] Is it worth putting a "/1.0" (or even, at this stage, a "/0.1") in the URL from the very start? Or do you deal with that inside rest.cgi? ::: Bugzilla/WebService.pm @@ +276,5 @@ > { users => [{ id => 1, name => 'user@domain.com' }] } > > +Note for REST, C<include_fields> may instead be a comma delimited string > +for GET type requests. > + Is this for compatibility, an artifact of implementation, an attempt to implement a newer include_fields spec or something else? ::: Bugzilla/WebService/Bug.pm @@ +1098,5 @@ > +part is the request method and the rest is the related path needed. > + > +To get information about all fields: > + > +GET /field/bug Clue me in on why this URL has "/bug" in it? @@ +1401,5 @@ > +GET /bug/<bug_id>/attachment > + > +To get a specific attachment based on attachment ID: > + > +GET /bug/attachment/<attachment_id> Is this URL intentionally or unintentionally incompatible with BzAPI? BzAPI uses /attachment/<attachment_id>. This is for a few reasons; for example, it's entirely possible one day (do we even support it now?) for the same attachment to be attached to multiple bugs. Or for an attachment to move between bugs. I'm sure I had more reasons, although they aren't coming to me right now. @@ +1628,5 @@ > +GET /bug/<id_or_alias>/comment > + > +To get a specific comment based on the comment ID: > + > +GET /bug/comment/<comment_id> What happens if a bug has the alias "comment"? Are you sure we want to support aliases in REST URLs? ::: Bugzilla/WebService/Bugzilla.pm @@ +287,5 @@ > +=item B<REST> > + > +GET /timezone > + > +The returned data format is the same as below. Can we please not return the legacy/deprecated values on our shiny new interface? And perhaps only return one time, now they are all the same? See https://wiki.mozilla.org/Bugzilla:API_Comparison#Get_info_about_Bugzilla.27s_notions_of_time . @@ +484,5 @@ > Gets the latest time of the audit_log table. > > +=item B<REST> > + > +GET /last_audit_time Is it really worth having a special call just for this value? ::: Bugzilla/WebService/Constants.pm @@ +240,5 @@ > ); > > +use constant REST_CONTENT_TYPE_WHITELIST => qw( > + text/html > + application/json-rpc What does it mean to have the application/json-rpc value here? ::: Bugzilla/WebService/Server/REST/Resources/Bug.pm @@ +4,5 @@ > +# > +# This Source Code Form is "Incompatible With Secondary Licenses", as > +# defined by the Mozilla Public License, v. 2.0. > + > +package Bugzilla::WebService::Server::REST::Resources::Bug; Is the "Resources::" in the package name a bit of overkill? :-) @@ +87,5 @@ > + }, > + success_code => STATUS_CREATED > + } > + }, > + qr{^/bug/attachment/([^/]+)$}, { This is where I think you should remove "/bug". Gerv ::: Bugzilla/WebService/User.pm @@ +612,5 @@ > Updates user accounts in Bugzilla. > > +=item B<REST> > + > +PUT /user/<user_id_or_name> Names are not unique; how can we do this with anything other than IDs? @@ +730,5 @@ > +=item B<REST> > + > +To get information about a single user: > + > +GET /user/<user_id_or_name> Same question as above. ::: rest.cgi @@ +24,5 @@ > +use Bugzilla::WebService::Server::REST; > +Bugzilla->usage_mode(USAGE_MODE_REST); > +local @INC = (bz_locations()->{extensionsdir}, @INC); > +my $server = new Bugzilla::WebService::Server::REST; > +$server->version('1.1'); Why 1.1? :-) ::: template/en/default/rest.html.tmpl @@ +9,5 @@ > + "http://www.w3.org/TR/html4/loose.dtd"> > +<html> > + <head> > + <title>Bugzilla::REST::API</title> > + <link href="[% urlbase FILTER none %]skins/standard/global.css' FILTER mtime %]" This line seems to have mismatched [% %] on it; have you tested the HTML output mode? @@ +13,5 @@ > + <link href="[% urlbase FILTER none %]skins/standard/global.css' FILTER mtime %]" > + rel="stylesheet" type="text/css"> > + </head> > + <body> > + <pre>[% result FILTER html %]</pre> Is result a string, then, or a structure? If a structure, this doesn't look like it would work.

(In reply to Gervase Markham [:gerv] from comment #22) > I'm a little bit sad that we haven't found time to discuss the suggestions > in the API Comparison document and whether they are worth making > https://wiki.mozilla.org/Bugzilla:API_Comparison . If this patch is > barrelling towards being checked in, I hope we will have time to do so soon > after it is, and that discussion won't be closed off because we've deployed > an initial implementation. initially the REST API will be compatible with the current webservice data format -- ie. it simply exposes the current xmlrpc/jsonrpc methods via rest. i agree we need to have the discussion about building a saner rest api for native bugzilla (leaning heavily on the decisions make for bzapi of course), however those discussions wouldn't have been relevant to this patch. > How can we best indicate to potential users of this API that it's currently > experimental? mark it as experimental in the documentation, which dkl has already done. > Is it worth putting a "/1.0" (or even, at this stage, a "/0.1") in the URL > from the very start? Or do you deal with that inside rest.cgi? for parity with the current endpoints, we don't need versioning in the url. once we start making the rest urls saner, then, yes, we'll need a versioned url (at the very least to clearly indicate divergence between the xmlrpc/jsonrpc/json-p endpoints and the rest endpoint). > > +To get information about all fields: > > +GET /field/bug > > Clue me in on why this URL has "/bug" in it? bugzilla's field method is Bug.field. we can't use /bug/field as this would make it impossible to access a bug named "field", so they are reversed. i agree this is one (of many) things that we'd want to change when we rework the rest methods, parameters and results. > Is this URL intentionally or unintentionally incompatible with BzAPI? compatibility with bzapi isn't a goal for this patch -- bug 880669 is about bzapi compatibility, and will have a different endpoint. > > +GET /bug/comment/<comment_id> > > What happens if a bug has the alias "comment"? that's fine; there's no conflict in the paths as you don't do /bug/$alias/$comment_id > Are you sure we want to support aliases in REST URLs? yes, because aliases are supported in the xmlrpc/jsonrpc endpoints. > Can we please not return the legacy/deprecated values on our shiny new > interface? And perhaps only return one time, now they are all the same? all calls in the current endpoints are implemented via the rest endpoint. if we're to remove legacy/deprecated methods, then they need to be removed from all endpoints, not just rest. > > +use constant REST_CONTENT_TYPE_WHITELIST => qw( > > + text/html > > + application/json-rpc > > What does it mean to have the application/json-rpc value here? good question -- dkl? > > +PUT /user/<user_id_or_name> > > Names are not unique; how can we do this with anything other than IDs? for the current webserivces, "name" means "login" not "real name". as logins are unique, we're ok here. also, urgh. > ::: template/en/default/rest.html.tmpl > @@ +9,5 @@ > > + "http://www.w3.org/TR/html4/loose.dtd"> > > +<html> > > + <head> > > + <title>Bugzilla::REST::API</title> > > + <link href="[% urlbase FILTER none %]skins/standard/global.css' FILTER mtime %]" > > This line seems to have mismatched [% %] on it; have you tested the HTML > output mode? good catch. i tested it, but this surprisingly doesn't result in a syntax error, instead the stylesheet loading is "http://fedora/866927/skins/standard/global.css' FILTER mtime %]". as the fix is trivial, i don't think we need another revision just for this. > > + <pre>[% result FILTER html %]</pre> > > Is result a string, then, or a structure? If a structure, this doesn't look > like it would work. it's a json object, which has a stringify override, and it works.

New patch with POD updates and also fixed the rest template error. Moving r+ forward.

(In reply to Byron Jones ‹:glob› from comment #23) > initially the REST API will be compatible with the current webservice data > format -- ie. it simply exposes the current xmlrpc/jsonrpc methods via rest. Is there some reason that we need to expose all the current methods via REST, even those that no-one has indicated a desire to use via REST, and that we think are bogus? > i agree we need to have the discussion about building a saner rest api for > native bugzilla (leaning heavily on the decisions make for bzapi of course), > however those discussions wouldn't have been relevant to this patch. That gives me comfort :-) My concern then is that we make sure to either a) namespace this API using a version number, from the very start, and/or b) communicate clearly to people that it may change at any time without warning. > > Is it worth putting a "/1.0" (or even, at this stage, a "/0.1") in the URL > > from the very start? Or do you deal with that inside rest.cgi? > > for parity with the current endpoints, we don't need versioning in the url. That's not all that strong an argument; the current endpoints have ".cgi" in the name, but we've eliminated that for REST :-) Given that we know this API will change a lot, I think there's a good argument for versioning it from the start. There is no backwards compatibility issue here with the URL format. > once we start making the rest urls saner, then, yes, we'll need a versioned > url (at the very least to clearly indicate divergence between the > xmlrpc/jsonrpc/json-p endpoints and the rest endpoint). But then we'll have "used" the unversioned version of the URL. > > Is this URL intentionally or unintentionally incompatible with BzAPI? > > compatibility with bzapi isn't a goal for this patch -- bug 880669 is about > bzapi compatibility, and will have a different endpoint. OK, but it seems like a gratuitous incompatibility... > > Are you sure we want to support aliases in REST URLs? > > yes, because aliases are supported in the xmlrpc/jsonrpc endpoints. I hope this argument will not be deployed when we get around to thinking about the way things _should_ be :-)) > > Can we please not return the legacy/deprecated values on our shiny new > > interface? And perhaps only return one time, now they are all the same? > > all calls in the current endpoints are implemented via the rest endpoint. > if we're to remove legacy/deprecated methods, then they need to be removed > from all endpoints, not just rest. I don't think it's technically difficult to have them returning different data, particularly if it's just a matter of stripping some stuff out. We need a "data hacking" callback of some sort anyway, I suspect - for the BzAPI compat addon, if nothing else. It seems weird to release the first version of an API with some fields already deprecated! Gerv

(In reply to Gervase Markham [:gerv] from comment #25) > It seems weird to release the first version of an API with some fields already deprecated! this is probably the crux of your confusion - this patch implements a new transport mechanism for the existing bugzilla api, warts and all. > Is there some reason that we need to expose all the current methods via REST, even > those that no-one has indicated a desire to use via REST, and that we think are bogus? on monday erik (bugzilla-ember) stated that this would be sufficient for his needs for the time being.

reply to Byron Jones ‹:glob› from comment #26) > on monday erik (bugzilla-ember) stated that this would be sufficient for his > needs for the time being. True, but he didn't say he needed _all_ of it. Are we basically going to end up with two REST APIs, an initial incomplete, not-very-RESTy[0] one and a complete, more-RESTy, backwardly-compatible one which comes later? If so, once we have the second, can we declare the first to be an experiment and shut it down? :-P Gerv [0] See the API comparison doc for examples.

(In reply to Gervase Markham [:gerv] from comment #25) > I don't think it's technically difficult to have them returning different > data, particularly if it's just a matter of stripping some stuff out. We > need a "data hacking" callback of some sort anyway, I suspect - for the > BzAPI compat addon, if nothing else. It seems weird to release the first > version of an API with some fields already deprecated! That is exactly how the new BzAPI extension works in that there are three new hooks in the core REST code that allows for data manipulation before the client sees it. Also the hook allows for overriding and adding new resources for example /count, /configuration, and /bug/<id>/flags. (In reply to Gervase Markham [:gerv] from comment #27) > If so, once we have the second, can we declare the first to be an experiment > and shut it down? :-P Shutting down once the compat REST API is available, no. The end goal is to merge the two into a single REST API, taking things that make sense from both. For example returning users in bug data as objects instead of simply an login name is something I would like to see happen in the other APIs as well so we would not just leave that in the compat layer. dkl

OK. So my suggestion is that we use the new hooks so that the new API endpoint, to which people will be coding specific clients, doesn't begin by exposing fields (and calls) which are already deprecated. There's just no point; the only thing it'll do is run the risk of leading people who don't notice the deprecation tags to write new code which relies on things which are going away soon. It would also be good to have core code using those hooks/callbacks anyway. Gerv

It was my understanding that this "native" REST API was supposed to be compatible with the existing REST API. It appears that the responses are not identical. Did I misunderstand this?

Erik: this is the thing I was attempting to explain on the call. :-) For better or worse, this patch merely exposes the current API over a REST-y endpoint. The result is similar to BzAPI, but by no means identical. A "compatibility" extension which adds full BzAPI compatibility is some weeks away. Gerv

Erik, oh, sadly, it seems that you did. This patch gives the functionality of the existing APIs (XMLRPC, JSONRPC, and JSONP) over a REST-like HTTP interface. I understood from Monday's meeting that this gave you most of what you would need in the short term.

(In reply to Erik Bryn from comment #30) > It was my understanding that this "native" REST API was supposed to be > compatible with the existing REST API. It appears that the responses are not > identical. Did I misunderstand this? Sorry for the confusion with all of the APIs floating around it seems. From my understanding in the call as that the JSONRPC API internal to Bugzilla had most of what you needed to perform the data calls for your Bugzilla API. The new REST endpoint is very similar in that the data returned is similar to what JSONRPC returns. Just the way you request the data is different. Also as we mentioned that main thing lacking in the native APIs (all) is the ability to the same more powerful search that the external BzAPI provides but we are working on adding that soon (bug 477601). We should meet soon to figure out what specific piece of the native API are not being returned that you need and discuss the differences in the data format as well. dkl

Thanks justdave. Any further changes/improvements will be addressed in new individual bug reports. Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk modified .htaccess modified Bugzilla.pm added rest.cgi modified Bugzilla/CGI.pm modified Bugzilla/Constants.pm modified Bugzilla/Error.pm modified Bugzilla/Mailer.pm modified Bugzilla/WebService.pm modified Bugzilla/Install/Requirements.pm modified Bugzilla/WebService/Bug.pm modified Bugzilla/WebService/Bugzilla.pm modified Bugzilla/WebService/Classification.pm modified Bugzilla/WebService/Constants.pm modified Bugzilla/WebService/Group.pm modified Bugzilla/WebService/Product.pm modified Bugzilla/WebService/User.pm added Bugzilla/WebService/Server/REST added Bugzilla/WebService/Server/REST.pm added Bugzilla/WebService/Server/REST/Resources added Bugzilla/WebService/Server/REST/Resources/Bug.pm added Bugzilla/WebService/Server/REST/Resources/Bugzilla.pm added Bugzilla/WebService/Server/REST/Resources/Classification.pm added Bugzilla/WebService/Server/REST/Resources/Group.pm added Bugzilla/WebService/Server/REST/Resources/Product.pm added Bugzilla/WebService/Server/REST/Resources/User.pm added template/en/default/rest.html.tmpl modified template/en/default/global/user-error.html.tmpl Committed revision 8646.

POD in Bug.pm was incorrect. You mixed comments from 3.4 with REST comments. Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/ modified Bugzilla/WebService/Bug.pm Committed revision 8737.

Comment on attachment 773541 [details] [diff] [review] Patch to enhance WebService API to allow REST style API calls (v7) >=== added file 'Bugzilla/WebService/Server/REST.pm' >+use Bugzilla; I just realized that you call Bugzilla.pm from a Perl module. We explicitly removed these dependencies in bug 303413 (Bugzilla 2.22). So I removed it from here too: To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git 9a2f20f..8796328 master -> master

Comment on attachment 773541 [details] [diff] [review] Patch to enhance WebService API to allow REST style API calls (v7) >=== added file 'template/en/default/rest.html.tmpl' >+ <link href="[% urlbase FILTER none %][% 'skins/standard/global.css' FILTER mtime %]" urlbase must be HTML filtered, else undesired characters can be injected.

Added to relnotes for 5.0rc1.