Fix: scroll over a collection that has duplicate values while data is…

… being modified in a way that causes a change in the natural sort order.

CHANGELOG.md

@@ -1,3 +1,8 @@
+0.2.1 (3/21/2013)
+=================
+
+* Fix: scroll over a collection that has duplicate values while data is being modified in a way that causes a change in the natural sort order - [@dblock](https://github.com/dblock).
+
 0.2.0 (3/14/2013)
 =================
 

lib/mongoid/criterion/scrollable.rb

@@ -13,14 +13,14 @@ def scroll(cursor = nil, &block)
         end
         # scroll field and direction
         scroll_field = criteria.options[:sort].keys.first
-        scroll_direction = criteria.options[:sort].values.first.to_i == 1 ? '$gt' : '$lt'
+        scroll_direction = criteria.options[:sort].values.first.to_i
         # scroll cursor from the parameter, with value and tiebreak_id
         field = criteria.klass.fields[scroll_field.to_s]
         cursor_options = { field_type: field.type, field_name: scroll_field, direction: scroll_direction }
         cursor = cursor.is_a?(Mongoid::Scroll::Cursor) ? cursor : Mongoid::Scroll::Cursor.new(cursor, cursor_options)
         # scroll
         if block_given?
-          criteria.where(cursor.criteria).each do |record|
+          criteria.where(cursor.criteria).order_by(_id: scroll_direction).each do |record|
             yield record, Mongoid::Scroll::Cursor.from_record(record, cursor_options)
           end
         else

lib/mongoid/scroll/cursor.rb

@@ -6,14 +6,15 @@ class Cursor
 
       def initialize(value = nil, options = {})
         @field_type, @field_name = Mongoid::Scroll::Cursor.extract_field_options(options)
-        @direction = options[:direction] || '$gt'
+        @direction = options[:direction] || 1
         parse(value)
       end
 
       def criteria
         mongo_value = value.class.mongoize(value) if value
-        cursor_criteria = { field_name => { direction => mongo_value } } if mongo_value
-        tiebreak_criteria = { field_name => mongo_value, :_id => { '$gt' => tiebreak_id } } if mongo_value && tiebreak_id
+        compare_direction = direction == 1 ? "$gt" : "$lt"
+        cursor_criteria = { field_name => { compare_direction => mongo_value } } if mongo_value
+        tiebreak_criteria = { field_name => mongo_value, :_id => { compare_direction => tiebreak_id } } if mongo_value && tiebreak_id
         (cursor_criteria || tiebreak_criteria) ? { '$or' => [ cursor_criteria, tiebreak_criteria].compact } : {}
       end
 

lib/mongoid/scroll/version.rb

@@ -1,5 +1,5 @@
 module Mongoid
   module Scroll
-    VERSION = '0.2.0'
+    VERSION = '0.2.1'
   end
 end

lib/moped/scrollable.rb

@@ -2,27 +2,31 @@ module Moped
   module Scrollable
 
     def scroll(cursor = nil, options = { field_type: Moped::BSON::ObjectId }, &block)
+      query = Query.new(collection, operation.selector.dup)
+      query.operation.skip = operation.skip
+      query.operation.limit = operation.limit
       # we don't support scrolling over a criteria with multiple fields
-      if operation.selector["$orderby"] && operation.selector["$orderby"].keys.size != 1
-        raise Mongoid::Scroll::Errors::MultipleSortFieldsError.new(sort: operation.selector["$orderby"])
-      elsif ! operation.selector.has_key?("$orderby") || operation.selector["$orderby"].empty?
+      if query.operation.selector["$orderby"] && query.operation.selector["$orderby"].keys.size != 1
+        raise Mongoid::Scroll::Errors::MultipleSortFieldsError.new(sort: query.operation.selector["$orderby"])
+      elsif ! query.operation.selector.has_key?("$orderby") || query.operation.selector["$orderby"].empty?
         # introduce a default sort order if there's none
-        sort("_id" => 1)
+        query.sort(_id: 1)
       end
       # scroll field and direction
-      scroll_field = operation.selector["$orderby"].keys.first
-      scroll_direction = operation.selector["$orderby"].values.first.to_i == 1 ? '$gt' : '$lt'
+      scroll_field = query.operation.selector["$orderby"].keys.first
+      scroll_direction = query.operation.selector["$orderby"].values.first.to_i
       # scroll cursor from the parameter, with value and tiebreak_id
       cursor_options = { field_name: scroll_field, field_type: options[:field_type], direction: scroll_direction }
       cursor = cursor.is_a?(Mongoid::Scroll::Cursor) ? cursor : Mongoid::Scroll::Cursor.new(cursor, cursor_options)
-      operation.selector["$query"].merge!(cursor.criteria)
+      query.operation.selector["$query"] = query.operation.selector["$query"].merge(cursor.criteria)
+      query.operation.selector["$orderby"] = query.operation.selector["$orderby"].merge(_id: scroll_direction)
       # scroll
       if block_given?
-        each do |record|
+        query.each do |record|
           yield record, Mongoid::Scroll::Cursor.from_record(record, cursor_options)
         end
       else
-        self
+        query
       end
     end
 

spec/mongoid/criteria_spec.rb

@@ -96,6 +96,13 @@
   context "with overlapping data" do
     before :each do
       3.times { Feed::Item.create! a_integer: 5 }
+      Feed::Item.first.update_attributes!(name: Array(1000).join('a'))
+    end
+    it "natural order is different from order by id" do
+      # natural order isn't necessarily going to be the same as _id order
+      # if a document is updated and grows in size, it may need to be relocated and
+      # thus cause the natural order to change
+      Feed::Item.order_by("$natural" => 1).to_a.should_not eq Feed::Item.order_by(_id: 1).to_a
     end
     [ { a_integer: 1 }, { a_integer: -1 }].each do |sort_order|
       it "scrolls by #{sort_order}" do
@@ -110,7 +117,7 @@
           records << record
         end
         records.size.should == 3
-        records.sort_by { |r| r.a_integer }.should eq Feed::Item.all.sort(a_integer: 1).to_a
+        records.should eq Feed::Item.all.sort(_id: sort_order[:a_integer]).to_a
       end
     end
   end

spec/moped/query_spec.rb

@@ -23,7 +23,7 @@
       Mongoid.default_session['feed_items'].find
     end
     it "adds a default sort by _id" do
-      subject.scroll.operation.selector["$orderby"].should == { "_id" => 1 }
+      subject.scroll.operation.selector["$orderby"].should == { _id: 1 }
     end
   end
   context "with data" do
@@ -98,20 +98,29 @@
   context "with overlapping data" do
     before :each do
       3.times { Feed::Item.create! a_integer: 5 }
+      Feed::Item.first.update_attributes!(name: Array(1000).join('a'))
     end
-    it "scrolls" do
-      records = []
-      cursor = nil
-      Mongoid.default_session['feed_items'].find.sort(a_integer: -1).limit(2).scroll do |record, next_cursor|
-        records << record
-        cursor = next_cursor
-      end
-      records.size.should == 2
-      Mongoid.default_session['feed_items'].find.sort(a_integer: -1).scroll(cursor) do |record, next_cursor|
-        records << record
+    it "natural order is different from order by id" do
+      # natural order isn't necessarily going to be the same as _id order
+      # if a document is updated and grows in size, it may need to be relocated and
+      # thus cause the natural order to change
+      Feed::Item.order_by("$natural" => 1).to_a.should_not eq Feed::Item.order_by(_id: 1).to_a
+    end
+    [ { a_integer: 1 }, { a_integer: -1 }].each do |sort_order|
+      it "scrolls by #{sort_order}" do
+        records = []
+        cursor = nil
+        Mongoid.default_session['feed_items'].find.sort(sort_order).limit(2).scroll do |record, next_cursor|
+          records << record
+          cursor = next_cursor
+        end
+        records.size.should == 2
+        Mongoid.default_session['feed_items'].find.sort(sort_order).scroll(cursor) do |record, next_cursor|
+          records << record
+        end
+        records.size.should == 3
+        records.should eq Mongoid.default_session['feed_items'].find.sort(_id: sort_order[:a_integer]).to_a
       end
-      records.size.should == 3
-      records.should eq Mongoid.default_session['feed_items'].find.to_a
     end
   end
 end