Merge pull request #344 from stokarenko/add-raise_on_blank_find_param-resource-setting

Add `raise_on_blank_find_param` resource setting

Author: gaorlov

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- [#344](https://github.com/JsonApiClient/json_api_client/pull/344) - introduce safe singular resource fetching with `raise_on_blank_find_param` resource setting
+
 ## 1.9.0
 
 - [#328](https://github.com/JsonApiClient/json_api_client/pull/328) - allow custom type for models
@@ -27,7 +29,7 @@
   * fix relationships passing to `new` and `create` methods
   * fix false positive tests on create
   * refactor tests on create/update to prevent false same positive tests in future
-  
+
 - [#315](https://github.com/JsonApiClient/json_api_client/pull/315) - add shallow_path feature to belongs_to
  * add `shallow_path` options to belongs_to to use model w/ and w/o nesting in parent resource
 

README.md

@@ -52,14 +52,14 @@ u.update_attributes(
   c: "d"
 )
 
-u.persisted? 
+u.persisted?
 # => true
 
 u.destroy
 
-u.destroyed? 
+u.destroyed?
 # => true
-u.persisted? 
+u.persisted?
 # => false
 
 u = MyApi::Person.create(
@@ -164,7 +164,7 @@ module MyApi
   class Account < JsonApiClient::Resource
     belongs_to :user
   end
-  
+
   class Customer < JsonApiClient::Resource
       belongs_to :user, shallow_path: true
     end
@@ -476,7 +476,7 @@ end
 
 ##### Custom status handler
 
-You can change handling of response status using `connection_options`. For example you can override 400 status handling. 
+You can change handling of response status using `connection_options`. For example you can override 400 status handling.
 By default it raises `JsonApiClient::Errors::ClientError` but you can skip exception if you want to process errors from the server.
 You need to provide a `proc` which should call `throw(:handled)` default handler for this status should be skipped.
 ```ruby
@@ -636,6 +636,37 @@ end
 
 ```
 
+### Safe singular resource fetching
+
+That is a bit curios, but `json_api_client` returns an array from `.find` method, always.
+The history of this fact was discussed [here](https://github.com/JsonApiClient/json_api_client/issues/75)
+
+So, when we searching for a single resource by primary key, we typically write the things like
+
+```ruby
+admin = User.find(id).first
+```
+
+The next thing which we need to notice - `json_api_client` will just interpolate the incoming `.find` param to the end of API URL, just like that:
+
+> http://somehost/api/v1/users/{id}
+
+What will happen if we pass the blank id (nil or empty string) to the `.find` method then?.. Yeah, `json_api_client` will try to call the INDEX API endpoint instead of SHOW one:
+
+> http://somehost/api/v1/users/
+
+Lets sum all together - in case if `id` comes blank (from CGI for instance), we can silently receive the `admin` variable equal to some existing resource, with all the consequences.
+
+Even worse, `admin` variable can equal to *random* resource, depends on ordering applied by INDEX endpoint.
+
+If you prefer to get `JsonApiClient::Errors::NotFound` raised, please define in your base Resource class:
+
+```ruby
+class Resource < JsonApiClient::Resource
+  self.raise_on_blank_find_param = true
+end
+```
+
 ## Contributing
 
 Contributions are welcome! Please fork this repo and send a pull request. Your pull request should have:

lib/json_api_client/query/builder.rb

@@ -86,25 +86,35 @@ def params
       end
 
       def to_a
-        @to_a ||= find
+        @to_a ||= _fetch
       end
       alias all to_a
 
       def find(args = {})
+        if klass.raise_on_blank_find_param && args.blank?
+          raise Errors::NotFound, 'blank .find param'
+        end
+
         case args
         when Hash
           scope = where(args)
         else
           scope = _new_scope( primary_key: args )
         end
 
-        klass.requestor.get(scope.params)
+        scope._fetch
       end
 
       def method_missing(method_name, *args, &block)
         to_a.send(method_name, *args, &block)
       end
 
+      protected
+
+      def _fetch
+        klass.requestor.get(params)
+      end
+
       private
 
       def _new_scope( opts = {} )

lib/json_api_client/resource.rb

@@ -36,6 +36,7 @@ class Resource
                     :keep_request_params,
                     :search_included_in_result_set,
                     :custom_type_to_class,
+                    :raise_on_blank_find_param,
                     instance_accessor: false
     class_attribute :add_defaults_to_changes,
                     instance_writer: false
@@ -54,6 +55,7 @@ class Resource
     self.add_defaults_to_changes = false
     self.search_included_in_result_set = false
     self.custom_type_to_class = {}
+    self.raise_on_blank_find_param = false
 
     #:underscored_key, :camelized_key, :dasherized_key, or custom
     self.json_key_format = :underscored_key

test/unit/query_builder_test.rb

@@ -257,6 +257,39 @@ def test_find_with_args
     assert_requested find_stub, times: 1
   end
 
+  def test_find_with_blank_args
+    blank_params = [nil, '', {}]
+
+    with_altered_config(Article, :raise_on_blank_find_param => true) do
+      blank_params.each do |blank_param|
+        assert_raises JsonApiClient::Errors::NotFound do
+          Article.find(blank_param)
+        end
+      end
+    end
+
+    # `.find('')` hits the INDEX URL with trailing slash, the others without one..
+    collection_stub = stub_request(:get, %r{\Ahttp://example.com/articles/?\z})
+        .to_return(headers: {content_type: "application/vnd.api+json"}, body: { data: [] }.to_json)
+
+    blank_params.each do |blank_param|
+      Article.find(blank_param)
+    end
+
+    assert_requested collection_stub, times: 3
+  end
+
+  def test_all_on_blank_raising_resource
+    with_altered_config(Article, :raise_on_blank_find_param => true) do
+      all_stub = stub_request(:get, "http://example.com/articles")
+          .to_return(headers: {content_type: "application/vnd.api+json"}, body: { data: [] }.to_json)
+
+      Article.all
+
+      assert_requested all_stub, times: 1
+    end
+  end
+
   def test_build_does_not_propagate_values
     query = Article.where(name: 'John').
         includes(:author).