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 broken count statements in Active Record 8.0 #655

Merged
merged 1 commit into from
Nov 30, 2024

Conversation

jukra
Copy link
Contributor

@jukra jukra commented Nov 27, 2024

This fixes the issue discussed here #654 (comment), #652 (comment) and #653 (comment)

I think this caused the second issue with Rails 8.0 and the AS statement with counts not getting wrapped correctly: rails/rails@ba468db

Previously we got the Arel::Nodes for column_name in execute_simple_calculation. With Rails 8 we actually get just the string "article_translations_en"."title" AS __mobility_title_en__, (https://github.com/rails/rails/blob/8-0-stable/activerecord/lib/active_record/relation/calculations.rb#L445)

This is why the count in Countable module does not get called at all (https://github.com/shioyama/mobility/blob/master/lib/mobility/plugins/arel.rb#L32)

Not particularly pretty, but it works because

  • Count queries don't include ORDER BY
    • The problematic counting behavior in Rails 8 is bypassed when there's no alias
  • Select queries that need proper column aliases do include ORDER BY

I ran the specs locally for also Rails 8.0 and they all passed with this change.

Maybe @shioyama you have a better way in mind on how to fix it?

@jukra jukra mentioned this pull request Nov 27, 2024
@jukra jukra changed the title Fix broken count statements in ActiveRecord 8.0 Fix broken count statements in Active Record 8.0 Nov 27, 2024
@n-rodriguez
Copy link
Contributor

@jukra CI is all green with your patch 👍 https://github.com/n-rodriguez/mobility/actions/runs/12072169189

@shioyama LGTM

@shioyama
Copy link
Owner

Thank you! ❤️

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.

3 participants