Closed
Bug 880669
Opened 11 years ago
Closed 10 years ago
Extend current BzAPI BMO extension to contain compatibility changes on top of native rest
Categories
(bugzilla.mozilla.org :: Extensions, enhancement, P1)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dkl, Assigned: dkl)
References
Details
(Keywords: bmo-goal)
Attachments
(1 file, 8 obsolete files)
(deleted),
patch
|
glob
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•11 years ago
|
||
I think that if we use an extension to achieve full BzAPI compatibility, then it should be a separate extension rather than part of the BMO extension. There are lots of other non-BMO sites using BzAPI which would probably like to use it.
Gerv
Assignee | ||
Comment 2•11 years ago
|
||
(In reply to Gervase Markham [:gerv] from comment #1)
> I think that if we use an extension to achieve full BzAPI compatibility,
> then it should be a separate extension rather than part of the BMO
> extension. There are lots of other non-BMO sites using BzAPI which would
> probably like to use it.
>
> Gerv
Extending the current BzAPI seemed to make sense at the time and I was planning of course to leave all the functionality that the current BzAPI extension provides intact where necessary. From what I can see it does the following:
1. Adds the json Template.pm filter which we already have in Bugzilla/Template.pm now so we can get rid of that.
2. Provides the config.json.tmpl template allowing config.cgi?ctype=json to work.
3. Adds the following var entries to config.json.tmpl in Extension.pm.
$vars->{'initial_status'} = Bugzilla::Status->can_change_to;
$vars->{'status_objects'} = [Bugzilla::Status->get_all];
So I can definitely leave 2 and 3 alone and combine the new functionality into the BzAPI. Or if you feel strongly enough, I suppose I could just create a new extension with a different name.
dkl
Comment 3•11 years ago
|
||
Er... there seems to be some confusion here. I read the title as suggesting that we extending the "BMO" extension. Did you mean extending the "BzAPI" extension, the one which currently ships with BzAPI?
Gerv
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Gervase Markham [:gerv] from comment #3)
> Er... there seems to be some confusion here. I read the title as suggesting
> that we extending the "BMO" extension. Did you mean extending the "BzAPI"
> extension, the one which currently ships with BzAPI?
>
> Gerv
Ah sorry for the confusion. I did not realize you had your own copy of the BzAPI Bugzilla extension in the mercurial repo for the BzAPI (Catalyst app). I only see what is in the bmo/4.2 repo and saw the current extensions/BzAPI code that is there. Basically I meant only to extend extensions/BzAPI that we currently have in the BMO tree and leave what is in your mercurial repo alone. The end goal of all this is to have 1) native rest in upstream bugzilla and 2) have a BzAPI like extension on top of it for use primarily with BMO. I do not see any reason that we cannot just continue to expand the current BzAPI BMO extension as people who work directly on BMO code should not be confused as to it's purpose.
dkl
Comment 5•11 years ago
|
||
Ah, now it's more clear. Yes, that seems like a fine plan, although we should make sure that the new BzAPI extension that you write does not have BMO-specific dependencies, so that other Bugzillas can use it too. After all, when the REST API goes upstream, lots of sites may want to shut down their BzAPI installations but still have BzAPI compatibility.
I actually hope that this extension will be very minimal because we will agree on a way of converging the two APIs in almost all cases. See https://wiki.mozilla.org/Bugzilla:API_Comparison , and your inbox :-)
Gerv
Assignee | ||
Comment 6•11 years ago
|
||
(In reply to Gervase Markham [:gerv] from comment #5)
> Ah, now it's more clear. Yes, that seems like a fine plan, although we
> should make sure that the new BzAPI extension that you write does not have
> BMO-specific dependencies, so that other Bugzillas can use it too. After
> all, when the REST API goes upstream, lots of sites may want to shut down
> their BzAPI installations but still have BzAPI compatibility.
The only purpose of the extension changes will be to make the current implementation of the native rest "look" to the client like it is coming from BzAPI. I should be self-supporting do not think it will need to rely on any other BMO specific customizations.
> I actually hope that this extension will be very minimal because we will
> agree on a way of converging the two APIs in almost all cases. See
> https://wiki.mozilla.org/Bugzilla:API_Comparison , and your inbox :-)
Hope so too :)
Comment 7•11 years ago
|
||
That design sounds to me like it will make it impossible to run both "Standard" and "BzAPI" versions side-by-side, meaning that any transition will have to be a flag day.
Would it make more sense for the BzAPI-compatible version to appear on another URL, e.g. "/bzapi"? That way, users can move immediately from api-dev to b.m.o/bzapi, and then (if we decide they need to) gradually from /bzapi to /rest.
Gerv
Assignee | ||
Comment 8•11 years ago
|
||
(In reply to Gervase Markham [:gerv] from comment #7)
> That design sounds to me like it will make it impossible to run both
> "Standard" and "BzAPI" versions side-by-side, meaning that any transition
> will have to be a flag day.
>
> Would it make more sense for the BzAPI-compatible version to appear on
> another URL, e.g. "/bzapi"? That way, users can move immediately from
> api-dev to b.m.o/bzapi, and then (if we decide they need to) gradually from
> /bzapi to /rest.
>
> Gerv
No. As I have mentioned in other discussions, I would have different entry points depending on which API is desired until we can get the native REST API up to speed. A basic .htaccess entry would do this for now.
<IfModule mod_rewrite.c>
RewriteEngine On
RewriteRule ^rest/(.*)$ rest.cgi/$1 [NE]
RewriteRule ^bzapi/(.*)$ extensions/BzAPI/bin/rest.cgi/$1 [NE]
</IfModule>
dkl
Comment 9•11 years ago
|
||
I'm confused. You said "No", then seemed to outline a way to do exactly what I was suggesting. What have I missed?
Gerv
Assignee | ||
Comment 10•11 years ago
|
||
(In reply to Gervase Markham [:gerv] from comment #9)
> I'm confused. You said "No", then seemed to outline a way to do exactly what
> I was suggesting. What have I missed?
>
> Gerv
Sorry. "No" was to your comment that "That design sounds to me like it will make it impossible to run both Standard and BzAPI versions side-by-side, meaning that any transition will have to be a flag day."
dkl
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #8343374 -
Flags: feedback?(glob)
Comment 13•11 years ago
|
||
Comment on attachment 8343374 [details] [diff] [review]
880669_1.patch
Review of attachment 8343374 [details] [diff] [review]:
-----------------------------------------------------------------
the design of the extension with hooks looks sound to me.
Attachment #8343374 -
Flags: feedback?(glob) → feedback+
Assignee | ||
Comment 14•11 years ago
|
||
WIP part 2. Closer to what I feel like is worthy of a finished review. Still have the empty stubs in places which will be removed once I deem that no alterations are needed.
dkl
Attachment #8365067 -
Flags: review?(glob)
Comment 15•11 years ago
|
||
Comment on attachment 8365067 [details] [diff] [review]
WIP 2
as it's still WIP, switching from review? to feedback?
Attachment #8365067 -
Flags: review?(glob) → feedback?(glob)
Comment 16•11 years ago
|
||
Comment on attachment 8365067 [details] [diff] [review]
WIP 2
Review of attachment 8365067 [details] [diff] [review]:
-----------------------------------------------------------------
it appears tracking flags are still missing from the output.
unfortunately this makes it very difficult for me to do side-by-side/code comparisons of the output.
i've done a quick stroll through the code, but would like to see that fixed before going any further.
::: Bugzilla/WebService/Server/REST.pm
@@ +311,4 @@
> sub rest_include_exclude {
> my ($params) = @_;
>
> + if (exists $params->{'include_fields'} && !ref $params->{'include_fields'}) {
i don't think we should be removing support for _all and _default.
::: extensions/BzAPI/lib/Util.pm
@@ +153,5 @@
> +
> + # Remove empty values
> + foreach my $key (keys %$data) {
> + next if $key eq 'qa_contact'; # Return qa_contact even if null
> + next if $key eq 'keywords'; # Return keywords even if empty
keywords shouldn't be returned if emtpy
@@ +159,5 @@
> + delete $data->{$key} if !$data->{$key};
> + }
> + else {
> + if (ref $data->{$key} eq 'ARRAY' && !@{$data->{$key}}) {
> + # Return empty string if blocks or depends_on is empty
bzapi doesn't return these fields if they are empty
@@ +353,5 @@
> +
> +# Copy of Bugzilla::WebService::Util::filter_wants but without the caching
> +# as we make several webservice calls in a single REST call and this doesn't
> +# always work as you would expect.
> +sub filter_wants ($$;$) {
to avoid confusion, please give this a different name.
Attachment #8365067 -
Flags: feedback?(glob) → feedback-
Assignee | ||
Comment 17•11 years ago
|
||
Fixed the tracking flag issue as well as added cc_count and dupe_count columns.
dkl
Attachment #8365067 -
Attachment is obsolete: true
Attachment #8366679 -
Flags: feedback?(glob)
Assignee | ||
Comment 18•11 years ago
|
||
Some more fixes.
Attachment #8366679 -
Attachment is obsolete: true
Attachment #8366679 -
Flags: feedback?(glob)
Attachment #8367648 -
Flags: feedback?(glob)
Comment 19•11 years ago
|
||
Comment on attachment 8367648 [details] [diff] [review]
WIP 4
Review of attachment 8367648 [details] [diff] [review]:
-----------------------------------------------------------------
this looks mostly good. f- due to the monkeypatch issues.
::: extensions/BzAPI/Extension.pm
@@ +27,5 @@
> +# Monkey Patches #
> +##################
> +
> +BEGIN {
> + *Bugzilla::WebService::Util::filter_wants = \&_filter_wants;
i don't like the idea of monkey patching over existing methods. won't it impact subsequent requests processed by the same process under mod_perl? it also results in 'redefined' errors when running tests.
it would be better to update Bugzilla::WebService::Util::filter_wants to accept an optional arg to disable caching, or to create a new sub with a different name and call that.
::: extensions/BzAPI/lib/Resources/Bugzilla.pm
@@ +109,5 @@
> + Bugzilla->template->process('config.json.tmpl', $vars, \$json);
> + my $result = {};
> + if ($json) {
> + $result = $self->json->decode($json);
> + }
it's a bit silly that we're building stuff in perl, using a template to create a json string, then parsing it again back into perl to return. it would be quicker to build the response in perl.
that said, that would take a reasonable amount of time/effort and probably isn't worth it.
::: extensions/BzAPI/lib/Util.pm
@@ +51,5 @@
> +
> + # Stuff you can't change, or change directly
> + my @immutable = ('reporter', 'creation_time', 'id',
> + 'ref', 'is_everconfirmed', 'remaining_time',
> + 'actual_time', 'percentage_complete');
nit: would look nicer if you used qw()
@@ +143,5 @@
> +
> + # Comments and history are optional and included if explicitly requested
> +
> + # Comments
> + if (grep($_ eq 'comments', @{ $params->{include_fields} })) {
out of interest, why aren't you using filter_wants() here?
@@ +216,5 @@
> + return { name => undef } if !$user;
> +
> + my $data = {
> + name => $rpc->type('email', $user->login),
> + real_name => $rpc->type('string', encode_utf8($user->name)),
i'm surprised to see encode_utf8 here; why is that required for the username and not for any other fields?
@@ +303,5 @@
> +}
> +
> +# convert certain attributes of a flag object from
> +# scalar values to related objects. Also remove other unwanted
> +# key/values.
nit: this doesn't appear to remove any unwanted key/values
@@ +319,5 @@
> + return $flag;
> +}
> +
> +# convert certain attributes of a group object from scalar
> +# values to related objects. Also remove other unwanted key/values.
or here :)
@@ +368,5 @@
> + 'flags', 'groups', 'comments', 'attachments', 'history');
> + }
> + }
> +
> + if ($call eq 'Bug.attachments') {
elsif
Attachment #8367648 -
Flags: feedback?(glob) → feedback-
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #8367648 -
Attachment is obsolete: true
Attachment #8390108 -
Flags: feedback?(glob)
Attachment #8343374 -
Attachment is obsolete: true
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #8390108 -
Attachment is obsolete: true
Attachment #8390108 -
Flags: feedback?(glob)
Attachment #8398629 -
Flags: review?(glob)
Assignee | ||
Comment 22•11 years ago
|
||
Comment on attachment 8398629 [details] [diff] [review]
880669_1.patch
Review of attachment 8398629 [details] [diff] [review]:
-----------------------------------------------------------------
::: report.cgi
@@ +129,5 @@
> +
> +use Data::Dumper;
> +print STDERR Dumper \@axis_fields;
> +print STDERR Dumper scalar $params->Vars;
> +
Ugh. These will be gone in the next revision. Keep calm and review on.
@@ +143,5 @@
> Bugzilla->switch_to_shadow_db();
> my ($results, $extra_data) = $search->data;
>
> +print STDERR Dumper $results;
> +
blech
Assignee | ||
Comment 23•11 years ago
|
||
Small fixes.
Attachment #8398629 -
Attachment is obsolete: true
Attachment #8398629 -
Flags: review?(glob)
Attachment #8398810 -
Flags: review?(glob)
Assignee | ||
Comment 24•11 years ago
|
||
Installed on https://bugzilla-dev.allizom.org/bzapi
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
b8f1c74..92bc99e 4.2-dev -> 4.2-dev
Assignee | ||
Comment 25•11 years ago
|
||
test comment. please ignore this.
Comment 26•11 years ago
|
||
https://bugzilla-dev.allizom.org/bzapi/bug/880669?include_fields=_all
Noticed at a glance:
* full email addresses, which are not available in BzAPI when not logged-in, are exposed
* "attachments" is not included
* "blocks" is empty
* "depends_on" has only open bugs
* "keywords" is empty
Comment 27•11 years ago
|
||
Please discard my comment above. The data on the dev server is just outdated.
Assignee | ||
Comment 28•11 years ago
|
||
Updated to match BzAPI in that it filters email addresses if a user is not logged in whereas for BMO currently we do not do such filtering.
dkl
Attachment #8398810 -
Attachment is obsolete: true
Attachment #8398810 -
Flags: review?(glob)
Attachment #8399470 -
Flags: review?(glob)
Assignee | ||
Comment 29•11 years ago
|
||
Comment on attachment 8399470 [details] [diff] [review]
880669_3.patch
Review of attachment 8399470 [details] [diff] [review]:
-----------------------------------------------------------------
::: Bugzilla/WebService/Bug.pm
@@ +933,5 @@
> my $flags = delete $params->{flags};
> + my $comment = delete $params->{comment};
> +
> + use Data::Dumper;
> + print STDERR Dumper $params;
Debugging will be gone in next revision or on commit :(
Comment 30•11 years ago
|
||
The bug "id" field is an integer, not a string.
Assignee | ||
Comment 31•11 years ago
|
||
(In reply to Kohei Yoshino [:kohei] from comment #30)
> The bug "id" field is an integer, not a string.
'id' is present in many places. Can you give me the URL you are using where it is
showing as a string which I can use to track it down.
Thanks
dkl
Comment 32•11 years ago
|
||
"id" : "880669" can be found in
https://bugzilla-dev.allizom.org/bzapi/bug/880669
Somehow it's integer if include_fields is set:
https://bugzilla-dev.allizom.org/bzapi/bug/880669?include_fields=id%2Csummary
Comment 33•11 years ago
|
||
(In reply to Kohei Yoshino [:kohei] from comment #32)
> Somehow it's integer if include_fields is set:
> https://bugzilla-dev.allizom.org/bzapi/bug/880669?include_fields=id%2Csummary
Ahh, it's still a string in a JSON response.
Comment 34•11 years ago
|
||
* The "blocks" and "depends_on" fields should not exist if there are no corresponding blocker bugs. Currently it's an empty string.
* The "qa_contact" > "name" field should be an empty string, not null, if it's not set.
* The "creator" and "assigned_to" fields should have "name" and "real_name". Currently it has null "name" when include_fields is set:
https://bugzilla-dev.allizom.org/bzapi/bug/880669?include_fields=id%2Csummary%2Ccreator%2Cassigned_to%2Cqa_contact%2Ccreation_time
https://api-dev.bugzilla.mozilla.org/latest/bug/880669?include_fields=id%2Csummary%2Ccreator%2Cassigned_to%2Cqa_contact%2Ccreation_time
Comment 35•11 years ago
|
||
BzAPI has the (undocumented) "raw_text" and "tags" fields in comments, which are not included in the native API:
https://api-dev.bugzilla.mozilla.org/latest/bug/906943/comment
https://bugzilla-dev.allizom.org/bzapi/bug/906943/comment
Looks like the "id" and "bug_id" fields in attachments are also strings, should be numbers. And the file sizes are 0:
https://api-dev.bugzilla.mozilla.org/latest/bug/818340/attachment
https://bugzilla-dev.allizom.org/bzapi/bug/818340/attachment
Assignee | ||
Comment 36•11 years ago
|
||
(In reply to Kohei Yoshino [:kohei] from comment #35)
> BzAPI has the (undocumented) "raw_text" and "tags" fields in comments, which
> are not included in the native API:
> https://api-dev.bugzilla.mozilla.org/latest/bug/906943/comment
> https://bugzilla-dev.allizom.org/bzapi/bug/906943/comment
FWIW, 'tags' is only dropped if it is empty. BzAPI will return the most extra fields that are returned via the native webservices API without question. The test suite for BzAPI did however complain about any key/values it didn't know about which is why they are not present. I will add them back in in the next revision.
> Looks like the "id" and "bug_id" fields in attachments are also strings,
> should be numbers. And the file sizes are 0:
> https://api-dev.bugzilla.mozilla.org/latest/bug/818340/attachment
> https://bugzilla-dev.allizom.org/bzapi/bug/818340/attachment
Found this issue here and will be fixed in my code revision. What was happening is the values were integers before the extension code was reached but since I used the values to create other key/values, Perl had converted them to strings automatically and I just need to reset them back to integers before completing.
dkl
Comment 37•10 years ago
|
||
Comment on attachment 8399470 [details] [diff] [review]
880669_3.patch
clearing review.
this patch will be updated to accommodate the changes from bug 1000913.
Attachment #8399470 -
Flags: review?(glob)
Assignee | ||
Comment 38•10 years ago
|
||
Attachment #8399470 -
Attachment is obsolete: true
Attachment #8437429 -
Flags: review?(glob)
Comment 39•10 years ago
|
||
Comment on attachment 8437429 [details] [diff] [review]
880669_4.patch
Review of attachment 8437429 [details] [diff] [review]:
-----------------------------------------------------------------
r=glob
where appropriate please file upstream bugs for the fixes in Bugzilla/WebService* (utf8 fixes, sorting, custom dt fields, ...)
::: extensions/BzAPI/bin/rest.cgi
@@ +7,5 @@
> +# defined by the Mozilla Public License, v. 2.0.
> +
> +use 5.10.1;
> +use strict;
> +use lib qw(../../.. . lib);
i don't think . and lib are correct there - qw(../../.. ../../../lib)
Attachment #8437429 -
Flags: review?(glob) → review+
Assignee | ||
Comment 40•10 years ago
|
||
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
dd10df6..0a2592d master -> master
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 41•10 years ago
|
||
https://bugzilla-dev.allizom.org/bzapi/bug/880669
Somehow the creator and assigned_to are missing?
Assignee | ||
Comment 42•10 years ago
|
||
(In reply to Kohei Yoshino [:kohei] from comment #41)
> https://bugzilla-dev.allizom.org/bzapi/bug/880669
>
> Somehow the creator and assigned_to are missing?
bugzilla-dev.allizom.org is not running the latest code that was just committed. bugzilla.allizom.org (LDAP protected) is. I will merge master into development today so you can continue to test.
Thanks
dkl
Assignee | ||
Comment 43•10 years ago
|
||
(In reply to Kohei Yoshino [:kohei] from comment #41)
> https://bugzilla-dev.allizom.org/bzapi/bug/880669
>
> Somehow the creator and assigned_to are missing?
bugzilla-dev has been been merged now so please give it another try.
Thanks
dkl
Comment 44•10 years ago
|
||
Awesome! Looks like the id, attachment.id, attachment.bug_id are still strings, not integers:
> id:"880669"
> id:"8343374"
> bug_id:"880669"
Comment 45•10 years ago
|
||
(In reply to Kohei Yoshino [:kohei] from comment #44)
> Awesome! Looks like the id, attachment.id, attachment.bug_id are still
> strings, not integers:
i was hoping to get this fixed prior to it being pushed, but it appears to be non-trivial
filed as bug 1026400
Comment 46•10 years ago
|
||
the plan with regards to migration is to continue to run the current bzapi proxy server until we're happy with this code. at that point we'll redirect calls to the api-dev proxy to bmo proper.
please help us test this by changing your endpoints from https://api-dev.bugzilla.mozilla.org/latest/ to https://bugzilla.mozilla.org/bzapi/
eg.
old: https://api-dev.bugzilla.mozilla.org/latest/bug/880669
new: https://bugzilla.mozilla.org/bzapi/bug/880669
if you find any issues don't hesitate to file bugs using https://bugzilla.mozilla.org/enter_bug.cgi?product=bugzilla.mozilla.org&component=Extensions%3A%20BzAPI%20Compatibility
Comment 47•10 years ago
|
||
Thanks!
Updated•10 years ago
|
Updated•5 years ago
|
Component: Extensions: BzAPI Compatibility → Extensions
You need to log in
before you can comment on or make changes to this bug.
Description
•