From 9dd663e1741deaa5c65273dcc0ae3ea3db1b56d2 Mon Sep 17 00:00:00 2001 From: Rafael Sales Date: Mon, 2 Apr 2018 22:48:36 -0300 Subject: [PATCH 1/5] Remove duplicated delegation `total_pages` is listed twice. --- lib/active_admin/resource_controller/decorators.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/active_admin/resource_controller/decorators.rb b/lib/active_admin/resource_controller/decorators.rb index ae6c99e4d3a..2ee87d90a37 100644 --- a/lib/active_admin/resource_controller/decorators.rb +++ b/lib/active_admin/resource_controller/decorators.rb @@ -60,7 +60,7 @@ def self.wrap(decorator) def self.wrap!(parent, name) ::Class.new parent do delegate :reorder, :page, :current_page, :total_pages, :limit_value, - :total_count, :total_pages, :offset, :to_key, :group_values, + :total_count, :offset, :to_key, :group_values, :except, :find_each, :ransack, to: :object define_singleton_method(:name) { name } From b73443590d94708b7b319ca5565d16ff7d0f9e70 Mon Sep 17 00:00:00 2001 From: Rafael Sales Date: Tue, 3 Apr 2018 01:58:29 -0300 Subject: [PATCH 2/5] 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 --- lib/active_admin/views/components/paginated_collection.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/active_admin/views/components/paginated_collection.rb b/lib/active_admin/views/components/paginated_collection.rb index 27238f5cab4..32ddf7add91 100644 --- a/lib/active_admin/views/components/paginated_collection.rb +++ b/lib/active_admin/views/components/paginated_collection.rb @@ -102,7 +102,9 @@ def build_pagination # you pass in the :total_pages option. We issue a query to determine # if there is another page or not, but the limit/offset make this # query fast. - offset = collection.offset(collection.current_page * collection.limit_value).limit(1).count + offset_scope = collection.offset(collection.current_page * collection.limit_value) + offset_scope = offset_scope.reorder(nil) if offset_scope.respond_to?(:reorder) + offset = offset_scope.limit(1).count options[:total_pages] = collection.current_page + offset options[:right] = 0 end From 58b10165de4aab168d758362dfec3e1a179ab9f2 Mon Sep 17 00:00:00 2001 From: Matias Grunberg Date: Tue, 27 Aug 2024 16:59:47 -0300 Subject: [PATCH 3/5] reimplement reorder using except --- lib/active_admin/views/components/paginated_collection.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/active_admin/views/components/paginated_collection.rb b/lib/active_admin/views/components/paginated_collection.rb index 80209af010d..cb94606e390 100644 --- a/lib/active_admin/views/components/paginated_collection.rb +++ b/lib/active_admin/views/components/paginated_collection.rb @@ -103,7 +103,8 @@ def build_pagination # if there is another page or not, but the limit/offset make this # query fast. offset_scope = @collection.offset(@collection.current_page * @collection.limit_value) - offset_scope = offset_scope.reorder(nil) if offset_scope.respond_to?(:reorder) + # Support array collections. Kaminari::PaginatableArray does not respond to except + offset_scope = offset_scope.except(:order) if offset_scope.respond_to?(:except) offset = offset_scope.limit(1).count options[:total_pages] = @collection.current_page + offset options[:right] = 0 From 948f71fd4a51147283ff942f01a6929c5d11dd7f Mon Sep 17 00:00:00 2001 From: Matias Grunberg Date: Tue, 27 Aug 2024 17:00:27 -0300 Subject: [PATCH 4/5] add specs about ensure count query does not include ORDER BY clause --- .../perform_database_query_matcher.rb | 4 +- .../components/paginated_collection_spec.rb | 41 +++++++++++++++---- 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/spec/support/matchers/perform_database_query_matcher.rb b/spec/support/matchers/perform_database_query_matcher.rb index 7fc0b79d16c..9f44e364103 100644 --- a/spec/support/matchers/perform_database_query_matcher.rb +++ b/spec/support/matchers/perform_database_query_matcher.rb @@ -2,10 +2,12 @@ RSpec::Matchers.define :perform_database_query do |query| match do |block| + query_regexp = query.is_a?(Regexp) ? query : Regexp.new(Regexp.escape(query)) + @match = nil callback = lambda do |_name, _started, _finished, _unique_id, payload| - @match = Regexp.new(Regexp.escape(query)).match?(payload[:sql]) + @match = query_regexp.match?(payload[:sql]) end ActiveSupport::Notifications.subscribed(callback, "sql.active_record", &block) diff --git a/spec/unit/views/components/paginated_collection_spec.rb b/spec/unit/views/components/paginated_collection_spec.rb index 81ac65a8f91..b4e36669d85 100644 --- a/spec/unit/views/components/paginated_collection_spec.rb +++ b/spec/unit/views/components/paginated_collection_spec.rb @@ -229,16 +229,43 @@ def paginated_collection(*args) end end - it "makes no expensive COUNT queries when pagination_total is false" do - undecorated_collection = Post.all.page(1).per(30) + describe "when pagination_total is false" do + it "makes no expensive COUNT queries" do + undecorated_collection = Post.all.page(1).per(30) + + expect { paginated_collection(undecorated_collection, pagination_total: false) } + .not_to perform_database_query("SELECT COUNT(*) FROM \"posts\"") + + decorated_collection = controller_with_decorator("index", PostDecorator).apply_collection_decorator(undecorated_collection.reset) + + expect { paginated_collection(decorated_collection, pagination_total: false) } + .not_to perform_database_query("SELECT COUNT(*) FROM \"posts\"") + end + + it "makes a performant COUNT query to figure out if we are on the last page" do + # "SELECT COUNT(*) FROM (SELECT 1". Let's make sure the subquery has LIMIT and OFFSET. It shouldn't have ORDER BY + count_query = %r{SELECT COUNT\(\*\) FROM \(SELECT 1 .*FROM "posts" (?=.*OFFSET \?)(?=.*LIMIT \?)(?!.*ORDER BY)} - expect { paginated_collection(undecorated_collection, pagination_total: false) } - .not_to perform_database_query("SELECT COUNT(*) FROM \"posts\"") + undecorated_collection = Post.all.page(1).per(30) - decorated_collection = controller_with_decorator("index", PostDecorator).apply_collection_decorator(undecorated_collection) + expect { paginated_collection(undecorated_collection, pagination_total: false) } + .to perform_database_query(count_query) - expect { paginated_collection(decorated_collection, pagination_total: false) } - .not_to perform_database_query("SELECT COUNT(*) FROM \"posts\"") + undecorated_sorted_collection = undecorated_collection.reset.order(id: :desc) + + expect { paginated_collection(undecorated_sorted_collection, pagination_total: false) } + .to perform_database_query(count_query) + + decorated_collection = controller_with_decorator("index", PostDecorator).apply_collection_decorator(undecorated_collection.reset) + + expect { paginated_collection(decorated_collection, pagination_total: false) } + .to perform_database_query(count_query) + + decorated_sorted_collection = controller_with_decorator("index", PostDecorator).apply_collection_decorator(undecorated_sorted_collection.reset) + + expect { paginated_collection(decorated_sorted_collection, pagination_total: false) } + .to perform_database_query(count_query) + end end it "makes no COUNT queries to figure out the last element of each page" do From 8d8ec07e392257d641bece50957965322e64a534 Mon Sep 17 00:00:00 2001 From: Matias Grunberg Date: Tue, 27 Aug 2024 17:18:09 -0300 Subject: [PATCH 5/5] exclude also select because based on https://github.com/activeadmin/activeadmin/pull/7489\#issuecomment-1554197081 --- lib/active_admin/views/components/paginated_collection.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/active_admin/views/components/paginated_collection.rb b/lib/active_admin/views/components/paginated_collection.rb index cb94606e390..3169d537628 100644 --- a/lib/active_admin/views/components/paginated_collection.rb +++ b/lib/active_admin/views/components/paginated_collection.rb @@ -104,7 +104,7 @@ def build_pagination # query fast. offset_scope = @collection.offset(@collection.current_page * @collection.limit_value) # Support array collections. Kaminari::PaginatableArray does not respond to except - offset_scope = offset_scope.except(:order) if offset_scope.respond_to?(:except) + offset_scope = offset_scope.except(:select, :order) if offset_scope.respond_to?(:except) offset = offset_scope.limit(1).count options[:total_pages] = @collection.current_page + offset options[:right] = 0