[WIP] fix belongs_to causing unnecessary db hits

[NullVoxPopuli]

Purpose

we only need id and type for belongs to relationships, and we can optimize render time by not touching objects at the other end of the belongs_to

Changes

Just notes for now. So others can see where the problem is. We're going to have to tweak how we build the relationship reflection objects, or maybe make all calls to the reflection lazy. idk.

I feel like in order to do this properly, we are going to need to more directly tie in to ActiveRecord.
as @beauby said in #1100:

What happens if we have belongs_to :author in the serializer, and belongs_to :author, class_name: "User" in the model? Then I fear AMS wouldn't know how to set the type property of the Resource Identifier.

So.. not sure if the team wants to have that much coupling with AR. I think its needed in order to properly address this issue though.

Related GitHub issues

#762
#1100 (failing test by @ssendev)

TODO before merging

• Remove binding.pry
• Actually fix the issue

[NullVoxPopuli]

test courtesy of @ssendev

[sandstrom]

This would be a godsend. 🌟 Currently using AMS and need manual work-arounds to avoid these costly DB-lookups.

I agree with using reflection to inspect the relationships of the model. Specifying a type manually would be rare and only needed for polymorphic relationships.

I don't use ActiveRecord, so not 100% sure but I think Author.reflect_on_association(:books).klass works[1]. Mongoid, another popular ORM, also has good reflection support.

Would be great if the decided-upon solution would support both Mongoid and ActiveRecord.

[1] https://github.com/rails/rails/blob/master/activerecord/lib/active_record/reflection.rb

[NullVoxPopuli]

I guess what we could do is maybe have support for looking up custom types / polymorphic types via ActiveRecord if that is available, and then also provide a way to override the serialized type name per relationship in the serializer?

maybe clearly document how that automatic type finding works, so people using mongoid can easily monkey patch without fear to also have automatic type finding?

coupling with AR kinda opens a can of worms though. Like, what's to prevent us from also implementing includes on associations to reduce n+1 on has_many relationships as well?

[sandstrom]

@NullVoxPopuli I agree, If it's easy to modify I think that's alright.

Just a short thrown-together snippet to illustrate my thought. Basically I'll try reflection for AR + it's public and overrideable so that someone using Mongoid can easily adjust in their ApplicationSerializer.

# public
# override to use your own strategy, e.g. for Mongoid
def retrieve_relation_id
  if defined?(ActiveRecord)
    # ... something using AR, e.g.
    # Author.reflect_on_association(:books).klass
  else
    # fallback to the current, naive solution
    record.relation.id
  end
end

[bf4]

in JSONAPI::Resources, the relationship object has a foreign_key_on option that may be self.

docs

The relationship methods (relationship, has_one, and has_many) support the following options:

• foreign_key - the method on the resource used to fetch the related resource. Defaults to <resource_name>_id for has_one and <resource_name>_ids for has_many relationships.
  to_one relationships support the additional option:
• foreign_key_on - defaults to :self. To indicate that the foreign key is on the related resource specify :related.
  Examples:

class ExpenseEntryResource < JSONAPI::Resource
  has_one :currency, class_name: 'Currency', foreign_key: 'currency_code'
end

see https://github.com/cerebris/jsonapi-resources/blob/master/lib/jsonapi/relationship.rb for full code

module JSONAPI
  class Relationship
    attr_reader :foreign_key

    def initialize(name, options = {})
      @foreign_key = options[:foreign_key] ? options[:foreign_key].to_sym : nil
    end

    def belongs_to?
      false
    end

    class ToOne < Relationship
      attr_reader :foreign_key_on

      def initialize(name, options = {})
        super
        @foreign_key ||= "#{name}_id".to_sym
        @foreign_key_on = options.fetch(:foreign_key_on, :self)
      end

      def belongs_to?
        foreign_key_on == :self
      end

    class ToMany < Relationship
      def initialize(name, options = {})
        super
        @foreign_key ||= "#{name.to_s.singularize}_ids".to_sym
      end
    end
  end
end

Customizing base records for finder methods

If you need to change the base records on which find and find_by_key operate, you can override the records method on the resource class.
For example to allow a user to only retrieve his own expense_entries you can customize self.records:

  def self.records(options = {})
   context = options[:context]
   context[:current_user].expense_entries
 end

When you create a relationship, a method is created to fetch record(s) for that relationship, using the relation name
for the relationship.

  has_one :currency
 # def record_for_currency
 #   relation_name = relationship.relation_name(context: @context)
 #   records_for(relation_name)
 # end

[NullVoxPopuli]

So, I'm just trying to get a handle on how all this existing reflection code works.
@bf4, can you look over this? I want to make sure I know what's happening.

It looks like the most problematic area is in reflection.rb#build_association

      def build_association(subject, parent_serializer_options)

        # evaluates the relationship
        # - this is only needed if we want to use 'includes', otherwise, we only need ids.
        association_value = value(subject)
        reflection_options = options.dup

        # we only need a serializer if we are using includes
        # without includes, all we need are the id and type
        serializer_class = subject.class.serializer_for(association_value, reflection_options)
        reflection_options[:include_data] = @_include_data

        # this would cover the case of includes not being used for this association
        if serializer_class
          begin
            serializer = serializer_class.new(
              association_value,
              serializer_options(subject, parent_serializer_options, reflection_options)
            )
          rescue ActiveModel::Serializer::CollectionSerializer::NoSerializerError
            reflection_options[:virtual_value] = association_value.try(:as_json) || association_value
          end
        elsif !association_value.nil? && !association_value.instance_of?(Object)

          # would this just be a hash of id / type?
          # maybe the relationship objects should have id/type accessors?
          # has many would just have an array of ids?
          reflection_options[:virtual_value] = association_value
        end

        # this is just for rendering
        Association.new(name, serializer, reflection_options, @_links, @_meta)
      end

if I understand things correctly, it's really just the value method and how the association/reflection is represented that needs to change?

[richmolj]

@NullVoxPopuli I know @bf4 knows this area better than I do, but wanted to address what I think one key problematic area is, and how it relates to #1720:

# evaluates the relationship
# - this is only needed if we want to use 'includes', otherwise, we only need ids.
association_value = value(subject)
reflection_options = options.dup

# we only need a serializer if we are using includes
# without includes, all we need are the id and type

The issue I see here is sometimes you want a link, not id/type combo. In this case:

• We need to fire #value to A) see if we want id/type in the response (include_data) and B) get the actual links data.
• This will run the association block
• If you have an in-line association, you're now firing the unnecessary SQL again.

In code:

belongs_to :author do
  link :related do
    href "/foo/bar"
  end

  # we only want to run this code if :author is included, but instead it
  # has to run in any case to get the link href above
  Author.first || Author.default
end

I guess what we could do is maybe have support for looking up custom types / polymorphic types via ActiveRecord if that is available, and then also provide a way to override the serialized type name per relationship in the serializer?

Unfortunately I think this would cause the same inline-association issue. Just for the sake of argument, let's say the solution was a type line, you'd have something like this:

belongs_to :author do
  type 'users'

  Author.first || Author.default
end

In order to get the type value, you need to run the block, which is going to fire the unnecessary SQL again.

This is the why #1720 changes inline-associations to be lazily evaluated by wrapping the data loading in a proc:

belongs_to :author do
  type 'users'

  # we can now conditionally/lazily fire this code
  load_data { Author.first || Author.default }
end

[NullVoxPopuli]

@beauby do your serializers account for this? or would this be a feature of jsonapi-rails?

[beauby]

@NullVoxPopuli They do as long as a link is defined for the corresponding relationship.

[NullVoxPopuli]

Closing for now.

ActiveRecord performance should be addressed in a more up to date branch (this branch just adds some in-code docs)