473646 - WebService methods should check list parameters for scalars and convert before use

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[David Lawrence [:dkl]]

When a WebService method is expecting a list reference as one of the parameters passed in, but the param is a scalar, an error occurs when the method tries to access the parameter.

One example would be retrieving a bug using Bug.get, if you pass a single bug id as a scalar for the 'ids' param when it is expecting a list, you get the following error to the client:

Can't use string ("1") as an ARRAY ref while "strict refs" in use at /var/www/html/bugzilla/Bugzilla/WebService/Bug.pm line 151

All WebService methods, should either automatically convert a single value to a list before trying to access as list or throw error to the client stating that it only accepts values in the form of lists. These instead of throwing a code
error.

I believe it should automatically convert the values to lists and then just do the right thing. What do others think?

Will work out a patch to attach soon.

Dave

Comment 1

[Max Kanat-Alexander]

Yes, for each function, we should be able to call a filtering function that will convert certain inbound parameters to lists, in a certain way, if found. Here's what I'd like to see supported:

Bug.get(1) => Bug.get({ ids => [1] })

Bug.get({ ids => 1 }) => Bug.get({ ids => [1] })

Comment 2

[David Lawrence [:dkl]]

Attachment: Patch to add parameter validation function for webservice methods (v.01)

Here is my first pass at adding a validation function for the parameters passed into the webservices methods.

This is heavily borrowed but changed alot from Param::Validate. We could just require Param::Validate but I was thinking one less module to depend on may be in order and we don't really need all of the functionality that Param::Validate provides.

Basically you would form your webservice method this way:

sub get {
    my $self = shift;

    my $params = validate(@_, {  ids => { type     => 'arrayref',
                                          alt_type => 'scalar',
                                          optional => 0 },
                                 permissive => { type => 'scalar',
                                                 optional => 1 } });

    ....
}

If all checks pass then $params will have the same values as were passed in. Valid types are scalar, arrayref, hashref, etc. and the alt_type is the second type it will look for and then convert to the primary type if the alt_type is correct.

This is very first pass and I will add documentation as well for everything if the concept is sane.

Let me know what you think.
Dave

Comment 3

[Max Kanat-Alexander]

I'd prefer something simpler. for example, for Bug.comments we'd see something like:

$params = validate(@_, 'bug_ids', 'comment_ids');

The first item would be what we'd use if there were no named arguments at all. The rest are just things to convert to arrayrefs if they're scalars.

Comment 4

[David Lawrence [:dkl]]

I still feel we need some sort of type specification that is passed to validate(). For example if the named parameters are of mixed types we will not know which to convert to arrayrefs and which to leave alone. We also will not know which are optional and which aren't unless we just not list them in the spec if they are optional and assume if they are listed then they are required.

So what if we did:

my $params = validate(@_, { bug_ids => 'arrayref', 
                            comment_ids => 'arrayref', 
                            some_scalar_param => 'scalar',
                            optional => [qw/bug_ids comment_ids some_scalar_param/] });

Then we can see that if bug_ids is a scalar and we wanted an arrayref we do the conversion. If the field is not listed in optional, we throw an error stating the field was required. Simpler?

Dave

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> For example if the named parameters are of mixed types we will not
> know which to convert to arrayrefs and which to leave alone.

  No, we would. We'd only convert the listed arguments, and they would always be converted to arrayrefs. This handles every function we currently have.

> We also will not
> know which are optional and which aren't unless we just not list them in the
> spec if they are optional and assume if they are listed then they are required.

  We could also just keep checking whether they're required manually, but I do agree that's not as optimal.

> [snip]
> the field was required. Simpler?

  Well, it doesn't handle the Bug.comments case (where it requires one or another parameter or both), and this bug isn't about checking whether or not a parameter is required, just about converting scalars to arrayrefs. I think we should just stick to that for now, and then we can expand upon it in a later bug.

Comment 6

[David Lawrence [:dkl]]

Attachment: Patch to add parameter validation function for webservice methods (v1)

A simplified patch that just converts scalars to arrayrefs if listed for now. It leaves all other params unaltered.

Please review
Dave

Comment 7

[Max Kanat-Alexander]

Comment on attachment 357826 [details] [diff] [review]
Patch to add parameter validation function for webservice methods (v1)

>Index: Bugzilla/WebService/Bug.pm
> [snip]
> sub get {
> [snip]
>+    my $params = validate(@_, 'ids');
>     my $ids = $params->{ids};
>-    defined $ids || ThrowCodeError('param_required', { param => 'ids' });

  That looks like a mistake?

>Index: Bugzilla/WebService/Util.pm
>+sub validate {
>+    my ($p, @params) = @_;
>+
>+    if (ref $p eq 'ARRAY') {
>+        # we were called as validate(@_, ...) where @_ has a
>+        # single element, a hash reference
>+        if (ref $p->[0]) {

  That doesn't sound right...you checked if $p was an arrayref. Actually, this whole block is confusing. I thought that for this bug we were just converting scalar elements in the hashref to arrays?

>+        } else {
>+            $p = {@$p};

  What is that for?

  This might all be clearer if we used some other variable name than $p.

>+    # If @params then we are using named parameters 
>+    # and the values listed need to be arrayrefs.

  That's actually always the case in the current patch, yeah?

>+    if (@params) { 
>+        foreach my $param (@params) {

  You don't need the wrapping if, because the foreach just won't run if the array is empty.

>+            if (exists $p->{$param}) {
>+                $p->{$param} = ref $p->{$param} ? $p->{$param} : [$p->{$param}];
>+            }

>+    return wantarray ? %$p : $p;

  I think we should just always return a hashref for now, since that's all we use.

Comment 8

[David Lawrence [:dkl]]

(In reply to comment #7)
> (From update of attachment 357826 [details] [diff] [review])
> >Index: Bugzilla/WebService/Bug.pm
> > [snip]
> > sub get {
> > [snip]
> >+    my $params = validate(@_, 'ids');
> >     my $ids = $params->{ids};
> >-    defined $ids || ThrowCodeError('param_required', { param => 'ids' });
> 
>   That looks like a mistake?
> 

I originally removed it using my earlier method since the earlier method would be doing the verification for us. But forgot to put it back for the last simplified version.

> >Index: Bugzilla/WebService/Util.pm
> >+sub validate {
> >+    my ($p, @params) = @_;
> >+
> >+    if (ref $p eq 'ARRAY') {
> >+        # we were called as validate(@_, ...) where @_ has a
> >+        # single element, a hash reference
> >+        if (ref $p->[0]) {
> 
>   That doesn't sound right...you checked if $p was an arrayref. Actually, this
> whole block is confusing. I thought that for this bug we were just converting
> scalar elements in the hashref to arrays?
> 
> >+        } else {
> >+            $p = {@$p};
> 
>   What is that for?
> 
>   This might all be clearer if we used some other variable name than $p.
> 

The whole block came from Params::Validate which checks for proper @_
format and tried to do the right thing if the caller passes key/values
as a list instead of hashref. But overengineering for our purposes. Will remove.

> >+    # If @params then we are using named parameters 
> >+    # and the values listed need to be arrayrefs.
> 
>   That's actually always the case in the current patch, yeah?
> 

Yes, but @params could be empty which means no conversions will occur so we
check for it. I will reword it.

> >+    if (@params) { 
> >+        foreach my $param (@params) {
> 
>   You don't need the wrapping if, because the foreach just won't run if the
> array is empty.
> 

agreed

> >+            if (exists $p->{$param}) {
> >+                $p->{$param} = ref $p->{$param} ? $p->{$param} : [$p->{$param}];
> >+            }
> 
> >+    return wantarray ? %$p : $p;
> 
>   I think we should just always return a hashref for now, since that's all we
> use.

ok

Comment 9

[David Lawrence [:dkl]]

Attachment: Patch to add parameter validation function for webservice methods (v2)

New patch with suggested changes.

Comment 10

[David Lawrence [:dkl]]

Attachment: Patch to add parameter validation function for webservice methods (v3)

New patch that I like better. No longer passing in @_ to validate. Just pass in $params and return $params. Also I also split on , or spaces in the code that
converts scalar to an array reference. This way a user can pass either a single scalar value or a comma/space delimited list of values.

Please review.
Dave

Comment 11

[Frédéric Buclin]

(In reply to comment #10)
> Also I also split on , or spaces

IMO, that's a mistake to do this. "foo, bar" is a valid product name AFAIK. Splitting it into ["foo", "bar"] is incorrect. I really think we shouldn't do this kind of assumptions. We shouldn't accept concatenated strings; it's prone to such problems.

Comment 12

[Max Kanat-Alexander]

Comment on attachment 358211 [details] [diff] [review]
Patch to add parameter validation function for webservice methods (v3)

Hmm. Actually, I'd kind of rather pass @_, so that we can allow things like:

  Bug.get(1, 2, 3)

In the future.

You could, however, even do:

 my ($self, $params) = validate(@_, 'bug_id', 'comment_id');

  If you wanted, and then we'd save a line.

  And also, yes, don't split on commas or spaces--there are lots of valid things that could have commas or spaces in them, even if most of our arguments couldn't currently. People should just pass arrays if they want multiple items.

Comment 13

[David Lawrence [:dkl]]

(In reply to comment #12)
> (From update of attachment 358211 [details] [diff] [review])
> Hmm. Actually, I'd kind of rather pass @_, so that we can allow things like:
> 
>   Bug.get(1, 2, 3)
> 
> In the future.
> 
> You could, however, even do:
> 
>  my ($self, $params) = validate(@_, 'bug_id', 'comment_id');
> 
>   If you wanted, and then we'd save a line.
> 
>   And also, yes, don't split on commas or spaces--there are lots of valid
> things that could have commas or spaces in them, even if most of our arguments
> couldn't currently. People should just pass arrays if they want multiple items.

I think we should omit passing of $self in so the validate could be used in places other than webservices, etc.

Maybe the following would be sufficient?

Bugzilla::WebService::Bug:

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

    [snip]
}

Bugzilla::WebService::Util:

sub validate (\@@) {
    my ($args, @keys) = @_;
    my $params = $args->[0];
    
    [snip]

    return $params;
}

Then we could do Bug.get(1,2,3) as well in the future? Sound sane?

Dave

Comment 14

[Max Kanat-Alexander]

(In reply to comment #13)
> I think we should omit passing of $self in so the validate could be used in
> places other than webservices, etc.

  Mmm, I don't think we'll be using it in places other than WebServices.

> Then we could do Bug.get(1,2,3) as well in the future? Sound sane?

  So you'd pass in \@_ or something? I don't think that's what \@ means in a prototype.

Comment 15

[David Lawrence [:dkl]]

Attachment: Patch to add parameter validation function for webservice methods (v4)

Simplified version of the validate() patch that takes in $self, $params, and list of keys to convert to arrayrefs. Then returns $self and $params.

Comment 16

[Max Kanat-Alexander]

Comment on attachment 358889 [details] [diff] [review]
Patch to add parameter validation function for webservice methods (v4)

Looks great. :-)

Comment 17

[David Lawrence [:dkl]]

tip:
Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.28; previous revision: 1.27
done
Checking in Bugzilla/WebService/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Product.pm,v  <--  Product.pm
new revision: 1.8; previous revision: 1.7
done
Checking in Bugzilla/WebService/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/User.pm,v  <--  User.pm
new revision: 1.14; previous revision: 1.13
done
Checking in Bugzilla/WebService/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Util.pm,v  <--  Util.pm
new revision: 1.3; previous revision: 1.2
done

Comment 18

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.4 in bug 494037.