Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix records_count when using group by #49

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

smridge
Copy link

@smridge smridge commented Dec 3, 2022

Changes

  • Fixed records_count to handle hashes (typically returned with group). As it stands, only ActiveRecord_Relation are handled. This fix ensures an integer is always returned.
  • Changed count to size to avoid any misleadings / documentation collisions.
  • Fixes Extending unscope? #8

Detail

Currently, when using select to load custom attributes that are grouped on the Model, the count breaks when evaluating @page.last?. Specifically, this breaks on while residual > 0 with the error of no implicit conversion of Integer into Hash.

After looking at the method-i-count docs

If count is used with Relation#group for multiple columns, it returns a Hash whose keys are an array containing the individual values of each column and the value of each key would be the count.

This explains the quirk.

Example usage:

# in model
scope :with_custom_selections, lambda {
  joins(:bars).select(
    <<~SQL.squish
      foos.*,
      count(bars.id) as bars_count
    SQL
  ).group(:id)
}

# in controller
@foos = set_page_and_extract_portion_from(Foo.with_custom_selections)

# in view
<% if @page.last? %>
...

@smridge smridge mentioned this pull request Dec 5, 2022
@@ -32,7 +32,7 @@ def page_count
end

def records_count
@records_count ||= records.unscope(:limit).unscope(:offset).unscope(:select).count
@records_count ||= records.unscope(:limit).unscope(:offset).unscope(:select).to_a.size

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, this is very inefficient: imagine a model with millions of records... to_a would load them all. This feels like a hack, but should a little bit better:

    def records_count
      if @records_count.nil?
        @records_count = records.unscope(:limit).unscope(:offset).unscope(:select).count
        # records count could be a hash if query has a group by clause -> in this case records count is number of groups
        @records_count = @records_count.size if @records_count.is_a?(Hash)
      end

      @records_count
    end

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great callout! I've updated the code and PR description.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@smridge smridge force-pushed the fix-record-count-for-group-by branch from 87a39af to fce042d Compare November 2, 2023 01:33
@koddsson
Copy link

Just hit this in our codebase and we've currently got it monkey-patched. It would be great to remove that patch with these changes merged upstream 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extending unscope?
3 participants