Remove unneeded SQL query when determining serializer class.

[phallstrom]

While experimenting with the gem I noticed that I was seeing two SQL queries that were almost identical except one was limited to a single result. I tracked it down to the serializer_class method which was calling resource.first in order to determine the class name of the collection of resources.

This is necessary if resource is an Array, but if it's an ActiveRecord::Relation, we can call model directly on the resource to get the class name.

I've seen elsewhere where you mention you don't want to rely on Rails' behavior, so I did this in a way that if ActiveRecord::Relation isn't defined it will fallback to the previous behavior.

I don't believe additional tests are necessary as there already exist tests that flip as_list true and false which will exercise this change.

[stas]

Hey @phallstrom

Thanks for looking into it, really appreciate it!

I'm tested your theory on a project where we have a collection of polymorphic resources, and here's the outcome:

irb(main):008:0> resource = Account.where.not(id: nil); 1
=> 1
irb(main):009:0> resource.model.name
=> "Account"
irb(main):010:0> resource.first.class.name
=> "Account::Ach"

As you can see, Rails dynamically sets the type for a record, which means that we'd have to peek in order to get the correct serializer. I'm afraid until v3 of the jsonapi-serialzier is not out, this might have to stay the way it is...

Let me know what are you thoughts though. :bow:

[phallstrom]

@stas Oh, that's interesting. Question for you.. if resource contains a collection of polymorphic records, wouldn't picking the first one's class possibly end up using the wrong serializer for the second, etc? In either case, I'll poke at it again and see if we can determine if it's polymorphic up front and if so just default to the original.

[stas]

@phallstrom it would, in fact I added some monkey patching to avoid that, but still, it's not something I'd release under a gem.

Btw, you might be interested in taking a look at how it's approached for v3: https://github.com/jsonapi-serializer/jsonapi-serializer/blob/rewrite/lib/jsonapi/serializer.rb#L28-L51

[phallstrom]

@stas Just so I understand, even if we could detect the polymorphism, you wouldn't want to merge it?

[stas]

@stas Just so I understand, even if we could detect the polymorphism, you wouldn't want to merge it?

I'd be happy to merge it if we take into account that polymorphic collection use-case. Just don't think the current PR is ready.

[phallstrom]

@stas Gotcha. Also, do you mean STI instead of polymorphic?

[stas]

@stas Gotcha. Also, do you mean STI instead of polymorphic?

Actually both would be correct. Either we serialize polymorphic relationships or a collection of STI records, the implementation would have to work transparently for both.

[phallstrom]

@stas Been so long since I've worked with polymorphic (or STI). Heh.

For polymorphic, wouldn't the default behavior kick in as resource wouldn't be a collection so it would simply call resource.class and be good to go?

STI is a bit more of a PITA.. only thing I can find that might work is subclasses and while it seems to work, it isn't specific to STI. Hrmph.

[stas]

For polymorphic, wouldn't the default behavior kick in as resource wouldn't be a collection so it would simply call resource.class and be good to go?

Polymorphic records would still require the right type to be serialized.

[a3626a]

Why don't you just add 'to_a' in add_renderer!?

    def self.add_renderer!
      ActionController::Renderers.add(:jsonapi) do |resource, options|
        self.content_type ||= Mime[:jsonapi]

        JSONAPI_METHODS_MAPPING.to_a[0..1].each do |opt, method_name|
          next unless respond_to?(method_name, true)
          options[opt] ||= send(method_name, resource)
        end

        # to_a HERE!
        # methods in JSONAPI_METHODS_MAPPING might require ActiveRecordRelation, not Array.
        resource = resource.to_a

        # If it's an empty collection, return it directly.
        many = JSONAPI::Rails.is_collection?(resource, options[:is_collection])
        if many && !resource.any?
          return options.slice(:meta, :links).merge(data: []).to_json
        end

        JSONAPI_METHODS_MAPPING.to_a[2..-1].each do |opt, method_name|
          options[opt] ||= send(method_name) if respond_to?(method_name, true)
        end

        if respond_to?(:jsonapi_serializer_class, true)
          serializer_class = jsonapi_serializer_class(resource, many)
        else
          serializer_class = JSONAPI::Rails.serializer_class(resource, many)
        end

        JSONAPI::Rails.serializer_to_json(
          serializer_class.new(resource, options)
        )
      end
    end

We can just feed Array to fast json api serializers. https://github.com/Netflix/fast_jsonapi/issues/154#issuecomment-383348145

[stas]

Why don't you just add 'to_a' in add_renderer!?

The resource is an active record object, meaning it was not populated with the data from the database. If you do #to_a it will hit the database and pull out all the records and serialize these. Meaning you will load potentially a big dataset just to check if it's single record or many....

[a3626a]

The resource is an active record object, meaning it was not populated with the data from the database. If you do #to_a it will hit the database and pull out all the records and serialize these. Meaning you will load potentially a big dataset just to check if it's single record or many....

I think they are just before to be serialized. Is it a really big problem to load them a bit earlier? I think type casting could be a problem. Because it is changing ActiveRecordRelation to Array. We can avoid this type casting in some way, too.

I might miss someting. I just assume that these lines are executed when I do :

my_records = User.some_filter_and_pagination_and_complicated_logic
render jsonapi: my_records

[stas]

Is it a really big problem to load them a bit earlier?

A resource could represent the whole table of records. ActiveRecord has caching, but it might or might not work depending on your filters or query, other DB adapter implementations might not have... So we just assume the worst case scenario always and optimize for that. But yep, it is not something we'd like to do.

[a3626a]

If someone want to avoid additional 'LIMIT 1' queries, load function can help.

query = User.filtered
render jsonapi: query
# This causes several ~~~ LIMIT 1 queries

query = User.filtered
render jsonapi: query.load
# This causes no ~~~ LIMIT 1 queries