From ac4cdfc2577b9d25ccbc9ac5d8f0333a81102367 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Fri, 21 Nov 2014 19:09:43 -0500 Subject: [PATCH] 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. --- .../app/controllers/application_controller.rb | 34 +++++++++++++++++ .../arvados/v1/schema_controller.rb | 8 +--- .../functional/application_controller_test.rb | 11 ++++++ .../arvados/v1/job_reuse_controller_test.rb | 38 ++++++++++--------- 4 files changed, 66 insertions(+), 25 deletions(-) diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb index 4f0364f15a..27c4bc852f 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 c5b2bcf2f2..2f7af3c428 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 4144d0a922..9aad981caf 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 f9ecbf518e..9b66851d7e 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 -- 2.30.2