#4529 ActiveResource does not serialize XML correctly - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#4529 ✓committed

ActiveResource does not serialize XML correctly

Reported by VirtualFunction | May 4th, 2010 @ 07:53 AM | in 3.0.2

Assuming I have

class RemoteArticle < ActiveResource::Base
  self.element_name = 'article'
end

RemoteArticle.new.to_xml

now results in:

<?xml version="1.0" encoding="UTF-8"?>
<remote-article>
</remote-article>

the node tag name should really be article, not remote-article.

I'm hacking my way round this by making the encode method call "to_xml :root => self.class.element_name" as I see methods like update/create call encode rather than to_xml/to_json however this is not a real fix.

I'm using Rails 3.0 trunk/master from about 2 hours ago. I don't seem to recall this being an issue in beta 1 or 2.

Comments and changes to this ticket

Neeraj Singh May 4th, 2010 @ 09:33 AM
Even Rails beta 3 is working fine. Problem is in the edge version.

I introduced a major change the way to_xml works a week back. Looks like that commit is causing the error. Looking into this issue.
Neeraj Singh May 4th, 2010 @ 11:02 AM
On further analysis the commit that is causing this problem is this one.

http://github.com/rails/rails/commit/7cd1d37a51f5f53f8fc1360f886d26...
Neeraj Singh May 4th, 2010 @ 04:05 PM
- Assigned user set to “Jeremy Kemper”
Commit http://github.com/rails/rails/commit/7cd1d37a51f5f53f8fc1360f886d26... deleted following line
```
self.class.format.encode(attributes, 
            {:root => self.class.element_name}.merge(options))
```
With that line gone there is no easy way to pass self.class.element_name as root.

I would like to submit a patch for it but I do not see any easy way. Please provide how should I proceed.

The way I see it there is need for to_xml method in the class.

I will work on a test patch that will add a failing test.

I know usually one should not directly assign a ticket to someone . However I am assigning this ticket to Jeremy because he signed off on the last commit.
Jeremy Kemper May 4th, 2010 @ 10:11 PM
- Milestone cleared.
- State changed from “new” to “open”
- Assigned user changed from “Jeremy Kemper” to “Santiago Pastorino”
Doh! Needs a regression test + fix.
Santiago Pastorino May 6th, 2010 @ 04:53 PM
- Tag changed from “rails 3.0 beta, activeresource, serialization, to_xml” to “rails 3.0 beta, activeresource, patch, serialization, to_xml”
- State changed from “open” to “verified”
On the patches i've made tests on ActiveResource use the methods to_xml and to_json.
I refactored AMo::Name and allow element and collection attributes to be writable.
Then i make ActiveRecord delegate to AMo the use of element_name and collection_name instead of managing his own copy itself.
Added a few tests also.
- to_xml_and_to_json_serialization.patch 8.8 KB
José Valim May 6th, 2010 @ 09:43 PM
Pastorino, your patch is changing the cached values. This means that every time I call human, it's going to execute this line:

ActiveSupport::Inflector.humanize(@element).freeze

And you should change the @human value if the @element changes. Consider this case:
```
class Post < ActiveResource::Base
  self.element_name = "api-post"
end

Post.model_name.human #=> "Post"
```
You should also cache the collection using "@collection ||=".
Santiago Pastorino May 6th, 2010 @ 10:16 PM
- Tag changed from “rails 3.0 beta, activeresource, patch, serialization, to_xml” to “rails 3.0 beta, activeresource, patch, serialization, to_json, to_xml”
Yes, you're right on the human cache. The other one shouldn't be cached because i want to return @collection only if someone does self.collection_name = "api-posts" if it's not setted we should return the inflection thing i did.
Patch again.
- to_xml_and_to_json_serialization.patch 8 KB
José Valim May 6th, 2010 @ 10:23 PM
Caching the result would still work for collection when you set it through self.collection_name
Santiago Pastorino May 6th, 2010 @ 10:28 PM
Hehehe, yes you're right. Perhaps i need to go bed :).
New patch here.
- to_xml_and_to_json_serialization.patch 8 KB
Jeremy Kemper May 7th, 2010 @ 05:51 PM
ActiveModel::Name isn't meant to be a mutable object, though. You could set and cache these on the instance itself, or instantiate and cache a new Name with updated values.
Santiago Pastorino May 7th, 2010 @ 08:06 PM
Ok i will redo with this in mind ;)
Santiago Pastorino May 7th, 2010 @ 10:09 PM
Take a look now Jeremy, thanks.
Santiago Pastorino May 7th, 2010 @ 10:10 PM
patch here
- to_xml_and_to_json_serialization.patch 10.2 KB
Santiago Pastorino May 7th, 2010 @ 10:43 PM
With the new changes
- to_xml_and_to_json_serialization.patch 11.9 KB
Santiago Pastorino May 7th, 2010 @ 11:29 PM
As Jeremy requested ;), thanks Jeremy
- to_xml_and_to_json_serialization.patch 8 KB
Jeremy Kemper May 12th, 2010 @ 01:12 AM
Pending tests for ActiveModel::Name changes.
Santiago Pastorino May 12th, 2010 @ 02:33 AM
I hope now the patch is ok
- to_xml_and_to_json_serialization.patch 8.8 KB

Jeremy Kemper May 12th, 2010 @ 02:58 AM

Setting a custom collection name is not preserved:

name.collection = 'foo'
name.element = 'bar'
assert_equal 'foo', name.collection  # but it returns 'bars'

Also, this is unneeded

-    attr_reader :singular, :plural, :element, :collection, :partial_path
-    alias_method :cache_key, :collection
+    attr_reader   :singular, :plural, :partial_path
+    attr_accessor :element, :collection
+    alias_method  :cache_key, :collection

since you defined the writer methods already.

Santiago Pastorino May 12th, 2010 @ 03:40 AM
Case fixed and more tests added
Santiago Pastorino May 12th, 2010 @ 05:04 AM
Jeremy, I leave here the another possibility we discussed
Santiago Pastorino May 12th, 2010 @ 03:55 PM
Jeremy I've uploaded again the two patches the first i leave as it was and the second one i've fixed a bit to cache partial_path.
Please tell me if this need something more to be done.
- to_xml_and_to_json_serialization.patch 9.2 KB
- to_xml_and_to_json_serialization2.patch 10.1 KB
Repository May 12th, 2010 @ 05:34 PM
(from [bea3c26833ad3e1e94f7331e0553a4e2164e7de5]) Make ActiveResource serialize XML correctly when element_name is set.

[#4529]

Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/bea3c26833ad3e1e94f7331e0553a4...
Jeremy Kemper May 12th, 2010 @ 05:38 PM
- Milestone set to “2.3.6”
Needs backport for 2.3.6.
Santiago Pastorino May 12th, 2010 @ 09:35 PM
Jeremy backport of tests http://github.com/spastorino/rails/commit/6267187f3c2c703cb1f1dcb1b... but nothing broke on 2-3-stable
Santiago Pastorino May 12th, 2010 @ 09:41 PM
- State changed from “verified” to “resolved”
Repository May 12th, 2010 @ 09:42 PM
(from [c4ef7bb2a0a2e2351f5ed2b1f98c9b40c441591e]) to_json and to_xml tests added to ActiveResource

[#4529 state:resolved]

Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/c4ef7bb2a0a2e2351f5ed2b1f98c9b...
You flagged this item as spam.
Chris Griego May 13th, 2010 @ 05:22 PM
This changeset includes a regression or an intentional change that should be documented.
```
module Foo
  class Bar < ActiveResource::Base
  end
end
```
Before the changeset, this would requests the url /bars
After the changeset, this now requests the url /foo/bars
José Valim May 13th, 2010 @ 06:05 PM
- State changed from “resolved” to “open”
Santiago Pastorino May 14th, 2010 @ 12:44 PM
- Milestone cleared.
- ar_modules.patch 2.7 KB
Santiago Pastorino May 14th, 2010 @ 01:35 PM
A better one ;)
- ar_modules.patch 2.7 KB
Santiago Pastorino May 14th, 2010 @ 07:36 PM
- Assigned user changed from “Santiago Pastorino” to “José Valim”
Repository May 15th, 2010 @ 08:51 AM
- State changed from “open” to “committed”
(from [7ffe76046ad142377bf6b286f4477b09513d1e37]) ActiveResource shouldn't consider modules in the path

[#4529 state:committed]

Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/7ffe76046ad142377bf6b286f4477b...
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to “3.0.2”
- Importance changed from “” to “High”
[bulk edit]