1051056 - The REST API needs to be versioned so that new changes can be made that do not break compatibility

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[David Lawrence [:dkl]]

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. 

During a recent BMO workweek, we came up with the following proposal for feedback:

* rewrite current rest api methods as v1
   * delete legacy code at the same time when xmlrpc, jsonrpc endpoints are removed
   * v1 default if no version specified (forever)
* endpoint structure
   * /api/core/2/bug/35
   * /api/review/5/suggestions/35
   * "/api/" . namespace . "/" . api-version . "/" . resource location
   * no "latest" version
* extensions directory layout
   * extensions/Review/api/v2/Suggestions.pm
   * extensions have their own separate versioning system
   * extensions cannot add resources to bugzilla's core namespace
   * api docs for extensions under extensions/Review/api/v2/docs/*.rst
* develop from scratch; not modifications of existing infra (but heavily cribbed from it)
* copy entire version structure for each version
   * sharing core service and logic across versions
   * strictly no per-version code in the core code
* deprecate XMLRPC, JSONRPC in 5; remove in version 7
   * existing REST methods will continue to be available as /api/1/..
* within six months:
   * finalised api contract design
   * monitor the bugzilla developers list for discussions
* api versions are integers
   * increased when the api contract is changed
   * will not be removed
* unstable resources will be denoted with a different url
   * eg. /api/2/dev/monkey
   * not necessarily reflected by the underlying perl module/directory structure

Version 2 proposal:

* targeting bugzilla 7 for the initial release of api/2
* zero logic in webservice, wrapping/mapping only
* standard response for each object
   * users are the same for each
* very minimal dataset by default
   * bug : id, summary, status, resolution
   * user : id, login, real_name
* need to ensure that include/exclude fields supports the new nested structures
   * eg. "only return the ids"

Comment 1

[Gervase Markham [:gerv]]

(In reply to David Lawrence [:dkl] from comment #0)
> * endpoint structure
>    * /api/core/2/bug/35
>    * /api/review/5/suggestions/35
>    * "/api/" . namespace . "/" . api-version . "/" . resource location

Can you explain the rationale for not doing

* "/api/" . api-version . "/" . namespace . "/" . resource location

? Is it so we can increment the API version in one namespace and not another? I sort of see why you might want that, but I think we might confuse people if they are using bug API version 3 but review API version 5.

>    * no "latest" version

So how do we develop new API versions? Do we rev. the version number to e.g. 6, and then have version 6 be known-unstable until we ship a release with it?

I wonder if it would be better to have a "dev" version which exists everywhere except perhaps on release branches, and about which no API guarantees are made. We can then use this API version for new ideas, and periodically "freeze" it as version N + 1.

> * unstable resources will be denoted with a different url
>    * eg. /api/2/dev/monkey

It seems to me that having a "dev" API version is a cleaner solution to this issue.

> * zero logic in webservice, wrapping/mapping only

I'm not sure that'll be achievable if we don't want some weird corners in our API, which surely version 2 is supposed to avoid, as it's supposed to be a best-of-breed merge of BzAPI and v1? But we'll see.

Gerv

Comment 2

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #1)
> (In reply to David Lawrence [:dkl] from comment #0)
> >    * "/api/" . namespace . "/" . api-version . "/" . resource location
> 
> Can you explain the rationale for not doing
> * "/api/" . api-version . "/" . namespace . "/" . resource location

extensions need their own versioned namespaces, separate from bugzilla's base api version.

ie. it should be possible to have
> /api/core/2.5/bug
and
> /api/foodextension/1.0/cheese

upgrades to the food extension shouldn't require a bump in the bugzilla's core api version.

> ? Is it so we can increment the API version in one namespace and not
> another? I sort of see why you might want that, but I think we might confuse
> people if they are using bug API version 3 but review API version 5.

bugzilla's features will only be exposed in the "core" namespace.
all other namespaces will be extensions.

> > * unstable resources will be denoted with a different url
> >    * eg. /api/2/dev/monkey
> 
> It seems to me that having a "dev" API version is a cleaner solution to this
> issue.

i like that idea:
> /api/core/dev/monkey

Comment 3

[Gervase Markham [:gerv]]

(In reply to Byron Jones ‹:glob› from comment #2)
> extensions need their own versioned namespaces, separate from bugzilla's
> base api version.

Makes sense. How are we going to avoid clashes between extensions? Or do we just assume it won't be a problem in practice?

Gerv

Comment 4

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #3)
> Makes sense. How are we going to avoid clashes between extensions? Or do we
> just assume it won't be a problem in practice?

the later - it would only be a problem with extensions with the same namespace, version, and method names.  not impossible but improbable.

Comment 5

[Gervase Markham [:gerv]]

I think you wouldn't want two extensions using the same namespace, even if they had different version and method names! But I agree it's improbable.

Gerv

Comment 6

[David Lawrence [:dkl]]

Attachment: 1051056_1.patch

Working version of the API versioning support patch.

Notes to consider when reviewing:

1. The old XMLRPC/JSON-RPC modules (server and logic) are still in the old place for backwards compat and will be simply deleted when support expires. So there is a lot of duplicated code for now.
2. Do not look at POD docs at all as I will will work on those while technical review is ongoing. I held off as the overall design my change as result of review/feedback. t/011pod.t will complain heavily of course til I fix them.
3. To enable API for the Example extension for testing, remove extensions/Example/disabled.
4. t/001compile.t complains of redefined methods/vars in the Example extension when enabled. Still looking into this one.
5. Try different error situations as well as trying to access private or public data and make sure it works as expected.

Thanks
dkl

Comment 7

[David Lawrence [:dkl]]

Attachment: 1051056_2.patch

Changes since last patch:

1. Some typos/style fixes mentioned by glob in our 1:1
2. Switched from using $self->type in all of the resource modules to using special methods in Bugzilla::API::1_0::Util as_* to convert to JSON values.
3. Updated Bugzilla::API::1_0::Server::_find_resource to no longer loop twice to allow for extension hooks. We just loop through the files once now and extensions will need to convert to the new API structure and use their own namespace. Overriding the core resources will not be allowed.
4. Some documentation work but still no need to look at docs just yet.

dkl

Comment 8

[David Lawrence [:dkl]]

Attachment: 1051056_3.patch

Changes since last patch:

1. Backported upstream use of headers for API authentication (bug 1139755)
2. Backported exporting API auth headers as part of CORS (bug 1141440)

dkl

Comment 9

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

Comment on attachment 8577366 [details] [diff] [review]
1051056_3.patch

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

the errors in t/001compile.t are not caused by this patch, from what I can tell.
I haven't bisected to the culprit yet though.

There is a regex error in the Example extension under my perl
  This is perl 5, version 18, subversion 4 (v5.18.4) built for x86_64-linux-thread-multi
  (with 23 registered patches, see perl -V for more detail)

One particular class of review comment is probably not... applicable to this review in particular (long chains of || that result in a thrown exception) but they make me got WAT so much that I had to say something.

Otherwise: I have no noticed any deficiencies between this and the original code -- yet. I do note that neither of them seem to allow /rest/valid_login to be called. The error message indicates I should use POST, and when I use POST instead of GET I get a 404. Didn't test BMO.

r- for the spaces-in-glob problem -- additionally I have not reviewed the code 100% so another round would be good. :)

::: Bugzilla.pm
@@ +210,4 @@
>      return $cache->{extensions};
>  }
>  
> +sub api_server {

I assume this won't work in the process cache, right? Just curious :)

::: Bugzilla/API/1_0/Resource/Product.pm
@@ +133,5 @@
> +    my $user = Bugzilla->user;
> +
> +    defined $params->{ids} || defined $params->{names} || defined $params->{type}
> +        || ThrowCodeError("params_required", { function => "Product.get",
> +                                               params => ['ids', 'names', 'type'] });

maybe ThrowCodeError(...) if none { defined $params->{$_} } qw( ids names type );...

There are similar constructs I've complained about earlier in my review (although this one will likely appear first..)

::: Bugzilla/API/1_0/Resource/User.pm
@@ +123,5 @@
> +
> +    # Username and password params are required
> +    foreach my $param ("login", "password") {
> +        (defined $params->{$param} || defined $params->{'Bugzilla_' . $param})
> +            || ThrowCodeError('param_required', { param => $param });

I'm not sure how to improve this, but it took me several attempts to understand what was going on...

@@ +193,5 @@
> +
> +    defined($params->{names}) || defined($params->{ids})
> +        || defined($params->{match})
> +        || ThrowCodeError('params_required',
> +               { function => 'User.get', params => ['ids', 'names', 'match'] });

yeah, multiple clauses like this really confuse me. I know that || is evaluated lazily, but when it's more than condition || action, I would rewrite it to use if/unless, in either C-style or as a postfix conditional.

::: Bugzilla/API/1_0/Server.pm
@@ +226,5 @@
> +    unless ($controller->can($method)) {
> +        return $self->error_data(302, "No such a method : '$method'.");
> +    }
> +
> +    my $result = eval q| $controller->$method($params) |;

why is there a string eval here?

@@ +374,5 @@
> +    }
> +
> +    # Load in the WebService modules from the appropriate version directory
> +    # and then call $module->REST_RESOURCES to get the resources array ref.
> +    foreach my $module_file (glob($resource_modules)) {

This will break if $resource_modules contains spaces. See https://metacpan.org/pod/File::Glob. If I understand the POD, using bsd_glob directly will work.

@@ +376,5 @@
> +    # Load in the WebService modules from the appropriate version directory
> +    # and then call $module->REST_RESOURCES to get the resources array ref.
> +    foreach my $module_file (glob($resource_modules)) {
> +        # Create a controller object
> +        next if $module_file !~ /\.pm$/;

This is a little paranoid, I think we can trust glob (ha ha) to only return pm files already.

@@ +390,5 @@
> +        my $this_resources = $controller->REST_RESOURCES;
> +        next if (ref $this_resources ne 'ARRAY' || scalar @{ $this_resources } % 2 != 0);
> +
> +        while (my $regex = shift @{ $this_resources }) {
> +            my $options_data = shift @{ $this_resources };

Use splice() instead of using shift() twice here.

Alternatively:
Using a non-destructive loop might be faster as well.
As we're changing things already, perhaps REST_RESOURCES should be an array of arrayrefs?

::: Bugzilla/API/1_0/Util.pm
@@ +25,5 @@
> +use parent qw(Exporter);
> +
> +# We have to "require", not "use" this, because otherwise it tries to
> +# use features of Test::More during import().
> +require Test::Taint;

nit: remove this comment and use "use Test::Taint ()"
This explicitely avoids calling Test::Taint->import and thus no magic happens. Same as BEGIN { require ... }.

Comment 10

[David Lawrence [:dkl]]

(In reply to Dylan William Hardison [:dylan] from comment #9)
> Comment on attachment 8577366 [details] [diff] [review]
> 1051056_3.patch
> 
> Review of attachment 8577366 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> the errors in t/001compile.t are not caused by this patch, from what I can
> tell.
> I haven't bisected to the culprit yet though.
> 
> There is a regex error in the Example extension under my perl
>   This is perl 5, version 18, subversion 4 (v5.18.4) built for
> x86_64-linux-thread-multi
>   (with 23 registered patches, see perl -V for more detail)
> 
> One particular class of review comment is probably not... applicable to this
> review in particular (long chains of || that result in a thrown exception)
> but they make me got WAT so much that I had to say something.
> 
> Otherwise: I have no noticed any deficiencies between this and the original
> code -- yet. I do note that neither of them seem to allow /rest/valid_login
> to be called. The error message indicates I should use POST, and when I use
> POST instead of GET I get a 404. Didn't test BMO.

Will look into the /valid_login issue which I think I know what is happening. And also the compile.t errors.

> r- for the spaces-in-glob problem -- additionally I have not reviewed the
> code 100% so another round would be good. :)
> 
> ::: Bugzilla.pm
> @@ +210,4 @@
> >      return $cache->{extensions};
> >  }
> >  
> > +sub api_server {
> 
> I assume this won't work in the process cache, right? Just curious :)

Not as implemented due to the fact that the server does setup for the current request (version, namespace, etc.) at construction time. I could look into breaking it up further so that part of
it could be cached in the process_cache but we can do that later.
 
> ::: Bugzilla/API/1_0/Resource/Product.pm
> @@ +133,5 @@
> > +    my $user = Bugzilla->user;
> > +
> > +    defined $params->{ids} || defined $params->{names} || defined $params->{type}
> > +        || ThrowCodeError("params_required", { function => "Product.get",
> > +                                               params => ['ids', 'names', 'type'] });
> 
> maybe ThrowCodeError(...) if none { defined $params->{$_} } qw( ids names
> type );...
> 
> There are similar constructs I've complained about earlier in my review
> (although this one will likely appear first..)

These are part of the normal legacy code that I merely copied over and verified still works. We can address these as separate bugs IMO. 
 
> ::: Bugzilla/API/1_0/Resource/User.pm
> @@ +123,5 @@
> > +
> > +    # Username and password params are required
> > +    foreach my $param ("login", "password") {
> > +        (defined $params->{$param} || defined $params->{'Bugzilla_' . $param})
> > +            || ThrowCodeError('param_required', { param => $param });
> 
> I'm not sure how to improve this, but it took me several attempts to
> understand what was going on...
> 
> @@ +193,5 @@
> > +
> > +    defined($params->{names}) || defined($params->{ids})
> > +        || defined($params->{match})
> > +        || ThrowCodeError('params_required',
> > +               { function => 'User.get', params => ['ids', 'names', 'match'] });
> 
> yeah, multiple clauses like this really confuse me. I know that || is
> evaluated lazily, but when it's more than condition || action, I would
> rewrite it to use if/unless, in either C-style or as a postfix conditional.

These two same as above.

> ::: Bugzilla/API/1_0/Server.pm
> @@ +226,5 @@
> > +    unless ($controller->can($method)) {
> > +        return $self->error_data(302, "No such a method : '$method'.");
> > +    }
> > +
> > +    my $result = eval q| $controller->$method($params) |;
> 
> why is there a string eval here?

Copied from JSON::RPC::Legacy::Server

https://metacpan.org/source/DMAKI/JSON-RPC-1.06/lib/JSON/RPC/Legacy/Server.pm#L113
 
> @@ +374,5 @@
> > +    }
> > +
> > +    # Load in the WebService modules from the appropriate version directory
> > +    # and then call $module->REST_RESOURCES to get the resources array ref.
> > +    foreach my $module_file (glob($resource_modules)) {
> 
> This will break if $resource_modules contains spaces. See
> https://metacpan.org/pod/File::Glob. If I understand the POD, using bsd_glob
> directly will work.

Done
 
> @@ +376,5 @@
> > +    # Load in the WebService modules from the appropriate version directory
> > +    # and then call $module->REST_RESOURCES to get the resources array ref.
> > +    foreach my $module_file (glob($resource_modules)) {
> > +        # Create a controller object
> > +        next if $module_file !~ /\.pm$/;
> 
> This is a little paranoid, I think we can trust glob (ha ha) to only return
> pm files already.

Fixed :)
 
> @@ +390,5 @@
> > +        my $this_resources = $controller->REST_RESOURCES;
> > +        next if (ref $this_resources ne 'ARRAY' || scalar @{ $this_resources } % 2 != 0);
> > +
> > +        while (my $regex = shift @{ $this_resources }) {
> > +            my $options_data = shift @{ $this_resources };
> 
> Use splice() instead of using shift() twice here.
> 

Done

> Alternatively:
> Using a non-destructive loop might be faster as well.
> As we're changing things already, perhaps REST_RESOURCES should be an array
> of arrayrefs?

Not sure if it will make all that much difference speed wise and the plan is to eventually move to Router::Simple which will make this unnecessary.
 
> ::: Bugzilla/API/1_0/Util.pm
> @@ +25,5 @@
> > +use parent qw(Exporter);
> > +
> > +# We have to "require", not "use" this, because otherwise it tries to
> > +# use features of Test::More during import().
> > +require Test::Taint;
> 
> nit: remove this comment and use "use Test::Taint ()"
> This explicitely avoids calling Test::Taint->import and thus no magic
> happens. Same as BEGIN { require ... }.

Done

Comment 11

[David Lawrence [:dkl]]

Attachment: 1051056_4.patch

1. Fixes from last review comments.
2. Documentation for Bugzilla/API/Server.pm and Bugzilla/API/1_0/Server.pm.
Still need to document remaining Resource modules, etc.

Comment 12

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

Comment on attachment 8579421 [details] [diff] [review]
1051056_4.patch

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

not done reviewing yet, but here's some notes:

::: Bugzilla/API/1_0/Server.pm
@@ +10,5 @@
> +use 5.10.1;
> +use strict;
> +use warnings;
> +
> +use parent qw(Bugzilla::API::Server);

delete the above line, and do:

use Moo;

extends 'Bugzilla::API::Server';

@@ +31,5 @@
> +# Constants #
> +#############
> +
> +use constant API_VERSION   => '1_0';
> +use constant API_NAMESPACE => 'core';

You could delete these constants and just...

has api_version => (is => 'ro', default => '1_0', init_arg => undef);
has api_namespace => (is => 'ro', default => 'core', init_arg => undef);

@@ +41,5 @@
> +sub new {
> +    my $invocant = shift;
> +    my $class = ref($invocant) || $invocant;
> +    my $self = bless({}, $class);
> +

This completely shadow's the parent's new(). Usually we don't need to override new(), we can do things in BUILD or BUILDARGS.
In this case, _best_content_type() could be called from content_type's builder. Delete the new() method and define the following method. Note this requires a change to the has definition in Bugzilla::API::Server::content_type

sub _build_content_type {
    my ($self) = @_;
   # you could also just inline _best_content_type here.
   return $self->_best_content_type(@{ $self->constants->{REST_CONTENT_TYPE_WHITELIST} });
}

::: Bugzilla/API/Server.pm
@@ +36,5 @@
> +has api_options     => (is => 'rw', default => sub { [] });
> +has api_params      => (is => 'rw', default => sub { {} });
> +has api_path        => (is => 'rw', default => '');
> +has api_version     => (is => 'rw', default => '');
> +has content_type    => (is => 'rw', default => 'application/json');

has content_type => (is => 'lazy');

# subclasses can redefine this.
sub _build_content_type {
    return 'application/json';
}

@@ +59,5 @@
> +    # requested version namespace if required.
> +    my $api_version = DEFAULT_API_VERSION;
> +
> +    my $server_class = "Bugzilla::API::${api_version}::Server";
> +    eval "require $server_class" || die $@;

use Module::Runtime qw(require_module) for this. If we're using Moo, we get Module::Runtime. :)

require_module($server_class);

@@ +89,5 @@
> +    # If the version pulled from the path is different than
> +    # what the server is currently, then reload as the new version.
> +    if ($api_version ne $self->api_version) {
> +        my $server_class = "Bugzilla::API::${api_version}::Server";
> +        eval "require $server_class" || die $@;

require_module($server_class);

@@ +110,5 @@
> +
> +    no strict 'refs';
> +
> +    my $class = "Bugzilla::API::${api_version}::Constants";
> +    eval "require $class" || die $@;

require_module($class);

@@ +138,5 @@
> +    # utf8 string."
> +    $json->utf8(0) if Bugzilla->params->{'utf8'};
> +    return $self->{json} = $json;
> +}
> +

Could replace that with...


has json => ( is => 'lazy' );

sub _build_json {
    my $json = JSON->new->utf8;
    $json->allow_blessed(1);
    $json->convert_blessed(1);
    # This may seem a little backwards, but what this really means is
    # "don't convert our utf8 into byte strings, just leave it as a
    # utf8 string."
    $json->utf8(0) if Bugzilla->params->{'utf8'};
    return $json;
}

@@ +147,5 @@
> +sub request {
> +    return $_[0]->{request}
> +        ||= HTTP::Request->new($_[0]->cgi->request_method, $_[0]->cgi->url);
> +}
> +

has request => (is => 'lazy');
sub _build_request { HTTP::Request->new($_[0]->cgi->request_method, $_[0]->cgi->url) }

@@ +273,5 @@
> +    }
> +
> +    # If we using an extension API, we need to determing which version of
> +    # the Core API it was written for.
> +    if ($namespace !~ /^core$/i) {

if (lc($namespace) ne 'core') {

::: t/001compile.t
@@ +36,4 @@
>  
>      if ($file =~ s/\.pm$//) {
>          $file =~ s{/}{::}g;
> +        say STDERR $file;

woops? Looks like this was not meant to be left in?

Comment 13

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

Comment on attachment 8579421 [details] [diff] [review]
1051056_4.patch

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

soft r-, for some issues relating to correct usage of Moo. :)

Need to add Moo >= 2 as a dependency. If Module::Runtime::require_module() is used, we need to depend on that as well. 
(A formality, as we indirectly depend on them already).

::: Bugzilla.pm
@@ +214,5 @@
> +    my $class = shift;
> +    my $cache = $class->request_cache;
> +    return $cache->{api_server} if defined $cache->{api_server};
> +    $cache->{api_server} = Bugzilla::API::Server->get_server;
> +    $cache->{api_server}->init_server;

init_server() should be called from a BUILD method in Bugzilla::API::Server

::: Bugzilla/API/Server.pm
@@ +63,5 @@
> +    eval "require $server_class" || die $@;
> +    return $server_class->new;
> +}
> +
> +sub init_server {

just rename this BUILD.

Comment 14

[Frédéric Buclin]

(In reply to Dylan William Hardison [:dylan] from comment #12)

> > +use parent qw(Bugzilla::API::Server);
> 
> delete the above line, and do:
> 
> use Moo;
> 
> extends 'Bugzilla::API::Server';


What's the advantage of writing use Moo; extends ...??


> > +use constant API_VERSION   => '1_0';
> > +use constant API_NAMESPACE => 'core';
> 
> You could delete these constants and just...
> 
> has api_version => (is => 'ro', default => '1_0', init_arg => undef);
> has api_namespace => (is => 'ro', default => 'core', init_arg => undef);

This is more code than using use constant. Again, what's the benefit of doing this??

Comment 15

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

(In reply to Frédéric Buclin from comment #14)
> (In reply to Dylan William Hardison [:dylan] from comment #12)
> 
> > > +use parent qw(Bugzilla::API::Server);
> > 
> > delete the above line, and do:
> > 
> > use Moo;
> > 
> > extends 'Bugzilla::API::Server';
Bugzilla::API::Server is using Moo, so its subclasses should also use Moo.

> 
> What's the advantage of writing use Moo; extends ...??
> 
> 
> > > +use constant API_VERSION   => '1_0';
> > > +use constant API_NAMESPACE => 'core';
> > 
> > You could delete these constants and just...
> > 
> > has api_version => (is => 'ro', default => '1_0', init_arg => undef);
> > has api_namespace => (is => 'ro', default => 'core', init_arg => undef);
> 
> This is more code than using use constant. Again, what's the benefit of
> doing this??

api_version and api_namespace are already attributes defined in the parent class. The constants were being used to set the values of those slots -- it's simpler to just redefine the slots. Or we could change the parent to use a builder, and define sub _build_api_version {  } etc.

For methods, using "use constant" is a code smell. Using regular sub foo { } is fewer characters, so if anything I'd go the _build_* route...

(use constant just declares a sub with an empty prototype, which will get constant folded by the compiler if called as a function (but not as a method)).

Comment 16

[David Lawrence [:dkl]]

Attachment: 1051056_5.patch

Changes of note:

1. Added Moo >= 2 and Module::Runtime (to be safe) to requirements
2. Single server() method that falls back to default api server in case of failure
3. Bugzilla.pm throws appropriate error if server requested fails to load. This keeps an infinite loop from occurring in Bugzilla::Error.
4. Use of BUILDARGS and _build_* Moo methods where appropriate.
5. Other minor fixes.
6. Still need to work on docs but that will be last step

dkl

Comment 17

[Frédéric Buclin]

Comment on attachment 8584100 [details] [diff] [review]
1051056_5.patch

+++ b/Bugzilla.pm

+use Bugzilla::API::Server;

So this module is always loaded... see below.



>+++ b/Bugzilla/Install/Requirements.pm

>     {
>+        package => 'Moo',
>+        module  => 'Moo',
>+        version => 2,
>+        feature => ['rest']
>+    },
>+    {
>+        package => 'Module-Runtime',
>+        module  => 'Module::Runtime',
>+        version => 0,
>+        feature => ['rest']
>+    },

These modules are not optional but required as Bugzilla.pm loads Bugzilla::API::Server unconditionally. If you don't have them, Bugzilla will crash. Also, there are many modules listed in Bugzilla::API::Server which do not exist in the core Perl package, meaning that they are all required in order to have Bugzilla to work. Something like 5 new dependencies at once. :(

Comment 18

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

(In reply to Frédéric Buclin from comment #17)
> Comment on attachment 8584100 [details] [diff] [review]
> 1051056_5.patch
> 
> +++ b/Bugzilla.pm
> 
> +use Bugzilla::API::Server;
we can surely change that to require from within the server method. That will be reflected in my review. :)

Comment 19

[David Lawrence [:dkl]]

Attachment: 1051056_6.patch

New patch that adds require to Bugzilla.pm so that error does not occur due to modules not being installed. Email::Sender (required) already requires Moo (and therefore Module::Runtime) so I am not sure we even need the 'rest' specific entries in the Requirements.pm but I added them for consistency.

dkl

Comment 20

[Frédéric Buclin]

Comment on attachment 8584642 [details] [diff] [review]
1051056_6.patch

>+++ b/Bugzilla.pm

>+sub api_server {
>+    my $class = shift;
>+    my $cache = $class->request_cache;
>+    return $cache->{api_server} if defined $cache->{api_server};
>+    require Bugzilla::API::Server;

Is it intentional to not call Bugzilla->feature('rest') first to make sure REST is available? This would be much better than a crash if a module is missing.



>+++ b/Bugzilla/API/1_0/Constants.pm

>+use Bugzilla;

Do not call |use Bugzilla| from within a .pm module (dependency loops).


>+our @EXPORT = qw(

Write: @Bugzilla::API::1_0::Constants::EXPORT = qw(...) instead, as we do everywhere else.
Same thing in Bugzilla::API::1_0::Util.


You still have many new modules which are not listed in Requirements.pm.

Comment 21

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

(In reply to Frédéric Buclin from comment #20)
> Comment on attachment 8584642 [details] [diff] [review]
> 1051056_6.patch
> 
> >+++ b/Bugzilla.pm
> 
> >+sub api_server {
> >+    my $class = shift;
> >+    my $cache = $class->request_cache;
> >+    return $cache->{api_server} if defined $cache->{api_server};
> >+    require Bugzilla::API::Server;
> 
> Is it intentional to not call Bugzilla->feature('rest') first to make sure
> REST is available? This would be much better than a crash if a module is
> missing.

Such a test is performed in rest.cgi prior to this method being called.


> You still have many new modules which are not listed in Requirements.pm.
Which modules are these? A list would probably be helpful to dkl.

Comment 22

[:glob ✱]

(In reply to Frédéric Buclin from comment #20)
> >+our @EXPORT = qw(
> 
> Write: @Bugzilla::API::1_0::Constants::EXPORT = qw(...) instead, as we do
> everywhere else.

this isn't correct -- |our @EXPORT| is already used in bugzilla.
imho it's cleaner than the more verbose form (especially for these long package names) and should stay.

Comment 23

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #20) 
> Write: @Bugzilla::API::1_0::Constants::EXPORT = qw(...) instead, as we do
> everywhere else.
> Same thing in Bugzilla::API::1_0::Util.

Bugzilla::WebService::Constants which I copied over, has had 'our @EXPORT' since Feb 2009. Grepping through the code base, it is about 50% one way and the other. Like glob mentioned, I feel like 'our @EXPORT' is simpler and cleaner.

dkl

Comment 24

[David Lawrence [:dkl]]

Attachment: 1051056_7.patch

Changes

1. I think I have the POD docs to a point where we can release this. Moving the error codes (or duplicating) to reST documentation is another bug that we can do later.
2. Added a few more Perl modules to Bugzilla/Install/Requirements mainly because when we do remove JSONRPC, we will then need to explicitly require modules such as HTTP::Request, HTTP::Response, etc. instead of relying on JSONRPC bringing them in for us.
3. Added Moo.pm styling to more of the Resource modules because...it is fun.

dkl

Comment 25

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

Comment on attachment 8585649 [details] [diff] [review]
1051056_7.patch

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

r=dylan

Remove the debugging and fix the undefined warnings. I was able to add attachments to bugs, update bug details, get an access token (obviously),
and a handful of requests along the objects. I didn't notice any difference between this patch and the original behavior, aside from debugging and warning messages.

I'd like glob to give it a second look.

::: Bugzilla.pm
@@ +214,5 @@
> +    my $cache = $class->request_cache;
> +    return $cache->{api_server} if defined $cache->{api_server};
> +    require Bugzilla::API::Server;
> +    $cache->{api_server} = Bugzilla::API::Server->server;
> +    if (my $load_error = $cache->{api_server}->load_error) {

More undefined warnings here. $load_error is {} by default, which is a true value.

::: Bugzilla/API/1_0/Server.pm
@@ +253,5 @@
> +    if ($self->request->method eq 'POST') {
> +        # CSRF is possible via XMLHttpRequest when the Content-Type header
> +        # is not application/json (for example: text/plain or
> +        # application/x-www-form-urlencoded).
> +        # application/json is the single official MIME type, per RFC 4627.

When testing this from REST::Client, I was able to send requests the trigger an undefined warning on $content_type.

@@ +352,5 @@
> +        $resource_modules = File::Spec->catdir('Bugzilla','API', $api_version,
> +            'Resource', '*.pm');
> +    }
> +
> +    say STDERR $resource_modules;

remove debugging

@@ +373,5 @@
> +
> +        while (my ($regex, $options_data) = splice(@$this_resources, 0, 2)) {
> +            next if ref $options_data ne 'HASH';
> +
> +            say STDERR $self->api_path;

remove debugging

::: Bugzilla/API/Server.pm
@@ +43,5 @@
> +has cgi             => (is => 'lazy');
> +has content_type    => (is => 'lazy');
> +has controller      => (is => 'rw', default => undef);
> +has json            => (is => 'lazy');
> +has load_error      => (is => 'rw', default => sub { {} });

there probably shouldn't be a default value here. Or else when we check if there is a load_error, it needs to be de-referenced.

Comment 26

[:glob ✱]

Comment on attachment 8585649 [details] [diff] [review]
1051056_7.patch

i don't think there's a need for me to conduct a full review of this, but i'm happy to provide feedback.

Comment 27

[David Lawrence [:dkl]]

Attachment: 1051056_8.patch

Last few changes Dylan mentioned. Moving forward r+.

Comment 28

[David Lawrence [:dkl]]

Would approval? be better than feedback?

dkl

Comment 29

[David Lawrence [:dkl]]

To ssh://[e-mail]/bugzilla/bugzilla.git
   e6d2fb7..dbfd620  master -> master

Comment 30

[Frédéric Buclin]

Comment on attachment 8586902 [details] [diff] [review]
1051056_8.patch

>+++ b/Bugzilla/Install/Requirements.pm
>+    {
>+        package => 'URI-Escape',
>+        module  => 'URI::Escape',
>+        version => 0,
>+        feature => ['rest']
>+    },

URI::Escape is part of the URI module, which is already listed as a required module. No need to duplicate it here.

To ssh://[e-mail]/bugzilla/bugzilla.git
   dbfd620..3c7ab83  master -> master

Comment 31

[Frédéric Buclin]

Comment on attachment 8586902 [details] [diff] [review]
1051056_8.patch

>+++ b/Bugzilla/API/Server.pm

>+sub response_header {
>+    my ($self, $code, $result) = @_;

>+    if (utf8::is_utf8($_[2])) {
>+        utf8::encode($_[2]);

This must be $result, not $_[2], else Bugzilla fails with:

  HTTP::Message content must be bytes at Bugzilla/API/Server.pm line 132.


To ssh://[e-mail]/bugzilla/bugzilla.git
   3c7ab83..2149ec1  master -> master

Comment 32

[David Lawrence [:dkl]]

Fix issue where PUT requests were treated as GET and failed when updating a bug, etc.

To ssh://[e-mail]/bugzilla/bugzilla.git
   a6d7498..06da34e7 master -> master