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.

Fix to_xml serialization in some rare cases.

#123

Hi,

this is my first patch, so... here's hoping that I'm posting this in the right place.

It is possible for a model to overwrite its to_xml method in a way that'll make sure some specific association is always included with that model. This can be done like this: 

def to_xml(options = {})
  options[:include] = [:my_assoc]
  super(options)
end

However, this breaks down when (for some reason) you create an array with records of more than one kind. For example: 

records = Page.find(:all) + Article.find(:all)
records.to_xml # errors out if Page or Article changes options in their to_xml method

The reason for this behavior is the fashion in which the options hash is distributed among the objects in the array. I've modified the to_xml method on Array (ActiveSupport/CoreExtensions/Array) to check whether there are objects of more than one class in the array and, if so, to create a copy of the options hash for each individual class of objects.

Here's my patch: 


def to_xml(options = {})
  raise "Not all elements respond to to_xml" unless all? { |e| e.respond_to? :to_xml }

  options[:root]     ||= all? { |e| e.is_a?(first.class) && first.class.to_s != "Hash" } ? first.class.to_s.underscore.pluralize : "records"
  options[:children] ||= options[:root].singularize
  options[:indent]   ||= 2
  options[:builder]  ||= Builder::XmlMarkup.new(:indent => options[:indent])

  root     = options.delete(:root).to_s
  children = options.delete(:children)

  if !options.has_key?(:dasherize) || options[:dasherize]
    root = root.dasherize
  end

  options[:builder].instruct! unless options.delete(:skip_instruct)

  opts = options.merge({ :root => children })

  classes = collect {|e| e.class}.uniq
  if(classes.length != 1)
    cloned_options = {}
    classes.each do |class_name|
      cloned_options[class_name] = opts.clone
    end
  end

  xml = options[:builder]
  if empty?
    xml.tag!(root, options[:skip_types] ? {} : {:type => "array"})
  else
    xml.tag!(root, options[:skip_types] ? {} : {:type => "array"}) {
      yield xml if block_given?
      each do |e|
        if(classes.length == 1)
          e.to_xml(opts.merge!({ :skip_instruct => true }))
        else
          e.to_xml(cloned_options[e.class].merge!({ :skip_instruct => true}))
        end
      end
    }
  end
end

Reported by Adam Sindelar · May 6th, 2008 @ 01:57 PM

State: stale
Milestone: none
Assigned to: Pratik Pratik
Importance: none

Activity

  1. Jonathan del Strother
    Jonathan del Strother

    Good idea, but -

    a) Why not just always clone the options?

    b) It would be much more likely that this gets accepted if you supply in patch form (http://www.tpope.net/rails-git-b...) rather than something that has to be manually cut & pasted, with no way of easily comparing the differences

    c) Tests would be nice

    May 6th, 2008 @ 02:26 PM

  2. Adam Sindelar
    Adam Sindelar

    Ok, so here's a git patch, hope it makes sense. I also ran the full rails test suite and there were no regressions on my machine (rails 2.0.2 with ruby 1.8.6). There is one assertion failure in mysql test, but that happens regardless of whether or not the patch is applied.

    As for the reason to not always clone the options hash - the hash contains the XML builder instance and some other data, and all in all, it seemed to me that cloning it under most circumstances would be an unnecessary slowdown. The code can be easily modified to simply always clone the hash, but that'll mean a performance deterioration even in those cases where cloning is not needed. That's just my guess though.

    Hope this is better.

    May 6th, 2008 @ 04:57 PM

  3. Adam Sindelar
    Adam Sindelar

    Hey,

    please don't take this the wrong way, I'm not trying to bug anyone... But what are the things I should do in order to have this patch accepted into the rails codebase (or to have it reviewed by someone)? I recognize that there's a lot of other tickets waiting for resolution, but I would hate for this to be forgotten - so what should I do next?

    If the delay is normal, fair enough; I'm just trying to make sure there's not something I'm missing.

    Thanks.

    May 8th, 2008 @ 11:58 AM

  4. Jonathan del Strother
    Jonathan del Strother

    The delay is pretty normal. You generally need 3 people to vote +1 on your patch before it gets accepted. Best way of doing that is by hopping on the #rails-contrib irc channel and coercing people into taking a look.

    Personally, I don't think the overhead of 'clone' is worth the extra code to avoid it. Bear in mind that when you clone a hash, you're cloning the references to the objects in it, rather than the objects themselves. That is, {:xml => Builder::XmlBase.new}.clone will produce two hashes, but both point to the same instance of the xml builder.

    May 8th, 2008 @ 12:10 PM

  5. Pratik
    Pratik
    • State changed from new to incomplete
    • Title changed from [patch] Fix to_xml serialization in some rare cases. to Fix to_xml serialization in some rare cases.

    Hi Adam,

    I agree with all suggestions ( only a. and c. are relevant now ) made by Jon.

    You need to add tests to rails' existing test suite, which would fail without your patch. That's the general drill for getting the patch accepted.

    Marking the ticket as "incomplete" till tests are added.

    Thanks.

    May 13th, 2008 @ 04:34 PM

  6. Adam Sindelar
    Adam Sindelar

    Hi again,

    Firstly, sorry that I took this long to get back to this ticket, I needed to work on a different project for the past week or so.

    Secondly, I've made the change to always clone the options hash, instead of checking if it's required.

    The thing I'm uncertain about are the tests, though, so if I might have a few inquisitive questions (I looked for this information on rails website, but I couldn't find any guidelines):

    Am I allowed (encouraged even?) to create a new model class to go with my test, or should I utilize one that already exists?

    I'm going to try and fit this test inside the serialization_test.rb, but it's probably going to require changes to that file that go beyond merely adding another assertion (would definitely need another object with data).

    Thanks.

    May 20th, 2008 @ 10:11 AM

  7. Pratik
    Pratik

    Adam, we don't have a strict set of rules for tests. If you think it's worth creating a new file for tests, go for it. If you think they're a better fit for some of the existing test file, squeeze it in that.

    Thanks.

    May 20th, 2008 @ 10:33 AM

  8. Pratik
    Pratik
    • Assigned user set to Pratik

    May 20th, 2008 @ 10:33 AM

  9. Rohit Arondekar
    Rohit Arondekar
    • State changed from incomplete to stale
    • Tag set to bug, core_ext, patch
    • Importance changed from to

    Marking ticket as stale. If this is still an issue please leave a comment with suggested changes, creating a patch with tests, rebasing an existing patch or just confirming the issue on a latest release or master/branches.

    October 6th, 2010 @ 06:47 AM

  10. Jeff Kreeftmeijer
    Jeff Kreeftmeijer
    • Tag cleared.

    Automatic cleanup of spam.

    November 7th, 2010 @ 04:53 PM