[PATCH] blank form fields pass empty params causing AR validations with :allow_nil to fail

Ok, I tested the validations I used earlier with the code that ends this comment. ":allow_nil" does work. The problem is with ActionController and how it handles empty form fields. If a form field is empty, "@params[:field]" will be an empty string and not nil.

The question is: Is this an ActiveRecord problem or an ActionController problem? Depending on the anwser we have to answer one of these: Should empty form fields automatically be set to nil, or should the record treat empty strings as nil?

Here's my validation test code:

class ValidationTest
  attr_accessor :attributes

  def initialize
    @attributes = Hash.new
  end

  def update_attribute(attr, value)
    @attributes[attr] = value
  end

  def new_record?
    true
  end

  def save
  end

  def method_missing(method_id, *args)
    method_id = method_id.to_s

    if args.length == 1
      if method_id[-1].chr == '='
        @attributes[method_id[0...-1]] = args[0]
      else
        super
      end
    else
      @attributes[method_id]
    end
  end

  include ActiveRecord::Validations

  validates_format_of :field, :with => /\d+/, :allow_nil => true, :message => 'Format is invalid'
  validates_length_of :field, :in => 2..5, :allow_nil => true, :too_short => 'is too short', :too_long => 'is too long'
end

This diff statisfies me. It adds a new keyword option, ":allow_empty", which is similiar to allow_nil but also allows empty strings. A proper test should be made.

--- validations.rb      2006-01-04 17:13:52.000000000 -0500
+++ validations_mine.rb 2006-01-04 17:13:39.000000000 -0500
@@ -292,7 +292,8 @@
           unless options[:if] && !evaluate_condition(options[:if], record)
             attrs.each do |attr|
               value = record.send(attr)
-              next if value.nil? && options[:allow_nil]
+              next if value.nil? && (options[:allow_nil] or options[:allow_empty])
+             next if value.empty? && options[:allow_empty]
               yield record, attr, value
             end
           end

Changed the summary

Ok, further experience suggests that that should be "allow_blank" instead of "allow_empty", and that "value.blank?" should be called.

Try this diff instead:

--- validations_old.rb  2006-01-04 17:13:52.000000000 -0500
+++ validations.rb      2006-01-04 17:32:48.000000000 -0500
@@ -279,6 +279,7 @@
       # Options:
       # * <tt>on</tt> - Specifies when this validation is active (default is :save, other options :create, :update)
       # * <tt>allow_nil</tt> - Skip validation if attribute is nil.
+      # * <tt>allow_blank</tt> - Skip validation if attribute.blank? is true
       # * <tt>if</tt> - Specifies a method, proc or string to call to determine if the validation should
       # occur (e.g. :if => :allow_validation, or :if => Proc.new { |user| user.signup_step > 2 }).  The
       # method, proc or string should return or evaluate to a true or false value.
@@ -292,6 +293,7 @@
           unless options[:if] && !evaluate_condition(options[:if], record)
             attrs.each do |attr|
               value = record.send(attr)
+             next if value.blank? && options[:allow_blank]
               next if value.nil? && options[:allow_nil]
               yield record, attr, value
             end

I disagree with this solution. I think the ActionController should convert empty parameters to nil. Empty is different than nil/null, especially when you're dealing with databases. If I have a database column declared as "NOT NULL", it won't complain when an empty string is inserted (I know, I shouldn't rely on the database for validation, but it does help enforce a minimum level of data integrity).

The ultimate question is, what good does an empty parameter do? Is there any reason for leaving it as empty instead of converting it to nil?

add compact method to Hash and call it to remove empty request params

I have attached a patch fo what I think is the proper solution to this issue. I added a compact and compact! method to Hash that removes elements that have an empty or nil value, and then call that method when getting the request params from CGI.

add compact method to Hash and call it to remove empty request params (minor cleanup)

Sorry, but this won't work.

If I have a value that is already set, and I try to delete the value (make it blank or nil), it won't save the change.

You're right. Thanks for catching that. Ignore the patch.

I think the best solution would be to modify whatever method sets the attributes in an AR object and have it set the value to nil if it is empty. I looked through the code and couldn't figure out the way to do that, but I'll keep looking.

Running into this problem again without my patch, I thought I try the ActionController path. A small change to CGIMethods.get_typed_value to check if a string is blank fixes my problem, but causes a test to fail. That test is in cgi_test.rb, test_parse_perms.

One of the input values is "" and the expected output is "" instead of nil. The test also expects a param set to nil to end up being "". I'm not sure if this is by design or not, but it causes hell with validations.

Below is a patch for ActionController. Either this or the one for ActiveRecord will do the trick:

diff -ru /usr/lib/ruby/gems/1.8/gems/actionpack-1.12.0/lib/action_controller/cgi_ext/cgi_methods.rb ./lib/action_controller/cgi_ext/cgi_methods.rb
--- /usr/lib/ruby/gems/1.8/gems/actionpack-1.12.0/lib/action_controller/cgi_ext/cgi_methods.rb  2006-04-03 02:10:18.000000000 -0500
+++ ./lib/action_controller/cgi_ext/cgi_methods.rb      2006-04-23 08:12:25.000000000 -0500
@@ -154,7 +154,9 @@

     def CGIMethods.get_typed_value(value)
       # test most frequent case first
-      if value.is_a?(String)
+      if value.blank?
+        nil
+      elsif value.is_a?(String)
         value
       elsif value.respond_to?(:content_type) && ! value.content_type.blank?
         # Uploaded file
Only in ./test/controller: .cgi_test.rb.swp
diff -ru /usr/lib/ruby/gems/1.8/gems/actionpack-1.12.0/test/controller/cgi_test.rb ./test/controller/cgi_test.rb
--- /usr/lib/ruby/gems/1.8/gems/actionpack-1.12.0/test/controller/cgi_test.rb   2006-04-03 02:10:18.000000000 -0500
+++ ./test/controller/cgi_test.rb       2006-04-23 08:11:18.000000000 -0500
@@ -104,8 +104,8 @@
         }
       },
       "something_else" => "blah",
-      "something_empty" => "",
-      "something_nil" => "",
+      "something_empty" => nil,
+      "something_nil" => nil,
       "products" => {
         "first" => "Apple Computer",
         "second" => "Pc"

I started using a before_filter that sets empty params to nil. This is probably similiar to what the Hash#compact attachment does. I've also ran into cases where I could have used empty strings, so it may not be best to do this in the controller by default. I'm not sure if it would be wise to have an allow_blank keyword for validations or not.

Added a patch for this in related ticket #7383.

Closing this, since we now have [7407] (:allow_blank) on the books to address this problem.

I guess that this is not going to be included in 1.x, is it?

I don't know, since it was definitely an enhancement, and the 1-2-stable branch is really for bug fixes now that we've moved on to post-2.0 development.

However, it should be almost trivial to write a plugin to backport this, if you really need it--[7407] was just a two line change plus tests and docs.

demisone,

This is the equivalent:

validates_length_of :something, :is => 5,

:if => Proc.new { |attrib| !attrib.something.blank? }

However these days I'm finding it to make more sense to strip and nil the desired attributes instead.

I've found this to be a good plugin: http://stripattributes.rubyforge.org/svn/tags/strip_attributes/

I've realized that the reason I initially wanted this behavior was because I also had a validates_presence_of on the same field and I didn't want both errors. But I think now its a poor line of thinking since that line of thinking centers around display logic (what errors will be displayed to a user). Instead, I changed my line of thinking: both errors should be thrown in the model because they are both true errors, and the display logic should handle what the user see's. It's much cleaner to show the error on a field than to show the full error_messages_for jumble.

This is some sort of solution (the plugin). I was also thinking patching the 1.2.6 in my machine with the proper changes to include the :allow_blank...