changing the default behaviour of to_xml for has-many: associations

I recently submitted a patch to work around an issue I was having & added a new optional behaviour to the to_xml method but on further reflection I wonder if the default behaviour should be changed.

The gory detail, documentation and test case is all here... http://dev.rubyonrails.org/ticket/7004

Here's a brief example...

class Person < ActiveRecord::Base   has_many: children end

class Male < Person; end class Female < Person; end

The default to_xml would look like this...

<person>    <males><child>...</child><child>...</child>...</males> </person>

i.e. choosing the type name of the first child and pluralizing it as the relationship name ('males' if the first child is male). So to find the children you're not sure if you need to look for 'males' or 'females' element which would make things like xpath a bit messier than it might to find, say, all the children's names. e.g. I'd much rather in XPath do /person/children/*/name

It gets worse if there are 2 relationships to people. e.g.

class Person < ActiveRecord::Base   has_many: children   has_many: siblings end

Now we've 2 elements inside <person> each of which could be <males> or <females> and one of them is the children relationship and one is the siblings - which makes using the XML pretty hard. Also the nested elements inside the <males> or <females> loose their type as they are all called <child> or <sibling>

So the patch i've submitted allows this behaviour to be reversed if you supply the :polymorphic option to to_xml. If so it would use the relationship name for the outer collection element and for the inner collection element use the type name of the child. This would make the more natural XML

<person> <children> <male .../> <female .../> </children> <siblings> <male .../> <female .../> </siblings> </person>

Which is nice and easy to XPath into to find what you need as well as keeping the polymorphic type information etc. It also works a bit nicer in XML schema / validation libraries and whatnot. So in XML to find the male children names its /person/children/male/name or to find all children names /person/chlldren/*/name etc.

I originally created this patch so that this new behaviour has to be enabled via the :polymorphic option to avoid breaking things which rely on the old behaviour. Being my first post here I didn't wanna break anything :). However I didn't see any tests for the old behaviour and I'm starting to wonder whether this new behaviour should be the default.

I admit to being a complete newbie Railser, but I admit being quiet surprised how this worked first time I saw it and I'd have thought that for has-many: relationships the to_xml would use the relationship name for the outer element (which is more likely to be unique unlike the type name) and then the type name for the inner relationship?

I'd be totally happy if my patch was submitted so folks could just enable this newer behaviour when they wanted it - I just wondered what others thought on what the default to_xml behaviour should be for has_many: with the possibility of inheritence?

I recently submitted a patch to work around an issue I was having & added a new optional behaviour to the to_xml method but on further reflection I wonder if the default behaviour should be changed.

Hmm, that is certainly annoying behaviour. I can definitely buy the case for an option to use the association name rather than foo.bars.first.whatever.that.code.does. I'm not necessarily sure that 'polymorphic' is the right name for it, as that's used elsewhere in AR with a slightly different meaning.

I'm hesitant to break backwards compatibility, but there's definitely a case here for doing something.

Hmm, that is certainly annoying behaviour. I can definitely buy the case for an option to use the association name rather than foo.bars.first.whatever.that.code.does. I'm not necessarily sure that 'polymorphic' is the right name for it, as that's used elsewhere in AR with a slightly different meaning.

I agree. I can certainly understand how the current behavior was arrived at, but there's room for improvement.

I don't think 'polymorphic' is the right thing to use here, either. My feeling is that the association name should be used as the containing element name by default. If you want to use a different name in the xml, perhaps a syntax like

  foo.to_xml( :include => [ { :spams => :hams }, :bars ] )

would do the trick?

I'm hesitant to break backwards compatibility, but there's definitely a case here for doing something.

Personally, I would not hesitate to break backwards compatibility if the current behavior is determined to be wrong. There's already a lot of house cleaning going on in the march towards 2.0 -- don't see much reason why this situation should be any different than any of the current deprecations (assuming a change is agreed upon).

John Wilger wrote:

Hmm, that is certainly annoying behaviour. I can definitely buy the case for an option to use the association name rather than foo.bars.first.whatever.that.code.does. I'm not necessarily sure that 'polymorphic' is the right name for it, as that's used elsewhere in AR with a slightly different meaning.

I agree. I can certainly understand how the current behavior was arrived at, but there's room for improvement.

I don't think 'polymorphic' is the right thing to use here, either. My feeling is that the association name should be used as the containing element name by default. If you want to use a different name in the xml, perhaps a syntax like

  foo.to_xml( :include => [ { :spams => :hams }, :bars ] )

would do the trick?

I'm hesitant to break backwards compatibility, but there's definitely a case here for doing something.

Personally, I would not hesitate to break backwards compatibility if the current behavior is determined to be wrong. There's already a lot of house cleaning going on in the march towards 2.0 -- don't see much reason why this situation should be any different than any of the current deprecations (assuming a change is agreed upon).

Given that has_many: associations are not even marshalled to XML unless you specifically ask for them - I wonder how many folks are really using the current behaviour. So I'd personally prefer the default behaviour change - rails should do the right thing by default - and we keep the old behaviour as an option folks can enable if they really want.

Incidentally I really hate the option name I picked of :polymorphic but every other option I considered seemed too long. Maybe we need options like :use_association_name_for_many or :use_first_type_name_for_many (depending on which is the default and which has to be enabled)?

James

Given that has_many: associations are not even marshalled to XML unless you specifically ask for them - I wonder how many folks are really using the current behaviour. So I'd personally prefer the default behaviour change - rails should do the right thing by default - and we keep the old behaviour as an option folks can enable if they really want.

Ok, I'm sold. If we change the default behaviour to use the association name, and provide an option to retain the old behaviour then people who rely on the current stuff (think @section.to_xml :include=>:published_posts) can retain it.

Incidentally I really hate the option name I picked of :polymorphic but every other option I considered seemed too long. Maybe we need options like :use_association_name_for_many or :use_first_type_name_for_many (depending on which is the default and which has to be enabled)?

I'll have a think about a decent name for this 'use first type name for collection' option, but if anyone else wants to chime in with the perfect solution, now's the time :).