Closed Bug 880669 Opened 7 years ago Closed 5 years ago

Extend current BzAPI BMO extension to contain compatibility changes on top of native rest

Categories

(bugzilla.mozilla.org :: Extensions, enhancement, P1)

Production
enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: dkl, Assigned: dkl)

References

Details

(Keywords: bmo-goal)

Attachments

(1 file, 8 obsolete files)

No description provided.
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
(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
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
(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
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
(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 :)
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
(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
I'm confused. You said "No", then seemed to outline a way to do exactly what I was suggesting. What have I missed?

Gerv
(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
Depends on: 477601
Depends on: 540818
Duplicate of this bug: 747201
Blocks: 885464
Attached patch 880669_1.patch (obsolete) — Splinter Review
Attachment #8343374 - Flags: feedback?(glob)
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+
Keywords: bmo-goal
Priority: -- → P1
Attached patch WIP 2 (obsolete) — Splinter Review
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 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 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-
Attached patch WIP 3 (obsolete) — Splinter Review
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)
Attached patch WIP 4 (obsolete) — Splinter Review
Some more fixes.
Attachment #8366679 - Attachment is obsolete: true
Attachment #8366679 - Flags: feedback?(glob)
Attachment #8367648 - Flags: feedback?(glob)
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-
Attached patch WIP 5 (obsolete) — Splinter Review
Attachment #8367648 - Attachment is obsolete: true
Attachment #8390108 - Flags: feedback?(glob)
Attachment #8343374 - Attachment is obsolete: true
Depends on: 983839
Depends on: 513212
Depends on: 986124
Attached patch 880669_1.patch (obsolete) — Splinter Review
Attachment #8390108 - Attachment is obsolete: true
Attachment #8390108 - Flags: feedback?(glob)
Attachment #8398629 - Flags: review?(glob)
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
Attached patch 880669_2.patch (obsolete) — Splinter Review
Small fixes.
Attachment #8398629 - Attachment is obsolete: true
Attachment #8398629 - Flags: review?(glob)
Attachment #8398810 - Flags: review?(glob)
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
Depends on: 989633
Depends on: 989647
test comment. please ignore this.
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
Please discard my comment above. The data on the dev server is just outdated.
Attached patch 880669_3.patch (obsolete) — Splinter Review
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)
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 :(
Depends on: 990070
Depends on: 990167
Depends on: 990252
The bug "id" field is an integer, not a string.
(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
(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.
* 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
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
(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
Depends on: 1000913
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)
Attached patch 880669_4.patchSplinter Review
Attachment #8399470 - Attachment is obsolete: true
Attachment #8437429 - Flags: review?(glob)
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+
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   dd10df6..0a2592d  master -> master
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
(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
(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
Awesome! Looks like the id, attachment.id, attachment.bug_id are still strings, not integers:


> id:"880669"

> id:"8343374"
> bug_id:"880669"
(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
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
Blocks: 1026557
Blocks: 1033394
Blocks: 1033258
No longer blocks: 1033258
Depends on: 1033258
Blocks: 1033258
No longer depends on: 1033258
Blocks: 1034524
Blocks: 1029500
Blocks: 1027025
Blocks: 1035799
Blocks: 1035804
No longer blocks: 1035804
Depends on: 508541
Component: Extensions: BzAPI Compatibility → Extensions
You need to log in before you can comment on or make changes to this bug.