diff --git a/CHANGELOG.md b/CHANGELOG.md index c600bb5..20ce3f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,25 @@ +## [0.2.1] - 2026-02-19 + +### Fixed +- **Race condition in slug generation**: When two processes create records with the same base slug simultaneously, the second process now retries with a new random suffix instead of crashing with `RecordNotUnique` + +### Added +- `around_create :retry_create_on_slug_unique_violation` for NOT NULL slug columns (pre-INSERT collision handling) +- `with_slug_retry` helper with PostgreSQL-safe savepoints (`requires_new: true`) +- `slug_column_not_null?` optimization to skip savepoint overhead for nullable slug columns +- `slug_unique_violation?` detection supporting SQLite, PostgreSQL, and MySQL error formats + +### Changed +- `set_slug` now uses shared retry handling with savepoints on `RecordNotUnique` for slug collisions (after_create path) +- Retry exhaustion raises `RecordNotUnique` instead of falling back to timestamp suffix (fail-fast behavior) +- `update_slug_if_nil` explicitly uses non-bang `save` to avoid exceptions on read operations + +### Known Limitations +- **`around_create` retries rely on Rails internals**: The NOT NULL create retry path re-invokes the `around_create` callback chain. This behavior is covered by tests, but it is not explicitly documented as a public Rails callback contract. +- **`before_create` callbacks will re-execute on retry**: If a retry is needed, `before_create` callbacks run again. Design callbacks to be idempotent or use guards. +- **`RecordInvalid` is not retried**: Only DB-level `RecordNotUnique` triggers retry. A validation-layer uniqueness race that raises `RecordInvalid` will bubble up. +- **Custom index names**: If your slug unique index has a non-standard name that doesn't contain `slug` or `_on_slug`, violations will bubble up instead of retrying. + ## [0.2.0] - 2026-01-16 - Added a full Minitest test suite diff --git a/README.md b/README.md index e16f659..cc689ea 100644 --- a/README.md +++ b/README.md @@ -80,9 +80,53 @@ 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. +### Nullable vs NOT NULL Slug Columns + +**Nullable columns (default, simpler):** +```ruby +# migration +add_column :products, :slug, :string +add_index :products, :slug, unique: true + +# model +class Product < ApplicationRecord + include Slugifiable::Model + generate_slug_based_on :name +end +``` + +**NOT NULL columns (requires `before_validation` setup):** +```ruby +# migration +add_column :products, :slug, :string, null: false +add_index :products, :slug, unique: true + +# model +class Product < ApplicationRecord + include Slugifiable::Model + generate_slug_based_on :name + + before_validation :ensure_slug_present, on: :create + + private + + def ensure_slug_present + self.slug = compute_slug if slug.blank? + end +end +``` + +> [!NOTE] +> When using NOT NULL slug columns, `slugifiable` handles race conditions automatically. If two processes try to create records with the same slug simultaneously, the second one will retry with a new random suffix. + +> [!CAUTION] +> For NOT NULL slug columns, retries re-run the `around_create` callback chain. If a slug collision happens, your `before_create` callbacks will run again. Keep `before_create` callbacks idempotent (or move side effects to `after_create`/background jobs). + +> [!NOTE] +> Retry handling is DB-constraint-driven (`RecordNotUnique`), not validation-driven. A validation-layer race that raises `RecordInvalid` will bubble up. + +> [!NOTE] +> Slug collision detection matches errors containing `slug`/`_on_slug`. If your index uses a custom name that does not include those patterns, retries will not trigger and the exception will bubble up. If you're generating slugs based off the model `id`, you can also set a desired length: ```ruby diff --git a/lib/slugifiable/model.rb b/lib/slugifiable/model.rb index e3bd5d6..9bcfa7c 100644 --- a/lib/slugifiable/model.rb +++ b/lib/slugifiable/model.rb @@ -33,11 +33,12 @@ 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 raising. + # Also used by generate_unique_slug for EXISTS? check loop (with timestamp 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 @@ -138,6 +139,53 @@ def compute_slug_based_on_attribute(attribute_name) private + # S1: around_create retry for NOT NULL slug columns (pre-INSERT collision) + # S2: Uses savepoint (requires_new: true) for PostgreSQL compatibility + # S4: Skips overhead when slug column is nullable (no INSERT-time collision possible) + def retry_create_on_slug_unique_violation + return yield unless slug_persisted? && slug_column_not_null? + + with_slug_retry(for_insert: true) do |attempts| + # `around_create` retries can re-enter create callbacks. Set a fresh slug + # before retrying so pre-insert slug strategies don't reuse a collided value. + self.slug = compute_slug if attempts.positive? + yield + end + end + + # S3: Shared retry helper with savepoints for both paths + # PostgreSQL requires savepoints: without them, retry fails with + # "current transaction is aborted, commands ignored until end" + def with_slug_retry(for_insert: false) + attempts = 0 + begin + self.class.transaction(requires_new: true) { yield(attempts) } + rescue ActiveRecord::RecordNotUnique => e + raise unless slug_unique_violation?(e) + raise if for_insert && persisted? # Already inserted; don't retry whole create + + attempts += 1 + raise if attempts >= MAX_SLUG_GENERATION_ATTEMPTS # S6: Raise on exhaustion + + retry + end + end + + # S4: Optimization guard - skip savepoint wrapper when slug is nullable + def slug_column_not_null? + return false unless self.class.respond_to?(:columns_hash) + column = self.class.columns_hash["slug"] + column && !column.null + end + + # S5: Detect slug unique violations across PostgreSQL/MySQL/SQLite + # Pattern matches: "slug" as word boundary OR "_on_slug" (index naming convention) + # Safe false-negative for custom index names (error bubbles up instead of silent retry) + def slug_unique_violation?(error) + message = error.message.to_s.downcase + message.match?(/\bslug\b|_on_slug\b/) + end + def normalize_length(length, default, max) length = length.to_i return default if length <= 0 @@ -170,7 +218,7 @@ 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 + if attempts >= MAX_SLUG_GENERATION_ATTEMPTS slug_candidate = "#{base_slug}-#{Time.current.to_i}-#{SecureRandom.random_number(1000)}" end @@ -218,15 +266,25 @@ def has_slug_method? self.class.method_defined?(:slug) || self.class.private_method_defined?(:slug) end + # S1: after_create retry for nullable slug columns (post-INSERT collision) + # S2: Uses savepoint via with_slug_retry for PostgreSQL compatibility def set_slug return unless slug_persisted? + return unless slug.blank? - self.slug = compute_slug if id_changed? || slug.blank? - self.save + with_slug_retry do |_attempts| + self.slug = compute_slug + save! + end end + # S7: Non-bang save for after_find repair path to avoid read-time exceptions + # This path handles legacy records that may have nil slugs def update_slug_if_nil - set_slug if slug_persisted? && self.slug.nil? + return unless slug_persisted? && slug.nil? + + self.slug = compute_slug + save # Non-bang: read operations should not raise end end diff --git a/test/slugifiable/race_condition_test.rb b/test/slugifiable/race_condition_test.rb new file mode 100644 index 0000000..f86d2ab --- /dev/null +++ b/test/slugifiable/race_condition_test.rb @@ -0,0 +1,240 @@ +# frozen_string_literal: true + +require "test_helper" + +# Focused behavioral tests for the race-condition retry paths. +class RaceConditionTest < Minitest::Test + def setup + TestModel.delete_all + StrictSlugModel.delete_all + SlugifiableTestHelper.reset_test_model! + end + + def teardown + TestModel.delete_all + StrictSlugModel.delete_all + SlugifiableTestHelper.reset_test_model! + end + + def test_sequential_creates_with_same_name_get_unique_slugs + TestModel.generate_slug_based_on :title + + 5.times { TestModel.create!(title: "Same Name") } + + slugs = TestModel.pluck(:slug) + assert_equal 5, slugs.uniq.count + assert slugs.all? { |slug| slug.start_with?("same-name") } + end + + def test_sequential_creates_for_not_null_model_get_unique_slugs + 5.times { StrictSlugModel.create!(name: "Same Name") } + + slugs = StrictSlugModel.pluck(:slug) + assert_equal 5, slugs.uniq.count + assert slugs.all? { |slug| slug.start_with?("same-name") } + end + + def test_slug_unique_violation_detection_sqlite + model = TestModel.new + error = ActiveRecord::RecordNotUnique.new("UNIQUE constraint failed: test_models.slug") + + assert model.send(:slug_unique_violation?, error) + end + + def test_slug_unique_violation_detection_postgresql + model = TestModel.new + error = ActiveRecord::RecordNotUnique.new("duplicate key value violates unique constraint \"index_organizations_on_slug\"") + + assert model.send(:slug_unique_violation?, error) + end + + def test_slug_unique_violation_detection_mysql + model = TestModel.new + error = ActiveRecord::RecordNotUnique.new("Duplicate entry 'acme-corp' for key 'index_organizations_on_slug'") + + assert model.send(:slug_unique_violation?, error) + end + + def test_non_slug_violation_does_not_match + model = TestModel.new + error = ActiveRecord::RecordNotUnique.new("Duplicate entry 'test@example.com' for key 'index_users_on_email'") + + refute model.send(:slug_unique_violation?, error) + end + + def test_false_positive_prevention_for_slugged_table_names + model = TestModel.new + error = ActiveRecord::RecordNotUnique.new("UNIQUE constraint failed: slugged_items.name") + + refute model.send(:slug_unique_violation?, error) + end + + def test_slug_column_not_null_detection + refute TestModel.new.send(:slug_column_not_null?) + assert StrictSlugModel.new.send(:slug_column_not_null?) + end + + def test_retry_create_skips_nullable_slug_columns + TestModel.generate_slug_based_on :title + model = TestModel.new(title: "Skip") + + model.define_singleton_method(:with_slug_retry) do |for_insert: false, &_block| + flunk("with_slug_retry should not run for nullable slug columns") + end + + result = model.send(:retry_create_on_slug_unique_violation) { :ok } + assert_equal :ok, result + end + + def test_retry_create_uses_insert_mode_for_not_null_slug_columns + model = StrictSlugModel.new(name: "Acme") + insert_mode = nil + + model.define_singleton_method(:with_slug_retry) do |for_insert: false, &block| + insert_mode = for_insert + block.call(0) + end + + result = model.send(:retry_create_on_slug_unique_violation) { :ok } + assert_equal :ok, result + assert_equal true, insert_mode + end + + def test_with_slug_retry_runs_inside_savepoint + model = TestModel.new + transaction_depths = [] + + model.send(:with_slug_retry) do |_attempts| + transaction_depths << model.class.connection.open_transactions + :ok + end + + assert transaction_depths.any? { |depth| depth >= 1 } + end + + def test_with_slug_retry_retries_slug_violation_for_insert_path + model = StrictSlugModel.new(name: "Acme") + block_calls = 0 + + model.send(:with_slug_retry, for_insert: true) do |attempts| + block_calls += 1 + if attempts.zero? + raise ActiveRecord::RecordNotUnique.new("duplicate key value violates unique constraint \"index_strict_slug_models_on_slug\"") + end + + :ok + end + + assert_equal 2, block_calls + end + + def test_with_slug_retry_bubbles_non_slug_violations + model = TestModel.new + + assert_raises(ActiveRecord::RecordNotUnique) do + model.send(:with_slug_retry) do |_attempts| + raise ActiveRecord::RecordNotUnique.new("duplicate key value violates unique constraint \"index_users_on_email\"") + end + end + end + + def test_with_slug_retry_raises_after_max_attempts + model = StrictSlugModel.new(name: "Acme") + block_calls = 0 + + assert_raises(ActiveRecord::RecordNotUnique) do + model.send(:with_slug_retry, for_insert: true) do |_attempts| + block_calls += 1 + raise ActiveRecord::RecordNotUnique.new("duplicate key value violates unique constraint \"index_strict_slug_models_on_slug\"") + end + end + + assert_equal Slugifiable::Model::MAX_SLUG_GENERATION_ATTEMPTS, block_calls + end + + def test_with_slug_retry_does_not_retry_insert_after_record_is_persisted + model = StrictSlugModel.create!(name: "Persisted") + block_calls = 0 + + assert_raises(ActiveRecord::RecordNotUnique) do + model.send(:with_slug_retry, for_insert: true) do |_attempts| + block_calls += 1 + raise ActiveRecord::RecordNotUnique.new("duplicate key value violates unique constraint \"index_strict_slug_models_on_slug\"") + end + end + + assert_equal 1, block_calls + end + + def test_set_slug_retries_after_slug_record_not_unique + TestModel.generate_slug_based_on :title + record = TestModel.create!(title: "Retry Path") + record.update_column(:slug, nil) + + save_calls = 0 + record.define_singleton_method(:save!) do |*args, **kwargs| + save_calls += 1 + if save_calls == 1 + raise ActiveRecord::RecordNotUnique.new("UNIQUE constraint failed: test_models.slug") + end + + super(*args, **kwargs) + end + + record.send(:set_slug) + record.reload + + assert_equal 2, save_calls + assert_match(/\Aretry-path/, record.slug) + end + + def test_set_slug_bubbles_non_slug_record_not_unique + TestModel.generate_slug_based_on :title + record = TestModel.create!(title: "Retry Path") + record.update_column(:slug, nil) + + save_calls = 0 + record.define_singleton_method(:save!) do |*args, **kwargs| + save_calls += 1 + raise ActiveRecord::RecordNotUnique.new("duplicate key value violates unique constraint \"index_users_on_email\"") + end + + assert_raises(ActiveRecord::RecordNotUnique) { record.send(:set_slug) } + assert_equal 1, save_calls + end + + def test_after_find_repair_sets_slug_for_nil + TestModel.generate_slug_based_on :title + record = TestModel.create!(title: "Legacy Record") + TestModel.where(id: record.id).update_all(slug: nil) + + reloaded = TestModel.find(record.id) + + assert_equal "legacy-record", reloaded.slug + end + + def test_update_slug_if_nil_uses_non_bang_save + TestModel.generate_slug_based_on :title + record = TestModel.create!(title: "Legacy Record") + record.update_column(:slug, nil) + + record.define_singleton_method(:save) { false } + record.define_singleton_method(:save!) { raise "save! should not be called from update_slug_if_nil" } + + record.send(:update_slug_if_nil) + + assert record.slug.present? + end + + def test_generate_slug_based_on_symbol_id_uses_default_string_strategy + TestModel.generate_slug_based_on :id + + model = TestModel.create!(title: "ID Strategy") + + assert_match(/\A[0-9a-f]{11}\z/, model.slug) + end + + def test_max_attempts_constant_exists + assert_equal 10, Slugifiable::Model::MAX_SLUG_GENERATION_ATTEMPTS + end +end diff --git a/test/test_helper.rb b/test/test_helper.rb index f071bd1..639638d 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -40,10 +40,21 @@ t.timestamps end + add_index :test_models, :slug, unique: true + create_table :test_models_without_slug do |t| t.string :title t.timestamps end + + # Model with NOT NULL slug for testing around_create retry path + create_table :strict_slug_models do |t| + t.string :name + t.string :slug, null: false + t.timestamps + end + + add_index :strict_slug_models, :slug, unique: true end # Test model with slug column @@ -65,6 +76,22 @@ class TestModelWithoutSlug < ActiveRecord::Base # (none in our test case) end +# Test model with NOT NULL slug column (tests around_create retry path) +# This simulates the organizations gem setup where slug is required at INSERT time +class StrictSlugModel < ActiveRecord::Base + include Slugifiable::Model + generate_slug_based_on :name + + # Must set slug before validation (like organizations gem does) + before_validation :ensure_slug_present, on: :create + + private + + def ensure_slug_present + 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 @@ -79,6 +106,7 @@ module SlugifiableTestHelper slug_source virtual_attribute title_with_location + ensure_slug_present ].freeze def self.reset_test_model! @@ -98,4 +126,8 @@ def self.reset_test_model! end end end + + def self.reset_strict_slug_model! + StrictSlugModel.delete_all + end end