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

Usage of id enum key results in unexpected filter with joins and subqueries #48524

Closed
nmichels opened this issue Jun 19, 2023 · 3 comments · Fixed by #48527
Closed

Usage of id enum key results in unexpected filter with joins and subqueries #48524

nmichels opened this issue Jun 19, 2023 · 3 comments · Fixed by #48527

Comments

@nmichels
Copy link

Steps to reproduce

# frozen_string_literal: true

require "bundler/inline"
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 :offers, force: true do |t|
    t.string :status
    t.integer :account_id
  end
  create_table :accounts, force: true do |t|
    t.string :name
    t.integer :group_id
  end
  create_table :groups, force: true do |t|
    t.string :name
    t.string :external_code
    t.integer :external_integration_attr
    t.integer :account_id
  end
  create_table :transactions, force: true do |t|
    t.integer :group_id
    t.decimal :amount
  end
end

class Offer < ActiveRecord::Base
  belongs_to :account
end

class Account < ActiveRecord::Base
  belongs_to :group
end


class Group < ActiveRecord::Base
  has_one :account
  enum external_integration_attr: ["id", "externalCode"]
end

class Transaction < ActiveRecord::Base
  belongs_to :group
end

class AddOfferAccountGroupAndTransactions < ActiveRecord::Migration[7.0]
  def change
    reversible do |dir|
      dir.up do
        Group.new(name: "Foo", external_integration_attr: "externalCode").save!
        Account.new(name: "Bar", group: Group.first).save!
        Offer.new(status: "approved", account: Account.first).save!
        Transaction.new(amount: 10, group: Group.first).save!
      end

      dir.down do
      end
    end
  end
end

class BugTest < Minitest::Test
  def test_migration_up
    
    AddOfferAccountGroupAndTransactions.migrate(:up)

    transactions = 
      Transaction
        .where(group_id: Group.joins(:account)
          .where(account: { id:  Offer.where(status: "approved").select(:account_id) })
          .select(:id))

    assert_equal 1, transactions.count
  end
end

Expected behavior

Query does not have the following filter: AND "groups"."external_integration_attr" = ? (["external_integration_attr", 0])

Actual behavior

Query has the following filter AND "groups"."external_integration_attr" = ? (["external_integration_attr", 0])

System configuration

Rails version:
Current main branch and 6.1.7.3

Ruby version:
Ruby 3.1.0 (alongside main branch) and Ruby 2.7.7 + Rails 6.1.7.3

@ghiculescu
Copy link
Member

ghiculescu commented Jun 20, 2023

executable test case:

# frozen_string_literal: true

require "bundler/inline"

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

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

  gem "rails", github: "rails/rails", branch: "main"
  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 :offers, force: true do |t|
    t.string :status
    t.integer :account_id
  end
  create_table :accounts, force: true do |t|
    t.string :name
    t.integer :group_id
  end
  create_table :groups, force: true do |t|
    t.string :name
    t.string :external_code
    t.integer :external_integration_attr
    t.integer :account_id
  end
  create_table :transactions, force: true do |t|
    t.integer :group_id
    t.decimal :amount
  end
end

class Offer < ActiveRecord::Base
  belongs_to :account
end

class Account < ActiveRecord::Base
  belongs_to :group
end

class Group < ActiveRecord::Base
  has_one :account
  enum external_integration_attr: ["id", "externalCode"]
end

class Transaction < ActiveRecord::Base
  belongs_to :group
end

class BugTest < Minitest::Test
  def test_migration_up
    Group.new(name: "Foo", external_integration_attr: "externalCode").save!
    Account.new(name: "Bar", group: Group.first).save!
    Offer.new(status: "approved", account: Account.first).save!
    Transaction.new(amount: 10, group: Group.first).save!

    groups = Group.joins(:account).where(account: { id: Offer.where(status: "approved").select(:account_id) })
    assert_equal 1, groups.count
    assert_equal 1, groups.select(:id).to_a.count

    transactions = Transaction.where(group_id: groups.pluck(:id))
    assert_equal 1, transactions.count

    transactions = Transaction.where(group_id: groups.select(:id))
    assert_equal 1, transactions.count
  end
end

@ghiculescu
Copy link
Member

As a workaround, enum external_integration_attr: ["id", "externalCode"], _prefix: true solves the issue.

ghiculescu added a commit to ghiculescu/rails that referenced this issue Jun 20, 2023
Fixes rails#48524

The test case in the issue breaks because `value.respond_to?(:id)` returns true [here](https://github.com/rails/rails/blob/51f2e2f80b86e0c3769cf5272b7d97035730f19d/activerecord/lib/active_record/relation/predicate_builder.rb#L58). This effectively adds a default scope to queries where it shouldn't.

There might be a way to fix this in Active Record but I'd be surprised if nothing else breaks from defining `id` instance and class methods. I think it is simpler to not allow it as a value since it really should be treated as a reserved method.
@ghiculescu
Copy link
Member

I don't think we should allow this in Rails: #48527

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.

2 participants