Merge pull request #293 from gaorlov/idempotent_scoping

#292 #254 making scopes unmodifiable

Author: gaorlov

lib/json_api_client/paginating/paginator.rb

@@ -82,7 +82,7 @@ def next_page
       def params_for_uri(uri)
         return {} unless uri
         uri = Addressable::URI.parse(uri)
-        uri.query_values || {}
+        ( uri.query_values || {} ).with_indifferent_access
       end
     end
   end

lib/json_api_client/query/builder.rb

@@ -5,60 +5,55 @@ class Builder
       attr_reader :klass
       delegate :key_formatter, to: :klass
 
-      def initialize(klass)
-        @klass = klass
-        @primary_key = nil
-        @pagination_params = {}
-        @path_params = {}
-        @additional_params = {}
-        @filters = {}
-        @includes = []
-        @orders = []
-        @fields = []
+      def initialize(klass, opts = {})
+        @klass             = klass
+        @primary_key       = opts.fetch( :primary_key, nil )
+        @pagination_params = opts.fetch( :pagination_params, {} )
+        @path_params       = opts.fetch( :path_params, {} )
+        @additional_params = opts.fetch( :additional_params, {} )
+        @filters           = opts.fetch( :filters, {} )
+        @includes          = opts.fetch( :includes, [] )
+        @orders            = opts.fetch( :orders, [] )
+        @fields            = opts.fetch( :fields, [] )
       end
 
       def where(conditions = {})
         # pull out any path params here
-        @path_params.merge!(conditions.slice(*klass.prefix_params))
-        @filters.merge!(conditions.except(*klass.prefix_params))
-        self
+        path_conditions = conditions.slice(*klass.prefix_params)
+        unpathed_conditions = conditions.except(*klass.prefix_params)
+
+        _new_scope( path_params: path_conditions, filters: unpathed_conditions )
       end
 
       def order(*args)
-        @orders += parse_orders(*args)
-        self
+        _new_scope( orders: parse_orders(*args) )
       end
 
       def includes(*tables)
-        @includes += parse_related_links(*tables)
-        self
+        _new_scope( includes: parse_related_links(*tables) )
       end
 
       def select(*fields)
-        @fields += parse_fields(*fields)
-        self
+        _new_scope( fields: parse_fields(*fields) )
       end
 
       def paginate(conditions = {})
-        scope = self
+        scope = _new_scope
         scope = scope.page(conditions[:page]) if conditions[:page]
         scope = scope.per(conditions[:per_page]) if conditions[:per_page]
         scope
       end
 
       def page(number)
-        @pagination_params[ klass.paginator.page_param ] = number || 1
-        self
+        _new_scope( pagination_params: { klass.paginator.page_param => number || 1 } )
       end
 
       def per(size)
-        @pagination_params[ klass.paginator.per_page_param ] = size
-        self
+        _new_scope( pagination_params: { klass.paginator.per_page_param => size } )
       end
 
       def with_params(more_params)
-        @additional_params.merge!(more_params)
-        self
+        _new_scope( additional_params: more_params )
       end
 
       def first
@@ -92,12 +87,12 @@ def to_a
       def find(args = {})
         case args
         when Hash
-          where(args)
+          scope = where(args)
         else
-          @primary_key = args
+          scope = _new_scope( primary_key: args )
         end
 
-        klass.requestor.get(params)
+        klass.requestor.get(scope.params)
       end
 
       def method_missing(method_name, *args, &block)
@@ -106,6 +101,18 @@ def method_missing(method_name, *args, &block)
 
       private
 
+      def _new_scope( opts = {} )
+        self.class.new( @klass,
+             primary_key:       opts.fetch( :primary_key, @primary_key ),
+             pagination_params: @pagination_params.merge( opts.fetch( :pagination_params, {} ) ),
+             path_params:       @path_params.merge( opts.fetch( :path_params, {} ) ),
+             additional_params: @additional_params.merge( opts.fetch( :additional_params, {} ) ),
+             filters:           @filters.merge( opts.fetch( :filters, {} ) ),
+             includes:          @includes + opts.fetch( :includes, [] ),
+             orders:            @orders + opts.fetch( :orders, [] ),
+             fields:            @fields + opts.fetch( :fields, [] ) )
+      end
+
       def path_params
         @path_params.empty? ? {} : {path: @path_params}
       end

test/unit/query_builder_test.rb

@@ -220,4 +220,40 @@ def test_can_specify_empty_string_filter_value
     Article.where(:'author.id' => '').to_a
   end
 
+  def test_scopes_are_nondestructive
+    first_stub = stub_request(:get, "http://example.com/articles?page[page]=1&page[per_page]=1")
+        .to_return(headers: {content_type: "application/vnd.api+json"}, body: { data: [] }.to_json)
+
+    all_stub = stub_request(:get, "http://example.com/articles")
+        .to_return(headers: {content_type: "application/vnd.api+json"}, body: { data: [] }.to_json)
+
+    scope = Article.where()
+
+    scope.first
+    scope.all
+
+    assert_requested first_stub, times: 1
+    assert_requested all_stub, times: 1
+  end
+
+  def test_find_with_args
+    first_stub = stub_request(:get, "http://example.com/articles?filter[author.id]=foo")
+        .to_return(headers: {content_type: "application/vnd.api+json"}, body: { data: [] }.to_json)
+
+    all_stub = stub_request(:get, "http://example.com/articles")
+        .to_return(headers: {content_type: "application/vnd.api+json"}, body: { data: [] }.to_json)
+
+    find_stub = stub_request(:get, "http://example.com/articles/6")
+        .to_return(headers: {content_type: "application/vnd.api+json"}, body: { data: [] }.to_json)
+
+    scope = Article.where()
+
+    scope.find( "author.id" => "foo" )
+    scope.find(6)
+    scope.all
+
+    assert_requested first_stub, times: 1
+    assert_requested all_stub, times: 1
+    assert_requested find_stub, times: 1
+  end
 end