When serialising a class, specify the type of any singular associations,... #7858

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
Contributor

cliochris commented Oct 5, 2012

When serialising a class, specify the type of any singular associations, if necessary. Rails already correctly specifies the :type of any enumerable association (e.g. a has_many association), but made no attempt to do so for non-enumerables (e.g. a has_one association).

We must specify the :type of any polymorphic association. A has_one association to a class which uses single-table inheritance is an example of a polymorphic association.

Fixes #7471

@cliochris cliochris When serialising a class, specify the type of any singular associatio…
…ns, if

necessary. Rails already correctly specifies the :type of any enumerable
association (e.g. a has_many association), but made no attempt to do so for
non-enumerables (e.g. a has_one association).
We must specify the :type of any polymorphic association. A has_one
association to a class which uses single-table inheritance is an example of
a polymorphic association.
Fixes #7471
e7f7e35
Contributor

acapilleri commented Oct 6, 2012

@cliochris thanks for you PR
cc / @rafaelfranca

@rafaelfranca rafaelfranca commented on the diff Oct 6, 2012

activemodel/lib/active_model/serializers/xml.rb
@@ -140,7 +140,9 @@ def add_associations(association, records, opts)
end
else
merged_options[:root] = association.to_s
- records.to_xml(merged_options)
+ association_name = association.to_s.tableize.singularize
@rafaelfranca

rafaelfranca Oct 6, 2012

Owner

This need a refactoring.

association_name = association.to_s
record_class = record.class

merged_options[:root] = association_name

if record_class.to_s.underscore == association_name.tableize.singularize
  merged_options[:type] = record_class.name
end

records.to_xml(merged_options)
Owner

rafaelfranca commented Oct 6, 2012

Is this an issue in the master branch? If it is please open the pull request pointing to master and I'll backport to 3-2-stable.

Contributor

al2o3cr commented Oct 10, 2012

A minor semantic issue with the commit message - describing a has_one that points to STI records as "polymorphic" is confusing, since isn't implemented as a polymorphic association (with :as / :polymorphic). Not sure what a better term would be, though...

Member

steveklabnik commented Oct 11, 2012

Agreed. Just say STI instead.

Contributor

frodsan commented Oct 26, 2012

@cliochris any news on this?

Contributor

cliochris commented Oct 26, 2012

Sorry, I've been busy on other work. I'll try to get to this over the weekend. I understand @al2o3cr's comment about "polymorphic" being confusing, but note that it is NOT just STI that is affected, as shown in the unit test. On the other hand, STI is what lead me to the bug in the first place.

Contributor

cliochris commented Nov 27, 2012

Sorry, unable to make further progress on this in 2012. Probably not a big deal as the issue has been outstanding since I reported it in February, 2011.

Member

steveklabnik commented Nov 27, 2012

I will take a look at updating this, then. Thanks for letting us know. Leaving this open till I put my money where my mouth is ;)

Member

steveklabnik commented Nov 28, 2012

I have submitted #8352 which supercedes this. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment