From eadc63bfc759e0eb4e1bbfa6e27b8bfe07238555 Mon Sep 17 00:00:00 2001
From: John Nunemaker <nunemaker@gmail.com>
Date: Mon, 17 May 2010 22:44:21 -0400
Subject: [PATCH 1/2] Reorganized the document tests a bit.

---
 test/unit/test_document.rb | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/test/unit/test_document.rb b/test/unit/test_document.rb
index df8f0fafb..3a6bcbd9d 100644
--- a/test/unit/test_document.rb
+++ b/test/unit/test_document.rb
@@ -40,30 +40,29 @@ class DocumentTest < Test::Unit::TestCase
       another_document.database.should == MongoMapper.database
     end
 
-    should "default collection name to class name tableized" do
-      class ::Item
-        include MongoMapper::Document
-      end
-
-      Item.collection.should be_instance_of(Mongo::Collection)
-      Item.collection.name.should == 'items'
+    should "allow setting the collection name" do
+      @document.set_collection_name('foobar')
+      @document.collection.name.should == 'foobar'
     end
 
-    should "default collection name of namespaced class to tableized with dot separation" do
-      module ::BloggyPoo
-        class Post
+    context ".collection" do
+      should "default collection name to class name tableized" do
+        class ::Item
           include MongoMapper::Document
-        end
+        end.collection.name.should == 'items'
       end
 
-      BloggyPoo::Post.collection.should be_instance_of(Mongo::Collection)
-      BloggyPoo::Post.collection.name.should == 'bloggy_poo.posts'
-    end
+      should "default collection name of namespaced class to tableized with dot separation" do
+        module ::BloggyPoo
+          class Post
+            include MongoMapper::Document
+          end.collection.name.should == 'bloggy_poo.posts'
+        end
+      end
 
-    should "allow setting the collection name" do
-      @document.set_collection_name('foobar')
-      @document.collection.should be_instance_of(Mongo::Collection)
-      @document.collection.name.should == 'foobar'
+      should "be an instance of a Mongo::Collection" do
+        @document.collection.should be_instance_of(Mongo::Collection)
+      end
     end
   end # Document class
 

From 389638dbd8a694016620bc7f19c6fb77edd87962 Mon Sep 17 00:00:00 2001
From: John Nunemaker <nunemaker@gmail.com>
Date: Tue, 18 May 2010 00:07:58 -0400
Subject: [PATCH 2/2] First round of swapping out MongoMapper::Query internals
 with Plucky::Query.

---
 Rakefile                                 |   1 +
 lib/mongo_mapper.rb                      |   2 +
 lib/mongo_mapper/document.rb             |  76 ++++---------
 lib/mongo_mapper/plugins/identity_map.rb |  11 +-
 lib/mongo_mapper/plugins/keys.rb         |   8 +-
 lib/mongo_mapper/plugins/modifiers.rb    |   4 +-
 lib/mongo_mapper/query.rb                | 138 ++---------------------
 lib/mongo_mapper/support.rb              |  24 +---
 test/functional/test_document.rb         |  10 --
 test/unit/test_keys.rb                   |  10 ++
 test/unit/test_query.rb                  |  77 +++++--------
 11 files changed, 89 insertions(+), 272 deletions(-)

diff --git a/Rakefile b/Rakefile
index 66dadf16e..e36c09aac 100644
--- a/Rakefile
+++ b/Rakefile
@@ -13,6 +13,7 @@ Jeweler::Tasks.new do |gem|
   gem.version     = MongoMapper::Version
 
   gem.add_dependency('activesupport', '>= 2.3.4')
+  gem.add_dependency('plucky', '0.1')
   gem.add_dependency('mongo', '1.0')
   gem.add_dependency('jnunemaker-validatable', '1.8.4')
 
diff --git a/lib/mongo_mapper.rb b/lib/mongo_mapper.rb
index c3ccd9545..f4d8e6d21 100644
--- a/lib/mongo_mapper.rb
+++ b/lib/mongo_mapper.rb
@@ -2,11 +2,13 @@
 #   gem 'activesupport', '>= 2.3.4'
 #   gem 'mongo', '1.0'
 #   gem 'jnunemaker-validatable', '1.8.4'
+#   gem 'plucky', '0.1'
 require 'set'
 require 'uri'
 require 'mongo'
 require 'validatable'
 require 'active_support/all'
+require 'plucky'
 
 module MongoMapper
   # generic MM error
diff --git a/lib/mongo_mapper/document.rb b/lib/mongo_mapper/document.rb
index bcc487d5c..2d82dfa04 100644
--- a/lib/mongo_mapper/document.rb
+++ b/lib/mongo_mapper/document.rb
@@ -45,34 +45,31 @@ def ensure_index(spec, options={})
       end
 
       def find(*args)
-        assert_no_first_last_or_all(args)
         options = args.extract_options!
         return nil if args.size == 0
 
         if args.first.is_a?(Array) || args.size > 1
           find_some(args, options)
         else
-          find_one(options.merge({:_id => args[0]}))
+          query = query(options).update(:_id => args[0])
+          find_one(query.to_hash)
         end
       end
 
       def find!(*args)
-        assert_no_first_last_or_all(args)
         options = args.extract_options!
         raise DocumentNotFound, "Couldn't find without an ID" if args.size == 0
 
         if args.first.is_a?(Array) || args.size > 1
           find_some!(args, options)
         else
-          find_one(options.merge({:_id => args[0]})) || raise(DocumentNotFound, "Document match #{options.inspect} does not exist in #{collection.name} collection")
+          query = query(options).update(:_id => args[0])
+          find_one(query.to_hash) || raise(DocumentNotFound, "Document match #{options.inspect} does not exist in #{collection.name} collection")
         end
       end
 
       def find_each(options={})
-        criteria, options = to_query(options)
-        collection.find(criteria, options).each do |doc|
-          yield load(doc)
-        end
+        query(options).find().each { |doc| yield load(doc) }
       end
 
       def find_by_id(id)
@@ -93,7 +90,7 @@ def first(options={})
 
       def last(options={})
         raise ':order option must be provided when using last' if options[:order].blank?
-        find_one(options.merge(:order => invert_order_clause(options[:order])))
+        find_one(query(options).reverse.to_hash)
       end
 
       def all(options={})
@@ -101,7 +98,7 @@ def all(options={})
       end
 
       def count(options={})
-        collection.find(to_criteria(options)).count
+        query(options).count
       end
 
       def exists?(options={})
@@ -126,11 +123,11 @@ def update(*args)
       end
 
       def delete(*ids)
-        collection.remove(to_criteria(:_id => ids.flatten))
+        query(:_id => ids.flatten).remove
       end
 
       def delete_all(options={})
-        collection.remove(to_criteria(options))
+        query(options).remove
       end
 
       def destroy(*ids)
@@ -153,6 +150,11 @@ def single_collection_inherited_superclass?
         superclass.respond_to?(:keys) && superclass.keys.key?(:_type)
       end
 
+      # @api private for now
+      def query(options={})
+        Query.new(self, options)
+      end
+
       private
         def initialize_each(*docs)
           instances = []
@@ -165,15 +167,9 @@ def initialize_each(*docs)
           instances.size == 1 ? instances[0] : instances
         end
 
-        def assert_no_first_last_or_all(args)
-          if args[0] == :first || args[0] == :last || args[0] == :all
-            raise ArgumentError, "#{self}.find(:#{args}) is no longer supported, use #{self}.#{args} instead."
-          end
-        end
-
         def find_some(ids, options={})
-          ids = ids.flatten.compact.uniq
-          find_many(options.merge(:_id => ids)).compact
+          query = query(options).update(:_id => ids.flatten.compact.uniq)
+          find_many(query.to_hash).compact
         end
 
         def find_some!(ids, options={})
@@ -189,30 +185,12 @@ def find_some!(ids, options={})
 
         # All query methods that load documents pass through find_one or find_many
         def find_one(options={})
-          criteria, options = to_query(options)
-          if doc = collection.find_one(criteria, options)
-            load(doc)
-          end
+          load(query(options).first)
         end
 
         # All query methods that load documents pass through find_one or find_many
         def find_many(options)
-          criteria, options = to_query(options)
-          collection.find(criteria, options).to_a.map do |doc|
-            load(doc)
-          end
-        end
-
-        def invert_order_clause(order)
-          order.split(',').map do |order_segment|
-            if order_segment =~ /\sasc/i
-              order_segment.sub /\sasc/i, ' desc'
-            elsif order_segment =~ /\sdesc/i
-              order_segment.sub /\sdesc/i, ' asc'
-            else
-              "#{order_segment.strip} desc"
-            end
-          end.join(',')
+          query(options).all().map { |doc| load(doc) }
         end
 
         def update_single(id, attrs)
@@ -220,9 +198,9 @@ def update_single(id, attrs)
             raise ArgumentError, "Updating a single document requires an id and a hash of attributes"
           end
 
-          doc = find(id)
-          doc.update_attributes(attrs)
-          doc
+          find(id).tap do |doc|
+            doc.update_attributes(attrs)
+          end
         end
 
         def update_multiple(docs)
@@ -234,14 +212,6 @@ def update_multiple(docs)
           docs.each_pair { |id, attrs| instances << update(id, attrs) }
           instances
         end
-
-        def to_criteria(options={})
-          Query.new(self, options).criteria
-        end
-
-        def to_query(options={})
-          Query.new(self, options).to_a
-        end
     end
 
     module InstanceMethods
@@ -274,9 +244,9 @@ def destroyed?
       end
 
       def reload
-        if attrs = collection.find_one({:_id => _id})
+        if doc = self.class.query(:_id => id).first
           self.class.associations.each { |name, assoc| send(name).reset if respond_to?(name) }
-          self.attributes = attrs
+          self.attributes = doc
           self
         else
           raise DocumentNotFound, "Document match #{_id.inspect} does not exist in #{collection.name} collection"
diff --git a/lib/mongo_mapper/plugins/identity_map.rb b/lib/mongo_mapper/plugins/identity_map.rb
index 08202d271..5b13cdad3 100644
--- a/lib/mongo_mapper/plugins/identity_map.rb
+++ b/lib/mongo_mapper/plugins/identity_map.rb
@@ -28,25 +28,26 @@ def identity_map=(v)
         end
 
         def find_one(options={})
-          criteria, query_options = to_query(options)
-
+          query = query(options)
+          criteria = query.criteria.to_hash
+          
           if simple_find?(criteria) && identity_map.key?(criteria[:_id])
             identity_map[criteria[:_id]]
           else
             super.tap do |document|
-              remove_documents_from_map(document) if selecting_fields?(query_options)
+              remove_documents_from_map(document) if selecting_fields?(query.options)
             end
           end
         end
 
         def find_many(options)
-          criteria, query_options = to_query(options)
           super.tap do |documents|
-            remove_documents_from_map(documents) if selecting_fields?(query_options)
+            remove_documents_from_map(documents) if selecting_fields?(query(options).options)
           end
         end
 
         def load(attrs)
+          return nil if attrs.nil?
           document = identity_map[attrs['_id']]
 
           if document.nil? || identity_map_off?
diff --git a/lib/mongo_mapper/plugins/keys.rb b/lib/mongo_mapper/plugins/keys.rb
index 149e11dfa..bd6590773 100644
--- a/lib/mongo_mapper/plugins/keys.rb
+++ b/lib/mongo_mapper/plugins/keys.rb
@@ -36,9 +36,12 @@ def using_object_id?
           object_id_key?(:_id)
         end
 
+        def object_id_keys
+          keys.keys.select { |key| keys[key].type == ObjectId }.map(&:to_sym)
+        end
+
         def object_id_key?(name)
-          key = keys[name.to_s]
-          key && key.type == ObjectId
+          object_id_keys.include?(name.to_sym)
         end
 
         def to_mongo(instance)
@@ -53,6 +56,7 @@ def from_mongo(value)
 
         # load is overridden in identity map to ensure same objects are loaded
         def load(attrs)
+          return nil if attrs.nil?
           begin
             klass = attrs['_type'].present? ? attrs['_type'].constantize : self
             klass.new(attrs, true)
diff --git a/lib/mongo_mapper/plugins/modifiers.rb b/lib/mongo_mapper/plugins/modifiers.rb
index af6e477d5..bb85c43d2 100644
--- a/lib/mongo_mapper/plugins/modifiers.rb
+++ b/lib/mongo_mapper/plugins/modifiers.rb
@@ -29,7 +29,7 @@ def unset(*args)
             criteria = {:id => ids}
           end
 
-          criteria  = to_criteria(criteria)
+          criteria  = query(criteria).criteria.to_hash
           modifiers = keys.inject({}) { |hash, key| hash[key] = 1; hash }
           collection.update(criteria, {'$unset' => modifiers}, :multi => true)
         end
@@ -68,7 +68,7 @@ def modifier_update(modifier, args)
           def criteria_and_keys_from_args(args)
             keys     = args.pop
             criteria = args[0].is_a?(Hash) ? args[0] : {:id => args}
-            [to_criteria(criteria), keys]
+            [query(criteria).criteria.to_hash, keys]
           end
       end
 
diff --git a/lib/mongo_mapper/query.rb b/lib/mongo_mapper/query.rb
index ee6a8e3d3..e6cb613cd 100644
--- a/lib/mongo_mapper/query.rb
+++ b/lib/mongo_mapper/query.rb
@@ -1,143 +1,23 @@
 module MongoMapper
-  # IMPORTANT
-  # This class is private to MongoMapper and should not be considered part of MongoMapper's public API.
-  #
   class Query
-    OptionKeys = [:fields, :select, :skip, :offset, :limit, :sort, :order]
-
-    attr_reader :model
-
-    def initialize(model, options)
+    def initialize(model, options={})
       raise ArgumentError, "Options must be a hash" unless options.is_a?(Hash)
-      @model, @options, @conditions, @original_options = model, {}, {}, options
-      separate_options_and_conditions
+      @model, @options, @conditions = model, {}, {}
+      query.update(options)
       add_sci_condition
     end
 
-    def criteria
-      to_criteria(@conditions)
-    end
-
-    def options
-      fields = @options[:fields] || @options[:select]
-      skip   = @options[:skip]   || @options[:offset] || 0
-      limit  = @options[:limit]  || 0
-      sort   = @options[:sort]   || normalized_sort(@options[:order])
-
-      {:fields => to_fields(fields), :skip => skip.to_i, :limit => limit.to_i, :sort => sort}
-    end
-
-    def to_a
-      [criteria, options]
-    end
-
     private
-      def separate_options_and_conditions
-        @original_options.each_pair do |key, value|
-          key = key.respond_to?(:to_sym) ? key.to_sym : key
-
-          if OptionKeys.include?(key)
-            @options[key] = value
-          elsif key == :conditions
-            @conditions.update(value)
-          else
-            @conditions[key] = value
-          end
-        end
+      def method_missing(method, *args, &block)
+        query.send(method, *args, &block)
       end
 
-      # adds _type single collection inheritance scope for models that need it
-      def add_sci_condition
-        @conditions[:_type] = model.to_s if model.single_collection_inherited?
+      def query
+        @query ||= Plucky::Query.new(@model.collection).object_ids(@model.object_id_keys)
       end
 
-      def modifier?(field)
-        field.to_s =~ /^\$/
-      end
-
-      def symbol_operator?(object)
-        object.respond_to?(:field, :operator)
-      end
-
-      def to_criteria(conditions, parent_key=nil)
-        criteria = {}
-
-        conditions.each_pair do |key, value|
-          key = normalized_key(key)
-
-          if model.object_id_key?(key)
-            case value
-              when String
-                value = ObjectId.to_mongo(value)
-              when Array
-                value.map! { |id| ObjectId.to_mongo(id) }
-            end
-          end
-
-          if symbol_operator?(key)
-            key, value = normalized_key(key.field), {"$#{key.operator}" => value}
-          end
-
-          criteria[key] = normalized_value(criteria, key, value)
-        end
-
-        criteria
-      end
-
-      def to_fields(keys)
-        return keys if keys.is_a?(Hash)
-        return nil  if keys.blank?
-
-        if keys.respond_to?(:flatten, :compact)
-          keys.flatten.compact
-        else
-          keys.split(',').map { |key| key.strip }
-        end
-      end
-
-      def to_order(key, direction=nil)
-        [normalized_key(key).to_s, normalized_direction(direction)]
-      end
-
-      def normalized_key(key)
-        key.to_s == 'id' ? :_id : key
-      end
-
-      # TODO: this is getting heavy enough to move to a class
-      def normalized_value(criteria, key, value)
-        case value
-          when Array, Set
-            modifier?(key) ? value.to_a : {'$in' => value.to_a}
-          when Hash
-            if criteria[key].kind_of?(Hash)
-              criteria[key].dup.merge(to_criteria(value, key))
-            else
-              to_criteria(value, key)
-            end
-          when Time
-            value.utc
-          else
-            value
-        end
-      end
-
-      def normalized_direction(direction)
-        direction ||= 'asc'
-        direction.downcase == 'asc' ? Mongo::ASCENDING : Mongo::DESCENDING
-      end
-
-      def normalized_sort(sort)
-        return if sort.blank?
-
-        if sort.respond_to?(:all?) && sort.all? { |s| symbol_operator?(s) }
-          sort.map { |s| to_order(s.field, s.operator) }
-        elsif symbol_operator?(sort)
-          [to_order(sort.field, sort.operator)]
-        else
-          sort.split(',').map do |str|
-            to_order(*str.strip.split(' '))
-          end
-        end
+      def add_sci_condition
+        query[:_type] = @model.to_s if @model.single_collection_inherited?
       end
   end
 end
diff --git a/lib/mongo_mapper/support.rb b/lib/mongo_mapper/support.rb
index f25f2c978..75734b88a 100644
--- a/lib/mongo_mapper/support.rb
+++ b/lib/mongo_mapper/support.rb
@@ -127,13 +127,7 @@ def self.from_mongo(value)
 
 class ObjectId
   def self.to_mongo(value)
-    if value.blank?
-      nil
-    elsif value.is_a?(BSON::ObjectID)
-      value
-    else
-      BSON::ObjectID.from_string(value.to_s)
-    end
+    Plucky.to_object_id(value)
   end
 
   def self.from_mongo(value)
@@ -161,22 +155,6 @@ def self.from_mongo(value)
   end
 end
 
-class SymbolOperator
-  attr_reader :field, :operator
-
-  def initialize(field, operator, options={})
-    @field, @operator = field, operator
-  end unless method_defined?(:initialize)
-end
-
-class Symbol
-  %w(gt lt gte lte ne in nin mod all size where exists asc desc).each do |operator|
-    define_method(operator) do
-      SymbolOperator.new(self, operator)
-    end unless method_defined?(operator)
-  end
-end
-
 class Time
   def self.to_mongo(value)
     if value.nil? || value == ''
diff --git a/test/functional/test_document.rb b/test/functional/test_document.rb
index 2897e4bc4..2a6fa6131 100644
--- a/test/functional/test_document.rb
+++ b/test/functional/test_document.rb
@@ -261,16 +261,6 @@ def setup
       end
     end
 
-    should "raise error if trying to find with :all, :first, or :last" do
-      [:all, :first, :last].each do |m|
-        assert_raises(ArgumentError) { @document.find(m) }
-      end
-
-      [:all, :first, :last].each do |m|
-        assert_raises(ArgumentError) { @document.find!(m) }
-      end
-    end
-
     context "(with a single id)" do
       should "work" do
         @document.find(@doc1._id).should == @doc1
diff --git a/test/unit/test_keys.rb b/test/unit/test_keys.rb
index a0bf17fed..cd8546385 100644
--- a/test/unit/test_keys.rb
+++ b/test/unit/test_keys.rb
@@ -65,6 +65,16 @@ def user=(user)
     end
   end
 
+  context ".load" do
+    setup do
+      @klass = Doc()
+    end
+
+    should "return nil if argument is nil" do
+      @klass.load(nil).should be_nil
+    end
+  end
+
   context "Initializing a new key" do
     should "allow setting the name" do
       Key.new(:foo, String).name.should == 'foo'
diff --git a/test/unit/test_query.rb b/test/unit/test_query.rb
index 46ba05926..c9ff3cf20 100644
--- a/test/unit/test_query.rb
+++ b/test/unit/test_query.rb
@@ -5,7 +5,7 @@ class QueryTest < Test::Unit::TestCase
   include MongoMapper
 
   should "raise error if provided something other than a hash" do
-    lambda { Query.new(Room) }.should raise_error(ArgumentError)
+    lambda { Query.new(Room, nil) }.should raise_error(ArgumentError)
     lambda { Query.new(Room, 1) }.should raise_error(ArgumentError)
   end
 
@@ -17,13 +17,13 @@ class QueryTest < Test::Unit::TestCase
 
   context "Converting conditions to criteria" do
     should "not add _type to query if model does not have superclass that is single collection inherited" do
-      Query.new(Message, :foo => 'bar').criteria.should == {
+      Query.new(Message, :foo => 'bar').criteria.to_hash.should == {
         :foo => 'bar'
       }
     end
 
     should "not add _type to nested conditions" do
-      Query.new(Enter, :foo => 'bar', :age => {'$gt' => 21}).criteria.should == {
+      Query.new(Enter, :foo => 'bar', :age => {'$gt' => 21}).criteria.to_hash.should == {
         :foo => 'bar',
         :age => {'$gt' => 21},
         :_type => 'Enter'
@@ -31,22 +31,12 @@ class QueryTest < Test::Unit::TestCase
     end
 
     should "automatically add _type to query if model is single collection inherited" do
-      Query.new(Enter, :foo => 'bar').criteria.should == {
+      Query.new(Enter, :foo => 'bar').criteria.to_hash.should == {
         :foo => 'bar',
         :_type => 'Enter'
       }
     end
 
-    %w{gt lt gte lte ne in nin mod all size where exists}.each do |operator|
-      next if operator == 'size' && RUBY_VERSION >= '1.9.1' # 1.9 defines Symbol#size
-
-      should "convert #{operator} conditions" do
-        Query.new(Room, :age.send(operator) => 21).criteria.should == {
-          :age => {"$#{operator}" => 21}
-        }
-      end
-    end
-
     should "normalize value when using symbol operators" do
       time = Time.now.in_time_zone('Indiana (East)')
       criteria = Query.new(Room, :created_at.gt => time).criteria
@@ -54,17 +44,17 @@ class QueryTest < Test::Unit::TestCase
     end
 
     should "work with multiple symbol operators on the same field" do
-      Query.new(Message, :position.gt => 0, :position.lte => 10).criteria.should == {
+      Query.new(Message, :position.gt => 0, :position.lte => 10).criteria.to_hash.should == {
         :position => {"$gt" => 0, "$lte" => 10}
       }
     end
 
     should "work with simple criteria" do
-      Query.new(Room, :foo => 'bar').criteria.should == {
+      Query.new(Room, :foo => 'bar').criteria.to_hash.should == {
         :foo => 'bar'
       }
 
-      Query.new(Room, :foo => 'bar', :baz => 'wick').criteria.should == {
+      Query.new(Room, :foo => 'bar', :baz => 'wick').criteria.to_hash.should == {
         :foo => 'bar',
         :baz => 'wick'
       }
@@ -72,34 +62,34 @@ class QueryTest < Test::Unit::TestCase
 
     should "convert id to _id" do
       id = BSON::ObjectID.new
-      Query.new(Room, :id => id).criteria.should == {:_id => id}
+      Query.new(Room, :id => id).criteria.keys.should include(:_id)
     end
 
     should "convert id with symbol operator to _id with modifier" do
       id = BSON::ObjectID.new
-      Query.new(Room, :id.ne => id).criteria.should == {
+      Query.new(Room, :id.ne => id).criteria.to_hash.should == {
         :_id => {'$ne' => id}
       }
     end
 
     should "make sure that _id's are object ids" do
       id = BSON::ObjectID.new
-      Query.new(Room, :_id => id.to_s).criteria.should == {:_id => id}
+      Query.new(Room, :_id => id.to_s).criteria.to_hash.should == {:_id => id}
     end
 
     should "work fine with _id's that are object ids" do
       id = BSON::ObjectID.new
-      Query.new(Room, :_id => id).criteria.should == {:_id => id}
+      Query.new(Room, :_id => id).criteria.to_hash.should == {:_id => id}
     end
 
     should "make sure other object id typed keys get converted" do
       id = BSON::ObjectID.new
-      Query.new(Message, :room_id => id.to_s).criteria.should == {:room_id => id}
+      Query.new(Message, :room_id => id.to_s).criteria.to_hash.should == {:room_id => id}
     end
 
     should "work fine with object ids for object id typed keys" do
       id = BSON::ObjectID.new
-      Query.new(Message, :room_id => id).criteria.should == {:room_id => id}
+      Query.new(Message, :room_id => id).criteria.to_hash.should == {:room_id => id}
     end
 
     should "convert times to utc if they aren't already" do
@@ -116,51 +106,41 @@ class QueryTest < Test::Unit::TestCase
     end
 
     should "use $in for arrays" do
-      Query.new(Room, :foo => [1,2,3]).criteria.should == {
+      Query.new(Room, :foo => [1,2,3]).criteria.to_hash.should == {
         :foo => {'$in' => [1,2,3]}
       }
     end
 
     should "not use $in for arrays if already using array operator" do
-      Query.new(Room, :foo => {'$all' => [1,2,3]}).criteria.should == {
+      Query.new(Room, :foo => {'$all' => [1,2,3]}).criteria.to_hash.should == {
         :foo => {'$all' => [1,2,3]}
       }
 
-      Query.new(Room, :foo => {'$any' => [1,2,3]}).criteria.should == {
+      Query.new(Room, :foo => {'$any' => [1,2,3]}).criteria.to_hash.should == {
         :foo => {'$any' => [1,2,3]}
       }
     end
 
     should "use $in for sets" do
-      Query.new(Room, :foo => Set.new([1,2,3])).criteria.should == {
+      Query.new(Room, :foo => Set.new([1,2,3])).criteria.to_hash.should == {
         :foo => {'$in' => [1,2,3]}
       }
     end
 
     should "not use $in for sets if already using array operator" do
-      Query.new(Room, :foo => {'$all' => Set.new([1,2,3])}).criteria.should == {
+      Query.new(Room, :foo => {'$all' => Set.new([1,2,3])}).criteria.to_hash.should == {
         :foo => {'$all' => [1,2,3]}
       }
 
-      Query.new(Room, :foo => {'$any' => Set.new([1,2,3])}).criteria.should == {
+      Query.new(Room, :foo => {'$any' => Set.new([1,2,3])}).criteria.to_hash.should == {
         :foo => {'$any' => [1,2,3]}
       }
     end
 
-    should "work arbitrarily deep" do
-      Query.new(Room, :foo => {:bar => [1,2,3]}).criteria.should == {
-        :foo => {:bar => {'$in' => [1,2,3]}}
-      }
-
-      Query.new(Room, :foo => {:bar => {'$any' => [1,2,3]}}).criteria.should == {
-        :foo => {:bar => {'$any' => [1,2,3]}}
-      }
-    end
-
     should "make sure that ids in array are object ids" do
       id1, id2, id3 = BSON::ObjectID.new, BSON::ObjectID.new, BSON::ObjectID.new
 
-      Query.new(Room, :_id => [id1.to_s, id2.to_s, id3.to_s]).criteria.should == {
+      Query.new(Room, :_id => [id1.to_s, id2.to_s, id3.to_s]).criteria.to_hash.should == {
         :_id => {'$in' => [id1, id2, id3]}
       }
     end
@@ -229,8 +209,8 @@ class QueryTest < Test::Unit::TestCase
   end
 
   context "skip" do
-    should "default to 0" do
-      Query.new(Room, {}).options[:skip].should == 0
+    should "default to nil" do
+      Query.new(Room, {}).options[:skip].should == nil
     end
 
     should "use skip provided" do
@@ -247,8 +227,8 @@ class QueryTest < Test::Unit::TestCase
   end
 
   context "limit" do
-    should "default to 0" do
-      Query.new(Room, {}).options[:limit].should == 0
+    should "default to nil" do
+      Query.new(Room, {}).options[:limit].should == nil
     end
 
     should "use limit provided" do
@@ -297,7 +277,7 @@ class QueryTest < Test::Unit::TestCase
   context "Condition auto-detection" do
     should "know :conditions are criteria" do
       finder = Query.new(Room, :conditions => {:foo => 'bar'})
-      finder.criteria.should == {:foo => 'bar'}
+      finder.criteria.to_hash.should == {:foo => 'bar'}
       finder.options.keys.should_not include(:conditions)
     end
 
@@ -310,7 +290,8 @@ class QueryTest < Test::Unit::TestCase
     # select gets converted to fields so just checking keys
     should "know select is an option" do
       finder = Query.new(Room, :select => 'foo')
-      finder.options.keys.should include(:sort)
+      finder.options.keys.should      include(:fields)
+      finder.options.keys.should_not  include(:select)
       finder.criteria.keys.should_not include(:select)
       finder.criteria.keys.should_not include(:fields)
     end
@@ -358,12 +339,12 @@ class QueryTest < Test::Unit::TestCase
         :skip => 10,
       })
 
-      query_options.criteria.should == {
+      query_options.criteria.to_hash.should == {
         :foo => 'bar',
         :baz => true,
       }
 
-      query_options.options.should == {
+      query_options.options.to_hash.should == {
         :sort => [['foo', 1]],
         :fields => ['foo', 'baz'],
         :limit => 10,