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

query method #eager_loading? works incorrectly when order by sql contains table name with schema #42331

Open
senid231 opened this issue May 31, 2021 · 9 comments · May be fixed by #42602
Open

Comments

@senid231
Copy link

senid231 commented May 31, 2021

Steps to reproduce

Here's bug report which reproduces the issue

# frozen_string_literal: true

require 'bundler/inline'

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

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

  if ENV['RAILS_EDGE']
    gem 'rails', github: 'rails/rails', branch: 'main'
  else
    version = ENV['RAILS_VERSION'] || '~> 6.0'
    gem 'rails', version
  end
  gem 'pg'
end

require 'active_record'
require 'minitest/autorun'
require 'logger'

# Keep note: modify below three lines according to your postgresql server configurations
`psql -c "DROP DATABASE IF EXISTS test_rails_references_values;"`
`psql -c "CREATE DATABASE test_rails_references_values;"`
ActiveRecord::Base.establish_connection(adapter: 'postgresql', database: 'test_rails_references_values')
ActiveRecord::Base.logger = Logger.new($stdout)

ActiveRecord::Schema.define do
  create_schema 'shop'

  create_table 'posts', force: true do |t|
    t.string :title
    t.string :content
  end

  create_table 'post_details', force: true do |t|
    t.integer :post_id
    t.string :summary
  end

  create_table 'shop.posts', force: true do |t|
    t.string :title
    t.string :content
  end

  create_table 'shop.post_details', force: true do |t|
    t.integer :post_id
    t.string :summary
  end
end

class ApplicationRecord < ActiveRecord::Base
  self.abstract_class = true
end

class Post < ApplicationRecord
  self.table_name = 'posts'
  has_one :post_detail, class_name: 'PostDetail', dependent: :delete
end

class PostDetail < ApplicationRecord
  self.table_name = 'post_details'
  belongs_to :post
end

class ShopPost < ApplicationRecord
  self.table_name = 'shop.posts'
  has_one :post_detail, class_name: 'PostDetail', dependent: :delete
end

class ShopPostDetail < ApplicationRecord
  self.table_name = 'shop.post_details'
  belongs_to :post
end

class BugTest < Minitest::Test
  def test_references_values_without_schema_when_order
    query_reorder = Post.includes(:post_detail).order(Arel.sql('posts.id DESC'))

    refute query_reorder.eager_loading?
    refute query_reorder.send(:references_eager_loaded_tables?)

    assert_equal [Post.table_name], query_reorder.references_values
  end

  def test_references_values_with_schema_when_order
    query_reorder = ShopPost.includes(:post_detail).order(Arel.sql('shop.posts.id DESC'))

    refute query_reorder.eager_loading?
    refute query_reorder.send(:references_eager_loaded_tables?)

    assert_equal [ShopPost.table_name], query_reorder.references_values
  end
end

Expected behavior

for query like ShopPost.includes(:post_detail).order(Arel.sql('shop.posts.id DESC')) #eager_load? should return false.
As result includes should not eager loaded because when are not present in where/order clauses.

Actual behavior

for query like ShopPost.includes(:post_detail).order(Arel.sql('shop.posts.id DESC')) #eager_load? returns false.
As result includes always eager loaded.

System configuration

Rails version: 6.1.3.2, 5.2.6, edge 509f3ca

Ruby version: 2.7.3

PostgreSQL: 12

Additional information

This bug has big performance impact on calculations like

ShopPost.includes(:post_detail).order(Arel.sql('shop.posts.id DESC')).count

when many associations are included, because it transforms into SELECT COUNT(DISTINCT ...

In ActiveAdmin resources always apply sort as strings which leads to slow queries for all tables which have table schema.

@zzak
Copy link
Member

zzak commented Jun 1, 2021

I wonder if this is related to #41489 🤔 @interip thoughts?

@intrip
Copy link
Contributor

intrip commented Jun 24, 2021

@zzak thanks for pointing this out but I think this is a different issue, anyways I'll work on a PR to address this issue too.

@intrip
Copy link
Contributor

intrip commented Jun 25, 2021

#42602 should fix this

@rails-bot
Copy link

rails-bot bot commented Sep 23, 2021

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 6-1-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Sep 23, 2021
@intrip
Copy link
Contributor

intrip commented Sep 23, 2021

still relevant, attached PR

@rails-bot rails-bot bot removed the stale label Sep 23, 2021
@rails-bot
Copy link

rails-bot bot commented Dec 22, 2021

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 6-1-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Dec 22, 2021
@intrip
Copy link
Contributor

intrip commented Dec 22, 2021

ping

@rails-bot rails-bot bot removed the stale label Dec 22, 2021
@rails-bot
Copy link

rails-bot bot commented Mar 22, 2022

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 7-0-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Mar 22, 2022
@rails-bot rails-bot bot closed this as completed Mar 29, 2022
@intrip
Copy link
Contributor

intrip commented May 25, 2022

There's an attached PR, can anyone please reopen it?

@skipkayhil skipkayhil reopened this May 25, 2022
@rails-bot rails-bot bot removed the stale label May 25, 2022
intrip added a commit to intrip/rails that referenced this issue May 26, 2022
Some DBMS (e.g. PostgreSQL) allows to define multiple schemas per database.
`eager_loading?` now works when ordering using a table name which contains a schema prefix.

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

Successfully merging a pull request may close this issue.

5 participants