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

Commit 10b9c8a

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. 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 10b9c8a

File tree

4 files changed

+59
-27
lines changed

4 files changed

+59
-27
lines changed

‎features/index/pagination.feature

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -43,17 +43,21 @@ Feature: Index Pagination
4343
When I am on the index page for posts
4444
Then I should not see pagination
4545

46-
Scenario: Viewing index with pagination_total set to false
47-
Given an index configuration of:
48-
"""
49-
ActiveAdmin.register Post do
50-
index :pagination_total => false do
51-
end
46+
Scenario: Viewing index with pagination_total set to false
47+
Given an index configuration of:
48+
"""
49+
ActiveAdmin.register Post do
50+
config.per_page = 10
51+
index :pagination_total => false do
5252
end
53-
"""
54-
Given 100 posts exist
55-
When I am on the index page for posts
56-
Then I should see pagination with 4 pages
57-
Then I should see "Displaying Posts 1 - 30"
58-
And I should not see "Displaying Posts 1 - 30 of 100 in total"
53+
end
54+
"""
55+
Given 11 posts exist
56+
When I am on the index page for posts
57+
Then I should see "Displaying Posts 1 - 10"
58+
And I should not see "11 in total"
59+
And I should see the pagination "Next" link
5960

61+
When I follow "Next"
62+
Then I should see "Displaying Posts 11 - 11"
63+
And I should not see the pagination "Next" link

‎features/step_definitions/pagination_steps.rb

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,3 +5,11 @@
55
Then /^I should see pagination with (\d+) pages$/ do |count|
66
expect(page).to have_css '.pagination span.page', count: count
77
end
8+
9+
Then /^I should see the pagination "Next" link/ do
10+
expect(page).to have_css "a", text: "Next"
11+
end
12+
13+
Then /^I should not see the pagination "Next" link/ do
14+
expect(page).to_not have_css "a", text: "Next"
15+
end

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

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,17 @@ 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
102+
# count(*) to determine how many pages there should be unless
103+
# you pass in the :total_pages option. We issue a query to determine
104+
# if there is another page or not, but the limit/offset make this
105+
# query fast.
106+
offset = collection.offset(collection.current_page * @per_page.to_i).limit(1).count
107+
options[:total_pages] = collection.current_page + offset
108+
options[:right] = 0
109+
end
110+
100111
text_node paginate collection, options
101112
end
102113

@@ -117,22 +128,30 @@ def page_entries_info(options = {})
117128
entries_name = I18n.translate key, count: collection.size, default: entry_name.pluralize
118129
end
119130

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)
131+
if @display_total
132+
if collection.num_pages < 2
133+
case collection_size
134+
when 0; I18n.t("active_admin.pagination.empty", model: entries_name)
135+
when 1; I18n.t("active_admin.pagination.one", model: entry_name)
136+
else; I18n.t("active_admin.pagination.one_page", model: entries_name, n: collection.total_count)
137+
end
138+
else
139+
offset = (collection.current_page - 1) * collection.limit_value
140+
total = collection.total_count
141+
I18n.t "active_admin.pagination.multiple",
142+
model: entries_name,
143+
total: total,
144+
from: offset + 1,
145+
to: offset + collection_size
125146
end
126147
else
148+
# Do not display total count, in order to prevent a `SELECT count(*)`.
149+
# To do so we must not call `collection.num_pages`
127150
offset = (collection.current_page - 1) * collection.limit_value
128-
if @display_total
129-
total = collection.total_count
130-
I18n.t 'active_admin.pagination.multiple', model: entries_name, total: total,
131-
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
135-
end
151+
I18n.t "active_admin.pagination.multiple_without_total",
152+
model: entries_name,
153+
from: offset + 1,
154+
to: offset + collection_size
136155
end
137156
end
138157

‎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