8000 Fix pagination_total index option to prevent SELECT count(*) queries · activeadmin/activeadmin@ae7145d · GitHub
[go: up one dir, main page]
Skip to content

Commit ae7145d

Browse files
author
Gabe Durazo
committed
Fix pagination_total index option to prevent SELECT count(*) queries
The pagination_total option is supposed to prevent expensive SELECT count(*) queries from being performed when viewing a resource's index page. However, while this option was hiding the display of the total number of pages, it did not actually prevent the query from being performed. There were two reasons: In `#build_pagination`, the line: `text_node paginate collection, options` would trigger Kaminari to call `#total_pages` on the collection, which would trigger the query: ``` def paginate(scope, options = {}, &block) options[:total_pages] ||= options[:num_pages] || scope.total_pages ``` By passing in the `:total_pages` option, we short circuit this assignment and avoid the query. The downside is we do so by hardcoding in an essentially random total pages count of 100. I chose the number 100, because I wanted to ensure ellipses would appear in Kaminari's "Page 1, 2, 3, ..., Next, Last" links, no matter how many links are configured to be displayed. However, if the resource only had 2 pages worth of items, then clicking the "3" would result in an empty page. That said, without querying the resource, there's no way to know how many pages there should be. That fix was not sufficient. In `#page_entries_info`, the call `collection.num_pages` also triggered a SELECT count(*). I re-arranged the conditionals to avoid that call if `@display_total` is false, while trying to keep the behavior the same.
1 parent 7476978 commit ae7145d

File tree

3 files changed

+27
-15
lines changed

3 files changed

+27
-15
lines changed

‎features/index/pagination.feature

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@ Feature: Index Pagination
5353
"""
5454
Given 100 posts exist
5555
When I am on the index page for posts
56-
Then I should see pagination with 4 pages
5756
Then I should see "Displaying Posts 1 - 30"
5857
And I should not see "Displaying Posts 1 - 30 of 100 in total"
5958

‎lib/active_admin/views/components/paginated_collection.rb

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,15 @@ def build_pagination
9797
options = {}
9898
options[:param_name] = @param_name if @param_name
9999

100+
if !@display_total
101+
# The #paginate method in kaminari will query the resource with a count(*) to determine
102+
# how many pages there should be, unless you pass in the :total_pages option. We choose
103+
# 100 because that should be enough to induce the ellipses in "1, 2, 3, ..., next, last"
104+
# no matter how many page links are configured to be displayed. If someone is hiding the
105+
# total count, then they likely have lots of pages anyway.
106+
options[:total_pages] = 100
107+
end
108+
100109
text_node paginate collection, options
101110
end
102111

@@ -117,22 +126,25 @@ def page_entries_info(options = {})
117126
entries_name = I18n.translate key, count: collection.size, default: entry_name.pluralize
118127
end
119128

120-
if collection.num_pages < 2
121-
case collection_size
122-
when 0; I18n.t('active_admin.pagination.empty', model: entries_name)
123-
when 1; I18n.t('active_admin.pagination.one', model: entry_name)
124-
else; I18n.t('active_admin.pagination.one_page', model: entries_name, n: collection.total_count)
125-
end
126-
else
127-
offset = (collection.current_page - 1) * collection.limit_value
128-
if @display_total
129+
if @display_total
130+
if collection.num_pages < 2
131+
case collection_size
132+
when 0; I18n.t('active_admin.pagination.empty', model: entries_name)
133+
when 1; I18n.t('active_admin.pagination.one', model: entry_name)
134+
else; I18n.t('active_admin.pagination.one_page', model: entries_name, n: collection.total_count)
135+
end
136+
else
137+
offset = (collection.current_page - 1) * collection.limit_value
129138
total = collection.total_count
130139
I18n.t 'active_admin.pagination.multiple', model: entries_name, total: total,
131140
from: offset + 1, to: offset + collection_size
132-
else
133-
I18n.t 'active_admin.pagination.multiple_without_total', model: entries_name,
134-
from: offset + 1, to: offset + collection_size
135141
end
142+
else
143+
# Do not display total count, in order to prevent a `SELECT count(*)`. To do so
144+
# we must not call `collection.num_pages`
145+
offset = (collection.current_page - 1) * collection.limit_value
146+
I18n.t 'active_admin.pagination.multiple_without_total', model: entries_name,
147+
from: offset + 1, to: offset + collection_size
136148
end
137149
end
138150

‎spec/unit/views/components/paginated_collection_spec.rb

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -198,9 +198,10 @@ def paginated_collection(*args)
198198
end
199199

200200
describe "set to false" do
201-
let(:pagination) { paginated_collection(collection, pagination_total: false) }
202-
203201
it "should not show the total item counts" do
202+
expect(collection).not_to receive(:num_pages)
203+
expect(collection).not_to receive(:total_pages)
204+
pagination = paginated_collection(collection, pagination_total: false)
204205
info = pagination.find_by_class('pagination_information').first.content.gsub('&nbsp;',' ')
205206
expect(info).to eq "Displaying posts <b>1 - 30</b>"
206207
end

0 commit comments

Comments
 (0)
0