8000 Backport Optimize count query for pagination_total: false option (#8470) · activeadmin/activeadmin@c0cff8f · GitHub
[go: up one dir, main page]
Skip to content

Commit c0cff8f

Browse files
mgrunbergdeivid-rodriguezrafaelsales
authored
Backport Optimize count query for pagination_total: false option (#8470)
* Optimize count query for `pagination_total: false` option (#6911) * Remove duplicated delegation `total_pages` is listed twice. * Remove ORDER BY from count subquery Queries like `SELECT COUNT(*) FROM (SELECT DISTINCT resources.* FROM resources ORDER BY resources.created_at DESC LIMIT 1 OFFSET 30) subquery_for_count` are too inefficient * add specs about ensure count query does not include ORDER BY clause * exclude also select because based on https://github.com/activeadmin/activeadmin/pull/7489\#issuecomment-1554197081 --------- Co-authored-by: Rafael Sales <rafaelcds@gmail.com> Co-authored-by: David Rodríguez <deivid-rodriguez> Co-authored-by: Matias Grunberg <matias@yellowspot.dev> * fix perform_database_query_matcher: make sure to keep match truthy after the first matched query --------- Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net> Co-authored-by: Rafael Sales <rafaelcds@gmail.com>
1 parent 276b0d4 commit c0cff8f

File tree

4 files changed

+42
-10
lines changed

4 files changed

+42
-10
lines changed

‎lib/active_admin/resource_controller/decorators.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ def self.wrap(decorator)
6161
def self.wrap!(parent, name)
6262
::Class.new parent do
6363
delegate :reorder, :page, :current_page, :total_pages, :limit_value,
64-
:total_count, :total_pages, :offset, :to_key, :group_values,
64+
:total_count, :offset, :to_key, :group_values,
6565
:except, :find_each, :ransack, to: :object
6666

6767
define_singleton_method(:name) { name }

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,10 @@ def build_pagination
103103
# you pass in the :total_pages option. We issue a query to determine
104104
# if there is another page or not, but the limit/offset make this
105105
# query fast.
106-
offset = collection.offset(collection.current_page * collection.limit_value).limit(1).count
106+
offset_scope = collection.offset(collection.current_page * collection.limit_value)
107+
# Support array collections. Kaminari::PaginatableArray does not respond to except
108+
offset_scope = offset_scope.except(:select, :order) if offset_scope.respond_to?(:except)
109+
offset = offset_scope.limit(1).count
107110
options[:total_pages] = collection.current_page + offset
108111
options[:right] = 0
109112
end

‎spec/support/matchers/perform_database_query_matcher.rb

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,12 @@
22

33
RSpec::Matchers.define :perform_database_query do |query|
44
match do |block|
5+
query_regexp = query.is_a?(Regexp) ? query : Regexp.new(Regexp.escape(query))
6+
57
@match = nil
68

79
callback = lambda do |_name, _started, _finished, _unique_id, payload|
8-
@match = Regexp.new(Regexp.escape(query)).match?(payload[:sql])
10+
@match ||= query_regexp.match?(payload[:sql])
911
end
1012

1113
ActiveSupport::Notifications.subscribed(callback, "sql.active_record", &block)

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

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -228,16 +228,43 @@ def paginated_collection(*args)
228228
end
229229
end
230230

231-
it "makes no expensive COUNT queries when pagination_total is false" do
232-
undecorated_collection = Post.all.page(1).per(30)
231+
describe "when pagination_total is false" do
232+
it "makes no expensive COUNT queries" do
233+
undecorated_collection = Post.all.page(1).per(30)
234+
235+
expect { paginated_collection(undecorated_collection, pagination_total: false) }
236+
.not_to perform_database_query("SELECT COUNT(*) FROM \"posts\"")
237+
238+
decorated_collection = controller_with_decorator("index", PostDecorator).apply_collection_decorator(undecorated_collection.reset)
239+
240+
expect { paginated_collection(decorated_collection, pagination_total: false) }
241+
.not_to perform_database_query("SELECT COUNT(*) FROM \"posts\"")
242+
end
243+
244+
it "makes a performant COUNT query to figure out if we are on the last page" do
245+
# "SELECT COUNT(*) FROM (SELECT 1". Let's make sure the subquery has LIMIT and OFFSET. It shouldn't have ORDER BY
246+
count_query = %r{SELECT COUNT\(\*\) FROM \(SELECT 1 .*FROM "posts" (?=.*OFFSET \?)(?=.*LIMIT \?)(?!.*ORDER BY)}
233247

234-
expect { paginated_collection(undecorated_collection, pagination_total: false) }
235-
.not_to perform_database_query("SELECT COUNT(*) FROM \"posts\"")
248+
undecorated_collection = Post.all.page(1).per(30)
236249

237-
decorated_collection = controller_with_decorator("index", PostDecorator).apply_collection_decorator(undecorated_collection)
250+
expect { paginated_collection(undecorated_collection, pagination_total: false) }
251+
.to perform_database_query(count_query)
238252

239-
expect { paginated_collection(decorated_collection, pagination_total: false) }
240-
.not_to perform_database_query("SELECT COUNT(*) FROM \"posts\"")
253+
undecorated_sorted_collection = undecorated_collection.reset.order(id: :desc)
254+
255+
expect { paginated_collection(undecorated_sorted_collection, pagination_total: false) }
256+
.to perform_database_query(count_query)
257+
258+
decorated_collection = controller_with_decorator("index", PostDecorator).apply_collection_decorator(undecorated_collection.reset)
259+
260+
expect { paginated_collection(decorated_collection, pagination_total: false) }
261+
.to perform_database_query(count_query)
262+
263+
decorated_sorted_collection = controller_with_decorator("index", PostDecorator).apply_collection_decorator(undecorated_sorted_collection.reset)
264+
265+
expect { paginated_collection(decorated_sorted_collection, pagination_total: false) }
266+
.to perform_database_query(count_query)
267+
end
241268
end
242269

243270
it "makes no COUNT queries to figure out the last element of each page" do

0 commit comments

Comments
 (0)
0