Bug 2072305 - Accept query-string params on Component create/update

Bugzilla/API/V1/Component.pm

@@ -10,12 +10,12 @@ package Bugzilla::API::V1::Component;
 use 5.10.1;
 use Mojo::Base qw( Mojolicious::Controller );
 
-use Mojo::JSON qw(decode_json false true);
-use Try::Tiny;
+use Mojo::JSON qw(false true);
 
 use Bugzilla::Component;
 use Bugzilla::Constants;
-use Bugzilla::Util qw(email_filter trim);
+use Bugzilla::Util             qw(email_filter trim);
+use Bugzilla::WebService::Util qw(merge_request_params);
 
 sub setup_routes {
   my ($class, $r) = @_;
@@ -47,7 +47,7 @@ sub create {
     || return $self->user_error('auth_failure',
     {group => 'editcomponents', action => 'add', object => 'components'});
 
-  my ($params, $error) = $self->_get_params();
+  my ($params, $error) = merge_request_params($self);
   return $self->user_error($error) if $error;
 
   my $product = Bugzilla::Product->check({name => $self->param('product')});
@@ -95,21 +95,36 @@ sub update {
   my $component = Bugzilla::Component->check(
     {name => $self->param('component'), product => $product});
 
-  my ($params, $error) = $self->_get_params();
+  my ($params, $error) = merge_request_params($self);
   return $self->user_error($error) if $error;
 
+  # Whitelist the documented update fields; set_all() throws unknown_method
+  # for any stray key (e.g. Bugzilla_api_token, include_fields).
+  my %values = map { $_ => $params->{$_} }
+    grep { exists $params->{$_} }
+    qw(name description default_assignee default_qa_contact default_bug_type
+    is_active triage_owner team_name bug_description_template);
+
+  # A JSON body sends is_active as a real boolean, but the query string and a
+  # form body send the literal string "true" or "false", and check_boolean
+  # treats any non-empty string as true.
+  if (exists $values{is_active} && !ref $values{is_active}) {
+    my $is_active = lc($values{is_active} // '');
+    return $self->user_error('invalid_params',
+      {type_error => 'is_active must be true or false'})
+      if $is_active !~ /^(?:true|false|1|0)$/;
+    $values{is_active} = ($is_active eq 'true' || $is_active eq '1') ? 1 : 0;
+  }
+
   # If the user is only able to edit triage owner and nothing else,
   # then we only allow that field to be passed to set_all()
   if (!$user->in_group('editcomponents')) {
-    if (exists $params->{triage_owner}) {
-      $params = {triage_owner => $params->{triage_owner}};
-    }
-    else {
-      $params = {};
-    }
+    %values = exists $values{triage_owner}
+      ? (triage_owner => $values{triage_owner})
+      : ();
   }
 
-  $component->set_all($params);
+  $component->set_all(\%values);
   $component->update();
 
   return $self->render(json => $self->_component_to_hash($component));
@@ -143,17 +158,4 @@ sub _component_to_hash {
   };
 }
 
-sub _get_params {
-  my ($self) = @_;
-  my $params = {};
-  my $error  = '';
-  try {
-    $params = decode_json($self->req->body);
-  }
-  catch {
-    $error = 'rest_malformed_json';
-  };
-  return ($params, $error);
-}
-
 1;

qa/t/rest_components.t

@@ -14,6 +14,7 @@ use Bugzilla;
 use QA::Util qw(get_config);
 
 use MIME::Base64 qw(encode_base64 decode_base64);
+use Mojo::JSON qw(false true);
 use Test::Mojo;
 use Test::More;
 
@@ -75,6 +76,14 @@ $t->post_ok($url
   ->json_is('/message' =>
     'The Firefox product already has a component named TestComponent.');
 
+# Fields may also be passed entirely via the query string, with no JSON body.
+$t->post_ok($url
+    . 'rest/component/Firefox?name=QueryStringComponent'
+    . '&description=Created%20via%20query%20string'
+    . '&default_assignee=admin%40mozilla.test&team_name=Mozilla' =>
+    {'X-Bugzilla-API-Key' => $api_key})->status_is(200)
+  ->json_is('/name' => 'QueryStringComponent');
+
 ### Section 2: Make updates to the component
 
 my $update = {
@@ -97,12 +106,52 @@ $t->put_ok($url
   ->json_is('/description'      => 'Updated description')
   ->json_is('/default_assignee' => 'permanent_user@mozilla.test');
 
+# A query-string parameter is also accepted on PUT, and wins over a matching
+# parameter in the JSON body.
+$t->put_ok($url
+    . 'rest/component/Firefox/TestComponent?description=Query%20String%20Wins' =>
+    {'X-Bugzilla-API-Key' => $api_key} =>
+    json => {description => 'Should Not Be Used'})->status_is(200)
+  ->json_is('/description' => 'Query String Wins');
+
 # Retrieve the new component and verify
 $t->get_ok($url
     . 'rest/component/Firefox/TestComponent' =>
     {'X-Bugzilla-API-Key' => $api_key})->status_is(200)
   ->json_is('/triage_owner' => 'admin@mozilla.test')
-  ->json_is('/description'  => 'Updated description');
+  ->json_is('/description'  => 'Query String Wins');
+
+# A form-urlencoded body and the query string may both carry the same field;
+# the query string wins and the value stays a plain string (it used to be
+# merged into an arrayref and stored as "ARRAY(0x...)").
+$t->put_ok($url
+    . 'rest/component/Firefox/TestComponent?description=Query%20Beats%20Form' =>
+    {'X-Bugzilla-API-Key' => $api_key} =>
+    form => {description => 'Form Body Loses'})->status_is(200)
+  ->json_is('/description' => 'Query Beats Form');
+
+# A malformed JSON body is rejected instead of being treated as an empty,
+# successful update.
+$t->put_ok($url
+    . 'rest/component/Firefox/TestComponent' =>
+    {'X-Bugzilla-API-Key' => $api_key} => '{"description": ')
+  ->status_is(400)->json_is('/code' => 32000)
+  ->json_like('/message' => qr/JSON data used for the request was malformed/);
+
+# is_active from the query string is the string "true"/"false", which must be
+# coerced rather than treated as a truthy string.
+$t->put_ok($url
+    . 'rest/component/Firefox/TestComponent?is_active=false' =>
+    {'X-Bugzilla-API-Key' => $api_key})->status_is(200)
+  ->json_is('/is_active' => false);
+$t->put_ok($url
+    . 'rest/component/Firefox/TestComponent?is_active=true' =>
+    {'X-Bugzilla-API-Key' => $api_key})->status_is(200)
+  ->json_is('/is_active' => true);
+$t->put_ok($url
+    . 'rest/component/Firefox/TestComponent?is_active=maybe' =>
+    {'X-Bugzilla-API-Key' => $api_key})->status_is(400)
+  ->json_like('/message' => qr/is_active must be true or false/);
 
 # Update an existing user and give edittriageowners permissions
 my $user_update = {groups => {add => ['edittriageowners']}};