thoughtbot / shoulda-matchers

Simple one-liner tests for common Rails functionality
https://matchers.shoulda.io
MIT License
3.5k stars 909 forks source link

Record uniqueness matcher fails with STI when type column is enum #1635

Open lake-effect opened 2 months ago

lake-effect commented 2 months ago

Description

The succ call described in https://github.com/thoughtbot/shoulda-matchers/issues/854 fails to match when the type column being used in STI is an enumerable, because that column's values are restricted.

Reproduction Steps

Modify the below reproduction script to match a locally running Postgres install, and then run it.

require 'bundler/inline'

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

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

ActiveRecord::Base.establish_connection(adapter: 'pg', database: 'YOUR_LOCATION_HERE')
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_enum "big_model_types", ["BigModel", "LittleModel"]

  create_table :big_models, force: true do |t|
    t.enum "type", default: "BigModel", null: false, enum_type: "big_model_types"
  end

  create_table "model_fields", force: :cascade do |t|
    t.string "name", null: false
    t.string "record_type", null: false
    t.bigint "record_id", null: false
  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

# TODO: Add any application specific code necessary to reproduce the bug
class BigModel < ActiveRecord::Base
  has_one :some_model_field,
          -> { where(name: 'some') },
          as: :record,
          autosave: true,
          class_name: 'ModelField',
          dependent: :destroy,
          inverse_of: :record
end

class ModelField < ActiveRecord::Base
  belongs_to :record, polymorphic: true, optional: false

  validates :name, presence: true, uniqueness: {scope: [:record_id, :record_type]}
end

# typed: false
FactoryBot.define do
  factory :model_field do
    name { "my_field_name" }
  end
end

# TODO: Write a failing test case to demonstrate what isn't working as
# expected
RSpec.describe ModelField do
  describe "name + record uniqueness" do
    subject { build(:model_field, record: nil) }

    before { create(:model_field, record: nil) }

    let(:record) { create(:big_model) }

    it { is_expected.to validate_uniqueness_of(:name).scoped_to([:record_id, :record_type]) }
  end
end

Expected behavior

Validation passes.

Actual behavior

1) ModelField validations name + record uniqueness is expected to validate that :name is case-sensitively unique within the scope of :record_id and :record_type
     Failure/Error: it { is_expected.to validate_uniqueness_of(:name).scoped_to([:record_id, :record_type]) }

     ActiveRecord::StatementInvalid:
       PG::InvalidTextRepresentation: ERROR:  invalid input value for enum big_model_types: "Shoulda::Matchers::ActiveRecord::Uniqueness::TestModels::BigModem"
       LINE 1: ...g_models" WHERE "big_models"."type" = 'Shoulda::...
                                                                    ^
     # ./spec/models/model_field_spec.rb:17:in `block (4 levels) in <main>'
     # -e:1:in `<main>'
     # ------------------
     # --- Caused by: ---
     # PG::InvalidTextRepresentation:
     #   ERROR:  invalid input value for enum big_model_types: "Shoulda::Matchers::ActiveRecord::Uniqueness::TestModels::BigModem"
     #   LINE 1: ...g_models" WHERE "big_models"."type" = 'Shoulda::...
     #                                                                ^
     #   ./spec/models/model_field_spec.rb:17:in `block (4 levels) in <main>'

System configuration

shoulda_matchers version: 6.2.0 rails version: 7.1.3.2 ruby version: 3.2.1

matsales28 commented 1 month ago

Hi, thanks for the report! Unfortunately, we can't do much in that case, as the defined values are only stored in the database. I searched for an interface to access those values in the Rails adapters, but it does not exist (so it could work for any database adapter); it might be something nice to add on Rails itself.

If I have some free time, I can try to move that forward in Rails, but I can't promise as I don't have much time now.

lake-effect commented 1 month ago

Would it work to add a manual override in the matcher API? Something like validate_uniqueness_of(attr, valid_names_to_try)? Then we could tell the matcher the other names it should try instead of going to the succ call.

matsales28 commented 1 month ago

Would it work to add a manual override in the matcher API? Something like validate_uniqueness_of(attr, valid_names_to_try)? Then we could tell the matcher the other names it should try instead of going to the succ call.

Yes, that's an excellent idea. I'll put that on my backlog of features!

lake-effect commented 1 month ago

Thanks so much!