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

JBuilder 2.11.3 introduces an extra DB query when rendering collection partials #514

Closed
cupakromer opened this issue Nov 16, 2021 · 5 comments · Fixed by #524
Closed

JBuilder 2.11.3 introduces an extra DB query when rendering collection partials #514

cupakromer opened this issue Nov 16, 2021 · 5 comments · Fixed by #524

Comments

@cupakromer
Copy link

Ruby Version: 2.7.4
Rails Version: 5.2.6
JBuilder Version: 2.11.3

This was introduced by #501 with the options[:collection].empty? call:

if options.key?(:collection) && (options[:collection].nil? || options[:collection].empty?)

When the collection object is an Active Record association relation/collection the empty? call introduces a SELECT 1 query statement from this code in ActiveRecord::Relation#empty?:

    # Returns true if there are no records.
    def empty?
      return @records.empty? if loaded?
      !exists?
    end

This SQL statement occurs for the case when the collection is not yet loaded; which is very common for the main collection on index actions. The exists? code is run which is defined in ActiveRecord::FinderMethods#exists?. This causes SQL to be executed via connection.select_one.

The extra SQL statement is an unnecessary DB trip across the network when there is data.

For comparison, in JBuild 2.11.2 the same process passes the collection to array! which ends up loading the full collection without this select_one call.

This is easily reproduced with a controller action such as:

# app/controllers/widgets_controller.rb
class WidgetsController < ApplicationController
  # GET /widgets or /widgets.json
  def index
    @widgets = Widget.all
  end
end
# app/views/widgets/index.json.jbuilder
json.array! @widgets, partial: "widgets/widget", as: :widget
# app/views/widgets/_widget.json.jbuilder
json.extract! widget, :id, :name, :created_at, :updated_at
json.url widget_url(widget, format: :json)

When hit without data the Rails default logger shows:

Started GET "/widgets.json" for 127.0.0.1 at 2021-11-16 17:02:20 -0500
Processing by WidgetsController#index as JSON
  Rendering widgets/index.json.jbuilder
  Widget Exists (1.3ms)  SELECT  1 AS one FROM "widgets" LIMIT $1  [["LIMIT", 1]]
  ↳ app/views/widgets/index.json.jbuilder:1
  Rendered widgets/index.json.jbuilder (3.1ms)
Completed 200 OK in 7ms (Views: 4.4ms | ActiveRecord: 1.3ms)

When run with data, the SELECT 1 and the SELECT * are both present:

Started GET "/widgets.json" for 127.0.0.1 at 2021-11-16 17:04:04 -0500
Processing by WidgetsController#index as JSON
  Rendering widgets/index.json.jbuilder
  Widget Exists (0.3ms)  SELECT  1 AS one FROM "widgets" LIMIT $1  [["LIMIT", 1]]
  ↳ app/views/widgets/index.json.jbuilder:1
  Widget Load (0.2ms)  SELECT "widgets".* FROM "widgets"
  ↳ app/views/widgets/index.json.jbuilder:1
  Rendered widgets/_widget.json.jbuilder (0.5ms)
  Rendered widgets/_widget.json.jbuilder (0.2ms)
  Rendered widgets/_widget.json.jbuilder (0.2ms)
  Rendered widgets/index.json.jbuilder (4.9ms)
Completed 200 OK in 9ms (Views: 6.7ms | ActiveRecord: 0.5ms)
@cristianzanfir
Copy link

Also experiencing the same issue with the additional SELECT COUNT(*). The .empty? method call triggers this query.
Do you think there is a way to improve this? Cheers 🍻

@tf
Copy link
Contributor

tf commented Nov 22, 2021

Also before, the collection option worked for anything implementing Enumerable. I now see failures, since the empty? method is undefined.

@lacco
Copy link

lacco commented Nov 24, 2021

Same error for me, 2.11.2 works, but 2.11.3 is broken.

As pointed out by @tf , just implementing Enumerable was enough.
With the recent change, empty? method needs to be implemented as well.

tf added a commit to tf/pageflow that referenced this issue Nov 24, 2021
Turn page_types into array since JBuilder
2.11.3 (rails/jbuilder#501) requires
collection to respond to `empty?`, which `Enumerable` does not
provide. See also rails/jbuilder#514

REDMINE-19357
tf added a commit to tf/pageflow that referenced this issue Nov 24, 2021
Turn enumerables into arrays since JBuilder
2.11.3 (rails/jbuilder#501) requires
collection to respond to `empty?`, which `Enumerable` does not
provide. See also rails/jbuilder#514

REDMINE-19357
@dhh
Copy link
Member

dhh commented Dec 16, 2021

cc @yuki24

@yuki24
Copy link
Contributor

yuki24 commented Dec 17, 2021

#524 should address this issue.

@dhh dhh closed this as completed in #524 Dec 17, 2021
UsmanMuhammad pushed a commit to UsmanMuhammad/pageflow that referenced this issue Aug 12, 2022
Turn enumerables into arrays since JBuilder
2.11.3 (rails/jbuilder#501) requires
collection to respond to `empty?`, which `Enumerable` does not
provide. See also rails/jbuilder#514

REDMINE-19357
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.

6 participants