search

rails / rails

Ruby on Rails
https://rubyonrails.org
MIT License
56.12k stars 21.69k forks source link

has_many relation with joins adding redundant inverse inner join #51259

Open segiddins opened 8 months ago

segiddins commented 8 months ago

Steps to reproduce

Have the following set of related models, and use the reverse_dependencies relation. Observe (via the inline comments in the test case below) that there is an unnecessary/redundant join

#!/usr/bin/env ruby
# frozen_string_literal: true

require "bundler/inline"

gemfile do
  source "https://rubygems.org"

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

  gem "rails", "~> 7.1.0"
  # If you want to test against edge Rails replace the previous line with this:
# gem "rails", github: "rails/rails", branch: "main"

  gem "sqlite3"

  gem 'anbt-sql-formatter', require: "anbt-sql-formatter/formatter"
end

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

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :rubygems, force: true do |t|
  end

  create_table :versions, force: true do |t|
    t.belongs_to :rubygem
    t.integer :position
    t.boolean :indexed
  end

  create_table :gem_downloads, force: true do |t|
    t.belongs_to :rubygem
    t.belongs_to :version, null: true
    t.integer :count
  end

  create_table :dependencies, force: true do |t|
    t.belongs_to :rubygem
    t.belongs_to :version
  end
end

class Rubygem < ActiveRecord::Base
  has_many :incoming_dependencies, lambda {
                                      joins(:version).where(versions: { indexed: true, position: 0 })
                                    }, class_name: "Dependency", inverse_of: :rubygem
  has_many :reverse_dependencies, through: :incoming_dependencies, source: :version_rubygem

  has_many :versions

  has_many :incoming_dependencies_no_join, lambda {
                                      where(versions: { indexed: true, position: 0 })
                                    }, class_name: "Dependency", inverse_of: :rubygem
  has_many :reverse_dependencies_no_join, through: :incoming_dependencies_no_join, source: :version_rubygem
end

class Dependency < ActiveRecord::Base
  belongs_to :rubygem, optional: true
  belongs_to :version
  has_one :version_rubygem, through: :version, source: :rubygem
end

class Version < ActiveRecord::Base
  belongs_to :rubygem, touch: true
  has_many :dependencies, -> { order("rubygems.name ASC").includes(:rubygem) }, dependent: :destroy, inverse_of: :version
end

class BugTest < Minitest::Test
  def test_assoc_1
    rubygem1 = Rubygem.create!
    v1 = Version.create!(indexed: true, position: 0, rubygem: rubygem1)
    rubygem2 = Rubygem.create!
    v2 = Version.create!(indexed: true, position: 0, rubygem: rubygem2)
    d = v2.dependencies.create!(rubygem: rubygem1)

    puts Rails.version

    # SELECT
    #   "rubygems" . *
    #   FROM
    #   "rubygems"
    #   INNER JOIN "versions"
    #     ON "rubygems"."id" = "versions"."rubygem_id"
    #   INNER JOIN "dependencies"
    #     ON "versions"."id" = "dependencies"."version_id"
    #   INNER JOIN "versions" "versions_dependencies"
    #     ON "versions_dependencies"."id" = "dependencies"."version_id"
    #   WHERE
    #   "dependencies"."rubygem_id" = ?
    #   AND "versions"."indexed" = ?
    #   AND "versions"."position" = ?

    puts mu_pp(rubygem1.reverse_dependencies.arel)

    assert_equal [rubygem2], rubygem1.reverse_dependencies
    assert_equal rubygem1.reverse_dependencies_no_join, rubygem1.reverse_dependencies

    # Failure:
    # BugTest#test_assoc_1 [Untitled.rb:118]:
    # --- expected
    # +++ actual
    # @@ -109,6 +109,53 @@
    #           #<ActiveRecord::TypeCaster::Map:0xXXXXXX
    #           @klass=
    #           Dependency(id: integer, rubygem_id: integer, version_id: integer)>>,
    # +               name="version_id">>>>,
    # +        #<Arel::Nodes::InnerJoin:0xXXXXXX
    # +         @left=
    # +          #<Arel::Nodes::TableAlias:0xXXXXXX
    # +           @left=
    # +            #<Arel::Table:0xXXXXXX
    # +             @klass=
    # +              Version(id: integer, rubygem_id: integer, position: integer, indexed: boolean),
    # +             @name="versions",
    # +             @table_alias=nil,
    # +             @type_caster=
    # +              #<ActiveRecord::TypeCaster::Map:0xXXXXXX
    # +               @klass=
    # +                Version(id: integer, rubygem_id: integer, position: integer, indexed: boolean)>>,
    # +           @right="versions_dependencies">,
    # +         @right=
    # +          #<Arel::Nodes::On:0xXXXXXX
    # +           @expr=
    # +            #<Arel::Nodes::Equality:0xXXXXXX
    # +             @left=
    # +              #<struct Arel::Attributes::Attribute
    # +               relation=
    # +                #<Arel::Nodes::TableAlias:0xXXXXXX
    # +                 @left=
    # +                  #<Arel::Table:0xXXXXXX
    # +                   @klass=
    # +                    Version(id: integer, rubygem_id: integer, position: integer, indexed: boolean),
    # +                   @name="versions",
    # +                   @table_alias=nil,
    # +                   @type_caster=
    # +                    #<ActiveRecord::TypeCaster::Map:0xXXXXXX
    # +                     @klass=
    # +                      Version(id: integer, rubygem_id: integer, position: integer, indexed: boolean)>>,
    # +                 @right="versions_dependencies">,
    # +               name="id">,
    # +             @right=
    # +              #<struct Arel::Attributes::Attribute
    # +               relation=
    # +                #<Arel::Table:0xXXXXXX
    # +                 @klass=
    # +                  Dependency(id: integer, rubygem_id: integer, version_id: integer),
    # +                 @name="dependencies",
    # +                 @table_alias=nil,
    # +                 @type_caster=
    # +                  #<ActiveRecord::TypeCaster::Map:0xXXXXXX
    # +                   @klass=
    # +                    Dependency(id: integer, rubygem_id: integer, version_id: integer)>>,
    #         name="version_id">>>>]>,
    #   @wheres=
    #     [#<Arel::Nodes::And:0xXXXXXX
    # @@ -213,6 +260,8 @@
    #     ON "rubygems"."id" = "versions"."rubygem_id"
    #   INNER JOIN "dependencies"
    #     ON "versions"."id" = "dependencies"."version_id"
    # +    INNER JOIN "versions" "versions_dependencies"
    # +      ON "versions_dependencies"."id" = "dependencies"."version_id"
    #   WHERE
    #   "dependencies"."rubygem_id" = ?
    #   AND "versions"."indexed" = ?

    assert_equal rubygem1.reverse_dependencies_no_join.arel.ast, rubygem1.reverse_dependencies.arel.ast
  end

  def test_assoc_2
    rubygem1 = Rubygem.create!
    v1 = Version.create!(indexed: true, position: 0, rubygem: rubygem1)
    rubygem2 = Rubygem.create!
    v2 = Version.create!(indexed: true, position: 0, rubygem: rubygem2)
    d = v2.dependencies.create!(rubygem: rubygem1)

    assert_equal [d], rubygem1.incoming_dependencies
    # shows that the joins is necessary
    assert_raises(ActiveRecord::StatementInvalid) { rubygem1.incoming_dependencies_no_join.to_a }
  end

  def mu_pp(obj)
    case obj
    when Arel::Nodes::SelectStatement
      rule = AnbtSql::Rule.new
      rule.keyword = AnbtSql::Rule::KEYWORD_UPPER_CASE
      rule.kw_multi_words << "INNER JOIN"
      rule.kw_nl_x << "INNER JOIN"
      rule.indent_string = "  "

      ["AST:", obj.pretty_inspect, "SQL:",
        AnbtSql::Formatter.new(rule).format(obj.to_sql)].join("\n\n")
    else
      obj.pretty_inspect
    end
  end
end

Expected behavior

The additional join should not be added, since it's an inner join and the "versions"."id" = "dependencies"."version_id" restriction guarantees it does not add any additional filtering

Actual behavior

There is an unused join to "versions" "versions_dependencies"

System configuration

Rails version: 7.1.3.2

Ruby version: 3.3.0

rails-bot[bot] commented 5 months ago

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-2-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.

segiddins commented 5 months ago

still an issue

Mathiou04 commented 3 months ago

I think I encountered the same issue.

With the following classes:

class Company
  has_many :accounts
  has_many :users, through: :accounts
end

class Account
    has_many :users 
    belongs_to :company
end

class User
    belongs_to :account
end

If I do:

company = Company.find(5)
company.users.joins(:accounts).any?

I get the following SQL query:

SELECT 1 AS one FROM "users"
INNER JOIN "accounts" ON "users"."account_id" = "accounts"."id"
INNER JOIN "accounts" "accounts_users" ON "accounts_users"."id" = "users"."account_id"
WHERE "accounts"."company_id" =  5 LIMIT 1

There are 2 joins for the same table, the second one (accounts_users) being completely useless.

In more complexe cases, this second useless join can apparently slow down queries.

rails-bot[bot] commented 3 weeks ago

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 8-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.

segiddins commented 3 weeks ago

still an issue

On Nov 3, 2024, at 2:32 AM, rails-bot[bot] @.***> wrote:

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 8-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.

— Reply to this email directly, view it on GitHub https://github.com/rails/rails/issues/51259#issuecomment-2453376448, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAO3H4UPA5UFTLPJ75IH6NLZ6X3SRAVCNFSM6AAAAABEIFY3WSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJTGM3TMNBUHA. You are receiving this because you authored the thread.