1093600 - REST shouldn't support multiple instances of parameters for resources which only support a single params (eg. POST bug/comment)

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Pete Moore [:pmoore][:pete]]

curl -d 'id=1091084' --data-urlencode 'comment=In production: https://hg.mozilla.org/build/mozharness/rev/6375288c157b' --data-urlencode 'login=[e-mail]' --data-urlencode 'password=XXXXXXXXX' 'https://bugzilla.mozilla.org/rest/bug/1091084/comment'

returned

{"documentation":"http://www.bugzilla.org/docs/tip/en/html/api/","error":true,"code":100500,"message":"Not a HASH reference at /data/www/bugzilla.mozilla.org/Bugzilla/Bug.pm line 389.\n"}


Normally this would post the comment "In production: https://hg.mozilla.org/build/mozharness/rev/6375288c157b" in bug 1091084 as user [e-mail].

Comment 1

[Pete Moore [:pmoore][:pete]]

Request headers sent:

0000: POST /rest/bug/1091084/comment HTTP/1.1
0029: User-Agent: curl/7.30.0
0042: Host: bugzilla.mozilla.org
005e: Accept: */*
006b: Content-Length: 169
0080: Content-Type: application/x-www-form-urlencoded
00b1: 


Request body sent:

0000: id=1091084&comment=In%20production%3A%20https%3A%2F%2Fhg.mozilla
0040: .org%2Fbuild%2Fmozharness%2Frev%2F6375288c157b&login=slaveapi%40
0080: mozilla.releng.tld&password=XXXXXXXXXXXXX


(password obfuscated)

Comment 2

[:glob ✱]

looks like bug 1088253 caused this.

    my $id = ref($param)
        ? ($param->{id} = trim($param->{id}))
        : ($param = trim($param));

$params is now an arrayref:

    $params = [
                '39',
                '39'
              ];

Comment 3

[:glob ✱]

the bug id is being passed in twice - as part of the rest url and in the post data.
with bug 1088253, this becomes an array with both of the id's, which Bugzilla::Bug doesn't understand.

i think the safest thing to do right now is to backout bug 1088253, then work on a fix.

Comment 4

[:glob ✱]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   6ba0783..64fc523  master -> master

Comment 5

[Pete Moore [:pmoore][:pete]]

Many thanks glob!

Comment 6

[Pete Moore [:pmoore][:pete]]

Attachment: bug1093600_tools_v1.patch

This patch just removed the bug id from the post parameters.

Comment 7

[:glob ✱]

Comment on attachment 8516709 [details] [diff] [review]
bug1093600_tools_v1.patch

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

r=glob this looks sane.

i can't test the python, but i can confirm that a the bug id isn't required in the post body if it's present as part of the url.

Comment 8

[Pete Moore [:pmoore][:pete]]

Comment on attachment 8516709 [details] [diff] [review]
bug1093600_tools_v1.patch

Checked in:
https://hg.mozilla.org/build/tools/rev/9169a90204fb

Comment 9

[Pete Moore [:pmoore][:pete]]

Comment on attachment 8516709 [details] [diff] [review]
bug1093600_tools_v1.patch

I mean:
https://hg.mozilla.org/build/tools/rev/a1fac9a50095

Comment 10

[David Lawrence [:dkl]]

I think we will update the code in REST.pm that combines the params to follow this flow:

if (rest resource accepts a single value only)
   honour the query string only (old behavior)
else
   merge params (as per current trunk/5.0 code)

We can determine if the method accepts multiple params or not by looking at the bz_rest_params data that is returned by the REST/Resource/*.pm resource files.

dkl

Comment 11

[David Lawrence [:dkl]]

Attachment: 1093600_1.patch

Comment 12

[Frédéric Buclin]

Comment on attachment 8518541 [details] [diff] [review]
1093600_1.patch

>+    use Data::Dumper; print STDERR Dumper $params;
> 
>+    print STDERR Dumper $params;

Hrm....

Comment 13

[David Lawrence [:dkl]]

Attachment: 1093600_2.patch

Oops. How did those get in there? :)

Comment 14

[:glob ✱]

Comment on attachment 8518916 [details] [diff] [review]
1093600_2.patch

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

r=glob

::: Bugzilla/WebService/Server/REST.pm
@@ +366,3 @@
>      if ($self->request->method ne 'GET') {
>          my $extra_params = {};
> +         # We do this manually cause CGI.pm doesn't understand JSON strings.

nit: indented too much by one space, and s/cause/because/

Comment 15

[David Lawrence [:dkl]]

To ssh://[e-mail]/bugzilla/bugzilla.git
   b9543e4..8d368ae  master -> master

Comment 16

[David Lawrence [:dkl]]

And 5.0:

To ssh://[e-mail]/bugzilla/bugzilla.git
   4e5a3b1..4a43627  5.0 -> 5.0