diff --git a/CHANGELOG.md b/CHANGELOG.md index c600bb5..b7f0195 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,17 @@ +## [Unreleased] + +- Added retry handling for `ActiveRecord::RecordNotUnique` slug collisions during post-create slug persistence +- Added `around_create` retry handling for insert-time slug collisions in pre-insert (`null: false`) slug strategies +- Wrapped insert retries in savepoint transactions (`requires_new: true`) to keep PostgreSQL transactions retry-safe +- Added regression coverage for insert-time and update-time slug race windows +- Added optional PostgreSQL integration test for transaction-abort-safe insert retries + +### Breaking Changes (Behavioral) + +- **For NOT NULL slug columns:** On insert-time slug collision, the entire `around_create` chain re-executes, which means `before_create` callbacks **will** fire multiple times per collision. Move non-idempotent side effects (emails, jobs) to `after_create` to avoid duplication. +- **Slug save now uses `save!` instead of `save`:** If a validation fails during slug-save (both `after_create` and `after_find` nil-slug repair), the new code raises `ActiveRecord::RecordInvalid` instead of silently skipping. This affects any record with nil slugs that triggers `update_slug_if_nil` on load. +- **Removed `id_changed?` check from `set_slug`:** The check was redundant for the `after_create` path (where ID always just changed from nil). This should not affect normal usage. + ## [0.2.0] - 2026-01-16 - Added a full Minitest test suite diff --git a/Gemfile b/Gemfile index d7d3202..815bdf2 100644 --- a/Gemfile +++ b/Gemfile @@ -11,6 +11,8 @@ group :development, :test do gem "appraisal" gem "minitest", "~> 6.0" gem "minitest-mock" + # Optional: install manually for PostgreSQL integration tests (requires libpq) + # gem "pg" gem "rack-test" gem "simplecov", require: false gem "sqlite3", ">= 2.1" diff --git a/README.md b/README.md index e16f659..5f8cbe4 100644 --- a/README.md +++ b/README.md @@ -81,8 +81,15 @@ Product.first.slug If your model has a `slug` attribute in the database, `slugifiable` will automatically generate a slug for that model upon instance creation, and save it to the DB. > [!IMPORTANT] -> Your `slug` attribute **SHOULD NOT** have `null: false` in the migration / database. If it does, `slugifiable` will not be able to save the slug to the database, and will raise an error like `ERROR: null value in column "slug" of relation "posts" violates not-null constraint (PG::NotNullViolation)` -> This is because records are created without a slug, and the slug is generated later. +> By default, `slugifiable` persists slugs after `create`, so a nullable `slug` column is the simplest setup. +> If you need `null: false`, generate the slug before `INSERT` (for example with a `before_validation` callback that sets `self.slug = compute_slug` when blank). +> `slugifiable` handles slug unique-collision retries for both post-create and pre-insert slug strategies. + +> [!CAUTION] +> **For NOT NULL slug columns with INSERT-time collision retry:** When a slug collision occurs and retry is needed, the entire `around_create` callback chain (including your `before_create` callbacks) re-executes. If you have non-idempotent callbacks like sending emails or enqueuing jobs, they may fire multiple times. Either make such callbacks idempotent, or move side-effects to `after_create` (which only fires once, after the record is persisted). + +> [!NOTE] +> **Exhaustion behavior differs by slug strategy:** For nullable slug columns (post-create slug), if all retries are exhausted, `slugifiable` falls back to a timestamp-based slug. For NOT NULL slug columns (pre-insert slug), exhaustion raises an error instead, as sustained collisions in this case indicate a systemic issue that warrants attention. If you're generating slugs based off the model `id`, you can also set a desired length: ```ruby @@ -214,6 +221,15 @@ bundle exec rake test The test suite uses SQLite3 in-memory database and requires no additional setup. +Optional PostgreSQL integration check: + +```bash +SLUGIFIABLE_TEST_POSTGRES_URL=postgres://user:pass@localhost:5432/slugifiable_test \ +bundle exec ruby -Itest test/slugifiable/postgresql_insert_race_retry_test.rb +``` + +This test validates PostgreSQL transaction behavior for retrying INSERT-time slug collisions. + ## Development After checking out the repo, run `bin/setup` to install dependencies. Then, run `rake spec` to run the tests. You can also run `bin/console` for an interactive prompt that will allow you to experiment. diff --git a/lib/slugifiable/model.rb b/lib/slugifiable/model.rb index e3bd5d6..bdb2cae 100644 --- a/lib/slugifiable/model.rb +++ b/lib/slugifiable/model.rb @@ -33,11 +33,13 @@ module Model # 10^18 fits safely in a 64-bit integer MAX_NUMBER_LENGTH = 18 - # Maximum number of attempts to generate a unique slug - # before falling back to timestamp-based suffix + # Maximum number of attempts to generate a unique slug before exhaustion. + # Total saves in worst case for nullable-slug path: 10 attempts + 1 exhaustion fallback = 11. + # For NOT NULL path: 10 attempts then raises (no fallback). MAX_SLUG_GENERATION_ATTEMPTS = 10 included do + around_create :retry_create_on_slug_unique_violation after_create :set_slug after_find :update_slug_if_nil validates :slug, uniqueness: true @@ -106,7 +108,57 @@ def compute_slug_based_on_attribute(attribute_name) # 4. Ensure uniqueness # 5. Fallback to random number if anything fails - # First check if we can get a value from the database + base_slug = parameterize_attribute_value(attribute_name) + + # Different fallback paths based on why parameterization failed + return compute_slug_as_string if base_slug == :attribute_missing + return generate_random_number_based_on_id_hex if base_slug == :value_nil_or_blank + + # Handle duplicate slugs by adding a random suffix if needed + # e.g. "my-title" -> "my-title-123456" + unique_slug = generate_unique_slug(base_slug) + unique_slug.presence || generate_random_number_based_on_id_hex + end + + private + + # Returns the raw parameterized slug without uniqueness handling. + # Used by the exhaustion fallback to avoid double-suffixing. + # + # Always uses generate_random_number_based_on_id_hex for ALL fallback cases + # (missing attribute, nil value, blank value). This ensures the exhaustion + # fallback produces a simple numeric suffix regardless of failure reason. + def compute_base_slug + strategy, options = determine_slug_generation_method + + case strategy + when :compute_slug_based_on_attribute + base_slug = parameterize_attribute_value(options) + + # All fallback cases use numeric ID-based slug for exhaustion path + return generate_random_number_based_on_id_hex if base_slug.is_a?(Symbol) + + base_slug + else + # For ID-based strategies, return the deterministic hash. + # NOTE: This path should never execute in practice since ID-based slug + # collisions are impossible (two records can't have the same ID). If it + # ever did, the exhaustion fallback would produce a double-suffixed slug + # like "d4735e3a265-1234567890-abcd1234". + compute_slug + end + end + + # Shared helper: extracts and parameterizes an attribute value. + # Returns one of: + # - String: the parameterized value (e.g. "my-title") + # - :attribute_missing: attribute/method doesn't exist + # - :value_nil_or_blank: attribute exists but value is nil or parameterizes to blank + # + # Used by both compute_slug_based_on_attribute and compute_base_slug + # to ensure consistent behavior. Callers handle fallback logic. + def parameterize_attribute_value(attribute_name) + # Check if we can get a value from the database has_attribute = self.attributes.include?(attribute_name.to_s) # Only check for methods if no DB attribute exists @@ -117,27 +169,18 @@ def compute_slug_based_on_attribute(attribute_name) self.class.protected_method_defined?(attribute_name) ) - # If we can't get a value from either source, fallback to using the record's ID - return compute_slug_as_string unless has_attribute || responds_to_method + # If we can't get a value from either source, signal missing attribute + return :attribute_missing unless has_attribute || responds_to_method # Get and clean the raw value (e.g. " My Title " -> "My Title") - # Works for both DB attributes and methods thanks to Ruby's send raw_value = self.send(attribute_name) - return generate_random_number_based_on_id_hex if raw_value.nil? + return :value_nil_or_blank if raw_value.nil? - # Convert to URL-friendly format - # e.g. "My Title" -> "my-title" + # Convert to URL-friendly format (e.g. "My Title" -> "my-title") base_slug = raw_value.to_s.strip.parameterize - return generate_random_number_based_on_id_hex if base_slug.blank? - - # Handle duplicate slugs by adding a random suffix if needed - # e.g. "my-title" -> "my-title-123456" - unique_slug = generate_unique_slug(base_slug) - unique_slug.presence || generate_random_number_based_on_id_hex + base_slug.presence || :value_nil_or_blank end - private - def normalize_length(length, default, max) length = length.to_i return default if length <= 0 @@ -170,8 +213,8 @@ def generate_unique_slug(base_slug) # If we couldn't find a unique slug after MAX_SLUG_GENERATION_ATTEMPTS, # append timestamp + random to ensure uniqueness - if attempts == MAX_SLUG_GENERATION_ATTEMPTS - slug_candidate = "#{base_slug}-#{Time.current.to_i}-#{SecureRandom.random_number(1000)}" + if attempts >= MAX_SLUG_GENERATION_ATTEMPTS + slug_candidate = "#{base_slug}-#{Time.current.to_i}-#{SecureRandom.hex(4)}" end slug_candidate @@ -221,8 +264,164 @@ def has_slug_method? def set_slug return unless slug_persisted? - self.slug = compute_slug if id_changed? || slug.blank? - self.save + set_slug_with_retry + end + + def set_slug_with_retry + # NOTE: The old code checked `id_changed? || slug.blank?`. The `id_changed?` + # guard is unnecessary in `after_create` — the ID always just changed from nil. + # It would only matter if a trigger reassigned the ID post-INSERT, which is rare. + return unless slug.blank? + + # For attribute-based slugs, compute_slug -> generate_unique_slug already + # handles uniqueness with random suffixes on each call. + # For ID-based slugs, collisions are impossible since two records can't + # have the same ID, so retries would never be triggered in practice. + # + # Each attempt runs in a savepoint so a unique-constraint violation does + # not abort the outer transaction in PostgreSQL. + # + # Fallback when all retries exhausted: use timestamp suffix for guaranteed uniqueness. + # Uses compute_base_slug to avoid double-suffixing (compute_slug includes random suffixes). + # NOTE: The exhaustion fallback uses save! and does not retry on failure. If this + # raises (unlikely given timestamp+random uniqueness), the exception propagates. + # This is intentional — exhaustion indicates a systemic issue warranting attention. + on_exhaustion = -> { + base_slug = compute_base_slug + self.slug = "#{base_slug}-#{Time.current.to_i}-#{SecureRandom.hex(4)}" + self.class.transaction(requires_new: true) { self.save! } + } + + with_slug_retry(-> { self.slug = nil }, on_exhaustion: on_exhaustion) do + self.slug = compute_slug + # Use save! for consistency with exhaustion fallback — both paths + # now raise on any failure, not just RecordNotUnique. + self.class.transaction(requires_new: true) { self.save! } + end + end + + # Detects if a RecordNotUnique error is related to the slug column. + # + # Uses patterns that handle common error message formats without false positives: + # - SQLite: "UNIQUE constraint failed: table.slug" -> matches \bslug\b (period is word boundary) + # - PostgreSQL: "Key (slug)=(value)" -> matches \bslug\b (parens are word boundaries) + # - MySQL/PG index: "index_posts_on_slug" -> matches _on_slug\b + # + # IMPORTANT: We use _on_slug\b specifically to avoid false positives on columns + # like canonical_slug, parent_slug, original_slug, etc. The pattern _slug alone + # would incorrectly match those. + # + # LIMITATION: Custom index names like `uq_slug_col`, `slug_idx`, or `posts_slug_unique` + # won't match. This results in a false negative (exception bubbles up), which is the + # safe outcome — no data corruption, just no automatic retry for that schema. + def slug_unique_violation?(error) + message = error.message.to_s.downcase + cause_message = error.cause&.message.to_s.downcase + # \bslug\b matches "slug" as standalone word (SQLite ".slug", PostgreSQL "(slug)") + # _on_slug\b matches Rails index naming convention "index_*_on_slug" + pattern = /\bslug\b|_on_slug\b/ + [message, cause_message].any? { |m| m.match?(pattern) } + end + + # Handle INSERT-time slug races for models that persist slugs at create-time + # (e.g., NOT NULL slug columns with before_validation slug generation). + # + # NOTE: This calls `yield` multiple times (once per attempt) via `retry`. + # This relies on Rails `around_create` yielding a re-invocable Proc, which + # is undocumented but has worked consistently in Rails 6-8. The behavior + # stems from Rails building callback chains as re-callable Procs/lambdas. + # See: activerecord/lib/active_record/callbacks.rb and + # activesupport/lib/active_support/callbacks.rb in Rails source. + # The test suite includes `around_create_state_machine_test.rb` which + # validates this behavior continues to work on Rails upgrades. + # + # Unlike set_slug_with_retry (which falls back to timestamp suffix on exhaustion), + # this method raises after MAX_SLUG_GENERATION_ATTEMPTS because INSERT-time + # collisions requiring 10+ retries indicate a systemic issue that warrants attention. + def retry_create_on_slug_unique_violation + return yield unless slug_persisted? + # Skip savepoint overhead for nullable slug columns — INSERT-time slug + # collisions are impossible when slug is NULL at INSERT time. + return yield unless slug_column_not_null? + + # Each attempt runs in a savepoint so a unique-constraint violation does + # not abort the outer transaction in PostgreSQL. + # retry_if guard: Only retry if the record wasn't actually persisted. + # This handles the edge case where an after_create callback raises a + # slug-named RecordNotUnique on an already-persisted record — we don't + # want to retry the INSERT in that case. + # + # pre_retry_action: Recompute slug via compute_slug_for_retry before each + # retry. Although validation callbacks DO re-run on retry (before_validation + # fires again when yield is called), models using ensure_slug_for_insert + # typically guard with `slug.blank?`, so the slug isn't recomputed there. + # The pre_retry_action ensures a fresh slug is generated for each attempt. + with_slug_retry( + -> { self.slug = compute_slug_for_retry }, + retry_if: ->(_error) { !persisted? } + ) do + self.class.transaction(requires_new: true) { yield } + end + end + + # Check if the slug column is NOT NULL. This uses ActiveRecord's schema cache + # (columns_hash), which is already memoized. We don't add our own memoization + # to avoid staleness issues in development when schema changes. + def slug_column_not_null? + self.class.columns_hash["slug"]&.null == false + end + + protected + + # Generates a slug for retry attempts during INSERT-time race conditions. + # + # SUBCLASS OVERRIDE POINT: Override this method in subclasses to customize + # retry slug generation. For example, to use a different suffix strategy or + # incorporate additional uniqueness factors. + # + # Default behavior: delegates to compute_slug, which for attribute-based + # strategies calls generate_unique_slug (adds random suffixes on each call), + # ensuring retry attempts try different values. + # + # NOTE: ID-based slug collisions are impossible (two records can't share the same ID), + # so this method is only meaningful for attribute-based strategies where concurrent + # inserts can race to claim the same slug. + def compute_slug_for_retry + compute_slug + end + + private + + # Shared retry logic for slug unique constraint violations. + # Makes up to MAX_SLUG_GENERATION_ATTEMPTS total attempts (1 initial + N-1 retries), + # calling pre_retry_action before each retry to regenerate the slug. + # + # When retries are exhausted, calls the optional on_exhaustion proc if provided, + # otherwise re-raises the exception. The on_exhaustion proc can apply a timestamp + # fallback to maintain parity with generate_unique_slug's lenient behavior. + # + # KNOWN LIMITATION: This catches RecordNotUnique (DB constraint violation) but not + # RecordInvalid (AR validation failure). If another process steals the slug between + # the AR uniqueness validation SELECT and the actual UPDATE, save! raises RecordInvalid + # instead of RecordNotUnique. This is a narrow window and the DB constraint is the + # ultimate safeguard, but be aware that RecordInvalid from a race is not retried. + def with_slug_retry(pre_retry_action, retry_if: ->(_error) { true }, on_exhaustion: nil) + attempts = 0 + + begin + yield + rescue ActiveRecord::RecordNotUnique => e + raise unless slug_unique_violation?(e) + raise unless retry_if.call(e) + + attempts += 1 + if attempts >= MAX_SLUG_GENERATION_ATTEMPTS + on_exhaustion ? on_exhaustion.call : raise + else + pre_retry_action.call + retry + end + end end def update_slug_if_nil diff --git a/test/slugifiable/around_create_state_machine_test.rb b/test/slugifiable/around_create_state_machine_test.rb new file mode 100644 index 0000000..8bc35ba --- /dev/null +++ b/test/slugifiable/around_create_state_machine_test.rb @@ -0,0 +1,193 @@ +# frozen_string_literal: true + +require "test_helper" + +# Regression tests for around_create retry behavior. +# These verify that Rails state machine (new_record?, persisted?, dirty tracking) +# works correctly when yield is called multiple times via retry. +class Slugifiable::AroundCreateStateMachineTest < Minitest::Test + def setup + StrictSlugModel.delete_all + end + + # ========================================================================== + # Rails state machine behavior during retry + # ========================================================================== + + def test_new_record_status_correct_before_retry + state_tracker = Class.new(StrictSlugModel) do + self.table_name = "strict_slug_models" + + class_attribute :states_seen, instance_accessor: false, default: [] + class_attribute :should_fail_once, instance_accessor: false, default: true + + before_create do + self.class.states_seen << { + new_record: new_record?, + persisted: persisted?, + id_present: id.present? + } + + # Simulate collision on first attempt + if self.class.should_fail_once + self.class.should_fail_once = false + # Inject a row with our slug + conn = self.class.connection + now = Time.current + conn.execute( + <<~SQL + INSERT INTO strict_slug_models (title, slug, created_at, updated_at) + VALUES (#{conn.quote("Injected")}, #{conn.quote(slug)}, #{conn.quote(now)}, #{conn.quote(now)}) + SQL + ) + end + end + end + + record = state_tracker.create!(title: "State Test") + + assert record.persisted?, "Record should be persisted after create" + refute record.new_record?, "Record should not be new_record after create" + + # CRITICAL: Verify the second attempt actually ran. + # This catches Rails-upgrade breakage where around_create + retry stops working. + # If this changes from 2 to 1, it means the retry mechanism is broken. + assert_equal 2, state_tracker.states_seen.length, + "Should have exactly 2 attempts: 1 failed + 1 successful retry. " \ + "If this is 1, the around_create multi-yield behavior may have changed in Rails." + + # On first attempt, should be new_record with no ID yet + first_state = state_tracker.states_seen.first + assert first_state[:new_record], "Should be new_record on first attempt" + refute first_state[:persisted], "Should not be persisted on first attempt" + end + + def test_dirty_tracking_works_across_retry_attempts + dirty_tracker = Class.new(StrictSlugModel) do + self.table_name = "strict_slug_models" + + class_attribute :slug_changes_seen, instance_accessor: false, default: [] + class_attribute :should_fail_once, instance_accessor: false, default: true + + before_create do + self.class.slug_changes_seen << { + slug_changed: slug_changed?, + slug_was: slug_was, + slug_current: slug + } + + if self.class.should_fail_once + self.class.should_fail_once = false + conn = self.class.connection + now = Time.current + conn.execute( + <<~SQL + INSERT INTO strict_slug_models (title, slug, created_at, updated_at) + VALUES (#{conn.quote("Injected")}, #{conn.quote(slug)}, #{conn.quote(now)}, #{conn.quote(now)}) + SQL + ) + end + end + end + + record = dirty_tracker.create!(title: "Dirty Test") + + assert record.persisted? + # Should have multiple change records if retry happened + assert dirty_tracker.slug_changes_seen.any?, "Should have tracked slug changes" + end + + def test_record_id_assigned_only_after_successful_insert + id_tracker = Class.new(StrictSlugModel) do + self.table_name = "strict_slug_models" + + class_attribute :ids_seen, instance_accessor: false, default: [] + + before_create do + self.class.ids_seen << id + end + + after_create do + self.class.ids_seen << id + end + end + + record = id_tracker.create!(title: "ID Test") + + assert record.id.present?, "Record should have ID after create" + # Before create, ID should be nil; after create, it should be set + assert id_tracker.ids_seen.include?(nil), "ID should be nil before INSERT" + assert id_tracker.ids_seen.include?(record.id), "ID should be set after INSERT" + end + + # ========================================================================== + # Verify retry doesn't corrupt record state + # ========================================================================== + + def test_final_record_has_correct_attributes_after_retry + record = StrictSlugModel.create!(title: "Final State Test") + + # Verify the record is in a clean state + assert record.persisted? + refute record.new_record? + assert record.id.present? + assert record.slug.present? + assert record.title == "Final State Test" + refute record.changed?, "Record should not have unsaved changes after create" + end + + def test_record_can_be_updated_after_retry_create + record = StrictSlugModel.create!(title: "Update Test") + original_slug = record.slug + + record.update!(title: "Updated Title") + + assert_equal "Updated Title", record.title + assert_equal original_slug, record.slug, "Slug should not change on update" + end + + def test_record_can_be_reloaded_after_retry_create + record = StrictSlugModel.create!(title: "Reload Test") + record_id = record.id + + reloaded = StrictSlugModel.find(record_id) + + assert_equal record.title, reloaded.title + assert_equal record.slug, reloaded.slug + end + + # ========================================================================== + # Transaction integrity + # ========================================================================== + + def test_failed_retry_rolls_back_cleanly + # Create a model that always fails slug generation + always_fails = Class.new(StrictSlugModel) do + self.table_name = "strict_slug_models" + + class_attribute :attempt_count, instance_accessor: false, default: 0 + + before_create do + self.class.attempt_count += 1 + # Raise unique constraint error to simulate collision + raise ActiveRecord::RecordNotUnique, "UNIQUE constraint failed: strict_slug_models.slug" + end + end + + initial_count = StrictSlugModel.count + + assert_raises(ActiveRecord::RecordNotUnique) do + always_fails.create!(title: "Will Fail") + end + + # Verify retry mechanism was invoked the expected number of times + assert_equal Slugifiable::Model::MAX_SLUG_GENERATION_ATTEMPTS, always_fails.attempt_count, + "Should have attempted MAX_SLUG_GENERATION_ATTEMPTS times before giving up" + + # The failed record should not exist + assert_equal initial_count, StrictSlugModel.count, + "No records should be created after all retries fail" + refute StrictSlugModel.exists?(title: "Will Fail"), + "Failed record should not exist after rollback" + end +end diff --git a/test/slugifiable/bug_verification_test.rb b/test/slugifiable/bug_verification_test.rb index 3b7bb38..18aa7e2 100644 --- a/test/slugifiable/bug_verification_test.rb +++ b/test/slugifiable/bug_verification_test.rb @@ -137,7 +137,7 @@ def test_collision_fallback_to_timestamp model = TestModel.create!(title: "Timestamp Test") # Should fall back to timestamp + random suffix after MAX_SLUG_GENERATION_ATTEMPTS - assert_match(/\Atimestamp-test-\d+-\d+\z/, model.slug, "Should include timestamp and random suffix") + assert_match(/\Atimestamp-test-\d+-[a-f0-9]+\z/, model.slug, "Should include timestamp and random hex suffix") assert attempt_count >= Slugifiable::Model::MAX_SLUG_GENERATION_ATTEMPTS ensure TestModel.singleton_class.send(:remove_method, :exists?) diff --git a/test/slugifiable/collision_resolution_test.rb b/test/slugifiable/collision_resolution_test.rb index 1576854..61634b7 100644 --- a/test/slugifiable/collision_resolution_test.rb +++ b/test/slugifiable/collision_resolution_test.rb @@ -128,7 +128,7 @@ def test_timestamp_fallback_after_max_attempts Time.stub(:current, Time.at(frozen_time)) do model = TestModel.create!(title: "Timestamp Test") # Timestamp fallback now includes random suffix for extra uniqueness - assert_match(/\Atimestamp-test-#{frozen_time}-\d+\z/, model.slug) + assert_match(/\Atimestamp-test-#{frozen_time}-[a-f0-9]+\z/, model.slug) end ensure TestModel.singleton_class.send(:remove_method, :exists?) diff --git a/test/slugifiable/extensive_model_test.rb b/test/slugifiable/extensive_model_test.rb index 549e3e9..041f91f 100644 --- a/test/slugifiable/extensive_model_test.rb +++ b/test/slugifiable/extensive_model_test.rb @@ -216,7 +216,7 @@ def test_collision_resolution_with_timestamp_fallback TestModel.define_singleton_method(:exists?) { |_conditions| true } model = TestModel.create!(title: "Test") # Expect a slug in the format "test-" (timestamp is 10+ digits) - assert_match(/\Atest-\d{10,}-\d+\z/, model.slug, "Should fall back to timestamp with random suffix after max attempts") + assert_match(/\Atest-\d{10,}-[a-f0-9]+\z/, model.slug, "Should fall back to timestamp with random hex suffix after max attempts") ensure TestModel.singleton_class.send(:remove_method, :exists?) TestModel.define_singleton_method(:exists?, original_exists) # Restore original method diff --git a/test/slugifiable/insert_race_retry_test.rb b/test/slugifiable/insert_race_retry_test.rb new file mode 100644 index 0000000..203cef3 --- /dev/null +++ b/test/slugifiable/insert_race_retry_test.rb @@ -0,0 +1,147 @@ +# frozen_string_literal: true + +require "test_helper" + +# Regression coverage for INSERT-time slug collisions. +# +# These tests validate models that persist slugs before INSERT +# (e.g., NOT NULL slug columns with before_validation slug generation). +class Slugifiable::InsertRaceRetryTest < Minitest::Test + def setup + StrictSlugModel.delete_all + end + + def test_insert_retry_hook_exists + model = StrictSlugModel.new(title: "Hook Check") + + assert model.respond_to?(:retry_create_on_slug_unique_violation, true) + end + + def test_insert_time_slug_collision_retries_and_succeeds + race_model = build_race_model do + class_attribute :insert_attempts, instance_accessor: false, default: 0 + class_attribute :injected_once, instance_accessor: false, default: false + class_attribute :compute_slug_calls, instance_accessor: false, default: 0 + + define_method(:compute_slug) do + self.class.compute_slug_calls += 1 + super() + end + + before_create do + self.class.insert_attempts += 1 + next if self.class.injected_once + + self.class.injected_once = true + now = Time.current + conn = self.class.connection + + conn.execute( + <<~SQL + INSERT INTO strict_slug_models (title, slug, created_at, updated_at) + VALUES ( + #{conn.quote("Injected")}, + #{conn.quote(slug)}, + #{conn.quote(now)}, + #{conn.quote(now)} + ) + SQL + ) + end + end + + record = race_model.create!(title: "Acme") + + assert record.persisted? + assert_equal 2, race_model.insert_attempts, "expected one failed INSERT then one successful retry" + assert_equal 2, race_model.compute_slug_calls, + "expected compute_slug in before_validation and again before retry" + assert record.slug.start_with?("acme") + end + + def test_insert_retry_wraps_each_attempt_in_requires_new_transaction + race_model = build_race_model do + class_attribute :transaction_options_seen, instance_accessor: false, default: [] + + singleton_class.class_eval do + define_method(:transaction) do |*args, **options, &block| + self.transaction_options_seen += [options] + super(*args, **options, &block) + end + end + end + + race_model.create!(title: "Transaction Check") + + assert race_model.transaction_options_seen.any? { |options| options[:requires_new] }, + "insert retry should execute inside requires_new transaction savepoints" + end + + def test_after_create_does_not_recompute_slug_when_insert_slug_is_present + race_model = build_race_model do + class_attribute :compute_slug_calls, instance_accessor: false, default: 0 + + define_method(:compute_slug) do + self.class.compute_slug_calls += 1 + super() + end + end + + record = race_model.create!(title: "Stable Slug") + + assert_equal 1, race_model.compute_slug_calls, + "compute_slug should run once during before_validation, not again in after_create" + assert_equal record.slug, race_model.find(record.id).slug + end + + def test_insert_time_non_slug_record_not_unique_bubbles + race_model = build_race_model do + before_create do + raise ActiveRecord::RecordNotUnique, "UNIQUE constraint failed: strict_slug_models.external_id" + end + end + + assert_raises(ActiveRecord::RecordNotUnique) do + race_model.create!(title: "Acme") + end + end + + def test_retry_limit_is_enforced + race_model = build_race_model do + class_attribute :insert_attempts, instance_accessor: false, default: 0 + + before_create do + self.class.insert_attempts += 1 + raise ActiveRecord::RecordNotUnique, "UNIQUE constraint failed: strict_slug_models.slug" + end + end + + assert_raises(ActiveRecord::RecordNotUnique) do + race_model.create!(title: "Always Collides") + end + + assert_equal Slugifiable::Model::MAX_SLUG_GENERATION_ATTEMPTS, race_model.insert_attempts + end + + def test_not_null_slug_model_still_generates_unique_slugs_for_duplicate_titles + records = 20.times.map { StrictSlugModel.create!(title: "Popular Name") } + slugs = records.map(&:slug) + + assert_equal records.size, slugs.uniq.size + assert_equal "popular-name", records.first.slug + records.drop(1).each do |record| + assert record.slug.start_with?("popular-name-") + end + end + + private + + def build_race_model(&block) + klass = Class.new(StrictSlugModel) do + self.table_name = "strict_slug_models" + end + + klass.class_eval(&block) if block + klass + end +end diff --git a/test/slugifiable/model_test.rb b/test/slugifiable/model_test.rb index a7b4923..ce7a46a 100644 --- a/test/slugifiable/model_test.rb +++ b/test/slugifiable/model_test.rb @@ -169,7 +169,7 @@ def test_collision_resolution_with_timestamp_fallback begin TestModel.define_singleton_method(:exists?) { |_conditions| true } model = TestModel.create!(title: "Test") - assert_match(/\Atest-\d{10,}-\d+\z/, model.slug, "Should fall back to timestamp with random suffix after max attempts") + assert_match(/\Atest-\d{10,}-[a-f0-9]+\z/, model.slug, "Should fall back to timestamp with random hex suffix after max attempts") ensure TestModel.singleton_class.send(:remove_method, :exists?) TestModel.define_singleton_method(:exists?, original_exists) # Restore original method diff --git a/test/slugifiable/postgresql_insert_race_retry_test.rb b/test/slugifiable/postgresql_insert_race_retry_test.rb new file mode 100644 index 0000000..127558a --- /dev/null +++ b/test/slugifiable/postgresql_insert_race_retry_test.rb @@ -0,0 +1,142 @@ +# frozen_string_literal: true + +require "test_helper" + +# PostgreSQL-specific integration coverage for INSERT-time slug collisions. +# +# This test is optional and only runs when: +# - the `pg` gem is available, and +# - `SLUGIFIABLE_TEST_POSTGRES_URL` is set. +# +# It verifies that `retry_create_on_slug_unique_violation` wraps create attempts +# in `requires_new` transactions, so the outer transaction remains healthy after +# a unique-constraint collision. +class Slugifiable::PostgresqlInsertRaceRetryTest < Minitest::Test + POSTGRES_URL_ENV = "SLUGIFIABLE_TEST_POSTGRES_URL" + + def setup + ensure_pg_driver! + ensure_postgres_url! + establish_postgres_connection! + ensure_postgres_schema! + postgres_strict_slug_model.delete_all + end + + def teardown + return unless defined?(@postgres_base) + + @postgres_base.connection_pool.disconnect! + rescue StandardError + nil + end + + def test_insert_retry_keeps_outer_transaction_usable_after_collision + race_model = build_postgres_race_model do + class_attribute :insert_attempts, instance_accessor: false, default: 0 + class_attribute :injected_once, instance_accessor: false, default: false + + before_create do + self.class.insert_attempts += 1 + next if self.class.injected_once + + self.class.injected_once = true + conn = self.class.connection + now = Time.current + + conn.execute( + <<~SQL + INSERT INTO pg_strict_slug_models (title, slug, created_at, updated_at) + VALUES ( + #{conn.quote("Injected")}, + #{conn.quote(slug)}, + #{conn.quote(now)}, + #{conn.quote(now)} + ) + SQL + ) + end + end + + first_record = nil + second_record = nil + + race_model.transaction do + first_record = race_model.create!(title: "Acme") + second_record = race_model.create!(title: "Outer Tx Healthy") + end + + assert first_record.persisted? + assert second_record.persisted? + assert_equal 2, race_model.insert_attempts, "expected one failed INSERT then one successful retry" + assert_equal 2, race_model.where(title: ["Acme", "Outer Tx Healthy"]).count, + "outer transaction should stay usable after collision retry" + end + + private + + def ensure_pg_driver! + require "pg" + rescue LoadError + skip "PostgreSQL integration test skipped: install the `pg` gem." + end + + def ensure_postgres_url! + return unless postgres_url.empty? + + skip "PostgreSQL integration test skipped: set #{POSTGRES_URL_ENV}." + end + + def establish_postgres_connection! + postgres_base.establish_connection(postgres_url) + postgres_base.connection + rescue StandardError => e + skip "PostgreSQL integration test skipped: #{e.class}: #{e.message}" + end + + def ensure_postgres_schema! + conn = postgres_base.connection + + conn.create_table :pg_strict_slug_models, force: true do |t| + t.string :title + t.string :slug, null: false + t.timestamps + end + conn.add_index :pg_strict_slug_models, :slug, unique: true + end + + def postgres_url + ENV.fetch(POSTGRES_URL_ENV, "").strip + end + + def postgres_base + @postgres_base ||= Class.new(ActiveRecord::Base) do + self.abstract_class = true + end + end + + def postgres_strict_slug_model + @postgres_strict_slug_model ||= Class.new(postgres_base) do + include Slugifiable::Model + + self.table_name = "pg_strict_slug_models" + + generate_slug_based_on :title + before_validation :ensure_slug_for_insert, on: :create + + private + + def ensure_slug_for_insert + self.slug = compute_slug if slug.blank? + end + end + end + + def build_postgres_race_model(&block) + klass = Class.new(postgres_strict_slug_model) do + self.table_name = "pg_strict_slug_models" + end + + klass.class_eval(&block) if block + klass + end +end diff --git a/test/slugifiable/race_condition_retry_test.rb b/test/slugifiable/race_condition_retry_test.rb new file mode 100644 index 0000000..d59662e --- /dev/null +++ b/test/slugifiable/race_condition_retry_test.rb @@ -0,0 +1,351 @@ +# frozen_string_literal: true + +require "test_helper" + +# Regression tests for slug race condition handling +# +# These tests verify that the `set_slug_with_retry` mechanism properly handles +# ActiveRecord::RecordNotUnique exceptions that occur due to concurrent slug +# generation race conditions. +# +# The fix: When `set_slug` encounters a unique constraint violation on the slug +# column, it clears the slug and retries up to MAX_SLUG_GENERATION_ATTEMPTS times. + +class Slugifiable::RaceConditionRetryTest < Minitest::Test + def setup + TestModel.delete_all + SlugifiableTestHelper.reset_test_model! + TestModel.class_eval do + generate_slug_based_on :title + end + end + + # ========================================================================== + # Core Regression Tests: Prove the fix works + # ========================================================================== + + def test_set_slug_with_retry_exists + model = TestModel.new(title: "Test") + assert model.respond_to?(:set_slug_with_retry, true), + "Model should have set_slug_with_retry method" + end + + def test_slug_unique_violation_detection + model = TestModel.new(title: "Test") + + # Test that slug violations are detected + slug_error = ActiveRecord::RecordNotUnique.new("UNIQUE constraint failed: test_models.slug") + assert model.send(:slug_unique_violation?, slug_error), + "Should detect slug unique violation" + + # Test that non-slug violations are not detected + other_error = ActiveRecord::RecordNotUnique.new("UNIQUE constraint failed: test_models.email") + refute model.send(:slug_unique_violation?, other_error), + "Should not detect non-slug unique violation" + end + + def test_retry_mechanism_regenerates_slug_on_collision + # Create a model to occupy a slug + existing = TestModel.create!(title: "Collision Test") + original_slug = existing.slug + + # Create another model and simulate collision retry + new_model = TestModel.create!(title: "Collision Test") + + # Both should exist with different slugs + assert_equal 2, TestModel.count + refute_equal original_slug, new_model.slug, + "Retry should generate different slug on collision" + assert new_model.slug.start_with?("collision-test"), + "Retried slug should still be based on title" + end + + # ========================================================================== + # Edge Cases + # ========================================================================== + + def test_max_retries_exceeded_raises_error + race_model = build_update_race_model do + class_attribute :create_attempts, instance_accessor: false, default: 0 + class_attribute :update_attempts, instance_accessor: false, default: 0 + + before_create do + self.class.create_attempts += 1 + end + + before_update do + self.class.update_attempts += 1 + raise ActiveRecord::RecordNotUnique, "UNIQUE constraint failed: test_models.slug" + end + end + + assert_raises(ActiveRecord::RecordNotUnique) do + race_model.create!(title: "Always Collides") + end + + assert_equal 1, race_model.create_attempts, + "around_create should not retry INSERT when failure comes from after_create slug save" + # MAX retries + 1 fallback attempt with timestamp suffix + assert_equal Slugifiable::Model::MAX_SLUG_GENERATION_ATTEMPTS + 1, race_model.update_attempts + end + + def test_non_slug_unique_violations_bubble_up + race_model = build_update_race_model do + before_update do + raise ActiveRecord::RecordNotUnique, "UNIQUE constraint failed: test_models.email" + end + end + + assert_raises(ActiveRecord::RecordNotUnique) do + race_model.create!(title: "Bubble Up Test") + end + end + + def test_retry_clears_slug_before_regeneration + race_model = build_update_race_model do + class_attribute :slug_values_before_compute, instance_accessor: false, default: [] + class_attribute :injected_once, instance_accessor: false, default: false + + define_method(:compute_slug) do + self.class.slug_values_before_compute += [slug] + super() + end + + before_update do + next if self.class.injected_once || slug.blank? + + self.class.injected_once = true + now = Time.current + conn = self.class.connection + + conn.execute( + <<~SQL + INSERT INTO test_models (title, slug, created_at, updated_at) + VALUES ( + #{conn.quote("Injected")}, + #{conn.quote(slug)}, + #{conn.quote(now)}, + #{conn.quote(now)} + ) + SQL + ) + end + end + + race_model.create!(title: "Clear Slug Test") + + assert_equal [nil, nil], race_model.slug_values_before_compute + end + + # ========================================================================== + # Integration Tests: Real-world scenarios + # ========================================================================== + + def test_sequential_creates_with_same_title_all_get_unique_slugs + titles = ["Same Title"] * 20 + + models = titles.map { |title| TestModel.create!(title: title) } + slugs = models.map(&:slug) + + assert_equal 20, slugs.uniq.length, + "All 20 models with same title should have unique slugs" + + # First should be clean, rest should have suffixes + assert_equal "same-title", models.first.slug + models[1..].each do |model| + assert model.slug.start_with?("same-title-"), + "Subsequent models should have suffixed slugs" + end + end + + def test_update_slug_if_nil_regenerates_slug + model = TestModel.create!(title: "Nil Slug Test") + model.update_column(:slug, nil) + + # Force a reload to trigger after_find -> update_slug_if_nil + reloaded = TestModel.find(model.id) + + refute_nil reloaded.slug, "Slug should be regenerated" + assert reloaded.slug.include?("nil-slug-test") || reloaded.slug.to_i > 0, + "Regenerated slug should be based on title or ID" + end + + def test_set_slug_called_via_after_create_hook + # Verify the callback chain works + model = TestModel.create!(title: "After Create Hook") + + refute_nil model.slug, "Slug should be set via after_create" + assert_equal "after-create-hook", model.slug + end + + # NOTE: The after_create UPDATE path retry mechanism is tested indirectly by + # test_retry_clears_slug_before_regeneration. For direct collision testing + # with savepoints, see test/slugifiable/insert_race_retry_test.rb which tests + # the INSERT-time retry path for NOT NULL slug columns. + + def test_compute_base_slug_returns_raw_parameterized_value + model = TestModel.new(title: "Base Slug Test") + + # compute_base_slug should return the raw parameterized value without uniqueness suffixes + base_slug = model.send(:compute_base_slug) + + assert_equal "base-slug-test", base_slug, "Should return raw parameterized title" + end + + def test_compute_base_slug_handles_nil_attribute + model = TestModel.new(title: nil) + + # Should fallback to ID-based slug (Integer) when attribute is nil + # This matches compute_slug_based_on_attribute's fallback behavior + base_slug = model.send(:compute_base_slug) + + assert_kind_of Integer, base_slug + end + + def test_compute_base_slug_handles_blank_attribute + model = TestModel.new(title: " ") + + # Should fallback to ID-based slug (Integer) when parameterized value is blank + # This matches compute_slug_based_on_attribute's fallback behavior + base_slug = model.send(:compute_base_slug) + + assert_kind_of Integer, base_slug + end + + def test_compute_base_slug_with_id_based_strategy + # Create a model that uses default ID-based strategy + id_based_model = Class.new(TestModel) do + self.table_name = "test_models" + + # Override to use default ID-based strategy + def determine_slug_generation_method + [:compute_slug_as_string, {}] + end + end + + model = id_based_model.new(title: "Ignored Title") + model.id = 123 + + # For ID-based strategy, compute_base_slug delegates to compute_slug + base_slug = model.send(:compute_base_slug) + + assert_kind_of String, base_slug + end + + def test_compute_base_slug_when_attribute_missing + # Create a model configured to use a non-existent attribute + model = TestModel.new(title: "Has Title") + + # Temporarily modify the model to reference a non-existent attribute + def model.determine_slug_generation_method + [:compute_slug_based_on_attribute, :nonexistent_attribute] + end + + # Should fallback to ID-based slug (Integer) + # This matches compute_slug_based_on_attribute's fallback behavior + base_slug = model.send(:compute_base_slug) + + assert_kind_of Integer, base_slug + end + + # ========================================================================== + # Regression tests for reviewer-identified issues + # ========================================================================== + + def test_compute_base_slug_nil_fallback_matches_compute_slug_nil_fallback + # Reviewer concern: compute_base_slug and compute_slug_based_on_attribute should + # have consistent fallback behavior for nil attributes. + # + # FIXED: Both now use generate_random_number_based_on_id_hex (returns Integer) + model = TestModel.new(title: nil) + model.id = 123 + + base_slug = model.send(:compute_base_slug) + full_slug = model.compute_slug + + # Both should return Integer (generate_random_number_based_on_id_hex) + assert_kind_of Integer, base_slug, "compute_base_slug should return Integer for nil" + assert_kind_of Integer, full_slug, "compute_slug should return Integer for nil" + + # Both should return the same value for same ID + assert_equal base_slug, full_slug, "Both methods should return same fallback value" + end + + def test_compute_base_slug_blank_fallback_matches_compute_slug_blank_fallback + # Test blank (whitespace-only) title behavior + model = TestModel.new(title: " ") + model.id = 456 + + base_slug = model.send(:compute_base_slug) + full_slug = model.compute_slug + + # Both should return Integer (generate_random_number_based_on_id_hex) + assert_kind_of Integer, base_slug, "compute_base_slug should return Integer for blank" + assert_kind_of Integer, full_slug, "compute_slug should return Integer for blank" + + # Both should return the same value for same ID + assert_equal base_slug, full_slug, "Both methods should return same fallback value" + end + + def test_exhaustion_fallback_raises_on_continued_unique_violation + # Test that RecordNotUnique exceptions in exhaustion fallback propagate + always_fails_model = build_update_race_model do + class_attribute :exhaustion_reached, instance_accessor: false, default: false + + before_update do + # Mark when we've gone past normal retries + if slug&.include?("-#{Time.current.to_i}-") + self.class.exhaustion_reached = true + end + # Always fail + raise ActiveRecord::RecordNotUnique, "UNIQUE constraint failed: test_models.slug" + end + end + + # This should raise because even exhaustion fallback fails + assert_raises(ActiveRecord::RecordNotUnique) do + always_fails_model.create!(title: "Exhaustion Test") + end + + # Verify exhaustion was actually attempted + assert always_fails_model.exhaustion_reached, + "Should have attempted exhaustion fallback with timestamp slug" + end + + def test_exhaustion_fallback_raises_on_validation_error + # Verify that validation errors in the exhaustion fallback path raise + # (save! is used, not save). A validation error during exhaustion should + # not be silently swallowed. + # + # We test this by verifying the source code uses save! rather than save. + # This approach is simpler than trying to trigger the exact behavior, + # which requires complex setup with anonymous classes that have Rails + # naming issues. The behavioral test test_exhaustion_fallback_raises_on_continued_unique_violation + # already covers that the exhaustion path propagates RecordNotUnique. + model = TestModel.new(title: "Save Bang Test") + + source_location = model.method(:set_slug_with_retry).source_location + refute_nil source_location, "Should be able to locate source" + + file_path, _line = source_location + source = File.read(file_path) + + # Verify save! is used in both the retry block AND the exhaustion fallback + # (both should propagate failures, not swallow them) + assert_match(/on_exhaustion.*\{.*self\.save!.*\}/m, source, + "on_exhaustion should use self.save! to ensure failures propagate") + assert_match(/with_slug_retry.*do.*self\.save!.*end/m, source, + "retry block should use self.save! to ensure failures propagate") + end + + private + + def build_update_race_model(&block) + klass = Class.new(TestModel) do + self.table_name = "test_models" + end + + klass.class_eval(&block) if block + klass + end +end diff --git a/test/slugifiable/slug_unique_violation_detection_test.rb b/test/slugifiable/slug_unique_violation_detection_test.rb new file mode 100644 index 0000000..d548de6 --- /dev/null +++ b/test/slugifiable/slug_unique_violation_detection_test.rb @@ -0,0 +1,166 @@ +# frozen_string_literal: true + +require "test_helper" + +# Regression tests for slug_unique_violation? detection across different +# database adapters and error message formats. +class Slugifiable::SlugUniqueViolationDetectionTest < Minitest::Test + def setup + @model = TestModel.new(title: "Test") + end + + # ========================================================================== + # SQLite error format tests + # ========================================================================== + + def test_detects_sqlite_slug_violation + error = ActiveRecord::RecordNotUnique.new( + "UNIQUE constraint failed: test_models.slug" + ) + assert @model.send(:slug_unique_violation?, error), + "Should detect SQLite slug constraint violation" + end + + def test_does_not_detect_sqlite_other_column_violation + error = ActiveRecord::RecordNotUnique.new( + "UNIQUE constraint failed: test_models.email" + ) + refute @model.send(:slug_unique_violation?, error), + "Should NOT detect SQLite violation on non-slug column" + end + + # ========================================================================== + # PostgreSQL error format tests + # ========================================================================== + + def test_detects_postgresql_slug_violation_via_detail + # PostgreSQL includes DETAIL line with the column name + error = ActiveRecord::RecordNotUnique.new( + "PG::UniqueViolation: ERROR: duplicate key value violates unique " \ + "constraint \"index_users_on_slug\"\nDETAIL: Key (slug)=(my-slug) already exists." + ) + assert @model.send(:slug_unique_violation?, error), + "Should detect PostgreSQL slug violation via DETAIL line" + end + + def test_detects_postgresql_slug_violation_via_constraint_name + # Even without DETAIL, constraint name contains "on_slug" + error = ActiveRecord::RecordNotUnique.new( + "PG::UniqueViolation: ERROR: duplicate key value violates unique " \ + "constraint \"index_users_on_slug\"" + ) + assert @model.send(:slug_unique_violation?, error), + "Should detect PostgreSQL slug violation via index name" + end + + def test_does_not_detect_postgresql_other_column_violation + error = ActiveRecord::RecordNotUnique.new( + "PG::UniqueViolation: ERROR: duplicate key value violates unique " \ + "constraint \"index_users_on_email\"\nDETAIL: Key (email)=(test@example.com) already exists." + ) + refute @model.send(:slug_unique_violation?, error), + "Should NOT detect PostgreSQL violation on non-slug column" + end + + # ========================================================================== + # MySQL error format tests + # ========================================================================== + + def test_detects_mysql_slug_violation + # MySQL format with Rails-default index name + error = ActiveRecord::RecordNotUnique.new( + "Duplicate entry 'my-slug' for key 'index_posts_on_slug'" + ) + assert @model.send(:slug_unique_violation?, error), + "Should detect MySQL slug violation via index name" + end + + def test_detects_mysql_slug_column_directly + # MySQL might also show column name directly in some cases + error = ActiveRecord::RecordNotUnique.new( + "Duplicate entry 'my-slug' for key 'slug'" + ) + assert @model.send(:slug_unique_violation?, error), + "Should detect MySQL slug violation via column name" + end + + def test_does_not_detect_mysql_other_column_violation + error = ActiveRecord::RecordNotUnique.new( + "Duplicate entry 'test@example.com' for key 'index_users_on_email'" + ) + refute @model.send(:slug_unique_violation?, error), + "Should NOT detect MySQL violation on non-slug column" + end + + # ========================================================================== + # False positive prevention tests - CRITICAL + # These test that columns ending in "_slug" don't trigger false matches + # ========================================================================== + + def test_does_not_detect_canonical_slug_violation + error = ActiveRecord::RecordNotUnique.new( + "UNIQUE constraint failed: posts.canonical_slug" + ) + refute @model.send(:slug_unique_violation?, error), + "Should NOT detect violation on 'canonical_slug' column (false positive)" + end + + def test_does_not_detect_parent_slug_violation + error = ActiveRecord::RecordNotUnique.new( + "UNIQUE constraint failed: categories.parent_slug" + ) + refute @model.send(:slug_unique_violation?, error), + "Should NOT detect violation on 'parent_slug' column (false positive)" + end + + def test_does_not_detect_original_slug_violation + error = ActiveRecord::RecordNotUnique.new( + "Duplicate entry 'foo' for key 'index_posts_on_original_slug'" + ) + refute @model.send(:slug_unique_violation?, error), + "Should NOT detect MySQL violation on 'original_slug' index (false positive)" + end + + def test_does_not_detect_user_slug_violation + error = ActiveRecord::RecordNotUnique.new( + "PG::UniqueViolation: ERROR: duplicate key value violates unique " \ + "constraint \"index_comments_on_user_slug\"\nDETAIL: Key (user_slug)=(foo) already exists." + ) + refute @model.send(:slug_unique_violation?, error), + "Should NOT detect PostgreSQL violation on 'user_slug' column (false positive)" + end + + # ========================================================================== + # Edge cases + # ========================================================================== + + def test_handles_nil_error_message + error = ActiveRecord::RecordNotUnique.new(nil) + refute @model.send(:slug_unique_violation?, error), + "Should handle nil error message gracefully" + end + + def test_handles_empty_error_message + error = ActiveRecord::RecordNotUnique.new("") + refute @model.send(:slug_unique_violation?, error), + "Should handle empty error message gracefully" + end + + def test_detects_via_cause_message + # Some adapters wrap the original error + cause = StandardError.new("Key (slug)=(my-slug) already exists") + error = ActiveRecord::RecordNotUnique.new("Wrapped error") + error.define_singleton_method(:cause) { cause } + + assert @model.send(:slug_unique_violation?, error), + "Should detect slug violation via error.cause.message" + end + + def test_case_insensitive_detection + error = ActiveRecord::RecordNotUnique.new( + "UNIQUE CONSTRAINT FAILED: TEST_MODELS.SLUG" + ) + assert @model.send(:slug_unique_violation?, error), + "Should detect slug violation case-insensitively" + end +end diff --git a/test/test_helper.rb b/test/test_helper.rb index f071bd1..7f7ef2e 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -39,11 +39,19 @@ t.string :slug t.timestamps end + add_index :test_models, :slug, unique: true create_table :test_models_without_slug do |t| t.string :title t.timestamps end + + create_table :strict_slug_models do |t| + t.string :title + t.string :slug, null: false + t.timestamps + end + add_index :strict_slug_models, :slug, unique: true end # Test model with slug column @@ -65,6 +73,21 @@ class TestModelWithoutSlug < ActiveRecord::Base # (none in our test case) end +# Model that requires a slug before INSERT (NOT NULL schema). +# This mirrors the organizations gem integration mode. +class StrictSlugModel < ActiveRecord::Base + include Slugifiable::Model + generate_slug_based_on :title + + before_validation :ensure_slug_for_insert, on: :create + + private + + def ensure_slug_for_insert + self.slug = compute_slug if slug.blank? + end +end + # Helper module for resetting test model state module SlugifiableTestHelper # List of methods that tests might define on TestModel that need cleanup