polymorph uniq check fails with 6.2

[Skulli]

Description

After the update to 6.2 one of my uniqueness-specs fails

Reproduction Steps

Spec:

is_expected.to validate_uniqueness_of(:select_option).scoped_to([:checklistable_type, :checklistable_id])

class:

#  checklistable_type :string           not null
#  checklistable_id   :bigint           not null
#  select_option_id   :bigint           not null

belongs_to :select_option
belongs_to :checklistable,
  polymorphic: true,
  inverse_of: :checked_elements

validates :select_option, uniqueness: {scope: [:checklistable_type, :checklistable_id]}

Expected behavior

The spec should return true, did this until 6.2

Actual behavior

fails with

Expected SelectOption::CheckedElement to validate that :select_option is case-sensitively unique within the scope of :checklistable_type und :checklistable_id, but this could not be proved. After taking the given SelectOption::CheckedElement, whose :select_option is ‹#<SelectOption id: 2, account_id: 3, name: "option_5", description: "MyText", data: {}, type: "SelectOption::InterruptionReason", start_date: "2022-01-01", end_date: nil, visible_service_types: [], created_at: "2024-03-18 09:07:42.564821000 +0100", updated_at: "2024-03-18 09:07:42.564821000 +0100", short_form: "kurz_5", invoiceable: true, sort_number: nil, classic_id: nil, group_type: nil, client_access: false>›, and saving it as the existing record, then making a new SelectOption::CheckedElement and setting its :select_option to ‹#<SelectOption id: 2, account_id: 3, name: "option_5", description: "MyText", data: {}, type: "SelectOption::InterruptionReason", start_date: "2022-01-01", end_date: nil, visible_service_types: [], created_at: "2024-03-18 09:07:42.564821000 +0100", updated_at: "2024-03-18 09:07:42.564821000 +0100", short_form: "kurz_5", invoiceable: true, sort_number: nil, classic_id: nil, group_type: nil, client_access: false>› as well, its :checklistable_type to a different value, ‹"Client"› und its :checklistable_id to a different value, ‹2›, the matcher expected the new SelectOption::CheckedElement to be valid, but it was invalid instead, producing these validation errors:

• select_option: ["ist bereits vergeben"]

System configuration

shoulda_matchers version: 6.2.0 rails version: 7.1.3.2 ruby version: 3.2.3

[matsales28]

Thanks for opening this issue. I'll take a look at this as soon as possible. Meanwhile do you mind trying it out validating the select_option_id instead of the whole object, to see if there's any difference?

validates :select_option_id, uniqueness: {scope: [:checklistable_type, :checklistable_id]}

[dark-panda]

@matsales28 Seeing this behaviour as well, and your alternative validation also fails in our code.

[matsales28]

I was trying to reproduce it on my machine using this script, but it's passing, I might be missing something. Can you try to give more details or create a script that I can reproduce?

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'
  gem 'shoulda-matchers'
  gem 'activerecord'
  gem 'sqlite3'
  gem 'rspec'
  gem 'byebug'
end

require 'active_record'
require 'shoulda-matchers'
require 'logger'
require 'byebug'

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)

# TODO: Update the schema to include the specific tables or columns necessary
# to reproduct the bug
ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.string :body
    t.bigint :author_id
    t.bigint :polymorphic_id
    t.string :polymorphic_type
  end

  create_table :authors, force: true do |t|
    t.string :name
  end

  create_table :polymorphics, force: true do |t|
    t.string :name
  end
end

Shoulda::Matchers.configure do |config|
  config.integrate do |with|
    with.test_framework :rspec

    with.library :active_record
    with.library :active_model
  end
end

RSpec.configure do |config|
  config.include Shoulda::Matchers::ActiveRecord
  config.include Shoulda::Matchers::ActiveModel
  config.include Shoulda::Matchers::ActionController
end

class Post < ActiveRecord::Base
  belongs_to :author
  belongs_to :polymorphic, polymorphic: true

  validates :author, uniqueness: { scope: [:polymorphic_id, :polymorphic_type] }
end

class Author < ActiveRecord::Base
  has_many :posts
end

class Polymorphic < ActiveRecord::Base
  has_many :posts, as: :polymorphic
end

RSpec.describe Post do
  describe 'validations' do
    subject { described_class.create(body: 'body', author: Author.create, polymorphic: Polymorphic.create) }

    it do
      is_expected.to validate_uniqueness_of(:author).scoped_to(:polymorphic_id, :polymorphic_type)
    end
  end
end

[dark-panda]

I found the issue in our particular case. The code that caused our issue is here:

https://github.com/thoughtbot/shoulda-matchers/commit/5781741482601323975f062038546bd236959a79#diff-fb173d61ef0ac2aa7419231de02635b3d7753c8c35ffcfbc6efb365f2430ebcfL32

Which I see fixes a few issues raised elsewhere with STI. In our case, we were relying on this particular behaviour elsewhere in our stack which is causing the unexpected result here. Tweaked our code, it appears we're off to the races again.

[matsales28]

I found the issue in our particular case. The code that caused our issue is here:

5781741#diff-fb173d61ef0ac2aa7419231de02635b3d7753c8c35ffcfbc6efb365f2430ebcfL32

Which I see fixes a few issues raised elsewhere with STI. In our case, we were relying on this particular behaviour elsewhere in our stack which is causing the unexpected result here. Tweaked our code, it appears we're off to the races again.

That's great, do you mind sharing more about how your project was relying on this behavior? It would be nice to know so I could try to fix it in the gem, to avoid this bug in other projects.

[a-lavis]

Here's a script that I believe shows this kind of failure:

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'
  gem 'shoulda-matchers'
  gem 'activerecord'
  gem 'sqlite3'
  gem 'rspec'
end

require 'active_record'
require 'shoulda-matchers'
require 'logger'

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :assets, force: true do |t|
    t.string :title
    t.bigint :attachable_id
    t.string :attachable_type
  end

  create_table :posts, force: true do |t|
    t.string :body
  end
end

Shoulda::Matchers.configure do |config|
  config.integrate do |with|
    with.test_framework :rspec

    with.library :active_record
    with.library :active_model
  end
end

RSpec.configure do |config|
  config.include Shoulda::Matchers::ActiveRecord
  config.include Shoulda::Matchers::ActiveModel
  config.include Shoulda::Matchers::ActionController
end

class Asset < ActiveRecord::Base
  belongs_to :attachable, polymorphic: true

  validates_uniqueness_of :title, scope: [:attachable_id, :attachable_type]

  def attachable_type=(class_name)
     super(class_name.constantize.base_class.to_s)
  end
end

class Post < ActiveRecord::Base
  # because we store "Post" in attachable_type now dependent: :destroy will work
  has_many :assets, as: :attachable, dependent: :destroy
end

class GuestPost < Post
end

class MemberPost < Post
end

RSpec.describe Asset do
  describe 'validations' do
    subject { described_class.create(title: 'foo', attachable: GuestPost.create) }

    it do
      is_expected.to validate_uniqueness_of(:title).scoped_to(:attachable_id, :attachable_type)
    end
  end
end

Run rspec on this file and it should fail.

It is based on this suggestion from the Rails docs: https://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html#module-ActiveRecord::Associations::ClassMethods-label-Polymorphic+Associations

@matsales28 I hope this is helpful!

[matsales28]

Yes, that's very helpful! I'll work on a fix for this tomorrow! Thanks for your help @a-lavis