896066 - Allow REST WebService API to for GET /product to allow retrieval of multiple product objects instead of ids

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[David Lawrence [:dkl]]

Currently the REST API for GET /product you have to specify type={accessible,selectable,enterable} to get a list of product ids. accessible being the default if type is undefined.

With these changes, I will improve Product.get to accept 'type' as well and return the appropriate list of product ids similar to the current behavior of GET /product. This will allow for simplifying the REST handler code as well as allow for a user to also use GET /product?ids=1&ids=2 to retrieve a list of specific product objects in a single call instead of doing GET /product/<id> for each one individually.

Patch coming
dkl

Comment 1

[David Lawrence [:dkl]]

Attachment: Allow REST /product to return multiple products based on type, ids, or names (v1)

This patch updates the following:

- Changes the REST API in several ways
-- Adds GET /product_accessible GET /product_enterable and GET /product_selectable
- Updates GET /product to allow for specifying a group of products instead of having to specify exact ids and/or names. The valid types accepted are 'accessible' (default), 'selectable', and 'enterable'. One or more of 'type', 'ids', or 'names' is still required as before.
- This also updates Product.get as well since GET /product uses that directly.

Comment 2

[Frédéric Buclin]

Comment on attachment 778993 [details] [diff] [review]
Allow REST /product to return multiple products based on type, ids, or names (v1)

>=== modified file 'Bugzilla/Install/Requirements.pm'
>--- Bugzilla/Install/Requirements.pm	2013-07-12 20:39:50 +0000
>+++ Bugzilla/Install/Requirements.pm	2013-07-19 21:46:21 +0000
>@@ -104,7 +104,7 @@
>     {
>         package => 'TimeDate',
>         module  => 'Date::Format',
>-        version => '2.23'
>+        version => '2.22'
>     },
>     # 0.28 fixed some important bugs in DateTime.
>     {
>
>=== modified file 'Bugzilla/Mailer.pm'
>--- Bugzilla/Mailer.pm	2013-07-12 20:39:50 +0000
>+++ Bugzilla/Mailer.pm	2013-07-21 20:13:41 +0000
>@@ -136,8 +136,6 @@
>     Bugzilla::Hook::process('mailer_before_send', 
>                             { email => $email, mailer_args => \@args });
> 
>-    return if $email->header('to') eq '';
>-


These changes have nothing to do with this bug.

Comment 3

[David Lawrence [:dkl]]

Attachment: Allow REST /product to return multiple products based on type, ids, or names (v1)

Ugh. Gotta fix my devel environment. Fixed patch.

Comment 4

[:glob ✱]

Comment on attachment 779022 [details] [diff] [review]
Allow REST /product to return multiple products based on type, ids, or names (v1)

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

::: Bugzilla/WebService/Product.pm
@@ +79,5 @@
> +    if (defined $params->{type}) {
> +        my %product_hash;
> +        my $found = 0;
> +        foreach my $type (@{ $params->{type} }) {
> +            my $method = "get_${type}_products";

you shouldn't take using user input and use it unfiltered to create method names.
as there's only three options, make this an if/then/else statement.

::: template/en/default/global/user-error.html.tmpl
@@ +1077,5 @@
>      A REST API resource was not found for '[% method FILTER html +%] [%+ path FILTER html %]'.
>  
> +  [% ELSIF error == "get_products_invalid_type" %]
> +    The product selection types were invalid. Valid choices
> +    are 'accessible', 'selectable', and 'enterable'.

nit: "The product selection type was invalid"; you only pass in one type, so the error message should be singular.

Comment 5

[David Lawrence [:dkl]]

Attachment: 896066_2.patch

(In reply to Byron Jones ‹:glob› from comment #4)
> ::: template/en/default/global/user-error.html.tmpl
> @@ +1077,5 @@
> >      A REST API resource was not found for '[% method FILTER html +%] [%+ path FILTER html %]'.
> >  
> > +  [% ELSIF error == "get_products_invalid_type" %]
> > +    The product selection types were invalid. Valid choices
> > +    are 'accessible', 'selectable', and 'enterable'.
> 
> nit: "The product selection type was invalid"; you only pass in one type, so
> the error message should be singular.

I worded it that way cause I thought it would be OK to make the param cumulative in that people could pass in one or more of the types and the %product_hash would weed out duplicates. So updated the doc appropriately. Of course if you feel we should only always allow one at a time, then I can update the patch and go that route.

dkl

Comment 6

[:glob ✱]

Comment on attachment 779210 [details] [diff] [review]
896066_2.patch

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

> I worded it that way cause I thought it would be OK to make the param
> cumulative in that people could pass in one or more of the types and the
> %product_hash would weed out duplicates. So updated the doc appropriately.
> Of course if you feel we should only always allow one at a time, then I can
> update the patch and go that route.

this makes sense, however you also need to update the documentation for the webservice call to indicate that this is possible.

::: Bugzilla/WebService/Product.pm
@@ +81,5 @@
> +        my $found = 0;
> +        foreach my $type (@{ $params->{type} }) {
> +            next if !grep($type eq $_, qw(accessible enterable selectable));
> +            my $method = "get_${type}_products";
> +            my $result = $user->$method();

calling methods via a generated string is overkill when there are only three choices, and this approach won't generate an error if someone passes in a valid _and_ an invalid type.

Comment 8

[David Lawrence [:dkl]]

Attachment: Allow REST /product to return multiple products based on type, ids, or names (v3)

Comment 9

[David Lawrence [:dkl]]

Attachment: Allow REST /product to return multiple products based on type, ids, or names (v4)

Sorry. Missed the point about the error change. This should hopefully be it.

Comment 10

[:glob ✱]

Comment on attachment 779832 [details] [diff] [review]
Allow REST /product to return multiple products based on type, ids, or names (v4)

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

r=glob

::: Bugzilla/WebService/Product.pm
@@ +77,5 @@
>  
> +    my $products = [];
> +    if (defined $params->{type}) {
> +        my %product_hash;
> +        my $found = 0;

$found is unused can should be removed on commit

Comment 11

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk
modified Bugzilla/WebService/Product.pm
modified Bugzilla/WebService/Server/REST/Resources/Product.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 8656.