[ARVADOS] created: ac4cdfc2577b9d25ccbc9ac5d8f0333a81102367

[git at public.curoverse.com]

        at  ac4cdfc2577b9d25ccbc9ac5d8f0333a81102367 (commit)


commit ac4cdfc2577b9d25ccbc9ac5d8f0333a81102367
Author: Tom Clegg <tom at curoverse.com>
Date:   Fri Nov 21 19:09:43 2014 -0500

    4651: Accept "false" as false for a boolean param. Reject bogus strings.
    
    Also, advertise find_or_create param from the controller, not just the
    discovery document generator.

diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 4f0364f..27c4bc8 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -35,6 +35,7 @@ class ApplicationController < ActionController::Base
   before_filter :catch_redirect_hint
   before_filter(:find_object_by_uuid,
                 except: [:index, :create] + ERROR_ACTIONS)
+  before_filter :load_required_parameters
   before_filter :load_limit_offset_order_params, only: [:index, :contents]
   before_filter :load_where_param, only: [:index, :contents]
   before_filter :load_filters_param, only: [:index, :contents]
@@ -446,6 +447,39 @@ class ApplicationController < ActionController::Base
     end
   end
 
+  def load_required_parameters
+    (self.class.send "_#{params[:action]}_requires_parameters" rescue {}).
+      each do |key, info|
+      if info[:required] and not params.include?(key)
+        raise ArgumentError("#{key} parameter is required")
+      elsif info[:type] == 'boolean'
+        # Make sure params[key] is either true or false -- not a
+        # string, not nil, etc.
+        if not params.include?(key)
+          params[key] = info[:default]
+        elsif [false, 'false'].include? params[key]
+          params[key] = false
+        elsif [true, 'true'].include? params[key]
+          params[key] = true
+        else
+          raise TypeError("#{key} parameter must be a boolean, true or false")
+        end
+      end
+    end
+    true
+  end
+
+  def self._create_requires_parameters
+    {
+      ensure_unique_name: {
+        type: "boolean",
+        description: "Adjust name to ensure uniqueness instead of returning an error on (owner_uuid, name) collision.",
+        location: "query",
+        default: false
+      }
+    }
+  end
+
   def self._index_requires_parameters
     {
       filters: { type: 'array', required: false },
diff --git a/services/api/app/controllers/arvados/v1/schema_controller.rb b/services/api/app/controllers/arvados/v1/schema_controller.rb
index c5b2bcf..2f7af3c 100644
--- a/services/api/app/controllers/arvados/v1/schema_controller.rb
+++ b/services/api/app/controllers/arvados/v1/schema_controller.rb
@@ -258,13 +258,7 @@ class Arvados::V1::SchemaController < ApplicationController
               path: "#{k.to_s.underscore.pluralize}",
               httpMethod: "POST",
               description: "Create a new #{k.to_s}.",
-              parameters: {
-                ensure_unique_name: {
-                  type: "boolean",
-                  description: "Adjust name to ensure uniqueness instead of returning an error on (owner_uuid, name) collision.",
-                  location: "query"
-                }
-              },
+              parameters: {},
               request: {
                 required: true,
                 properties: {
diff --git a/services/api/test/functional/application_controller_test.rb b/services/api/test/functional/application_controller_test.rb
index 4144d0a..9aad981 100644
--- a/services/api/test/functional/application_controller_test.rb
+++ b/services/api/test/functional/application_controller_test.rb
@@ -46,4 +46,15 @@ class ApplicationControllerTest < ActionController::TestCase
     assert_response 422
     check_error_token
   end
+
+  ['foo', '', 'FALSE', 'TRUE', nil, [true], {a:true}, '"true"'].each do |bogus|
+    test "bogus boolean parameter #{bogus.inspect} returns error" do
+      authorize_with :active
+      post :create, {
+        specimen: {},
+        ensure_unique_name: bogus
+      }
+      assert_response 422
+    end
+  end
 end
diff --git a/services/api/test/functional/arvados/v1/job_reuse_controller_test.rb b/services/api/test/functional/arvados/v1/job_reuse_controller_test.rb
index f9ecbf5..9b66851 100644
--- a/services/api/test/functional/arvados/v1/job_reuse_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/job_reuse_controller_test.rb
@@ -90,24 +90,26 @@ class Arvados::V1::JobReuseControllerTest < ActionController::TestCase
     assert_equal '4fe459abe02d9b365932b8f5dc419439ab4e2577', new_job['script_version']
   end
 
-  test "do not reuse job because find_or_create=false" do
-    post :create, {
-      job: {
-        script: "hash",
-        script_version: "4fe459abe02d9b365932b8f5dc419439ab4e2577",
-        repository: "foo",
-        script_parameters: {
-          input: 'fa7aeb5140e2848d39b416daeef4ffc5+45',
-          an_integer: '1'
-        }
-      },
-      find_or_create: false
-    }
-    assert_response :success
-    assert_not_nil assigns(:object)
-    new_job = JSON.parse(@response.body)
-    assert_not_equal 'zzzzz-8i9sb-cjs4pklxxjykqqq', new_job['uuid']
-    assert_equal '4fe459abe02d9b365932b8f5dc419439ab4e2577', new_job['script_version']
+  [false, "false"].each do |whichfalse|
+    test "do not reuse job because find_or_create=#{whichfalse.inspect}" do
+      post :create, {
+        job: {
+          script: "hash",
+          script_version: "4fe459abe02d9b365932b8f5dc419439ab4e2577",
+          repository: "foo",
+          script_parameters: {
+            input: 'fa7aeb5140e2848d39b416daeef4ffc5+45',
+            an_integer: '1'
+          }
+        },
+        find_or_create: whichfalse
+      }
+      assert_response :success
+      assert_not_nil assigns(:object)
+      new_job = JSON.parse(@response.body)
+      assert_not_equal 'zzzzz-8i9sb-cjs4pklxxjykqqq', new_job['uuid']
+      assert_equal '4fe459abe02d9b365932b8f5dc419439ab4e2577', new_job['script_version']
+    end
   end
 
   test "do not reuse job because output is not readable by user" do

-----------------------------------------------------------------------


hooks/post-receive
--