357482 - Webservice should have a get_products method

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[Mads Bondo Dydensborg]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.5; Linux) KHTML/3.5.2 (like Gecko) Kubuntu 6.06 Dapper
Build Identifier: 

The webservice component should have a get_products method, that should return information on a set of products, given a list of product_ids

Reproducible: Always

Comment 1

[Mads Bondo Dydensborg]

Attachment: Add get_products to Webservice/Products.pm

This patch add a method "get_products" to WebService/Products.pm, that takes a
hash with an item "ids", that contains a list of product ids (the argument type
is identical to the result types from the get_*_products in patch 242850). It
then builds an array of the products, with an internal field, and the id, name
and description of the product.

There are some open issues:
- it is not tested wheter or not the product exists. This should probably be
done, a missing product currently returns a perl error message.
- the "internals" field may be controversial?
- in order to force the returned name/description to always be of type string
(xmlrpc), I explicitly mark them as  "Soap strings". This makes my danish
letters work in a "clean" cvs bugzilla. I have no idea how this influences
non-utf8 encodings of strings!
- no docs (will follow, need to know a bit more about patch 242850 before it
makes sense to do the docs).

I do not expect this patch to be accepted, but would, as usual, very much
appreciate the reviewers comments.

Comment 2

[Max Kanat-Alexander]

Comment on attachment 242989 [details] [diff] [review]
Add get_products to Webservice/Products.pm


>+# Get a list of actual products, based on list of ids
>+sub get_products {
>+    my ( $self, $params ) = @_;
>+    my @products;
>+    foreach my $product_id ( @{ $params->{ ids } } ) {
>+        my $product = new Bugzilla::Product( $product_id );

  Instead of this, use Bugzilla::Product->new_from_list.

>+        push @products, {
>+            # Note, internals is sent raw, which means that any strings
>+            # that contain UTF8 chars, will be encoded as base64.
>+            internals   => $product,

  "Internals" is fine. Document it as UNSTABLE. (See the current patch that I have for get_bug--it's on a bug somewhere.)

>+            id          => $product->id,

  You need to use type() here, also.

>+            # We force SOAP/XMLRPC::Lite to accept this as string.
>+            # This will keep an existing UTF8 encoding, but I do 
>+            # not know what happens, if the string is not UTF8...

  Don't worry about that comment--that's true for all our stuff.

>+            name        => SOAP::Data->type( string => $product->name ),

  Just import type like we do in the other modules. Also, you don't need the extra whitespace inside the parens.

  Also, this is supposed to be type('string')->value($product->name).

>+            description => SOAP::Data->type( string => $product->description )
>+            };

  The indentation there is incorrect.


  Otherwise this looks good. :-) Make sure to add some POD, too.

Comment 3

[Mads Bondo Dydensborg]

Attachment: Add get_products to Webservice/Products.pm, v2

This is the second go at adding a "get_products" method ot WebService/Products.pm.

It still lacks POD! I am very confused as to what to do, because the Products.pm file lacks the "POD intro" stuff. This I have written as part of patch 242850, which is pending review. So, I fear that if I were to write the POD for this patch, the two "POD intro"'s would clash? Please advise.

The following POD should follow this patch:

=item C>get_products> B<UNSTABLE>

Description: Returns a list of information about the products passed to it.

Params: A hash containing one item, C<ids>, that is an array of product ids. 

Returns: A hash containing one item, C<products>, that is an array of
         hashes. Each hash describes a product, and has the following 
         items: C<id>, C<name>, C<description>, and C<internals>. The 
         C<id> item is the id of the product. The C<name> item is the name
         of the product. The C<description> is the description of the
         product. Finally, the C<internal> is an internal representation
         of the product.

=back

Comment 4

[Max Kanat-Alexander]

Comment on attachment 243363 [details] [diff] [review]
Add get_products to Webservice/Products.pm, v2

Yeah, this looks right. It just needs some POD. The POD you posted looks right. It just needs an "internals" in once place instead of "internal."

Comment 5

[Max Kanat-Alexander]

Comment on attachment 243363 [details] [diff] [review]
Add get_products to Webservice/Products.pm, v2

Oh wait, no! What am I thinking!

This needs security controls!

It should only work for accessible products. That is, the union of selectable and enterable. (I think you should just create a function for that in Bugzilla::User.)

Comment 6

[Mads Bondo Dydensborg]

Attachment: Add get_products to Webservice/Products.pm, v3

Third try at adding a "get_products" method to WebService.

This adds a method to get the accessible products from Bugzilla "core". It removes the "get_product" method from the web service API, and adds the "get_products" instead. This is with POD, and the POD of the whole WebService/Product.pm has been updated to the standard format for the webservice part. A single error message mapping has been added to constants.

The user can only access bugs that are in his "accessible" list.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 244983 [details] [diff] [review]
Add get_products to Webservice/Products.pm, v3

>+sub get_accessible_products {
>+    my $self = shift;
>+    my %union = ();

  You can just do "my %union". You don't need the "= ()".

>+    map $union{ $_->id } = 1, @{$self->get_selectable_products};
>+    map $union{ $_->id } = 1, @{$self->get_enterable_products};
>+    return Bugzilla::Product->new_from_list( [keys %union] );

  get_selectable_products and get_enterable_products already return Product objects, don't they? So creating whole new objects is kind of a waste...

>Index: Bugzilla/WebService/Constants.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Constants.pm,v
>retrieving revision 1.4
>diff -u -r1.4 Constants.pm
>--- Bugzilla/WebService/Constants.pm	2 Nov 2006 19:59:16 -0000	1.4
>+++ Bugzilla/WebService/Constants.pm	8 Nov 2006 08:17:25 -0000
>@@ -70,6 +70,8 @@
>     product_disabled    => 106,
>     # Invalid Summary
>     require_summary => 107,

>+    # Some parameter should have been numeric
>+    param_must_be_numeric => 108,

  No, that doesn't need to have a specific code. It's also not bug related, so even if it had a specific code, it wouldn't be in the 100 range.

>+sub get_products {
>+    my %union = my %isect = ();
>+    foreach my $e (@{$params->{ids}}, @{get_accessible_products()->{ids}}) { 
>+        $union{$e}++ && $isect{$e}++;
>+    }

  I think you could just do this with a grep, yes? Or maybe not...

>+    foreach my $product (@{Bugzilla::Product->new_from_list([keys %isect])}) {

  Once again, it seems like a waste to create whole new Product objects.

Comment 8

[Mads Bondo Dydensborg]

> >+    map $union{ $_->id } = 1, @{$self->get_selectable_products};
> >+    map $union{ $_->id } = 1, @{$self->get_enterable_products};
> >+    return Bugzilla::Product->new_from_list( [keys %union] );
> 
>   get_selectable_products and get_enterable_products already return Product
> objects, don't they? So creating whole new objects is kind of a waste...

Agreed. Problem is, that I do not really know how to combine the arrays of products returned from get_*_products. Please keep in mind, that I am no Perl coder (as in, I hardly understand the syntax for manipulating datastructures :-). But, I reckon, I will just have to look harder at it.

Would this be a better solution:

- store the arrays in temps
- build the hash using ids
- traverse the arrays, picking out the right objects?

Or, should I just set the hash values to the actual objects, and look for some nifty way of turning the values in the hash into an array reference? Something like this:

map $union{ $_->id } = $_, @{$self->get_selectable_products};
...
return { products => some-nifty-perl( %union ) };

(I honestly do not know what approach is best).

> 
> >Index: Bugzilla/WebService/Constants.pm
> >===================================================================
> >RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Constants.pm,v
> >retrieving revision 1.4
> >diff -u -r1.4 Constants.pm
> >--- Bugzilla/WebService/Constants.pm   2 Nov 2006 19:59:16 -0000       1.4
> >+++ Bugzilla/WebService/Constants.pm   8 Nov 2006 08:17:25 -0000
> >@@ -70,6 +70,8 @@
> >     product_disabled    => 106,
> >     # Invalid Summary
> >     require_summary => 107,
> 
> >+    # Some parameter should have been numeric
> >+    param_must_be_numeric => 108,
> 
>   No, that doesn't need to have a specific code. It's also not bug related, so
> even if it had a specific code, it wouldn't be in the 100 range.

You lost me here.

This is, AFAICT, the only error that can actually be returned from the function. Does it not need to have a number? Should it not be documented either then?


> 
> >+sub get_products {
> >+    my %union = my %isect = ();
> >+    foreach my $e (@{$params->{ids}}, @{get_accessible_products()->{ids}}) { 
> >+        $union{$e}++ && $isect{$e}++;
> >+    }
> 
>   I think you could just do this with a grep, yes? Or maybe not...

Again - "Not a Perl coder(tm)" ;-)

This is taken from a Perl Cookbook (by O'Reily, I think)... I assumed it was pretty efficient.


> 
> >+    foreach my $product (@{Bugzilla::Product->new_from_list([keys %isect])}) {
> 
>   Once again, it seems like a waste to create whole new Product objects.

Sure thing. Same problem as above, however....

Looking forward to your advice ;-)

Comment 9

[Max Kanat-Alexander]

(In reply to comment #8)
> > >+    map $union{ $_->id } = 1, @{$self->get_selectable_products};

  You could just do map $union {$_->id} = $_.

  And then you could get all the objects by doing "values %union".

> Does it not need to have a number? Should it not be documented either then?

  Right, to both. It can just be a general error.

  We don't want to keep track of ALL the errors that something could throw, because that would be a big pain. We only want to keep track of the ones that might be particularly meaningful to clients.

> This is taken from a Perl Cookbook (by O'Reily, I think)... I assumed it was
> pretty efficient.

  Okay. Do we not have a function for this already in Bugzilla::Util? If we don't, why don't you add one?

Comment 10

[Mads Bondo Dydensborg]

Attachment: Add get_products to Webservice/Products.pm, v4

New version of patch. Should reflect reviewrs comments. More 'perl like', as few object creatins as possible (given that User::get*products must return objects, instead of ids)

Comment 11

[Mads Bondo Dydensborg]

(In reply to comment #9)
> (In reply to comment #8)

> > This is taken from a Perl Cookbook (by O'Reily, I think)... I assumed it was
> > pretty efficient.
> 
>   Okay. Do we not have a function for this already in Bugzilla::Util? If we
> don't, why don't you add one?

The patch do not need this anymore. I did spot the pattern in Bug.pm, btw, (grep for isect).

(Writing this using only one hand)

> 

Comment 12

[Max Kanat-Alexander]

Comment on attachment 245093 [details] [diff] [review]
Add get_products to Webservice/Products.pm, v4

>+sub get_accessible_products {
>+    my $self = shift;
>+    
>+    # Map the objects into a hash using the ids as keys
>+    my %products = map { $_->id => $_ } 
>+    @{$self->get_selectable_products}, 
>+    @{$self->get_enterable_products};

  Nit: Those need to be spaced up to align with the {.

>+    # Create a hash with the ids the user wants
>+    my %ids = (map { $_ => 1 } @{$params->{ids}});

  Nit: You don't need the parentheses.

>+    # Return the intersection of this, by grepping the ids from 
>+    # accessible products, and creating a result entry for each.
>+    return { products => 
>+                 [ map {
>+                       {
>+                           internals   => $_,
>+                           id          => type('int')->value($_->id),
>+                           name        => type('string')->value($_->name),
>+                           description => type('string')->value($_->description),
>+                       }
>+                   } grep { $ids{$_->id} } @{$accessible_products}
>+                   ] };

  Nit: That's a lot of code in one line. Could you move the grep to a separate line, maybe? It would make it more readable.>+=item 

>+=item B<Returns>    
>+
>+A hash containing one item, C<ids>, that contains an array of product
>+ids.

  Just so you know, when something has no errors, and no parameters, and it just returns something, you don't have to do the Description/Params/Returns/Errors thing--that just takes up extra space. Usually for accessors we just write a single paragraph.

Comment 13

[Max Kanat-Alexander]

Okay, I fixed all the nits on checkin, including re-writing the last bit to be three lines instead of one long line.

Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.140; previous revision: 1.139
done
Checking in Bugzilla/WebService/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Product.pm,v  <--  Product.pm
new revision: 1.3; previous revision: 1.2
done

Comment 14

[Frédéric Buclin]

Comment on attachment 245093 [details] [diff] [review]
Add get_products to Webservice/Products.pm, v4

>Index: Bugzilla/User.pm

>+sub get_accessible_products {
>+    my $self = shift;
>+    
>+    # Map the objects into a hash using the ids as keys
>+    my %products = map { $_->id => $_ } 
>+    @{$self->get_selectable_products}, 
>+    @{$self->get_enterable_products};


This routine has nothing to do in Bugzilla::User, IMO. It's not used in the main code and has no special meaning as it does an union between products you can see and products you can edit, but is unusable as is. This stuff should be moved into WebService/*.pm.

I would have r- it if I saw this patch before its checkin.

Comment 15

[Max Kanat-Alexander]

(In reply to comment #14)
> It's not used in the
> main code and has no special meaning as it does an union between products you
> can see and products you can edit, but is unusable as is.

  No, it's useful. For example, we should use it in describecomponents.cgi.

Comment 16

[Frédéric Buclin]

test script: webservice_product.t