Better handling of unique and sparse index constraints.

CHANGELOG.md

@@ -6,6 +6,7 @@
 * [#219](https://github.com/digitalplaywright/mongoid-slug/pull/219): Mongoid HEAD and Rails 5.0.0.rc1 support - [@Fudoshiki](https://github.com/Fudoshiki).
 * [#224](https://github.com/digitalplaywright/mongoid-slug/pull/224): Use Danger, PR linter - [@dblock](https://github.com/dblock).
 * [#222](https://github.com/digitalplaywright/mongoid-slug/pull/225): Fix: `Mongo::Error::OperationFailure: E11000 duplicate key error index` error with blank slugs, default `_slugs` to `nil` instead of `[]` - [@dblock](https://github.com/dblock).
+* [#172](https://github.com/digitalplaywright/mongoid-slug/pull/172): Improved handling of unique and sparse index constraints - [@johnnyshields](https://github.com/johnnyshields).
 * Your contribution here.
 
 ## 5.2.0 (2016/01/03)

UPGRADING.md

@@ -1,5 +1,7 @@
 ### Upgrading to 5.3.0
 
+#### Default value for _slugs is now nil
+
 Neither `_slugs` nor `slugs` default to an empty `Array` and `nil` slugs are allowed since a sparse index is used. Previously a confusing `Mongo::Error::OperationFailure: E11000 duplicate key error index` error was raised on the second record with an empty set of slugs inserted into the database.
 
 You should unset `_slugs` for any documents that have an empty array value.
@@ -11,6 +13,12 @@ Klass.where(_slugs: nil).unset(:_slugs)
 
 See [#225](https://github.com/digitalplaywright/mongoid-slug/pull/225) for more information.
 
+#### Changes in unique index definitions
+
+The `:sparse` option on the `_slugs` index is now set in all cases. The `:unique` option is set except in an edge case with Paranoid documents.
+
+See [#172](https://github.com/digitalplaywright/mongoid-slug/pull/172) and [#227](https://github.com/digitalplaywright/mongoid-slug/pull/227) for more information.
+
 ### Upgrading to 5.2.0
 
 MongoDB has a default limit around 1KB to the size of the index keys and will raise error 17280, `key too large to index` when trying to create a record that causes an index key to exceed that limit. MongoDB 2.4 did not enforce this limit (see [mongodb#SERVER-5290](https://jira.mongodb.org/browse/SERVER-5290)). MongoDB 2.6 corrected this. Slugs of the form `text[-number]` and the text portion is limited in size to `Mongoid::Slug::MONGO_INDEX_KEY_LIMIT_BYTES - 32` bytes since mongoid-slug 5.2. To restore old behavior, change the constant in your code or set the limit for each model to `nil`. This can be useful when running MongoDB with [failIndexKeyTooLong](https://docs.mongodb.org/manual/reference/parameters/#param.failIndexKeyTooLong) set to `false`, however be advised that MongoDB will make those documents impossible to find when the index is used. Note that setting this limit for exsiting data has no effect on such data.

lib/mongoid/slug/index.rb

@@ -6,16 +6,40 @@ module Index
       #
       # @return [ Array(Hash, Hash) ] the indexable fields and index options.
       def self.build_index(scope_key = nil, by_model_type = false)
-        fields  = { _slugs: 1 }
-        options = {}
-
+        # The order of field keys is intentional.
+        # See: http://docs.mongodb.org/manual/core/index-compound/
+        fields = {}
+        fields.merge!(_type: 1)       if by_model_type
         fields.merge!(scope_key => 1) if scope_key
+        fields.merge!(_slugs: 1)
 
-        if by_model_type
-          fields.merge!(_type: 1)
-        else
-          options.merge!(unique: true, sparse: true)
-        end
+        # By design, we use the unique index constraint when possible to enforce slug uniqueness.
+        # When migrating legacy data to Mongoid slug, the _slugs field may be null on many records,
+        # hence we set the sparse index option to ignore these from the unique index.
+        # See: http://docs.mongodb.org/manual/core/index-sparse/
+        #
+        # There are three edge cases where the index must not be unique:
+        #
+        # 1) Legacy tables with `scope_key`. The sparse indexes on compound keys (scope + _slugs) are
+        #    whenever ANY of the key values are present (e.g. when scope is set and _slugs is unset),
+        #    and collisions will occur when multiple records have the same scope but null slugs.
+        #
+        # 2) Single Table Inheritance (`by_model_type`). MongoDB creates indexes on the parent collection,
+        #    irrespective of how STI is defined in Mongoid, i.e. ANY child index will be applied to EVERY child.
+        #    This can cause collisions using various combinations of scopes.
+        #
+        # 3) Paranoid docs rely on sparse indexes to exclude paranoid-deleted records
+        #    from the unique index constraint (i.e. when _slugs is unset.) However, when
+        #    using compound keys (`by_model_type` or `scope_key`), paranoid-deleted records
+        #    can become inadvertently indexed when _slugs is unset, causing duplicates. This
+        #    is already covered by #1 and #2 above.
+        #
+        # In the future, MongoDB may implement partial indexes or improve sparse index behavior.
+        # See: https://jira.mongodb.org/browse/SERVER-785
+        #      https://jira.mongodb.org/browse/SERVER-13780
+        #      https://jira.mongodb.org/browse/SERVER-10403
+        options = {}
+        options.merge!(unique: true, sparse: true) unless scope_key || by_model_type
 
         [fields, options]
       end

spec/models/paranoid_permanent.rb

@@ -4,5 +4,7 @@ class ParanoidPermanent
   include Mongoid::Slug
 
   field :title
-  slug :title, permanent: true
+  field :foo
+
+  slug :title, scope: :foo, permanent: true
 end

spec/mongoid/index_spec.rb

@@ -11,17 +11,19 @@
 
     context 'when by_model_type is true' do
       let(:by_model_type) { true }
+
       it { is_expected.to eq [{ _slugs: 1, foo: 1, _type: 1 }, {}] }
     end
 
     context 'when by_model_type is false' do
-      it { is_expected.to eq [{ _slugs: 1, foo: 1 }, { unique: true, sparse: true }] }
+      it { is_expected.to eq [{ _slugs: 1, foo: 1 }, {}] }
     end
   end
 
   context 'when scope_key is not set' do
     context 'when by_model_type is true' do
       let(:by_model_type) { true }
+
       it { is_expected.to eq [{ _slugs: 1, _type: 1 }, {}] }
     end
 

spec/mongoid/paranoia_spec.rb

@@ -2,10 +2,78 @@
 require 'spec_helper'
 
 describe 'Mongoid::Paranoia with Mongoid::Slug' do
+  let(:paranoid_doc)    { ParanoidDocument.create!(title: 'slug') }
+  let(:paranoid_doc_2)  { ParanoidDocument.create!(title: 'slug') }
+  let(:paranoid_perm)   { ParanoidPermanent.create!(title: 'slug') }
+  let(:paranoid_perm_2) { ParanoidPermanent.create!(title: 'slug') }
+  let(:non_paranoid_doc) { Article.create!(title: 'slug') }
+  subject { paranoid_doc }
+
+  describe '.paranoid?' do
+    context 'when Mongoid::Paranoia is included' do
+      subject { paranoid_doc.class }
+      specify { expect(subject.is_paranoid_doc?).to be true }
+    end
+
+    context 'when Mongoid::Paranoia not included' do
+      subject { non_paranoid_doc.class }
+      specify { expect(subject.is_paranoid_doc?).to be false }
+    end
+  end
+
+  describe '#paranoid_deleted?' do
+    context 'when Mongoid::Paranoia is included' do
+      context 'when not destroyed' do
+        specify { expect(subject.paranoid_deleted?).to be false }
+      end
+
+      context 'when destroyed' do
+        before { subject.destroy }
+        specify { expect(subject.paranoid_deleted?).to be true }
+      end
+    end
+
+    context 'when Mongoid::Paranoia not included' do
+      subject { non_paranoid_doc }
+      specify { expect(subject.paranoid_deleted?).to be false }
+    end
+  end
+
+  describe 'restore callbacks' do
+    context 'when Mongoid::Paranoia is included' do
+      subject { paranoid_doc.class }
+      it { is_expected.to respond_to(:before_restore) }
+      it { is_expected.to respond_to(:after_restore) }
+    end
+
+    context 'when Mongoid::Paranoia not included' do
+      it { is_expected.to_not respond_to(:before_restore) }
+      it { is_expected.to_not respond_to(:after_restore) }
+    end
+  end
+
+  describe 'index' do
+    context 'simple index' do
+      before  { ParanoidDocument.create_indexes }
+      after   { ParanoidDocument.remove_indexes }
+      subject { ParanoidDocument }
+
+      it_should_behave_like 'has an index', { _slugs: 1 }, unique: true, sparse: true
+    end
+
+    context 'compound index' do
+      before  { ParanoidPermanent.create_indexes }
+      after   { ParanoidPermanent.remove_indexes }
+      subject { ParanoidPermanent }
+
+      it_should_behave_like 'has an index', { foo: 1, _slugs: 1 }, unique: nil, sparse: nil
+    end
+  end
+
   shared_examples_for 'paranoid slugs' do
     context 'querying' do
       it 'returns paranoid_doc for correct slug' do
-        expect(subject.class.find(subject.slug)).to eq(subject)
+        expect(subject.class.find(subject.slug)).to eq subject
       end
     end
 
@@ -95,30 +163,30 @@
       describe '.paranoid?' do
         context 'when Mongoid::Paranoia is included' do
           subject { paranoid_doc.class }
-          its(:is_paranoid_doc?) { should be_truthy }
+          its(:is_paranoid_doc?) { is_expected.to be_truthy }
         end
 
         context 'when Mongoid::Paranoia not included' do
           subject { non_paranoid_doc.class }
-          its(:is_paranoid_doc?) { should be_falsey }
+          its(:is_paranoid_doc?) { is_expected.to be_falsey }
         end
       end
 
       describe '#paranoid_deleted?' do
         context 'when Mongoid::Paranoia is included' do
           context 'when not destroyed' do
-            its(:paranoid_deleted?) { should be_falsey }
+            its(:paranoid_deleted?) { is_expected.to be_falsey }
           end
 
           context 'when destroyed' do
             before { subject.destroy }
-            its(:paranoid_deleted?) { should be_truthy }
+            its(:paranoid_deleted?) { is_expected.to be_truthy }
           end
         end
 
         context 'when Mongoid::Paranoia not included' do
           subject { non_paranoid_doc }
-          its(:paranoid_deleted?) { should be_falsey }
+          its(:paranoid_deleted?) { is_expected.to be_falsey }
         end
       end
 

spec/shared/indexes.rb

@@ -1,27 +1,41 @@
+shared_context 'with an index' do |key|
+  if Mongoid::Compatibility::Version.mongoid3?
+    let(:index) { subject.index_options[key] }
+    let(:index_keys) { key }
+    let(:index_options) { index }
+  else
+    let(:index) { subject.index_specifications.detect { |spec| spec.key == key } }
+    let(:index_keys) { index.key }
+    let(:index_options) { index.options }
+  end
+end
+
 shared_examples 'has an index' do |key, options|
-  it "has a #{key} index" do
-    index = if Mongoid::Compatibility::Version.mongoid3?
-              subject.index_options[key]
-            else
-              subject.index_specifications.detect { |spec| spec.key == key }
-            end
+  include_context 'with an index', key
+
+  it "has a #{[key, options].compact.join(', ')} index" do
     expect(index).not_to be_nil
+  end
+
+  it 'has the correct order of keys' do
+    expect(index_keys.keys).to eq key.keys
+  end
+
+  it 'has the correct order of key values' do
+    expect(index_keys.values).to eq key.values
+  end
+
+  it 'matches option values' do
     options.each_pair do |name, value|
-      if Mongoid::Compatibility::Version.mongoid3?
-        expect(index[name]).to eq(value)
-      else
-        expect(index.options[name]).to eq(value)
-      end
-    end if options
+      expect(index_options[name]).to eq(value)
+    end
   end
 end
 
-shared_examples 'does not have an index' do |key, _option|
+shared_examples 'does not have an index' do |key|
+  include_context 'with an index', key
+
   it "does not have the #{key} index" do
-    if Mongoid::Compatibility::Version.mongoid3?
-      expect(subject.index_options[key]).to be_nil
-    else
-      expect(subject.index_specifications.detect { |spec| spec.key == key }).to be_nil
-    end
+    expect(index).to be nil
   end
 end