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 does not serialize XML correctly

#4529

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.

Reported by VirtualFunction · May 4th, 2010 @ 07:53 AM

State: committed
Milestone: 3.0.2
Assigned to: José Valim José Valim
Importance: High

Activity

  1. Neeraj Singh
    Neeraj Singh

    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.

    May 4th, 2010 @ 09:33 AM

  2. Neeraj Singh
    Neeraj Singh

    On further analysis the commit that is causing this problem is this one.

    http://github.com/rails/rails/commit/7cd1d37a51f5f53f8fc1360f886d26...

    May 4th, 2010 @ 11:02 AM

  3. Neeraj Singh
    Neeraj Singh
    • 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.

    May 4th, 2010 @ 04:05 PM

  4. Jeremy Kemper
    Jeremy Kemper
    • Milestone cleared.
    • State changed from new to open
    • Assigned user changed from Jeremy Kemper to Santiago Pastorino

    Doh! Needs a regression test + fix.

    May 4th, 2010 @ 10:11 PM

  5. Santiago Pastorino
    Santiago Pastorino
    • 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.

    May 6th, 2010 @ 04:53 PM

  6. José Valim
    José Valim

    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 ||=".

    May 6th, 2010 @ 09:43 PM

  7. Santiago Pastorino
    Santiago Pastorino
    • 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.

    May 6th, 2010 @ 10:16 PM

  8. José Valim
    José Valim

    Caching the result would still work for collection when you set it through self.collection_name

    May 6th, 2010 @ 10:23 PM

  9. Santiago Pastorino
    Santiago Pastorino

    Hehehe, yes you're right. Perhaps i need to go bed :).
    New patch here.

    May 6th, 2010 @ 10:28 PM

  10. Jeremy Kemper
    Jeremy Kemper

    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.

    May 7th, 2010 @ 05:51 PM

  11. Santiago Pastorino
    Santiago Pastorino

    Ok i will redo with this in mind ;)

    May 7th, 2010 @ 08:06 PM

  12. Santiago Pastorino
    Santiago Pastorino

    Take a look now Jeremy, thanks.

    May 7th, 2010 @ 10:09 PM

  13. Santiago Pastorino
  14. Santiago Pastorino
  15. Santiago Pastorino
  16. Jeremy Kemper
    Jeremy Kemper

    Pending tests for ActiveModel::Name changes.

    May 12th, 2010 @ 01:12 AM

  17. Santiago Pastorino
  18. Jeremy Kemper
    Jeremy Kemper

    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.

    May 12th, 2010 @ 02:58 AM

  19. Santiago Pastorino
    Santiago Pastorino

    Case fixed and more tests added

    May 12th, 2010 @ 03:40 AM

  20. Santiago Pastorino
    Santiago Pastorino

    Jeremy, I leave here the another possibility we discussed

    May 12th, 2010 @ 05:04 AM

  21. Santiago Pastorino
    Santiago Pastorino

    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.

    May 12th, 2010 @ 03:55 PM

  22. Repository
  23. Jeremy Kemper
    Jeremy Kemper
    • Milestone set to 2.3.6

    Needs backport for 2.3.6.

    May 12th, 2010 @ 05:38 PM

  24. Santiago Pastorino
  25. Santiago Pastorino
    Santiago Pastorino
    • State changed from verified to resolved

    May 12th, 2010 @ 09:41 PM

  26. Repository
  27. Chris Griego
    Chris Griego

    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

    May 13th, 2010 @ 05:22 PM

  28. José Valim
    José Valim
    • State changed from resolved to open

    May 13th, 2010 @ 06:05 PM

  29. Santiago Pastorino
  30. Santiago Pastorino
  31. Santiago Pastorino
    Santiago Pastorino
    • Assigned user changed from Santiago Pastorino to José Valim

    May 14th, 2010 @ 07:36 PM

  32. Repository
    Repository
    • State changed from open to committed

    May 15th, 2010 @ 08:51 AM

  33. Jeremy Kemper
    Jeremy Kemper
    • Milestone set to 3.0.2
    • Importance changed from to High

    October 15th, 2010 @ 11:01 PM