This project is archived and is in readonly mode.
ActiveResource breaks when encounters <object> tag in XML
Reported by Maxim Chernyak | January 30th, 2009 @ 05:20 PM | in 3.x
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?
Comments and changes to this ticket
-
CancelProfileIsBroken August 5th, 2009 @ 01:28 PM
- Tag changed from “2.3, activeresource, bug” to “2.3, activeresource, bug, bugmash”
-
Dan Pickett August 8th, 2009 @ 10:38 PM
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
-
Maxim Chernyak August 8th, 2009 @ 10:59 PM
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.
-
Peer Allan August 11th, 2009 @ 01:53 PM
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.
-
hsume2 (Henry) September 27th, 2009 @ 03:54 PM
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.
-
-
Elomar França September 27th, 2009 @ 04:24 PM
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?
-
Elad Meidar September 27th, 2009 @ 04:33 PM
+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 )
-
hsume2 (Henry) September 27th, 2009 @ 04:49 PM
+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 -
Maxim Chernyak September 27th, 2009 @ 05:11 PM
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.
-
Elad Meidar September 27th, 2009 @ 09:29 PM
@Maxim: correct, but stilll Game.new.objective is a much more clear / safe phrase to use.
-
Rizwan Reza February 12th, 2010 @ 12:46 PM
- Tag changed from “2.3, activeresource, bug, bugmash” to “2.3, activeresource, bug”
-
-
Santiago Pastorino February 2nd, 2011 @ 04:42 PM
- 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.
-
Santiago Pastorino February 2nd, 2011 @ 04:42 PM
- State changed from “open” to “stale”
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>
CancelProfileIsBroken
Josh Nichols
Maxim Chernyak