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

Status: RESOLVED FIXED

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

Comment 1

[Gervase Markham [:gerv]]

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

Comment 2

[David Lawrence [:dkl]]

(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

[Gervase Markham [:gerv]]

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

Comment 4

[David Lawrence [:dkl]]

(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

[Gervase Markham [:gerv]]

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

Comment 6

[David Lawrence [:dkl]]

(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

[Gervase Markham [:gerv]]

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

Comment 8

[David Lawrence [:dkl]]

(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

[Gervase Markham [:gerv]]

I'm confused. You said "No", then seemed to outline a way to do exactly what I was suggesting. What have I missed?

Gerv

Comment 10

[David Lawrence [:dkl]]

(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

Comment 12

[David Lawrence [:dkl]]

Attachment: 880669_1.patch

Comment 13

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

Comment 14

[David Lawrence [:dkl]]

Attachment: WIP 2

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

Comment 15

[:glob ✱]

Comment on attachment 8365067 [details] [diff] [review]
WIP 2

as it's still WIP, switching from review? to feedback?

Comment 16

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

Comment 17

[David Lawrence [:dkl]]

Attachment: WIP 3

Fixed the tracking flag issue as well as added cc_count and dupe_count columns.

dkl

Comment 18

[David Lawrence [:dkl]]

Attachment: WIP 4

Some more fixes.

Comment 19

[: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

Comment 20

[David Lawrence [:dkl]]

Attachment: WIP 5

Comment 21

[David Lawrence [:dkl]]

Attachment: 880669_1.patch

Comment 22

[David Lawrence [:dkl]]

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

Comment 23

[David Lawrence [:dkl]]

Attachment: 880669_2.patch

Small fixes.

Comment 24

[David Lawrence [:dkl]]

Installed on https://bugzilla-dev.allizom.org/bzapi

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   b8f1c74..92bc99e  4.2-dev -> 4.2-dev

Comment 25

[David Lawrence [:dkl]]

test comment. please ignore this.

Comment 26

[Kohei Yoshino [:kohei]]

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

[Kohei Yoshino [:kohei]]

Please discard my comment above. The data on the dev server is just outdated.

Comment 28

[David Lawrence [:dkl]]

Attachment: 880669_3.patch

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

Comment 29

[David Lawrence [:dkl]]

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

[Kohei Yoshino [:kohei]]

The bug "id" field is an integer, not a string.

Comment 31

[David Lawrence [:dkl]]

(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

[Kohei Yoshino [:kohei]]

"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

[Kohei Yoshino [:kohei]]

(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

[Kohei Yoshino [:kohei]]

* 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

[Kohei Yoshino [:kohei]]

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

Comment 36

[David Lawrence [:dkl]]

(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

[:glob ✱]

Comment on attachment 8399470 [details] [diff] [review]
880669_3.patch

clearing review.

this patch will be updated to accommodate the changes from bug 1000913.

Comment 38

[David Lawrence [:dkl]]

Attachment: 880669_4.patch

Comment 39

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

Comment 40

[David Lawrence [:dkl]]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   dd10df6..0a2592d  master -> master

Comment 41

[Kohei Yoshino [:kohei]]

https://bugzilla-dev.allizom.org/bzapi/bug/880669

Somehow the creator and assigned_to are missing?

Comment 42

[David Lawrence [: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.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

Comment 43

[David Lawrence [: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

Comment 44

[Kohei Yoshino [:kohei]]

Awesome! Looks like the id, attachment.id, attachment.bug_id are still strings, not integers:


> id:"880669"

> id:"8343374"
> bug_id:"880669"

Comment 45

[:glob ✱]

(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

[:glob ✱]

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

[Kohei Yoshino [:kohei]]

Thanks!