From 1a2303ec9b2259a1123ecd62b32f2c6835dd6be0 Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Thu, 3 Jun 2021 15:59:19 -0400 Subject: [PATCH] ActiveModel types should only cast and not validate --- README.md | 16 +++++---- .../maintenance_tasks/tasks_controller.rb | 6 ++-- .../parameters/type_error.rb | 8 ----- .../integer_array_type.rb | 17 ++-------- .../{parameters => task}/string_array_type.rb | 17 ++-------- .../app/tasks/maintenance/params_task.rb | 2 +- .../parameters/string_array_type_test.rb | 34 ------------------- test/models/maintenance_tasks/run_test.rb | 10 ------ .../integer_array_type_test.rb | 15 ++------ .../task/string_array_type_test.rb | 16 +++++++++ test/system/maintenance_tasks/runs_test.rb | 14 -------- 11 files changed, 36 insertions(+), 119 deletions(-) delete mode 100644 app/models/maintenance_tasks/parameters/type_error.rb rename app/models/maintenance_tasks/{parameters => task}/integer_array_type.rb (61%) rename app/models/maintenance_tasks/{parameters => task}/string_array_type.rb (61%) delete mode 100644 test/models/maintenance_tasks/parameters/string_array_type_test.rb rename test/models/maintenance_tasks/{parameters => task}/integer_array_type_test.rb (56%) create mode 100644 test/models/maintenance_tasks/task/string_array_type_test.rb diff --git a/README.md b/README.md index dd88a4391..38762ff40 100644 --- a/README.md +++ b/README.md @@ -230,19 +230,21 @@ expects, and to sanitize any inputs if necessary. The gem provides custom ActiveModel::Type::Value objects for complex parameter types: -- `MaintenanceTasks::Parameters::IntegerArrayType`: Accepts a comma-delimited -string of integers and turns it into an array of integers. -- `MaintenanceTasks::Parameters::StringArrayType`: Accepts an alphanumeric, -comma-delimited string and turns it into an array of strings. +- `IntegerArrayType`: Accepts a comma-delimited string of integers and turns it +into an array of integers. +- `StringArrayType`: Accepts an alphanumeric, comma-delimited string and turns +it into an array of strings. -If you use one of these types for a parameter in your Task, the input will be -validated automatically and typecast appropriately. +If you use one of these types for a parameter in your Task, your input will be +coerced appropriately. **You may still want write validations for your input, +as type coercion is fairly lax. For example, if you input "xyz" for an +`IntegerArrayType` parameter, it will be cast to `[0]`.** ```ruby # app/tasks/maintenance/update_posts_via_params_task.rb module Maintenance class UpdatePostsViaParamsTask < MaintenanceTasks::Task - attribute :post_ids, MaintenanceTasks::Parameters::IntegerArrayType.new + attribute :post_ids, IntegerArrayType.new validates :post_ids, presence: true def collection diff --git a/app/controllers/maintenance_tasks/tasks_controller.rb b/app/controllers/maintenance_tasks/tasks_controller.rb index 81c505dc7..2e307a2ed 100644 --- a/app/controllers/maintenance_tasks/tasks_controller.rb +++ b/app/controllers/maintenance_tasks/tasks_controller.rb @@ -33,11 +33,11 @@ def run redirect_to(task_path(task)) rescue ActiveRecord::RecordInvalid => error redirect_to(task_path(error.record.task_name), alert: error.message) - rescue Runner::EnqueuingError => error - redirect_to(task_path(error.run.task_name), alert: error.message) - rescue ActiveRecord::ValueTooLong, Parameters::TypeError => error + rescue ActiveRecord::ValueTooLong => error task_name = params.fetch(:id) redirect_to(task_path(task_name), alert: error.message) + rescue Runner::EnqueuingError => error + redirect_to(task_path(error.run.task_name), alert: error.message) end private diff --git a/app/models/maintenance_tasks/parameters/type_error.rb b/app/models/maintenance_tasks/parameters/type_error.rb deleted file mode 100644 index 7b384031e..000000000 --- a/app/models/maintenance_tasks/parameters/type_error.rb +++ /dev/null @@ -1,8 +0,0 @@ -# frozen_string_literal: true -module MaintenanceTasks - module Parameters - # Error to raise when parameter ActiveModel::Value::Type objects fail - # validation. - class TypeError < StandardError; end - end -end diff --git a/app/models/maintenance_tasks/parameters/integer_array_type.rb b/app/models/maintenance_tasks/task/integer_array_type.rb similarity index 61% rename from app/models/maintenance_tasks/parameters/integer_array_type.rb rename to app/models/maintenance_tasks/task/integer_array_type.rb index 388464b15..05c1f618c 100644 --- a/app/models/maintenance_tasks/parameters/integer_array_type.rb +++ b/app/models/maintenance_tasks/task/integer_array_type.rb @@ -1,31 +1,18 @@ # frozen_string_literal: true module MaintenanceTasks - module Parameters + class Task # Type class representing an array of integers. Tasks using the Attributes # API for parameter support can use this class to turn input from the UI # into an array of integers within their Task. class IntegerArrayType < ActiveModel::Type::Value - # Casts string from form input field to an array of integers after - # validating that the input is correct. + # Casts string from form input field to an array of integers. # # @param input [String] the value to cast to an array of integers. # @return [Array] the data cast as an array of integers. def cast(input) return unless input.present? - - validate_input(input) input.split(",").map(&:to_i) end - - private - - def validate_input(input) - unless /\A(\s?\d+\s?(,\s?\d+\s?)*\s?)\z/.match?(input) - error_message = "#{self.class} expects a " \ - "comma-delimited string of integers. Input received: #{input}" - raise TypeError, error_message - end - end end end end diff --git a/app/models/maintenance_tasks/parameters/string_array_type.rb b/app/models/maintenance_tasks/task/string_array_type.rb similarity index 61% rename from app/models/maintenance_tasks/parameters/string_array_type.rb rename to app/models/maintenance_tasks/task/string_array_type.rb index 827296084..99e027d59 100644 --- a/app/models/maintenance_tasks/parameters/string_array_type.rb +++ b/app/models/maintenance_tasks/task/string_array_type.rb @@ -1,31 +1,18 @@ # frozen_string_literal: true module MaintenanceTasks - module Parameters + class Task # Type class representing an array of strings. Tasks using the Attributes # API for parameter support can use this class to turn input from the UI # into an array of strings within their Task. class StringArrayType < ActiveModel::Type::Value - # Casts string from form input field to an array of strings after - # validating that the input is correct. + # Casts string from form input field to an array of strings. # # @param input [String] the value to cast to an array of strings. # @return [Array] the data cast as an array of strings. def cast(input) return unless input.present? - - validate_input(input) input.split(",") end - - private - - def validate_input(input) - unless /\A(\s?\w+\s?(,\s?\w+\s?)*)\z/.match?(input) - error_message = "#{self.class} expects alphanumeric, "\ - "comma-delimited string. Input received: #{input}" - raise TypeError, error_message - end - end end end end diff --git a/test/dummy/app/tasks/maintenance/params_task.rb b/test/dummy/app/tasks/maintenance/params_task.rb index 4aedbf052..28c209ee1 100644 --- a/test/dummy/app/tasks/maintenance/params_task.rb +++ b/test/dummy/app/tasks/maintenance/params_task.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module Maintenance class ParamsTask < MaintenanceTasks::Task - attribute :post_ids, MaintenanceTasks::Parameters::IntegerArrayType.new + attribute :post_ids, IntegerArrayType.new validates :post_ids, presence: true class << self diff --git a/test/models/maintenance_tasks/parameters/string_array_type_test.rb b/test/models/maintenance_tasks/parameters/string_array_type_test.rb deleted file mode 100644 index f29d4b0c5..000000000 --- a/test/models/maintenance_tasks/parameters/string_array_type_test.rb +++ /dev/null @@ -1,34 +0,0 @@ -# frozen_string_literal: true -require "test_helper" - -module MaintenanceTasks - module Parameters - class StringArrayTypeTest < ActiveSupport::TestCase - test "#cast returns nil if input not present" do - assert_nil StringArrayType.new.cast("") - end - - test "#cast converts string to array of strings" do - assert_equal ["abc", "def"], StringArrayType.new.cast("abc,def") - end - - test "#cast converts string to array of strings, including extra whitespace" do - assert_equal ["abc ", "def", " ghi", " jkl "], - StringArrayType.new.cast("abc ,def, ghi, jkl ") - end - - test "#cast raises if input is not valid" do - input = "$!$" - error = assert_raises(TypeError) do - StringArrayType.new.cast(input) - end - - expected_error_message = <<~MSG.squish - MaintenanceTasks::Parameters::StringArrayType expects alphanumeric, - comma-delimited string. Input received: #{input} - MSG - assert_equal(expected_error_message, error.message) - end - end - end -end diff --git a/test/models/maintenance_tasks/run_test.rb b/test/models/maintenance_tasks/run_test.rb index e2a5e0ba4..deecd83bf 100644 --- a/test/models/maintenance_tasks/run_test.rb +++ b/test/models/maintenance_tasks/run_test.rb @@ -315,16 +315,6 @@ class RunTest < ActiveSupport::TestCase assert_equal [1, 2, 3], run.task.post_ids end - test "#validate_task_arguments raises if casting arguments to custom parameter value type fails" do - run = Run.new( - task_name: "Maintenance::ParamsTask", - arguments: { post_ids: "xyz" } - ) - assert_raises MaintenanceTasks::Parameters::TypeError do - run.validate_task_arguments - end - end - private def count_uncached_queries(&block) diff --git a/test/models/maintenance_tasks/parameters/integer_array_type_test.rb b/test/models/maintenance_tasks/task/integer_array_type_test.rb similarity index 56% rename from test/models/maintenance_tasks/parameters/integer_array_type_test.rb rename to test/models/maintenance_tasks/task/integer_array_type_test.rb index 0502d4675..7fa8e81ae 100644 --- a/test/models/maintenance_tasks/parameters/integer_array_type_test.rb +++ b/test/models/maintenance_tasks/task/integer_array_type_test.rb @@ -2,7 +2,7 @@ require "test_helper" module MaintenanceTasks - module Parameters + class Task class IntegerArrayTypeTest < ActiveSupport::TestCase test "#cast returns nil if input not present" do assert_nil IntegerArrayType.new.cast("") @@ -16,17 +16,8 @@ class IntegerArrayTypeTest < ActiveSupport::TestCase assert_equal [123, 456, 78], IntegerArrayType.new.cast("123 ,456, 78 ") end - test "#cast raises if input is not valid" do - input = "abc" - error = assert_raises(TypeError) do - IntegerArrayType.new.cast(input) - end - - expected_error_message = <<~MSG.squish - MaintenanceTasks::Parameters::IntegerArrayType expects a - comma-delimited string of integers. Input received: #{input} - MSG - assert_equal(expected_error_message, error.message) + test "#cast handles non-integer input" do + assert_equal [0], IntegerArrayType.new.cast("abc") end end end diff --git a/test/models/maintenance_tasks/task/string_array_type_test.rb b/test/models/maintenance_tasks/task/string_array_type_test.rb new file mode 100644 index 000000000..862d18e45 --- /dev/null +++ b/test/models/maintenance_tasks/task/string_array_type_test.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true +require "test_helper" + +module MaintenanceTasks + class Task + class StringArrayTypeTest < ActiveSupport::TestCase + test "#cast returns nil if input not present" do + assert_nil StringArrayType.new.cast("") + end + + test "#cast converts string to array of strings" do + assert_equal ["abc", "def"], StringArrayType.new.cast("abc,def") + end + end + end +end diff --git a/test/system/maintenance_tasks/runs_test.rb b/test/system/maintenance_tasks/runs_test.rb index 14c27a466..4980e3eb9 100644 --- a/test/system/maintenance_tasks/runs_test.rb +++ b/test/system/maintenance_tasks/runs_test.rb @@ -57,20 +57,6 @@ class RunsTest < ApplicationSystemTestCase assert_text alert_text end - test "errors for Task with arguments using custom parameter types shown" do - visit maintenance_tasks_path - - click_on("Maintenance::ParamsTask") - fill_in("_task_arguments_post_ids", with: "xyz") - click_on "Run" - - alert_text = <<~MSG.squish - MaintenanceTasks::Parameters::IntegerArrayType expects - a comma-delimited string of integers. Input received: xyz - MSG - assert_text alert_text - end - test "download the CSV attached to a run for a CSV Task" do visit(maintenance_tasks_path)