Matcher validate_comparison_of is not working with ActiveRecord objects

[cesarjr]

Description

The matcher validate_comparison_of does not work when the attribute of the subject is also an ActiveRecord.

Reproduction Steps

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 :accounts, force: true do |t|
    t.references :transfer_account, foreign_key: { to_table: :accounts }
  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
end

class Account < ActiveRecord::Base
  validates :transfer_account, comparison: { other_than: :itself }
end

RSpec.describe Account do
  describe '#transfer_account' do
    it { is_expected.to validate_comparison_of(:transfer_account).is_other_than(:itself) }
  end
end

RSpec::Core::Runner.run([$__FILE__])

Save the script above and run:

$ ruby test.rb

Expected behavior

The test should pass.

Actual behavior

Complement

I'm not sure if the problem is around of here.

System configuration

shoulda_matchers version: 6.4.0 rails version: 7.2.2 ruby version: 3.3.5p100

[matsales28]

Hey @cesarjr, unfortunately, this is a limitation of the current implementation, I'm planning to make it more robust in the future, just need to find some time 😢

But you should probably be able to to that if you slightly change your validation. If you compare using the IDs, instead of the whole object it should work the way you want.

  validates :transfer_account_id, comparison: { other_than: :itself_id } 
  # In the spec
  it { is_expected to validate_comparison_of(:transfer_account_id).is_other_than(:itself_id) } 

Please let me know if that works for you I'll leave this issue open as I intend to work on it in the future.

[mohammednasser-32]

Hey @matsales28, I would be glad to help on this but I just want to confirm the idea first

should we add a case in this switch for ActiveRecord::Base, which only works for equal_to and other_than..and simply compares the equality of the objects? (the error is raised here)

Update: second thoughts, maybe instead of limiting it to ActiveRecord::Base the case should be used for any object not responding to +?

[matsales28]

Hey @mohammednasser-32, I think limiting to objects that don't respond to the + method is better for this case, and also restricting for the equal_to, and other_than qualifiers.

[cesarjr]

Hey @cesarjr, unfortunately, this is a limitation of the current implementation, I'm planning to make it more robust in the future, just need to find some time 😢

But you should probably be able to to that if you slightly change your validation. If you compare using the IDs, instead of the whole object it should work the way you want.

  validates :transfer_account_id, comparison: { other_than: :itself_id } 
  # In the spec
  it { is_expected to validate_comparison_of(:transfer_account_id).is_other_than(:itself_id) } 

Please let me know if that works for you I'll leave this issue open as I intend to work on it in the future.

Hi @matsales28 !

First of all, I'd like to thank you to have used your time to answer my issue 😀.

Well, unfortunately your solution is not viable for me because I also have others validations running for transfer_account and I wouldn't like to have two different kind of responses in my API.

I ended up solving my problem writing a group of tests without shoulda-matcher. Anyway, I wanted to report this because I spent some time trying to figure it out, and someone else might be "saved" by reading this issue 😂.

Thanks again for your help.

[mohammednasser-32]

@matsales28 It turned out this needs more refactoring than I expected, The basic scenario can be handled using the approach mentioned before (changes here), however to handle every possible scenario it seems more refactoring needs to be done and I don't want to be causing a a lot of big changes...sorry for the noise :sweat: