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

[Bug][ActiveRecord] #count(:all) on grouped query should clear ORDER BY if it clears SELECT #51434

Open
james-em opened this issue Mar 28, 2024 · 0 comments

Comments

@james-em
Copy link

james-em commented Mar 28, 2024

Steps to reproduce

Build a query that uses .count(:all) and .group(...) with a custom .select("... AS alias") and a .order(:alias)

It seems ActiveRecord when calling .count(:all) will remove from the SELECT the alias column and everything else but the primary key, however it keeps the ORDER BY alias. To execute COUNT(...), the ORDER BY is kind of irrelevant, it should probably be removed.

ActiveRecord seems to already clear ORDER BY for some specific scenario as seen here: https://github.com/rails/rails/blob/main/activerecord/lib/active_record/relation/calculations.rb#L240 however in my scenario, the has_include(...) returns false and goes straight to perform_calculation(operation, column_name)

Here https://github.com/rails/rails/blob/main/activerecord/lib/active_record/relation/calculations.rb#L536 select values are redefined, maybe we should simply add

relation.order_values = []

Test code

Note: It ain't specific to Postgres. Error is present with SQLite too.

# frozen_string_literal: true

require "bundler/inline"

# Toggle this to test with SQLite
use_postgres = false

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", "7.1.3.2"

  gem "database_cleaner"

  if use_postgres
    gem "pg"
  else 
    gem "sqlite3"
  end
end

require "active_record"
require "minitest/autorun"
require "logger"
require "database_cleaner"

if use_postgres
  ActiveRecord::Base.establish_connection(adapter: "postgresql", host: "localhost", database: "dev", username: "dev",
  password: "dev", port: 5437)
else
  ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
end

ActiveRecord::Base.logger = Logger.new($stdout)

ActiveRecord::Schema.define do
  create_table :conversations, force: true do |t| # rubocop:disable Style/SymbolProc
    t.timestamps
  end

  create_table :messages, force: true do |t|
    t.references :conversation
    t.timestamps
  end
end

class Conversation < ActiveRecord::Base # rubocop:disable Rails/ApplicationRecord
  has_many :messages, dependent: :destroy
end

class Message < ActiveRecord::Base # rubocop:disable Rails/ApplicationRecord
  belongs_to :conversation
end

DatabaseCleaner.strategy = :truncation

class BugTest < Minitest::Test
  class SuccessException < StandardError; end

  def setup
    DatabaseCleaner.start
    Conversation.create!.tap do |conversation|
      Message.create!(conversation: conversation)
      Message.create!(conversation: conversation)
      Message.create!(conversation: conversation)
    end
  end
  
  def teardown
    DatabaseCleaner.clean
  end

  def assert_not_raise(&block)
    assert_raises(SuccessException) do
      block.call
      raise SuccessException
    end
  end

  def sql_now
    if ActiveRecord::Base.connection_db_config.adapter == "sqlite3"
      "DATE('NOW')"
    else
      "NOW()"
    end
  end

  def base_query
    Conversation
            .select(
              "conversations.*",
              "COUNT(messages.id) AS messages_count",
              "#{sql_now} AS last_activity_at"
            )
            .left_joins(:messages)
            .group(:id)
  end

  def test_it_should_work_with_aliased_order_by
    query = base_query.order("last_activity_at DESC")

    assert_not_raise { query.count(:all) }
  end

  def test_it_demonstrates_that_non_aliased_order_by_work
    query = base_query.order(Arel.sql("#{sql_now} DESC"))


    assert_not_raise { query.count(:all) }
  end

  def test_without_order_by_it_always_work
    query = base_query # .order("last_activity_at DESC")

    assert_not_raise { query.count(:all) }
  end
end

Expected behavior

This query should work

    Conversation
            .select(
              "conversations.*",
              "COUNT(messages.id) AS messages_count",
              "NOW() AS last_activity_at"
            )
            .left_joins(:messages)
            .order(:last_activity_at)
            .group(:id)

Expected query

SELECT 
  COUNT(*) AS "count_all", 
  "conversations"."id" AS "conversations_id" 
FROM "conversations" 
LEFT OUTER JOIN "messages" ON "messages"."conversation_id" = "conversations"."id" 
GROUP BY "conversations"."id"

-- WITHOUT THIS
-- ORDER BY last_activity_at DESC

Actual behavior

ActiveRecord::StatementInvalid <"PG::UndefinedColumn: ERROR:  column \"last_activity_at\" does not exist

SELECT 
  COUNT(*) AS "count_all", 
  "conversations"."id" AS "conversations_id" 
FROM "conversations" 
LEFT OUTER JOIN "messages" ON "messages"."conversation_id" = "conversations"."id" 
GROUP BY "conversations"."id"
ORDER BY last_activity_at DESC

System configuration

Rails version: 7.1.3.2

Ruby version: 3.0.0

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

No branches or pull requests

2 participants