jsonapi-rb / jsonapi-serializable

Conveniently build and efficiently render JSON API resources.
http://jsonapi-rb.org
MIT License
45 stars 35 forks source link

performance issues with linkage(always: true) #106

Open spencerroan opened 6 years ago

spencerroan commented 6 years ago

In our serializers we make liberal use of linkage(always: true). This fully renders the relationship including any calls out to new linkages. This is a performance issue and generates n+1 queries for objects we were not planning on loading. I'm going to build something that looks up what the include tree should be based on what we always include and what is asked for.

For instance:

class SerializableBar < JSONAPI::Serializable::Resource
  type 'bars'
  has_many :foos do
    linkage always: true
  end
end

class SerializableFoo < JSONAPI::Serializable::Resource
  type 'foos'
  # I do not expect any of this to be run except what is used to generate id/type info for the linkage.
  has_many :quxes do
    linkage always: true
  end
end

When rendering a Bar without including quxes, I don't expect the has_many quxes to be serialized.

In the short term I've made this monkey patch to avoid loading extra layers of relationships:

class JSONAPI::Serializable::Relationship
  def belongs_to_linkage(association)
    reflection = @object.class.reflect_on_association(association)
    foreign_key = @object.public_send(reflection.foreign_key)
    foreign_key && {
      id: foreign_key,
      type: reflection.plural_name
    }
  end

  def has_one_linkage(association, receiver = @object)
    associated = receiver.public_send(association)
    associated ? linkage_for(associated) : nil
  end

  def has_many_linkage(association)
    associated = @object.public_send(association)
    associated.map { |assoc| linkage_for(assoc) }
  end

  def linkage_for(associated)
    {
      id: associated.id,
      type: associated.class.table_name
    }
  end
end

I'll explore ways of not monkey patching like creating a sister to linkage() that does more of what I need performance wise. This seems related to: https://github.com/jsonapi-rb/jsonapi-serializable/issues/91 https://github.com/jsonapi-rb/jsonapi-serializable/issues/100 https://github.com/jsonapi-rb/jsonapi-serializable/issues/49

I guess an additional issue I have is that the related object is loaded even on a belongs_to. I'd assume the reflection of the relationship could generate id and type without loading the object, but it does make for more conditional/brittle code.

beauby commented 6 years ago

The idea here is that the serialization layer is not responsible for the way you fetch your data. Note that if you are using ActiveRecord, you can simply do render jsonapi: Bars.where(:foo).includes(JSONAPI::IncludeDirective.new(params[:include]).to_hash) to automatically avoid n+1 queries.

spencerroan commented 6 years ago

True, Thanks for the quick feedback! However, that is not the issue I am having (though I do need to do that as well). I think I did a poor job of explaining. Let me try again.

If I have A has_many B and B has_many C And I do a simple GET on an A with no includes through the API, Given the following Serializers with linkage always: true I do not expect to have to include C!

class SerializableA
  has_many :a do
    linkage always: true
  end
end
class SerializableB
  has_many :c do
    # I do not expect this to be called, but it is!
    linkage always: true
  end
end
class SerializableC
  meta do
    I expect this not to be called, but it is!
  end
end

This gem loads too much data in my opinion.

When I go a get on an A I expect to need to include B, but not include(b: :c)!

Further more I expect that if A belongs_to B that I should not have to include B because the A has enough information to generate the linkage. Now If I did an include through the api I would expect to do an include in the ActiveRecord query. I am doing linkage always:true and this gem treats it as if I have done an include through the api. It serializes the entire relationship when I just want the linkage.

jkeen commented 4 years ago

@spencerroan It's been a while, but any more developments on this issue?

I'm using graphiti (which uses jsonapi-serializable) and am running into an n+1 when including linkages. I'm looking at your monkey patch, but can't find the place where it's patching. Do you have to call anything extra when defining your relationships?

spencerroan commented 4 years ago

@jkeen

Drop the above code somewhere that will get loaded. Then in the serializer:

class SerializableWidget < JSONAPI::Serializable::Resource
  ## whatever is normal

  has_many(:foos) do
    # call the monkey patched method for the linkage in place of # linkage always: true
    has_many_linkage(:foos, always: true)
    link(:related) do 
      ## normal
    end
  end

  has_one :bar do
    # call the monkey patched method for the linkage in place of # linkage always: true
    has_one_linkage(:bar, always: true)
      link :related do
        ## normal
      end
  end

  belongs_to  :baz do
    # call the monkey patched method for the linkage in place of # linkage always: true    
    belongs_to_linkage(:baz, always: true)
    link :related do
      ## normal
    end
  end

Good Luck!