Skip to content

Commit

Permalink
ActiveModel types should only cast and not validate
Browse files Browse the repository at this point in the history
  • Loading branch information
adrianna-chang-shopify committed Jun 3, 2021
1 parent de5a832 commit 1a2303e
Show file tree
Hide file tree
Showing 11 changed files with 36 additions and 119 deletions.
16 changes: 9 additions & 7 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions app/controllers/maintenance_tasks/tasks_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 0 additions & 8 deletions app/models/maintenance_tasks/parameters/type_error.rb

This file was deleted.

Original file line number Diff line number Diff line change
@@ -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<Integer>] 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
Original file line number Diff line number Diff line change
@@ -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<Integer>] 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
2 changes: 1 addition & 1 deletion test/dummy/app/tasks/maintenance/params_task.rb
Original file line number Diff line number Diff line change
@@ -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
Expand Down
34 changes: 0 additions & 34 deletions test/models/maintenance_tasks/parameters/string_array_type_test.rb

This file was deleted.

10 changes: 0 additions & 10 deletions test/models/maintenance_tasks/run_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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("")
Expand All @@ -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
Expand Down
16 changes: 16 additions & 0 deletions test/models/maintenance_tasks/task/string_array_type_test.rb
Original file line number Diff line number Diff line change
@@ -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
14 changes: 0 additions & 14 deletions test/system/maintenance_tasks/runs_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down

0 comments on commit 1a2303e

Please sign in to comment.