Avoid requesting sorted sql query for paginated collection.

[jan-bw]

Hi,

The #first method used in the PaginatedCollection#page_entries_info (L126, L128) can cause performance issues on the bigger tables because it's building an SQL query with ORDER BY in it.

Using the collection[0] instead is going to take the first element from the paginated collection.

I was considering using take, but it works only for rails associations (won't pass the specs; won't work with PaginatedArray). Also considered using collection.model_name with same effect. Having that in mind I think the collection[0] might be a way to go.

[deivid-rodriguez]

Sorry, it's been a while since I don't come back to activeadmin stuff. Could you show an example of these performance issues? If I understand correctly, pagination info is displayed after showing a page of a collection so whatever sorting needed has already happened at this point no?

[jan-bw]

It looks to me that when using paginated_collection(collection) { yield } (with block) the pagination code will kick in before rendering the collection. That would mean the collection can be still not yet loaded (i.e. being the lazy ActiveRecord::AssociationRelation).
Then in case of an unordered collection (to avoid costly ordering) this will be the first place to make a query with an ORDER BY ... ASC in it.

For example this collection load will be quick:

# Car.where(brand_id: 10).limit(10).offset(100)
Code Load (55.2ms) SELECT "cars"."id" FROM "cars" WHERE "cars"."brand_id" = 10 LIMIT 20 OFFSET 100;

And at the same time this might be slow (when you have 7_000_000 cars for example):

# Car.where(brand_id: 10).limit(10).offset(100).first
Code Load (16349.1ms)  SELECT "cars"."id" FROM "cars" WHERE "cars"."brand_id" = 10 ORDER BY "cars"."id" ASC LIMIT 1 OFFSET 100

[deivid-rodriguez]

That's right, but won't it be eventually kicked anyways when you display the page of the collection?

[jan-bw]

I am not sure if I got you correctly now :) the select query will happen anyways, but in case it's triggered after the #first is called on collection then both queried get executed... which is causing the issues

[deivid-rodriguez]

Oh, I see, you're right. How about this alternative patch?

diff --git a/lib/active_admin/views/components/paginated_collection.rb b/lib/active_admin/views/components/paginated_collection.rb
index 07128088..8875f32e 100644
--- a/lib/active_admin/views/components/paginated_collection.rb
+++ b/lib/active_admin/views/components/paginated_collection.rb
@@ -123,10 +123,8 @@ module ActiveAdmin
           entry_name = I18n.t "active_admin.pagination.entry", count: 1, default: "entry"
           entries_name = I18n.t "active_admin.pagination.entry", count: 2, default: "entries"
         else
-          key = "activerecord.models." + collection.first.class.model_name.i18n_key.to_s
-
-          entry_name = I18n.translate key, count: 1, default: collection.first.class.name.underscore.sub("_", " ")
-          entries_name = I18n.translate key, count: collection.size, default: entry_name.pluralize
+          entry_name = active_admin_config.resource_label
+          entries_name = active_admin_config.plural_resource_label(count: collection_size)
         end
 
         if @display_total

Does it work for you? It does break some unit specs, but it seems ok, and it feels nicer to me, because it reuses the same internationalization logic used elsewhere.

[jan-bw]

Unfortunately this doesn't seem to work for the relations. The resource name will be taken from the main resource instead of the associated. I.e. it will be "users" instead of "cars"

ActiveAdmin.register User do
  
  show do
    paginated_collection(resource.cars.page(0).per(50), pagination_total: false) { ... } 
  end
end

I think the collection helper would have to be changed in order to get the proper collection name. E.g.

# frozen_string_literal: true

require "active_admin/resource/naming"

module ActiveAdmin
  module Helpers
    module Collection
      def collection_resource_name
        @collection_resource_name ||= begin
          if collection.respond_to?(:model_name)
            ActiveAdmin::Resource::Name.new collection.model_name
          else
            ActiveAdmin::Resource::Name.new collection[0].class
          end
        end
      end

      # Returns the name to call this resource such as "Bank Account"
      def collection_resource_label
        collection_resource_name.translate count: 1,
                                           default: collection_resource_name.to_s.gsub("::", " ").titleize
      end

      # Returns the plural version of this resource such as "Bank Accounts"
      def plural_collection_resource_label(options = {})
        defaults = { count: ActiveAdmin::Helpers::I18n::PLURAL_MANY_COUNT,
                     default: collection_resource_label.pluralize.titleize }
        collection_resource_name.translate defaults.merge options
      end
...

that's awfully a lot of changes :D

we could go more inline e.g.

diff --git a/lib/active_admin/views/components/paginated_collection.rb b/lib/active_admin/view
s/components/paginated_collection.rb
index c34e690c..a80150b3 100644
--- a/lib/active_admin/views/components/paginated_collection.rb
+++ b/lib/active_admin/views/components/paginated_collection.rb
@@ -123,10 +123,12 @@ module ActiveAdmin
           entry_name = I18n.t "active_admin.pagination.entry", count: 1, default: "entry"
           entries_name = I18n.t "active_admin.pagination.entry", count: 2, default: "entries"
         else
-          key = "activerecord.models." + collection[0].class.model_name.i18n_key.to_s
-
-          entry_name = I18n.translate key, count: 1, default: collection[0].class.name.underscore.sub("_", " ")
-          entries_name = I18n.translate key, count: collection.size, default: entry_name.pluralize
+          resource_name = ActiveAdmin::Resource::Name.new(collection.try(:model_name) || collection[0])
+          entry_name = collection_resource_name.translate count: 1, default: collection_resource_name.to_s.gsub("::", " ").titleize
+          entries_name = collection_resource_name.translate({
+                                                              count: ActiveAdmin::Helpers::I18n::PLURAL_MANY_COUNT,
+                                                              default: collection_resource_label.pluralize.titleize
+                                                            })
         end

         if @display_total

[deivid-rodriguez]

I see, in that case I guess the current patch is ok. It'd be nice to add a spec for it though, so that it's not reverted in the future in the name of a more readable style. Perhaps we can leverage #7232 to make sure that the query we want to avoid does not happen.

[jan-bw]

What would you say about yet another approach:

diff --git a/lib/active_admin/views/components/paginated_collection.rb b/lib/active_admin/views/components/paginated_collection.rb
index 07128088..98e089b5 100644
--- a/lib/active_admin/views/components/paginated_collection.rb
+++ b/lib/active_admin/views/components/paginated_collection.rb
@@ -123,9 +123,10 @@ module ActiveAdmin
           entry_name = I18n.t "active_admin.pagination.entry", count: 1, default: "entry"
           entries_name = I18n.t "active_admin.pagination.entry", count: 2, default: "entries"
         else
-          key = "activerecord.models." + collection.first.class.model_name.i18n_key.to_s
-
-          entry_name = I18n.translate key, count: 1, default: collection.first.class.name.underscore.sub("_", " ")
+          model_name = collection.respond_to?(:model_name) ? collection.model_name.i18n_key.to_s : collection.first.class.name.underscore
+          key = "activerecord.models.#{model_name}"
+
+          entry_name = I18n.translate key, count: 1, default: model_name.sub("_", " ")
           entries_name = I18n.translate key, count: collection.size, default: entry_name.pluralize
         end

(END)

this way there would be no query for ActiveRecord relations at all 🤔

[deivid-rodriguez]

@jan-bw Sorry for the delay, that seems fine to me.

[jan-bw]

@deivid-rodriguez when writing specs I have realised that the issue got fixed by this change ccb3925

the ActiveRecord_Relation will get loaded when the collection.length gets called on it and therefor collection.first won't make a db call anymore

[deivid-rodriguez]

That's great, thank you!