This project is archived and is in readonly mode.
Fix to_xml serialization in some rare cases.
Reported by Adam Sindelar | May 6th, 2008 @ 01:57 PM
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
Comments and changes to this ticket
-
Jonathan del Strother May 6th, 2008 @ 02:26 PM
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
-
Adam Sindelar May 6th, 2008 @ 04:57 PM
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.
-
Adam Sindelar May 8th, 2008 @ 11:58 AM
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.
-
Jonathan del Strother May 8th, 2008 @ 12:10 PM
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.
-
Pratik May 13th, 2008 @ 04:34 PM
- 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.
-
Adam Sindelar May 20th, 2008 @ 10:11 AM
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.
-
Pratik May 20th, 2008 @ 10:33 AM
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.
-
Pratik May 20th, 2008 @ 10:33 AM
- Assigned user set to “Pratik”
-
Rohit Arondekar October 6th, 2010 @ 06:47 AM
- 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.
-
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>
Adam Sindelar
Jeremy Kemper
Pratik
Rohit Arondekar