Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

performance issues with linkage(always: true) #106

Open
spencerroan opened this issue Jul 31, 2018 · 4 comments
Open

performance issues with linkage(always: true) #106

spencerroan opened this issue Jul 31, 2018 · 4 comments

Comments

@spencerroan
Copy link

spencerroan commented Jul 31, 2018

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:
#91
#100
#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
Copy link
Member

beauby commented Jul 31, 2018

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
Copy link
Author

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
Copy link

jkeen commented Mar 2, 2020

@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
Copy link
Author

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants