Fix #876 - include meta when using json adapter with custom root

[chrisbranson]

This PR fixes issue #876 where meta was not being included when using the JSON adapter and a custom root.

[bf4]

Looks good! Should also have a test for the array serializer

[bf4]

Probably marginally better to have @options.fetch(:root) { serializer.json_key } so that the fallback is only evaluated when the key is missing.

[joaomdmoura]

Cool! Thank you both of you! A quick win 😄 . Also agree that was cleaver to not use #915 for now.
@chrisbranson could you just squash the commits into one with a cool and clear description?
Meanwhile @kurko just pinging you on this.

[bf4]

Looking great!!

Question: should any of these tests be in test/serializers/meta_test.rb (or should any of them be failing right now?)

I think it would be great to add a test around a meta key present, but no root given in the render, to show that meta is ignored in the Array scenario.

I don't know if there's an advantage to also testing the below root-related settings:

• ActiveModel::Serializer._root = true/nil
• ItemSerializer._root = true/nil
• ActiveModel::Serializer::ArraySerializer._root = true/nil
• ActiveSupport.on_load(:active_model_serializers) do
self._root = true/nil
end
• ActiveRecord::Base.include_root_in_json = true/false
• ActiveSupport.on_load(:active_record) do
self.include_root_in_json = true/false
end


Also related:

• 0.10.stable / master is ignoring (key/root) option in has_many method  #882

[kurko]

Yes, I agree with everything @bf4 is saying. We need moar teeeeests

[joaomdmoura]

Great, would be awesome if we could already add some of this tests in this PR 😄

[chrisbranson]

The tests in test/serializers/meta_test.rb do not fail because they pass the :root option to the serializer whereas the controller tests go via lib/action_controller/serialization.rb which is passing :root to the adapter (due to the partitioning on line 39).

I've altered meta_test.rb to partition the options in the same way as the serialization controller so they fail without my changes, and added a new test to check for the presence of meta when using the array serializer and a custom root.

Commits squashed with a hopefully cool and clear enough commit message 😎

[chrisbranson]

Not sure why the CI tests are failing - it seems to be falling over on one unrelated test when run against Rubinius on Rails 4.0 ...

[bf4]

@chrisbranson CI is green. All of the failures I see are pre-existing and ignored. Your'e good :)

[chrisbranson]

@bf4 Good news - it was failing earlier but seems to have sorted itself out now 😄

[bf4]

wow is this screaming for a refactor of

active_model_serializers/lib/action_controller/serialization.rb

Lines 37 to 49 in 35fb9de

	define_method renderer_method do |resource, options|
	@_adapter_opts, @_serializer_opts =
	options.partition { |k, _| ADAPTER_OPTION_KEYS.include? k }.map { |h| Hash[h] }
	if use_adapter? && (serializer = get_serializer(resource))
	@_serializer_opts[:scope] ||= serialization_scope
	@_serializer_opts[:scope_name] = _serialization_scope
	# omg hax
	object = serializer.new(resource, @_serializer_opts)
	adapter = ActiveModel::Serializer::Adapter.create(object, @_adapter_opts)
	super(adapter, options)

or what? (not in this PR, just saying)

I remember reading through all these tests and code while trying to figure out how to test my serializers. I ended up doing something like in my comment in 878 which was a lot more work that you'd think. Also, too bad minitest doesn't have shared examples, though I think ActiveModel::Lint might be a fine pattern.

[bf4]

@chrisbranson This is so great! Thanks for doing all the actual work! Lemme know if you'd rather limit the scope of work (and comments) in any way and defer some work to future PRs. (especially since I'm not even a collab, just have experience with this code not working right and wanna help :)

[chrisbranson]

I think this PR is done for the issue it was trying to address - but as I discovered earlier today and you've highlighted in your comments there is considerable scope for refactoring. That said, what's there does work as expected now.

Much as I'd like to wade in and help out, I'm not going to get much more time in the near future to focus on non-essential changes simply due to day job time commitments - I've got an API to finish 😄

[bf4]

👍 @joaomdmoura @kurko LGTM

[chrisbranson]

Updated with the cleaner coding style as recommended by @bf4 👍

[iMacTia]

Waiting for this pull request to be accepted and starting using the root option on has_many again :)!

[bf4]

@iMacTia are you saying you're waiting to use this PR or you have another PR you want to make (ref plz)?

[iMacTia]

I mean I'm waiting for this one, should solve #882 as well if I got it right, isn't it?

[bf4]

@iMacTia what I do in this situation is in my Gemfile

# Bundling from source until https://github.com/rails-api/active_model_serializers/pull/936 is merged
gem "active_model_serializers", github: "insphire/active_model_serializers", branch: "fix-meta-with-custom-root"

[joaomdmoura]

Awesome work fellows! Really happy that you guys made it!
Merging it.