This project is archived and is in readonly mode.

#2229 ✓stale
Yuri

ActiveResource validation error not recognized as belonging to a certain field

Reported by Yuri | March 13th, 2009 @ 03:25 PM | in 3.x

If the server side ActiveRecord class contains a validates_presence_of constraint, and this is exposed as a resource, saving a new ActiveResource instance without providing this obligatory attribute will result in the validation error assigned to base instead of this attribute.

Explaining in more details:

Underlying ActiveRecord


class Shop < ActiveRecord::Base
  validates_presence_of :name
end

Creating an ActiveResource:


class Shop < < ActiveResource::Base
  self.site = ...
end

shop = Shop.new(:some_other_attribute => 'foo')
shop.save  #=> false, server side validation failed
obj.errors.on(:name)  #=> nil   !!!

The incoming XML containing the output of @my_shop_active_record.errors.to_xml is parsed incorrectly and such validation error is placed to base instead of being recognized as specific to a certain attribute.

The reason lies in the following code from validation.rb (activeresource/lib/active_resource/validations.rb):


# Grabs errors from the XML response.
    def from_xml(xml)
      clear
      humanized_attributes = @base.attributes.keys.inject({}) { |h, attr_name| h.update(attr_name.humanize => attr_name) }
      messages = Array.wrap(Hash.from_xml(xml)['errors']['error']) rescue []
      messages.each do |message|
        attr_message = humanized_attributes.keys.detect do |attr_name|
          if message[0, attr_name.size + 1] == "#{attr_name} "
            add humanized_attributes[attr_name], message[(attr_name.size + 1)..-1]
          end
        end
        
        add_to_base message if attr_message.nil?
      end
    end

The problem with this code is that while parsing the xml it relies on the initial hash submitted to the constructor of the ActiveResource instance, iterating through humanized_attributes. All other errors go to :base.

I expect obj.errors.on(:name) to return the validation error and I think this would be the correct behavior.

I monkey-patched ActiveResource and ActiveRecord to fix this.

First, dealing with a structured format like XML we can perfectly embed any info into XML attributes and the just parse it, instead of (1) Humanizing the name of the attribute (2) Appending it to the error message (what for?) (3) Applying some perlish string manipulations on the client ActiveResource side trying to guess which attribute this error refers to

Here's first my fix for AR to add field name to error.to_xml


module ActiveRecord
  class Errors
    
    def each_attribute_and_message(options = {})
      @errors.each_key do |attr|
        @errors[attr].each do |message|
          next unless message

          if attr == "base"
            yield [:base, message]
          else
            yield [attr, message]
          end
        end
      end
    end 


    def to_xml(options={})
      options[:root] ||= "errors"
      options[:indent] ||= 2
      options[:builder] ||= Builder::XmlMarkup.new(:indent => options[:indent])

      options[:builder].instruct! unless options.delete(:skip_instruct)
      options[:builder].errors do |e|
        each_attribute_and_message do |attr, msg|
          e.error(msg, :attr => attr) 
        end
      end
    end
  end
end

The generated xml looks like:


<?xml version="1.0" encoding="UTF-8"?>
<errors>
  <error attr="name">shop.error.presence_of_name</error>
</errors>

And now let's rewrite from_xml to parse it:


module ActiveResource #:nodoc:
  class Errors #:nodoc:
    def from_xml(xml)
      clear
      REXML::XPath.each( REXML::Document.new(xml), "//error") do |element|
        attrs =  element.attributes
        if attrs['attr']
          add attrs['attr'], element.text
        else
          add_to_base element.text
        end
      end
    end
  end
end

Now obj.errors.on(:name) will return the appropriate error message.

I don't know if there are any standards as to how XML is parsed in Rails, you might feel like changing it, fine.

Also, one might notice that my to_xml does not append the humanized name of the field to the message, it's just that I consider this as confusing redundancy, I need the message to be exactly as I specified it in ActiveRecord, but that's not the main point of this bugreport.

Comments and changes to this ticket

  • Jeremy Kemper

    Jeremy Kemper May 4th, 2010 @ 06:48 PM

    • Milestone changed from 2.x to 3.x
  • Taryn East

    Taryn East July 27th, 2010 @ 05:49 PM

    Neat solution.

    In from_xml - I'd do a check to make sure we have the attribute before trying to add an error onto it (or preferably, auto-add the attribute if it doesn't exist yet).

    May run into backwards-compatibility issues though - what if we're talking to an old Rails app (that still spits out errors the old way) ? Do we need to try your way first, then also check for an attribute-name, just to maintain existing functionality?

  • Santiago Pastorino

    Santiago Pastorino February 9th, 2011 @ 12:31 AM

    • State changed from “new” to “open”

    This issue has been automatically marked as stale because it has not been commented on for at least three months.

    The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

    Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.

  • Santiago Pastorino

    Santiago Pastorino February 9th, 2011 @ 12:31 AM

    • State changed from “open” to “stale”
  • bingbing
  • csnk

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Referenced by

Pages