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

Included resources are not eagerly fetched for #get_related_resources calls (v0.9.0) #1163

Closed
4 of 7 tasks
hstrowd opened this issue Jun 5, 2018 · 0 comments · Fixed by hatchloyalty/old-jsonapi-resources#1

Comments

@hstrowd
Copy link

hstrowd commented Jun 5, 2018

This issue is a (choose one):

  • Problem/bug report.
  • Feature request.
  • Request for support. Note: Please try to avoid submitting issues for support requests. Use Gitter instead.

Checklist before submitting:

  • I've searched for an existing issue.
  • I've asked my question on Gitter and have not received a satisfactory answer.
  • I've included a complete bug report template. This step helps us and allows us to see the bug without trying to reproduce the problem from your description. It helps you because you will frequently detect if it's a problem specific to your project.
  • The feature I'm asking for is compliant with the JSON:API spec.

Description

This bug applies specifically to the v0.9.0 version of the gem. Calls to the #get_related_resources action that use the includes URL parameter are resulting in N+1 queries to fetch the included records. The reason for this is because the apply_includes resource method is not being called in the relationship resource fetching logic (see the RelationshipBuilder class) as it does in the base resource fetching logic. The following bug report template provides a clear reproduction of this bug.

Bug Report Template

begin
  require 'bundler/inline'
  require 'bundler'
rescue LoadError => e
  STDERR.puts 'Bundler version 1.10 or later is required. Please update your Bundler'
  raise e
end

gemfile(true, ui: ENV['SILENT'] ? Bundler::UI::Silent.new : Bundler::UI::Shell.new) do
  source 'https://rubygems.org'

  gem 'rails', require: false
  gem 'sqlite3', platform: :mri

#   gem 'activerecord-jdbcsqlite3-adapter'
  gem 'activerecord-jdbcsqlite3-adapter',
      git: 'https://github.com/jruby/activerecord-jdbc-adapter',
      branch: 'test-rails-5',
      platform: :jruby

  # This bug is specific to v0.9.0
  gem 'jsonapi-resources', "~> 0.9.0", require: false
  # if ENV['JSONAPI_RESOURCES_PATH']
  #   gem 'jsonapi-resources', path: ENV['JSONAPI_RESOURCES_PATH'], require: false
  # else
  #   gem 'jsonapi-resources', git: 'https://github.com/cerebris/jsonapi-resources', require: false
  # end

end

# prepare active_record database
require 'active_record'

class NullLogger < Logger
  def initialize(*_args)
  end

  def add(*_args, &_block)
  end
end

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = ENV['SILENT'] ? NullLogger.new : Logger.new(STDOUT)
ActiveRecord::Migration.verbose = !ENV['SILENT']

ActiveRecord::Schema.define do
  # Add your schema here
  create_table :authors, force: true do |t|
    t.string :name
  end
  create_table :posts, force: true do |t|
    t.string :title
    t.belongs_to :author
  end
  create_table :comments, force: true do |t|
    t.string :content
    t.belongs_to :post
  end
end

# create models
class Author < ActiveRecord::Base
  has_many :posts
end
class Post < ActiveRecord::Base
  belongs_to :author
  has_many :comments
end
class Comment < ActiveRecord::Base
  belongs_to :post
end

# prepare rails app
require 'action_controller/railtie'
# require 'action_view/railtie'
require 'jsonapi-resources'

class ApplicationController < ActionController::Base
end

# prepare jsonapi resources and controllers
class AuthorsController < ApplicationController
  include JSONAPI::ActsAsResourceController
end
class PostsController < ApplicationController
  include JSONAPI::ActsAsResourceController
end
class CommentsController < ApplicationController
  include JSONAPI::ActsAsResourceController
end

class AuthorResource < JSONAPI::Resource
  attribute :name
  has_many :posts
end
class PostResource < JSONAPI::Resource
  attribute :title
  has_many :comments
end
class CommentResource < JSONAPI::Resource
  attribute :content
  has_one :post
end

class TestApp < Rails::Application
  config.root = File.dirname(__FILE__)
  config.logger = ENV['SILENT'] ? NullLogger.new : Logger.new(STDOUT)
  Rails.logger = config.logger

  secrets.secret_token = 'secret_token'
  secrets.secret_key_base = 'secret_key_base'

  config.eager_load = false
end

# initialize app
Rails.application.initialize!

JSONAPI.configure do |config|
  config.json_key_format = :underscored_key
  config.route_format = :underscored_key
end

# draw routes
Rails.application.routes.draw do
  jsonapi_resources :author, only: [:index, :create] do
    jsonapi_relationships only: :show
  end
  jsonapi_resources :post, only: [:index, :create, :show]
end

# prepare tests
require 'minitest/autorun'
require 'rack/test'

# Replace this with the code necessary to make your test fail.
class BugTest < Minitest::Test
  include Rack::Test::Methods

  def json_api_headers
    {'Accept' => JSONAPI::MEDIA_TYPE, 'CONTENT_TYPE' => JSONAPI::MEDIA_TYPE}
  end

  def test_fetch_related_resources_with_include
    author = Author.create! name: 'John Doe'
    10.times do |i|
      post = Post.create! title: "Post #{i + 1}", author: author
      Comment.create! content: "Comment #{i + 1}", post: post
    end

    get "/author/#{author.id}/posts?include=comments", nil, json_api_headers
    assert last_response.ok?
    json_response = JSON.parse(last_response.body)
    refute_nil json_response['data']
    refute_empty json_response['data']
    refute_empty json_response['data'].first
    assert author.id.to_s, json_response['data'].first['id']
    assert 'posts', json_response['data'].first['type']
    assert '11', json_response['included'].size

    puts <<-EOS
    =============================================================
    Review the query logs above and observe that the `comments`
    records were each fetch individually. Then add the following
    at line 62 of the RelationshipBuilder JR class and rerun
    the test:
    ```
    records = resource_klass.apply_includes(records, options)
    ```
    =============================================================
    EOS
  end

  private

  def app
    Rails.application
  end
end
hstrowd added a commit to hatchloyalty/old-jsonapi-resources that referenced this issue Jun 12, 2018
…ed-resources

Eager load includes for related resources (cerebris#1163)
dgeb added a commit that referenced this issue Aug 10, 2018
Fixup of #1164 - Eager load includes for related resources (#1163)
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

Successfully merging a pull request may close this issue.

1 participant