ActiveModel support for the :include serialization option #195

Merged
merged 6 commits into from Jul 18, 2011

Projects

None yet

6 participants

@jfirebaugh

I'd like to use the :include option with non-AR classes.

This pull request includes some preliminary clean up and test organization. The meat is in the final commit:

This commit moves support for the :include serialization option for
serializing associated objects out of ActiveRecord in into ActiveModel.
The following methods support the :include option:

  • serializable_hash
  • to_json
  • to_xml

Instances must respond to methods named by the values of the :includes
array (or keys of the :includes hash). If an association method returns
an object that is_a?(Enumerable) (which AR collection proxies do), it
is assumed to be a collection association, and its elements must respond
to :serializable_hash. Otherwise it must respond to :serializable_hash
itself.

There's a bit more documentation and test reorganization that should happen (for instance a lot of the AR serialization tests should be refactored to AM tests), but I wanted to get some feedback before I went any further.

@jbescoyez

That is exactly what I need. Moreover, this is a more consistent approach.

+1

@smartinez87

Please remember to update the guides with this change in order to consider this a complete pull request.

@josevalim
Member

@jfirebaugh looks good. Just one small thing, could you please not remove tests from AR? It is ok to have a bit of duplication in AR and AMo tests. Thanks!

@sikachu
Member
sikachu commented Jun 1, 2011

@jfirebaugh would you mind update your pull request according to the feedback? Thank you :)

John Firebaugh added some commits Feb 22, 2011
John Firebaugh serializable_hash(:only => [], :methods => [:foo]) should work da4e1fa
John Firebaugh Add a test 2a9a10f
John Firebaugh Replace map+compact with select cbf924e
John Firebaugh We already have the record; no need to retrieve it again 0933b6d
John Firebaugh Move to_xml tests to xml_serialization_test.rb
One duplicate was eliminated: test_to_xml_including_methods/
test_methods_are_called_on_object.
1723a7a
John Firebaugh ActiveModel support for the :include serialization option
This commit moves support for the :include serialization option for
serializing associated objects out of ActiveRecord in into ActiveModel.
The following methods support the :include option:

  * serializable_hash
  * to_json
  * to_xml

Instances must respond to methods named by the values of the :includes
array (or keys of the :includes hash). If an association method returns
an object that is_a?(Enumerable) (which AR has_many associations do), it
is assumed to be a collection association, and its elements must respond
to :serializable_hash. Otherwise it must respond to :serializable_hash
itself.

While here, fix #858, XmlSerializer should not singularize already
singular association names.
4860143
@jfirebaugh

I've rebased the commits onto current master, fixed a few problems, and added AM-level tests for to_xml with :include.

Passing all activemodel and activerecord specs. No AR specs were removed (well, there was one that was duplicating an existing AR spec, but I didn't remove duplication between AR and AMo).

@smartinez87 I don't see anywhere in the guides where this is covered.

@josevalim josevalim merged commit da14489 into rails:master Jul 18, 2011
@bogdan
bogdan commented Aug 26, 2011

It seems that :include option is totally replicate the functionality of :methods now. The only difference is nesting that is only available for :include.

Why do we need both of them? Can we just add ability of nested attributes for :methods(as more natural) and drop :include? Because :include now looks like a historical feature.

@josevalim
Member

I don't think it makes much sense to couple those two options. What if we want to add more features to :include in the feature that does not make sense for :methods? However, if we can refactor more the code with this behavior in mind, the refactoring is welcome. :)

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