From 3cd75ded93f82adfcb1c17a8b9c98715c536b680 Mon Sep 17 00:00:00 2001 From: dblock Date: Thu, 21 Feb 2013 11:34:40 -0500 Subject: [PATCH] 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 | 5 +++++ lib/mongoid/criterion/scrollable.rb | 4 ++-- lib/mongoid/scroll/cursor.rb | 7 +++--- lib/mongoid/scroll/version.rb | 2 +- lib/moped/scrollable.rb | 22 ++++++++++-------- spec/mongoid/criteria_spec.rb | 9 +++++++- spec/moped/query_spec.rb | 35 ++++++++++++++++++----------- 7 files changed, 55 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c447924..d6b3897 100644 --- a/CHANGELOG.md +++ b/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) ================= diff --git a/lib/mongoid/criterion/scrollable.rb b/lib/mongoid/criterion/scrollable.rb index 21f066a..33d63c5 100644 --- a/lib/mongoid/criterion/scrollable.rb +++ b/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 diff --git a/lib/mongoid/scroll/cursor.rb b/lib/mongoid/scroll/cursor.rb index 38f6898..9ec94d5 100644 --- a/lib/mongoid/scroll/cursor.rb +++ b/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 diff --git a/lib/mongoid/scroll/version.rb b/lib/mongoid/scroll/version.rb index 796c781..f268fac 100644 --- a/lib/mongoid/scroll/version.rb +++ b/lib/mongoid/scroll/version.rb @@ -1,5 +1,5 @@ module Mongoid module Scroll - VERSION = '0.2.0' + VERSION = '0.2.1' end end diff --git a/lib/moped/scrollable.rb b/lib/moped/scrollable.rb index 269366e..bafcd4c 100644 --- a/lib/moped/scrollable.rb +++ b/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 diff --git a/spec/mongoid/criteria_spec.rb b/spec/mongoid/criteria_spec.rb index 27bbcc5..75aa2b4 100644 --- a/spec/mongoid/criteria_spec.rb +++ b/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 diff --git a/spec/moped/query_spec.rb b/spec/moped/query_spec.rb index a2984d6..c587b8f 100644 --- a/spec/moped/query_spec.rb +++ b/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