commit 9f88e8a17ed07d323c82234e0b2524e606b5d515
Author: Tom Caspy <tcaspy@gmail.com>
Date:   Thu Apr 16 16:39:01 2015 +0300

    fixes #10232 - moving validations and casting out of lookup key and value

diff --git a/app/models/lookup_key.rb b/app/models/lookup_key.rb
index 4e3603775..eb4a731fe 100644
--- a/app/models/lookup_key.rb
+++ b/app/models/lookup_key.rb
@@ -31,14 +31,14 @@ class LookupKey < ActiveRecord::Base
                                 :reject_if => :reject_invalid_lookup_values,
                                 :allow_destroy => true
 
-  before_validation :validate_and_cast_default_value, :unless => Proc.new{|p| p.use_puppet_default }
+  before_validation :cast_default_value, :unless => Proc.new{|p| p.use_puppet_default }
   validates :key, :uniqueness => {:scope => :is_param }, :unless => Proc.new{|p| p.is_param?}
 
   validates :key, :presence => true
   validates :puppetclass, :presence => true, :unless => Proc.new {|k| k.is_param?}
   validates :validator_type, :inclusion => { :in => VALIDATOR_TYPES, :message => N_("invalid")}, :allow_blank => true, :allow_nil => true
   validates :key_type, :inclusion => {:in => KEY_TYPES, :message => N_("invalid")}, :allow_blank => true, :allow_nil => true
-  validate :validate_list, :validate_regexp
+  validate :validate_default_value
   validates_associated :lookup_values
   validate :ensure_type, :disable_merge_overrides, :disable_avoid_duplicates
 
@@ -154,13 +154,6 @@ class LookupKey < ActiveRecord::Base
     val
   end
 
-  # Returns the casted value, or raises a TypeError
-  def cast_validate_value(value)
-    method = "cast_value_#{key_type}".to_sym
-    return value unless self.respond_to? method, true
-    self.send(method, value) rescue raise TypeError
-  end
-
   def path_elements
     path.split.map do |paths|
       paths.split(KEY_DELM).map do |element|
@@ -218,10 +211,10 @@ class LookupKey < ActiveRecord::Base
     end.join("\n")
   end
 
-  def validate_and_cast_default_value
+  def cast_default_value
     return true if default_value.nil? || contains_erb?(default_value)
     begin
-      self.default_value = cast_validate_value self.default_value
+      Foreman::Parameters::Caster.new(self, :attribute_name => :default_value, :to => key_type).cast!
       true
     rescue
       errors.add(:default_value, _("is invalid"))
@@ -229,39 +222,6 @@ class LookupKey < ActiveRecord::Base
     end
   end
 
-  def cast_value_boolean(value)
-    casted = Foreman::Cast.to_bool(value)
-    raise TypeError if casted.nil?
-    casted
-  end
-
-  def cast_value_integer(value)
-    return value.to_i if value.is_a?(Numeric)
-
-    if value.is_a?(String)
-      if value =~ /^0x[0-9a-f]+$/i
-        value.to_i(16)
-      elsif value =~ /^0[0-7]+$/
-        value.to_i(8)
-      elsif value =~ /^-?\d+$/
-        value.to_i
-      else
-        raise TypeError
-      end
-    end
-  end
-
-  def cast_value_real(value)
-    return value if value.is_a? Numeric
-    if value.is_a?(String)
-      if value =~ /\A[-+]?(?:\d+(?:\.\d*)?|\.\d+)(?:[eE][-+]?\d+)?\Z/
-        value.to_f
-      else
-        cast_value_integer value
-      end
-    end
-  end
-
   def load_yaml_or_json(value)
     return value unless value.is_a? String
     begin
@@ -271,47 +231,17 @@ class LookupKey < ActiveRecord::Base
     end
   end
 
-  def cast_value_array(value)
-    return value if value.is_a? Array
-    return value.to_a if not value.is_a? String and value.is_a? Enumerable
-    value = load_yaml_or_json value
-    raise TypeError unless value.is_a? Array
-    value
-  end
-
-  def cast_value_hash(value)
-    return value if value.is_a? Hash
-    value = load_yaml_or_json value
-    raise TypeError unless value.is_a? Hash
-    value
-  end
-
-  def cast_value_yaml(value)
-    YAML.load value
-  end
-
-  def cast_value_json(value)
-    JSON.load value
-  end
-
   def ensure_type
     if puppetclass_id.present? and is_param?
       self.errors.add(:base, _('Global variable or class Parameter, not both'))
     end
   end
 
-  def validate_regexp
-    return true if (validator_type != 'regexp' || (contains_erb?(default_value) && Setting[:interpolate_erb_in_parameters]))
-    valid = (default_value =~ /#{validator_rule}/)
-    errors.add(:default_value, _("is invalid")) unless valid
-    valid
-  end
-
-  def validate_list
-    return true if (validator_type != 'list' || (contains_erb?(default_value) && Setting[:interpolate_erb_in_parameters]))
-    valid = validator_rule.split(KEY_DELM).map(&:strip).include?(default_value)
-    errors.add(:default_value, _("%{default_value} is not one of %{validator_rule}") % { :default_value => default_value, :validator_rule => validator_rule }) unless valid
-    valid
+  def validate_default_value
+    Foreman::Parameters::Validator.new(self,
+      :type => validator_type,
+      :validate_with => validator_rule,
+      :getter => :default_value).validate!
   end
 
   def disable_merge_overrides
diff --git a/app/models/lookup_value.rb b/app/models/lookup_value.rb
index c0ba3c94f..710f3587d 100644
--- a/app/models/lookup_value.rb
+++ b/app/models/lookup_value.rb
@@ -10,8 +10,9 @@ class LookupValue < ActiveRecord::Base
   validate :value_present?
   delegate :key, :to => :lookup_key
   before_validation :sanitize_match
+
   before_validation :validate_and_cast_value, :unless => Proc.new{|p| p.use_puppet_default }
-  validate :validate_list, :validate_regexp, :ensure_fqdn_exists, :ensure_hostgroup_exists
+  validate :validate_value, :ensure_fqdn_exists, :ensure_hostgroup_exists
 
   attr_accessor :host_or_hostgroup
   attr_writer :managed_id, :hostgroup_id
@@ -46,6 +47,13 @@ class LookupValue < ActiveRecord::Base
     lookup_key.value_before_type_cast self.value
   end
 
+  def validate_value
+    Foreman::Parameters::Validator.new(self,
+      :type => lookup_key.validator_type,
+      :validate_with => lookup_key.validator_rule,
+      :getter => :value).validate!
+  end
+
   private
 
   #TODO check multi match with matchers that have space (hostgroup = web servers,environment = production)
@@ -57,7 +65,7 @@ class LookupValue < ActiveRecord::Base
     return true if self.marked_for_destruction? or !self.value.is_a? String
     begin
       unless self.lookup_key.contains_erb?(value)
-        self.value = lookup_key.cast_validate_value self.value
+        Foreman::Parameters::Caster.new(self, :attribute_name => :value, :to => lookup_key.key_type).cast!
       end
       true
     rescue StandardError, SyntaxError => e
@@ -67,20 +75,6 @@ class LookupValue < ActiveRecord::Base
     end
   end
 
-  def validate_regexp
-    return true if (lookup_key.validator_type != 'regexp' || (lookup_key.contains_erb?(value) && Setting[:interpolate_erb_in_parameters]))
-    valid = (value =~ /#{lookup_key.validator_rule}/)
-    errors.add(:value, _("is invalid")) unless valid
-    valid
-  end
-
-  def validate_list
-    return true if (lookup_key.validator_type != 'list' || (lookup_key.contains_erb?(value) && Setting[:interpolate_erb_in_parameters]))
-    valid = lookup_key.validator_rule.split(LookupKey::KEY_DELM).map(&:strip).include?(value)
-    errors.add(:value, _("%{value} is not one of %{rules}") % { :value => value, :rules => lookup_key.validator_rule }) unless valid
-    valid
-  end
-
   def ensure_fqdn_exists
     md = ensure_matcher(/fqdn=(.*)/)
     return md if md == true || md == false
diff --git a/app/services/classification/base.rb b/app/services/classification/base.rb
index f3ccd53dd..f3be21356 100644
--- a/app/services/classification/base.rb
+++ b/app/services/classification/base.rb
@@ -144,12 +144,12 @@ module Classification
 
     def validate_lookup_value(key, value)
       lookup_value = key.lookup_values.build(:value => value)
-      return true if lookup_value.send(:validate_list) && lookup_value.send(:validate_regexp)
+      return true if lookup_value.validate_value
       raise "Invalid value '#{value}' of parameter #{key.id} '#{key.key}'"
     end
 
     def type_cast(key, value)
-      key.cast_validate_value(value)
+      Foreman::Parameters::Caster.new(key, :attribute_name => :value, :to => key.key_type, :value => value).cast
     rescue TypeError
       Rails.logger.warn "Unable to type cast #{value} to #{key.key_type}"
     end
diff --git a/app/services/foreman/parameters/caster.rb b/app/services/foreman/parameters/caster.rb
new file mode 100644
index 000000000..250c8b982
--- /dev/null
+++ b/app/services/foreman/parameters/caster.rb
@@ -0,0 +1,122 @@
+module Foreman
+  module Parameters
+    class Caster
+      attr_reader :value
+
+      def initialize(item, options = {})
+        defaults = {
+          :attribute_name => :value,
+          :to => :string
+        }
+        options.reverse_merge!(defaults)
+        @item, @options = item, options
+        @value = @options[:value] || @item.send(@options[:attribute_name])
+      end
+
+      def cast!
+        @item.send("#{@options[:attribute_name]}=", casted_value)
+      end
+
+      def cast
+        casted_value
+      end
+
+      private
+
+      def casted_value
+        case @options[:to].to_s
+        when "string"
+          cast_string
+        when "integer"
+          cast_integer
+        when "real"
+          cast_real
+        when "boolean"
+          cast_boolean
+        when "array"
+          cast_array
+        when "hash"
+          cast_hash
+        when "json"
+          cast_json
+        when "yaml"
+          cast_yaml
+        when nil, ""
+          value
+        else
+          Rails.logger.warn("Unable to type cast #{value} to #{@options[:to]}")
+          raise TypeError
+        end
+      end
+
+      def cast_string
+        value.to_s
+      end
+
+      def cast_boolean
+        val = Foreman::Cast.to_bool(value)
+        return val if [true, false].include?(val)
+        raise TypeError
+      end
+
+      def cast_integer
+        return value.to_i if value.is_a?(Numeric)
+
+        if value.is_a?(String)
+          if value =~ /^0x[0-9a-f]+$/i
+            value.to_i(16)
+          elsif value =~ /^0[0-7]+$/
+            value.to_i(8)
+          elsif value =~ /^-?\d+$/
+            value.to_i
+          else
+            raise TypeError
+          end
+        end
+      end
+
+      def cast_real
+        return value if value.is_a? Numeric
+        if value.is_a?(String)
+          if value =~ /\A[-+]?(?:\d+(?:\.\d*)?|\.\d+)(?:[eE][-+]?\d+)?\Z/
+            value.to_f
+          else
+            cast_value_integer value
+          end
+        end
+      end
+
+      def cast_array
+        return value if value.is_a? Array
+        return value.to_a if not value.is_a? String and value.is_a? Enumerable
+        val = load_yaml_or_json
+        raise TypeError unless val.is_a? Array
+        val
+      end
+
+      def cast_hash
+        return value if value.is_a? Hash
+        val = load_yaml_or_json
+        raise TypeError unless val.is_a? Hash
+        val
+      end
+
+      def cast_json
+        JSON.load value
+      end
+
+      def cast_yaml
+        YAML.load value
+      end
+
+      def load_yaml_or_json
+        return value unless value.is_a? String
+        begin
+          JSON.load value
+        rescue
+          YAML.load value
+        end
+      end
+    end
+  end
+end
diff --git a/app/services/foreman/parameters/validator.rb b/app/services/foreman/parameters/validator.rb
new file mode 100644
index 000000000..00ac9f23e
--- /dev/null
+++ b/app/services/foreman/parameters/validator.rb
@@ -0,0 +1,56 @@
+module Foreman
+  module Parameters
+    class Validator
+      KEY_DELM = ","
+
+      def initialize(item, options = {})
+        @item, @options = item, options
+      end
+
+      def validate!
+        case @options[:type].to_s
+        when "regexp"
+          validate_regexp
+        when "list", "array"
+          validate_list
+        else
+          return true
+        end
+      end
+
+      private
+
+      def value
+        @item.send(@options[:getter])
+      end
+
+      def validate_regexp
+        return true if contains_erb?(value) && Setting[:interpolate_erb_in_parameters]
+
+        unless value =~ /#{@options[:validate_with]}/
+          add_error(_("is invalid"))
+          return false
+        end
+        true
+      end
+
+      def validate_list
+        return true if contains_erb?(value) && Setting[:interpolate_erb_in_parameters]
+
+        unless @options[:validate_with].split(KEY_DELM).map(&:strip).include?(value)
+          add_error(_("%{value} is not one of %{rules}") % { :value => value, :rules => @options[:validate_with] })
+          return false
+        end
+        true
+      end
+
+      def add_error(message)
+        @item.errors.add(@options[:getter], message)
+      end
+
+      def contains_erb?(value)
+        value =~ /<%.*%>/
+      end
+    end
+  end
+end
diff --git a/test/lib/parameters/caster_test.rb b/test/lib/parameters/caster_test.rb
new file mode 100644
index 000000000..6cbcd7caf
--- /dev/null
+++ b/test/lib/parameters/caster_test.rb
@@ -0,0 +1,75 @@
+require 'test_helper'
+
+class CasterTest < ActiveSupport::TestCase
+  context "Casting to different stuff (successfully)" do
+    test "string" do
+      item = OpenStruct.new(:foo => :bar)
+      Foreman::Parameters::Caster.new(item, :attribute_name => :foo).cast!
+      assert_equal item.foo, "bar"
+    end
+
+    test "integer" do
+      #this also tests that "132" isn't octal
+      item = OpenStruct.new(:foo => "132")
+      Foreman::Parameters::Caster.new(item, :attribute_name => :foo, :to => :integer).cast!
+      assert_equal item.foo, 132
+    end
+
+    test "hex int" do
+      item = OpenStruct.new(:foo => "0xabba")
+      Foreman::Parameters::Caster.new(item, :attribute_name => :foo, :to => :integer).cast!
+      assert_equal item.foo, 43962
+    end
+
+    test "octal int" do
+      item = OpenStruct.new(:foo => "012")
+      Foreman::Parameters::Caster.new(item, :attribute_name => :foo, :to => :integer).cast!
+      assert_equal item.foo, 10
+    end
+
+    test "the truth" do
+      item = OpenStruct.new(:foo => "true")
+      Foreman::Parameters::Caster.new(item, :attribute_name => :foo, :to => :boolean).cast!
+      assert_equal item.foo, true
+    end
+
+    test "the lies" do
+      item = OpenStruct.new(:foo => "false")
+      Foreman::Parameters::Caster.new(item, :attribute_name => :foo, :to => :boolean).cast!
+      assert_equal item.foo, false
+    end
+
+    test "array (json)" do
+      item = OpenStruct.new(:foo => [1,2,3].to_json)
+      Foreman::Parameters::Caster.new(item, :attribute_name => :foo, :to => :array).cast!
+      assert_equal item.foo, [1,2,3]
+    end
+
+    test "array (yml)" do
+      item = OpenStruct.new(:foo => [1,2,3].to_yaml)
+      Foreman::Parameters::Caster.new(item, :attribute_name => :foo, :to => :array).cast!
+      assert_equal item.foo, [1,2,3]
+    end
+
+    test "hash (json)" do
+      item = OpenStruct.new(:foo => {:a => :b}.to_json)
+      Foreman::Parameters::Caster.new(item, :attribute_name => :foo, :to => :hash).cast!
+      assert_equal item.foo, {"a" => "b"}
+    end
+
+    test "hash (yml)" do
+      item = OpenStruct.new(:foo => {:a => :b}.to_yaml)
+      Foreman::Parameters::Caster.new(item, :attribute_name => :foo, :to => :hash).cast!
+      assert_equal item.foo, {:a => :b}
+    end
+  end
+
+  context "failures" do
+    test "caster raises TypeError" do
+      item = OpenStruct.new(:foo => "blah")
+      assert_raises(TypeError) do
+        Foreman::Parameters::Caster.new(item, :attribute_name => :foo, :to => :zibi).cast!
+      end
+    end
+  end
+end
diff --git a/test/lib/parameters/validator_test.rb b/test/lib/parameters/validator_test.rb
new file mode 100644
index 000000000..5ccfed3fd
--- /dev/null
+++ b/test/lib/parameters/validator_test.rb
@@ -0,0 +1,43 @@
+require 'test_helper'
+
+class ValidatorTest < ActiveSupport::TestCase
+  class ValidatedItem
+    include ActiveModel::Validations
+    attr_accessor :value
+  end
+  before do
+    @item = ValidatedItem.new
+  end
+
+  it "adds errors on wrong regexp" do
+    @item.value = "123"
+    validator = Foreman::Parameters::Validator.new(@item, :type => :regexp, :validate_with => "[a-z]", :getter => :value)
+    refute validator.validate!
+    assert @item.errors.present?
+    assert_equal @item.errors[:value], ["is invalid"]
+  end
+
+  it "validates regexp" do
+    @item.value = "abdfgfdger"
+    validator = Foreman::Parameters::Validator.new(@item, :type => :regexp, :validate_with => "[a-z]", :getter => :value)
+    assert validator.validate!
+    assert @item.errors.blank?
+  end
+
+  it "adds errors on wrong item" do
+    validator_rule = "a,b,c"
+    @item.value = "d"
+    validator = Foreman::Parameters::Validator.new(@item, :type => :list, :validate_with => validator_rule, :getter => :value)
+    refute validator.validate!
+    assert @item.errors.present?
+    assert_equal @item.errors[:value], ["d is not one of a,b,c"]
+  end
+
+  it "validates inclusion in list" do
+    validator_rule = "a,b,c"
+    @item.value = "a"
+    validator = Foreman::Parameters::Validator.new(@item, :type => :list, :validate_with => validator_rule, :getter => :value)
+    assert validator.validate!
+    assert @item.errors.blank?
+  end
+end
diff --git a/test/unit/classification_test.rb b/test/unit/classification_test.rb
index fa25f2aab..451ca924b 100644
--- a/test/unit/classification_test.rb
+++ b/test/unit/classification_test.rb
@@ -745,8 +745,8 @@ class ClassificationTest < ActiveSupport::TestCase
   context 'lookup value type cast error' do
     setup do
       @lookup_key = mock('lookup_key')
-      @lookup_key.expects(:cast_validate_value).raises(TypeError)
-      @lookup_key.expects(:key_type).returns('footype')
+      Foreman::Parameters::Caster.any_instance.expects(:cast).raises(TypeError)
+      @lookup_key.expects(:key_type).twice.returns('footype')
     end
 
     test 'TypeError exceptions are logged' do
