Rendering array objects that doesn't have serializers

[joaomdmoura]

As reported on #772 and #945.
There was a crash when rendering arrays that don't consist of "serializables" classes.
This PR is a first implementation to quickly fix it.

P.S. It won't fix #877 yet.
P.S. #954 will need to be updated based on this once merged, around here

[bf4]

ref: #955 (comment)

and a fix I wrote that works at a different level, in the serializer itself.

56a7bd1

# https://github.com/rails-api/active_model_serializers/blob/56a7bd1b6dd9f083b0f3a9d6d24e9dc85efbffac/lib/active_model/serializer.rb
    def each_association(&block)
      self.class._associations.dup.each do |name, association_options|
        next unless object
        association_value = send(name)

        serializer_class = ActiveModel::Serializer.serializer_for(association_value, association_options)

         if serializer_class
-          serializer = serializer_class.new(
-            association_value,
-            options.except(:serializer).merge(serializer_from_options(association_options))
-          )
+          begin
+            serializer = serializer_class.new(
+              association_value,
+              options.except(:serializer).merge(serializer_from_options(association_options))
+            )
+          rescue NoMethodError
+            # 1. Failure to serialize an element in a collection, e.g. [ {hi: "Steve" } ] will fail
+            #    with NoMethodError when the ArraySerializer finds no serializer for the hash { hi: "Steve" },
+            #    and tries to call new on that nil.
+            # 2. Convert association_value to hash using implicit as_json
+            # 3. Set as virtual value (serializer is nil)
+            # 4. Consider warning when this happens
+            virtual_value = association_value
+            virtual_value = virtual_value.as_json if virtual_value.respond_to?(:as_json)
+            association_options[:association_options][:virtual_value] = virtual_value
+          end

[joaomdmoura]

@bf4, indeed a better solution, tried to backport it, but it didn't fixed the problem.
test failed:

def render_json_array_object_without_serializer
  render json: [{error: 'Result is Invalid'}]
end

...

def test_render_json_array_object_without_serializer
  get :render_json_array_object_without_serializer

  assert_equal 'application/json', @response.content_type
  assert_equal ([{error: 'Result is Invalid'}]).to_json, @response.body
end

[bf4]

@joaomdmoura Yeah, looking at that now, was going to make a PR to your branch

[joaomdmoura]

@bf4 tks! I was working on it too, but no worries 😄 make yourself comfortable just let me know if you need help or anything.

[bf4]

they're closely related, but have somewhat different paths, both triggered by serializable_hash, but no common place that's called, ugh. I think I'm trying to do too much at once by combining those tests... gonna make the PR just for discussion joaomdmoura#1

[bf4]

So, I think I got it figured out in that PR. Basically, the special case can be boiled down to ActiveModel::Serializer.serializer_for(object) returning an ActiveModel::Serializer.config.array_serializer that has elements that have no serializer i.e.

object.any? {|elem| ActiveModel::Serializer.serializer_for(object).nil? }

[bf4]

So, I'm thinking maybe the solution should be in serializer_for rather than in ArraySerializer, but I'm not sure how to efficiently do that without essentially iterating over all the elements twice.

[bf4]

Do you want to keep objects reader around? It's no longer used. I could see an argument that it's a private interface.

[joaomdmoura]

No, indeed, there is no reason to keep it

[bf4]

What do you think about having serializer_for return nil for if any elements don't have an associated serializer? That would maintain current PR behavior, I think, might make more sense SRP-wise, but might be a performance hit.

Should we add a test for a collection or association that contains an element with a known serializer and another with a nil serializer class? I'm really not sure how to handle that in AMS without more complexity. I'd rather push that back to the end user, but that could be laziness.

[joaomdmoura]

We would need to update this PR if we change this behaviour at serializer_for.

I think it's an edge-case. I haven't heard about ppl using AMS to renders arrays that mix serializable and non-serializable objects yet, so let's not worry about it for now 😄

[bf4]

winning

[bf4]

We should add some docs on building your own array serializer as described in https://github.com/rails-api/active_model_serializers#2-for-an-array-resource and the use of this exception class at some point. Want me to make an issue?

[joaomdmoura]

I'm on a quest to keep the readme as clean as possible, but I agree that this can be helpful for some ppl, like others details that are poping up.
I'm thinking about start the AMS wiki (using gh wiki feature). For now, make yourself comfortable to open the issue to make sure we won't forget it while I think it trough.

[bf4]

I like wikis, but then it's another thing to garden. For AMS I think it could be a plus.

[bf4]

Failure are Rails 4.0 only and have to do with deep munging of json keys. I think you can configure that with config.action_dispatch.perform_deep_munge = false ? rails/rails#13420 ? or maybe not? http://guides.rubyonrails.org/4_1_release_notes.html#action-pack-notable-changes

[bf4]

Submitted PR to this branch in joaomdmoura#2

[joaomdmoura]

Great team work here folks! Tks so much @JustinAiken and @bf4
I'm merging it.