rails icon indicating copy to clipboard operation
rails copied to clipboard

ActiveRecord malformed SQL statements with `where.missing` and `where.associated` clauses for parent/children associations

Open EmCousin opened this issue 2 years ago • 5 comments

There is an edge case with ActiveRecord's where.associated and where.missing clauses when using them with same-table parent/children associations, causing those to generate what I believe is not the expected SQL statement.

Classic behavior ✅

class Post < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base
  belongs_to :post
end

Post.where.associated(:comments).to_sql
# => "SELECT \"posts\".* FROM \"posts\" INNER JOIN \"comments\" ON \"comments\".\"post_id\" = \"posts\".\"id\" WHERE \"comments\".\"id\" IS NOT NULL"

Post.where.missing(:comments).to_sql
# => "SELECT \"posts\".* FROM \"posts\" LEFT OUTER JOIN \"comments\" ON \"comments\".\"post_id\" = \"posts\".\"id\" WHERE \"comments\".\"id\" IS NULL"

Edge case behavior 👁️

class Post < ActiveRecord::Base
  belongs_to :template, class_name: "Post", inverse_of: :children, optional: true
  has_many :children, class_name: "Post", inverse_of: :template, foreign_key: :template_id
end

Expected behavior

ActiveRecord's joins method generates an alias children_posts. This alias is properly used in the SQL statement

Post.where.associated(:children).to_sql
# => ""SELECT \"posts\".* FROM \"posts\" INNER JOIN \"posts\" \"children_posts\" ON \"children_posts\".\"template_id\" = \"posts\".\"id\" WHERE \"children_posts\".\"id\" IS NOT NULL""

Post.where.missing(:children).to_sql
# => "SELECT \"posts\".* FROM \"posts\" LEFT OUTER JOIN \"posts\" \"children_posts\" ON \"children_posts\".\"template_id\" = \"posts\".\"id\" WHERE \"children_posts\".\"id\" IS NULL""

Actual behavior ❌

ActiveRecord's joins method generates an alias children_posts. but that alias is not used in the SQL statement. Instead, it's the original table name posts

Post.where.associated(:children).to_sql
# => ""SELECT \"posts\".* FROM \"posts\" INNER JOIN \"posts\" \"children_posts\" ON \"children_posts\".\"template_id\" = \"posts\".\"id\" WHERE \"posts\".\"id\" IS NOT NULL""

Post.where.missing(:children).to_sql
# => "SELECT \"posts\".* FROM \"posts\" LEFT OUTER JOIN \"posts\" \"children_posts\" ON \"children_posts\".\"template_id\" = \"posts\".\"id\" WHERE \"posts\".\"id\" IS NULL""

System configuration

Rails version: Edge, main branch (to the date of creation of this issue)

Ruby version: tested on 2.7.4

Steps to reproduce

This branch contains a test that reproduces the problem as well as a proposition to fix it. The solution I came up with is to add a #aliased_table_name instance method to the Reflection class, that returns the proper alias if needed.

You can also run the test below. It fails with rails/rails's main branch, whereas it succeeds in EmCousin/rails/bug/active-record-where-missing-same-table

# frozen_string_literal: true

require "bundler/inline"

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

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

  ### The test fails with "rails/rails"'s `main` branch ###
  # gem "rails", github: "rails/rails", branch: "main"
  gem "rails", github: "EmCousin/rails", branch: "bug/active-record-where-missing-same-table"

  gem "sqlite3"
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 :posts, force: true do |t|
    t.references :template, foreign_key: { to_table: :posts }
  end
end

class Post < ActiveRecord::Base
  belongs_to :template, class_name: "Post", optional: true
  has_many :children, class_name: "Post", inverse_of: :template, foreign_key: :template_id, dependent: :nullify
end

class WhereAssociatedMissingTest < Minitest::Test
  def test_where_associated_expected_sql_statement
    expected_sql_statement = <<-SQL.squish
      SELECT "posts".*
      FROM "posts"
      INNER JOIN "posts" "children_posts" ON "children_posts"."template_id" = "posts"."id"
      WHERE "children_posts"."id" IS NOT NULL
    SQL

    assert_equal expected_sql_statement, Post.where.associated(:children).to_sql
  end

  def test_where_associated_unexpected_sql_statement
    unexpected_sql_statement = <<-SQL.squish
      SELECT "posts".*
      FROM "posts"
      INNER JOIN "posts" "children_posts" ON "children_posts"."template_id" = "posts"."id"
      WHERE "posts"."id" IS NOT NULL
    SQL

    assert unexpected_sql_statement != Post.where.associated(:children).to_sql
  end

  def test_where_associated
    Post.destroy_all
    post = Post.create!
    post.children << Post.create!

    assert_equal 2, Post.count
    assert_equal 1, Post.where.associated(:children).count
  end

  def test_where_missing_expected_sql_statement
    expected_sql_statement = <<-SQL.squish
      SELECT "posts".*
      FROM "posts"
      LEFT OUTER JOIN "posts" "children_posts" ON "children_posts"."template_id" = "posts"."id"
      WHERE "children_posts"."id" IS NULL
    SQL

    assert_equal expected_sql_statement, Post.where.missing(:children).to_sql
  end

  def test_where_missing_unexpected_sql_statement
    unexpected_sql_statement = <<-SQL.squish
      SELECT "posts".*
      FROM "posts"
      LEFT OUTER JOIN "posts" "children_posts" ON "children_posts"."template_id" = "posts"."id"
      WHERE "posts"."id" IS NULL
    SQL

    assert unexpected_sql_statement != Post.where.missing(:children).to_sql
  end

  def test_where_missing
    Post.destroy_all
    post = Post.create!
    post.children << Post.create!

    assert_equal 2, Post.count
    assert_equal 1, Post.where.missing(:children).count
  end
end

Note : my sincerest apologies for my imperfect English. This is my first issue to the Rails repository. Any suggestions welcome so I can improve it 🙏

EmCousin avatar Apr 26 '22 18:04 EmCousin

I'm pretty new to open source as well but I've read through the code/test you've written and I think that this is OK to merge with the main branch.

gardnerapp avatar Apr 26 '22 21:04 gardnerapp

On a side note I'm trying to figure out how to push some commits of my own and I struggle with figuring out how to figure out how things work. How'd you know to make changes to the reflection class?

gardnerapp avatar Apr 26 '22 21:04 gardnerapp

@gardnerapp Only contributors can push commits directly to the rails repo ; which I'm not. I had to fork the repo first before committing changes into it

EmCousin avatar Apr 27 '22 12:04 EmCousin

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[bot] avatar Jul 28 '22 18:07 rails-bot[bot]

Still valid.

fatkodima avatar Aug 03 '22 21:08 fatkodima