Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

ActiveResource breaks when encounters <object> tag in XML

#1828

I'm not sure if this is a bug, or is it me misusing #to_xml, but I get "Wrong number of arguments" when trying to load a model which has <object> tag in its xml representation.

The reason seems to lie in find_or_create_resource_for(name) in ActiveResource::Base. I traced the issue down to the following point.

Let's say our model is Post. Here's what's going to happen:


      def find_or_create_resource_for(name)
        # name is "object"

        resource_name = name.to_s.camelize
        # resouce_name is "Object"
        
        ancestors = self.class.name.split("::")
        # ancestors is ["Post"]

        if ancestors.size > 1
          find_resource_in_modules(resource_name, ancestors)
        else
          
          self.class.const_get(resource_name)
          # ===> This line will return Object, not Post::Object,
          #      which breaks because ActiveResource calls Object.new(value)
          #      but Object's initializer doesn't take args.
        
        end
      rescue NameError
        if self.class.const_defined?(resource_name)
          resource = self.class.const_get(resource_name)
        else
          resource = self.class.const_set(resource_name, Class.new(ActiveResource::Base))
        end
        resource.prefix = self.class.prefix
        resource.site   = self.class.site
        resource
      end

As mentioned above - the const_get returns Object, and then ActiveResource tries to call Object.new(value), whereas Object constructor doesn't take params.

This causes "Wrong number of arguments (1 for 0)" error when attempting to do Post.find(:last) on the ActiveResource client.

One way to fix it is to raise NameError explicitly.


      def find_or_create_resource_for(name)
        resource_name = name.to_s.camelize
        
        # ==> Adding this line fixes the error. 
        raise NameError if resource_name == "Object"
        
        ancestors = self.class.name.split("::")
        if ancestors.size > 1
          find_resource_in_modules(resource_name, ancestors)
        else
          self.class.const_get(resource_name)
        end
      rescue NameError
        if self.class.const_defined?(resource_name)
          resource = self.class.const_get(resource_name)
        else
          resource = self.class.const_set(resource_name, Class.new(ActiveResource::Base))
        end
        resource.prefix = self.class.prefix
        resource.site   = self.class.site
        resource
      end

Raising NameError causes execution to jump to the rescue clause where "object" is handled properly. Should this be fixed?

Reported by Maxim Chernyak · January 30th, 2009 @ 05:20 PM

State: stale
Milestone: 3.x
Assigned to: nobody
Importance: none

Activity

  1. CancelProfileIsBroken
    CancelProfileIsBroken
    • Tag changed from 2.3, activeresource, bug to 2.3, activeresource, bug, bugmash

    August 5th, 2009 @ 01:28 PM

  2. Dan Pickett
    Dan Pickett

    verified

    -1 on this - I think accommodating the object element will lead to lots of special handling

    From an XML perspective as well, it's not very descriptive of the "object" it's referring to - it should be renamed

    August 8th, 2009 @ 10:38 PM

  3. Maxim Chernyak
    Maxim Chernyak

    Rails produces an obscure error for certain completely valid xml. Isn't it worth it to at least throw a better message saying "sorry but tag is not allowed in rails"? What about legacy XML or generic interfaces where IS intended for generic use?

    Besides it's creepy that I can get someone's rails app to execute Object.new by simply sending some xml. You don't always know what you're getting.

    August 8th, 2009 @ 10:59 PM

  4. Peer Allan
    Peer Allan

    I have to agree with both Dan and Maxim in this case (can I do that?). I don't like the idea of adding special, possibly hardcoded, handling into this. However is it creepy think that someone posting some XML with can get the app to execute the Object.new.

    That said, a reserved work list for the XML generator may be the way to get around this. If, at minimum, the reserved words throw a useful error back to the user.

    August 11th, 2009 @ 01:53 PM

  5. hsume2 (Henry)
    hsume2 (Henry)

    verified on master and 2-3-stable

    I've attached a patch; failing test for master (cherry picks into 2-3-stable) as well a patch for 2-3-stable.

    September 27th, 2009 @ 03:54 PM

  6. hsume2 (Henry)
  7. Elomar França
    Elomar França

    Verified the bug, but I do not agree with the hardcoded solution.

    I think it would be better to have some kind of reserved words list, as Peer Allan proposed. Is it possible and desirable (but I think it isn't) to "unload" some constants, as Object, before loading the XML?

    September 27th, 2009 @ 04:24 PM

  8. Elad Meidar
    Elad Meidar

    +1 Verified, but i am too not very fond of the hard-code solution. I agree with Dan and Elomar, that Object should be left alone and a more descriptive name should be used ( + a reserved words list )

    September 27th, 2009 @ 04:33 PM

  9. hsume2 (Henry)
    hsume2 (Henry)

    +1 for reserved words list, especially for the case of nested, existing resources, since we wouldn't want anyone doing:

    class Object < ActiveResource::Base
    end
    

    September 27th, 2009 @ 04:49 PM

  10. Maxim Chernyak
    Maxim Chernyak

    IMO, when dealing with APIs it's important to keep in mind flexible input -> exact output principle. Object tag may appear on a more specific level, you know. It's a widely used english word after all. For example:

    <game>
      <title>Paintball</title>
      <object>Kill everyone</object>
      <rules>...</rules>
    </game>
    

    I'm sure Game.new.object is not the only example.

    September 27th, 2009 @ 05:11 PM

  11. Elad Meidar
    Elad Meidar

    @Maxim: correct, but stilll Game.new.objective is a much more clear / safe phrase to use.

    September 27th, 2009 @ 09:29 PM

  12. Rizwan Reza
    Rizwan Reza
    • Tag changed from 2.3, activeresource, bug, bugmash to 2.3, activeresource, bug

    February 12th, 2010 @ 12:46 PM

  13. Jeremy Kemper
    Jeremy Kemper
    • Milestone changed from 2.x to 3.x

    May 4th, 2010 @ 06:48 PM

  14. Santiago Pastorino
    Santiago Pastorino
    • State changed from new to open
    • Tag changed from 2.3, activeresource, bug to 23, activeresource, bug
    • Importance changed from to

    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.

    February 2nd, 2011 @ 04:42 PM

  15. Santiago Pastorino
    Santiago Pastorino
    • State changed from open to stale

    February 2nd, 2011 @ 04:42 PM