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.

Sign up for GitHub

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

[WIP] fix belongs_to causing unnecessary db hits #1750

Closed
wants to merge 1 commit into from
Closed

[WIP] fix belongs_to causing unnecessary db hits #1750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ group :bench do
end

group :test do
gem 'pry-byebug'
gem 'sqlite3', platform: (@windows_platforms + [:ruby])
gem 'activerecord-jdbcsqlite3-adapter', platform: :jruby
gem 'codeclimate-test-reporter', require: false
Expand Down
4 changes: 4 additions & 0 deletions lib/active_model/serializer/associations.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,14 @@ def associations(include_tree = DEFAULT_INCLUDE_TREE)
return unless object

Enumerator.new do |y|
# _reflections is an AMS::BelongsToReflection or
# an AMS::HasManyReflection
self.class._reflections.each do |reflection|
next if reflection.excluded?(self)
key = reflection.options.fetch(:key, reflection.name)
next unless include_tree.key?(key)
# until this is called, the reflection / association
# is _not_ evaluated
y.yield reflection.build_association(self, instance_options)
end
end
Expand Down
14 changes: 14 additions & 0 deletions lib/active_model/serializer/reflection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ def include_data(value = true)
:nil
end

# This gets the value of the reflection.
# this needs to be avoided if we have a belongs_to relationship
# because we only care about the id and type, and not about the object.
# This causes n+1 queries unnecessarily
#
# @param serializer [ActiveModel::Serializer]
# @yield [ActiveModel::Serializer]
# @return [:nil, associated resource or resource collection]
Expand All @@ -70,6 +75,9 @@ def include_data(value = true)
# end
# end
def value(serializer)
require 'pry-byebug'
binding.pry

@object = serializer.object
@scope = serializer.scope

Expand All @@ -78,9 +86,15 @@ def value(serializer)
if block_value != :nil
Copy link
Member

@bf4 bf4 May 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

block_value
elsif @_include_data
# this is what needs to be avoided in the case of belongs_to.
# should an id and type hash just be returned?
# maybe how associations work in general needs to be re-looked at
serializer.read_attribute_for_serialization(name)
end
else
# this is what needs to be avoided in the case of belongs_to.
# should an id and type hash just be returned?
# maybe how associations work in general needs to be re-looked at
serializer.read_attribute_for_serialization(name)
end
end
Expand Down
5 changes: 4 additions & 1 deletion lib/active_model_serializers/adapter/json_api.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,10 @@ def resource_object_for(serializer)
# }.reject! {|_,v| v.nil? }
def relationships_for(serializer, requested_associations)
include_tree = ActiveModel::Serializer::IncludeTree.from_include_args(requested_associations)
serializer.associations(include_tree).each_with_object({}) do |association, hash|
# the association object evaluates the relationship, which we
# don't want it to do
included_associations = serializer.associations(include_tree)
included_associations.each_with_object({}) do |association, hash|
hash[association.key] = Relationship.new(
serializer,
association.serializer,
Expand Down
15 changes: 15 additions & 0 deletions test/serializers/associations_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,21 @@ def test_associations_custom_keys
assert expected_association_keys.include? :site
end

class BelongsToTestPostSerializer < ActiveModel::Serializer
belongs_to :blog
end

test 'belongs_to should not load record' do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test courtesy of @ssendev

post = Post.new
post.blog_id = 'blog'
post.blog = 'should not touch this'

actual = serializable(post, adapter: :json_api, serializer: BelongsToTestPostSerializer).as_json
expected = { data: { id: 'post', type: 'posts', relationships: { blog: { data: { id: 'blog', type: 'blogs' } } } } }

assert_equal expected, actual
end

class InlineAssociationTestPostSerializer < ActiveModel::Serializer
has_many :comments
has_many :comments, key: :last_comments do
Expand Down