694755 - Add Classification API to Web Service (get() method)

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[Frédéric Buclin]

As Product.get no longer returns data about the classification a product belongs to, we need to implement Product.get_classification to restore access to this information. Ideally, I would take it for 4.2 despite the feature freeze, as I think this data was available in the past thanks to 'internals'.

Comment 1

[Max Kanat-Alexander]

(In reply to Frédéric Buclin from comment #0)
> Ideally, I would take it for 4.2 despite the
> feature freeze, as I think this data was available in the past thanks to
> 'internals'.

  Ah, I don't think it usually was, most likely. Some code would have had to call ->classification on the product objects in order for the data to show up in internals, and I don't think anything did.

  Adding it to 4.2 means also adding a test for it, which is a fair bit of extra work.

Comment 2

[Koosha KM]

Wouldn't that be nicer to add Bugzilla::WebService::Classification?

Comment 3

[David Lawrence [:dkl]]

I would definitely like to see a Bugzilla/WebService/Classification.pm module to contain all the related methods. Methods needed would be:

Classification.get
Classification.update
Classification.remove
Classification.add_product
Classification.remove_product 

The latter two should table one or more product names as params. We can debate whether the latter two could be part of Classification.update. I don't feel strongly either way. Bonus points for making work both ways.

dkl

Comment 4

[Koosha KM]

Attachment: patch - v1

Comment 5

[Koosha KM]

Attachment: patch - v1

Comment 6

[David Lawrence [:dkl]]

Comment on attachment 642271 [details] [diff] [review]
patch - v1

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

OK I apologize that I didn't speak up sooner as this may impact some other WebService related patches
we are currently reviewing. But I want to bring it up before we go too far down the path we are going
at the moment.

One thing I didn't realize and should have mentioned during the review of the other new methods, is that
one of of the goals we had had in the past for the WebService methods, was for a method to be able to 
return one or multiple values based on how many items the client requested. For example if you look at User.get,
you can pass in one or more many id's and/or names. The return data is a hash with a 'users' key which 
points to an array of hashes each containing information about each user. The benefit of doing it this way
is that you can get information about many users in a single call without making many smaller calls. This
can be helpful for getting many classifications as well. 

So we need to do the same here and in the other new methods (I will comment in the separate bugs about 
this). Classification.get should be able to return multiple classifications in a single method call.

So the params need to be changed from 'id' and 'name' to 'ids' and 'names' and then return a hash
with a key called 'classifications' which points to an array of hashes for each classification requested.
'ids' and 'names' can be called with a single value or a list of values. Bugzilla::WebService::Util::validate() 
fixes this for you. 

sub get {
    my ($self, $params) = validate(@_, 'names', 'ids');

Again, I apologize for bringing this up now, but we need to make sure we do this before people start
using the methods and then we try to change it later. I did test this functionally and so far it looks
good to me.

Thanks
dkl

::: Bugzilla/WebService/Classification.pm
@@ +114,5 @@
> +=back
> +
> +=item B<Returns>    
> +
> +A hash containing up to 4 elements with the following keys:

Nit-pick (better not to display exact number of items so we can expand later):

A hash describing the classification that has the following items:

@@ +120,5 @@
> +=over
> +
> +=item C<id>
> +
> +The id of the classification.

C<int> The id of the classification.

@@ +124,5 @@
> +The id of the classification.
> +
> +=item C<name>
> +
> +The name of the classification.

C<string> The name of the classification.

@@ +128,5 @@
> +The name of the classification.
> +
> +=item C<description>
> +
> +The description of the classificaion.

C<string> The description of the classification.

@@ +132,5 @@
> +The description of the classificaion.
> +
> +=item C<sort_key>
> +
> +The associated C<sort_key> with the classification.

C<int> The value which determines the order the classification is sorted.

Comment 7

[Koosha KM]

Attachment: patch - v1.1

Comment 8

[Koosha KM]

Attachment: patch - v1.1

Making use of params_to_objects to make code cleaner.

Comment 9

[David Lawrence [:dkl]]

Comment on attachment 646505 [details] [diff] [review]
patch - v1.1

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

Looks good overall. Couple changes.

::: Bugzilla/WebService/Classification.pm
@@ +39,5 @@
> +        id          => $self->type('int',    $classification->id),
> +        name        => $self->type('string', $classification->name),
> +        description => $self->type('string', $classification->description),
> +        sort_key    => $self->type('int',    $classification->sortkey),
> +

remove extra line

@@ +40,5 @@
> +        name        => $self->type('string', $classification->name),
> +        description => $self->type('string', $classification->description),
> +        sort_key    => $self->type('int',    $classification->sortkey),
> +
> +    );

We should at a minimum also return a list of current products assigned to the classification.

        products => [ map { $self->_product_to_hash($_) } @{ $classification->products } ],

sub _product_to_hash {
    my ($self, $product) = @_;
    my %hash = (
        id => $self->type('int', $product->id),
        name => $self->type('string', $product->name),
        description => $self->type('string', $product->description),
    );
    return $hash;
}

For more detail for each product they would then use a subsequent Product.get.

@@ +117,5 @@
> +C<string> The description of the classificaion.
> +
> +=item C<sort_key>
> +
> +C<id> The value which determines the order the classification is sorted.

C<int>

Comment 10

[Koosha KM]

Attachment: patch - v1.2

Comment 11

[David Lawrence [:dkl]]

Comment on attachment 646688 [details] [diff] [review]
patch - v1.2

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

Looks good and works as expected. Pod change on commit. r=dkl

::: Bugzilla/WebService/Classification.pm
@@ +132,5 @@
> +C<int> The value which determines the order the classification is sorted.
> +
> +=item C<products>
> +
> +Array of hashes. An array of hashes, each of which represents information about a product 

Nit-pick:

Array of hashes, each of which represents information about a product

Comment 12

[Frédéric Buclin]

Comment on attachment 646688 [details] [diff] [review]
patch - v1.2

>=== added file 'Bugzilla/WebService/Classification.pm'

>+        products    => [ map { $self->_product_to_hash($_) } @{ $classification->products }],

Wow, this means you can get all products from all classifications independently of your privileges to see these classifications and products (where are $user->get_selectable_classifications and $user->get_accessible_products?). When reviewing patches, make sure to always have security in mind and prevent confidential data leak.

Comment 13

[Koosha KM]

Attachment: patch - v1.3

Comment 14

[Frédéric Buclin]

Comment on attachment 647022 [details] [diff] [review]
patch - v1.3

>+use Bugzilla;

None of the Bugzilla modules need to load Bugzilla.pm itself, see troubles this generated in bug 303413. |use Bugzilla;| is already called by jsonrpc.cgi and xmlrpc.cgi, so this line can go away. I know Bugzilla::WebService::User has this line too, but this is a mistake and it should go away from there too.


>+sub get {

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

We are pretty inconsistent with Product.get where missing 'names' and 'ids' do not throw an error. But I agree that throwing an error is the way to go. A separate bug should be filed against Product.get to fix that.


>+    my $user = Bugzilla->login(LOGIN_REQUIRED);

There is no reason to require the user to be logged in. We don't require it in Product.get nor in User.get, for instance.


>+    my %user_classifications = map { $_->id => $_ } @{ $user->get_selectable_classifications };

Before this call, please shift to the slave DB, using Bugzilla->switch_to_shadow_db(). Also, name the hash %accessible_classifications or %selectable_classifications, which more self-explanatory.


>+    for (@$classification_objs) {

For consistency with the rest of our codebase, please write: foreach my $classification (...).


>+        ThrowUserError("classification_not_authorized", { name => $_->name })
>+            if not exists $user_classifications{$_->id};

Throwing this error leaks the existence of this classification. Please be consistent with Product.get, and silently ignore classifications you cannot access. This way, we don't leak if the classification doesn't exist or is not accessible.


>+    my @hashed_objects = map { filter($params, $self->_classification_to_hash($_, $user)) } @$classification_objs;

No need to pass $user as argument. _classification_to_hash() can already get this information itself via Bugzilla->user. Also, rename @hashed_objects to @classifications for clarity (you will note that this is how we do it in Product.get and User.get).


>+sub _classification_to_hash {

>+    my $user_products = $user->get_selectable_products($classification->id);

I wonder if it wouldn't make more sense to let _product_to_hash() call get_selectable_products() itself.


>+=head2 get
>+
>+B<STABLE>

This method is in no way stable. We just created it, and it will very likely change in the near future. It should be marked EXPERIMENTAL.

I didn't read the POD as it will be affected by changes mentioned above.

Comment 15

[Koosha KM]

Attachment: patch - v1.4

Comment 16

[Frédéric Buclin]

Comment on attachment 647108 [details] [diff] [review]
patch - v1.4

Sorry for taking so long to review your patch. Globally, it looks good. A few things to fix, though.


>=== added file 'Bugzilla/WebService/Classification.pm'

>+use Bugzilla::Constants;
>+use Bugzilla::WebService::Constants;

You don't call any constant from get(), so there is no need to load these modules.


>+    my %selectable_classifications = map { $_->id => $_ } @{ Bugzilla->user->get_selectable_classifications };

If I have editclassifications privileges, I want to get all classifications, including those with no products or with no products I can see.
Also, there is no need to store the objects in the hash as you never use them. You can write => 1 instead of => $_.
If the useclassifications parameter is turned off, a user shouldn't be allowed to access classifications at all, unless this is a power user with editclassifications privileges, in which case it's fine to let him go through. This will also help to mitigate the problem I mention below.


>+    my $user_products = Bugzilla->user->get_selectable_products($classification->id);

Name the variable $products instead of $user_products. Also, there is a problem with this code when useclassifications is turned off. get_selectable_products() ignores the classification ID and returns products from all classifications, which is confusing. As we shouldn't allow a powerless user access this method when classifications are disabled, this is not an issue (see above). If the user has editclassifications privileges, then he needs to see all products, so you should call $classification->products instead, which also fixes this issue.


>+Array of C<int>s. An array of classification C<id>s.

No need to specify "Array of C<int>s." Everybody knows that an ID is an integer. Also, don't write C<id>s; this doesn't look nice in the output. Simply write ids.


>+Array of C<string>s. An array of classification C<name>s.

Same here.


>+=item B<Returns>    
>+
>+A hash with the key C<classifications> and an array of hashes as the corresponding value.

You should specify that an empty array will be returned if classifications are disabled and the user hasn't editclassifications privileges.

Comment 17

[Koosha KM]

Attachment: patch - v1.5

Comment 18

[Koosha KM]

(In reply to Frédéric Buclin from comment #16)

> You should specify that an empty array will be returned if classifications
> are disabled and the user hasn't editclassifications privileges.

Well, I think it would be nicer to throw an error in that case to be more descriptive and not be vague as the user cannot tell whether he is not privileged or the classifications are not used at all if return an empty array.

Comment 19

[Frédéric Buclin]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #18)
> Well, I think it would be nicer to throw an error in that case

Hum, I agree, this makes sense. Let's do that. If classifications are disabled and the user hasn't editclassifications privileges, an error is returned.

Comment 20

[Koosha KM]

The latest patch does this. ;-)

Comment 21

[Frédéric Buclin]

Comment on attachment 657717 [details] [diff] [review]
patch - v1.5

>+use strict;

|use 5.10.1| must be added for trunk only.


>+    $user->in_group('editclassifications')
>+        || Bugzilla->params->{'useclassification'}

Nit: as it's less expensive to look at Bugzilla->params, both lines should be shifted.


>+    my %selectable_classifications = map { $_->id => 1 } @{ $user->in_group('editclassifications') ? 
>+                                                                $classification_objs : 
>+                                                                $user->get_selectable_classifications };

You try to do too many things at once, making the code harder to read. As %selectable_classifications is only useful for powerless users, the code should be rewritten accordingly.


>+array: An array of classification ids.
>+array: An array of classification names.

s/array://


>+=item B<Errors> (none)

If classifications are disabled, powerless users get the auth_classification_not_enabled error back. This error must be listed in Constants.pm. Let's take the range 900-1000 for classifications.


I will address all these comments myself to spare some time and attach the new patch here. Thanks for your patch! r=LpSolit

Comment 22

[Frédéric Buclin]

Attachment: final patch

Comment 23

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
added Bugzilla/WebService/Classification.pm
modified Bugzilla/WebService/Constants.pm
Committed revision 8400.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
added Bugzilla/WebService/Classification.pm
modified Bugzilla/WebService/Constants.pm
Committed revision 8396.

Comment 24

[Koosha KM]

Thanks for your modifications.

Comment 25

[Koosha KM]

Attachment: Doc update

Updates the list of current modules available defined in WebService.pm.

Comment 26

[Frédéric Buclin]

Comment on attachment 675826 [details] [diff] [review]
Doc update

r=LpSolit

Comment 27

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/WebService.pm
Committed revision 8449.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified Bugzilla/WebService.pm
Committed revision 8435.

Comment 28

[Frédéric Buclin]

Added to relnotes for 4.4.