1289886 - backport upstream bug 1051056 to bmo/master to make REST API versioned

Status: RESOLVED WONTFIX

(bugzilla.mozilla.org :: API, task)

Description

[David Lawrence [:dkl]]

As described in bug 1051056:

We need to version the REST API so that clients can connect to stable versions with any breaking changes but yet give us a way to improve the API in newer versions that people can upgrade to. This is a backport from upstream that is also included in bmo/upstream-merge branch.

Comment 1

[David Lawrence [:dkl]]

Attachment: 1289886_1.patch

All GET operations seem to work fine. Am continuing to test with creating/updating operations. Overall ready for review. I did not remove Bugzilla/WebService/Server/REST.pm as I did upstream as it is still needed for the time being for BzAPI (which I would love to turn off). 

dkl

Comment 2

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8775606 [details] [diff] [review]
1289886_1.patch

clearing review because this doesn't include the dependency info (Moo is a requirement) and this bug is likely blocked on bug 1283930

Comment 3

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8775606 [details] [diff] [review]
1289886_1.patch

Review of attachment 8775606 [details] [diff] [review]:
-----------------------------------------------------------------

this has some bit rot in rest.cgi.

also needs the dependency on Moo declared in Makefile.PL

Comment 4

[David Lawrence [:dkl]]

Attachment: 1289886_2.patch

Comment 5

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8797614 [details] [diff] [review]
1289886_2.patch

Review of attachment 8797614 [details] [diff] [review]:
-----------------------------------------------------------------

So far while I can detect some differences between this and the existing API,
aside from the string/number problem (which I was able to fix with the changes listed below) nothing seems to be amiss.

I'm writing a script that does a/b comparisons between two bmo instances.

https://github.com/dylanwh/bmo-rest-tests/blob/master/tests.pl

::: Bugzilla/API/1_0/Util.pm
@@ +16,5 @@
> +use Bugzilla::Flag;
> +use Bugzilla::FlagType;
> +use Bugzilla::Util qw(datetime_from email_filter);
> +
> +use JSON;

replace with JSON::MaybeXS

also find/replace on s/JSON::(true|false|null)/JSON->$1/g

::: Bugzilla/API/Server.pm
@@ +17,5 @@
> +use Digest::MD5 qw(md5_base64);
> +use File::Spec qw(catfile);
> +use HTTP::Request;
> +use HTTP::Response;
> +use JSON;

replace with JSON::MaybeXS to make numbers come through as numbers.

@@ +150,5 @@
> +    my ($self, $status_code, $message, $error_code) = @_;
> +    if ($status_code && $message) {
> +        $self->{_return_error} = {
> +            status_code => $status_code,
> +            error       => JSON::true,

write this as JSON->true for compatibility with JSON::MaybeXS

@@ +230,5 @@
> +    # This may seem a little backwards to set utf8(0), but what this really
> +    # means is "don't convert our utf8 into byte strings, just leave it as a
> +    # utf8 string."
> +    return JSON->new->utf8(0)
> +           ->allow_blessed(1)

This can stay the same, as JSON::MaybeXS exports a 'JSON' constant that does the right thing.
You can also replace it with
JSON::MaybeXS->new(utf8 => 0, allow_blessed => 1, convert_blessed => 1);

Comment 6

[Dylan Hardison [:dylan] (he/him)]

Sorry, for JSON->null, use "undef".
There is a JSON::null, but MaybeXS doesn't do that.

Comment 7

[Dylan Hardison [:dylan] (he/him)]

Attachment: rest_bug_888.diff

There are a lot more fields returned for /rest/bug/888

take a look.

Comment 8

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8797614 [details] [diff] [review]
1289886_2.patch

this needs to not return all those custom fields and tracking flags when they're not relevant. it's a waste of processing time and bandwidth.

https://bug1289886.bmoattachments.org/attachment.cgi?id=8798444

Comment 9

[David Lawrence [:dkl]]

Attachment: 1289886_3.patch

Comment 10

[Dylan Hardison [:dylan] (he/him)]

Do we care if is-active is now included in the flag results?

Comment 11

[David Lawrence [:dkl]]

(In reply to Dylan Hardison [:dylan] from comment #10)
> Do we care if is-active is now included in the flag results?

Can you be more specific? I so not see is_active for set flags using /rest/bug/<bugid>
but I do see is_active for flag_types when using /rest/flag_type/<product> which is 
same behavior as before.

dkl

Comment 12

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8804464 [details] [diff] [review]
1289886_3.patch

r- for some API differences.

I think this won't be landable without also implementing the extra REST methods for extensions -- can you easily lift that code from upstream-merge?

For further testing, you might find my test suite helpful: https://github.com/dylanwh/bmo-rest-tests

test.pl may require some tweaks, but this lets you run A/B testing against the old API and this patch.

Comment 13

[Kohei Yoshino [:kohei]]

Given that we are leaning toward GraphQL instead of REST API v2, this won’t happen.