Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Link method_missing delegation wrongly returns nil when field value is false #126

Closed
ivoanjo opened this issue Aug 26, 2017 · 1 comment
Closed
Labels

Comments

@ivoanjo
Copy link
Contributor

ivoanjo commented Aug 26, 2017

When a resource includes some field that is false, trying to read it through the Link class will result in the value being returned as nil instead of false.

Test case:

diff --git a/test/hyperclient/link_test.rb b/test/hyperclient/link_test.rb
index 823efa6..20e8872 100644
--- a/test/hyperclient/link_test.rb
+++ b/test/hyperclient/link_test.rb
@@ -479,6 +479,16 @@ module Hyperclient
           resource.orders.first.id.must_equal 1
         end
 
+        it 'can handle false values in the response' do
+          resource = Resource.new({ '_links' => { 'orders' => { 'href' => '/orders' } } }, entry_point)
+
+          stub_request(entry_point.connection) do |stub|
+            stub.get('http://api.example.org/orders') { [200, {}, { 'any' => false }] }
+          end
+
+          resource.orders.any.must_equal false
+        end
+
         it "doesn't delegate when link key doesn't match" do
           resource = Resource.new({ '_links' => { 'foos' => { 'href' => '/orders' } } }, entry_point)

Current result:

Hyperclient::Link::method_missing::delegation
     FAIL (0:00:00.145) test_0002_can handle false values in the response
          Expected: false
            Actual: nil
        @ (eval):8:in `must_equal'
          test/hyperclient/link_test.rb:489:in `block (4 levels) in <module:Hyperclient>'

This seems to me to be due to a bad interaction in Link#method_missing (https://github.com/codegram/hyperclient/blob/master/lib/hyperclient/link.rb#L129):

    # Internal: Delegate the method further down the API if the resource cannot serve it.
    def method_missing(method, *args, &block)
      if _resource.respond_to?(method.to_s)
        _resource.send(method, *args, &block) || delegate_method(method, *args, &block)
      else
        super
      end
    end

In this case, _resource.send(method, *args, &block) is returning false, which we are mistaking for the case where we're trying to call an array method on the resource, and thus instead of returning false we are calling delegate_method (which will then return the nil).

I was thinking about it and it seems to me that changing it to treat false differently would fix the issue:

diff --git a/lib/hyperclient/link.rb b/lib/hyperclient/link.rb
index 1fd1169..cc5b635 100644
--- a/lib/hyperclient/link.rb
+++ b/lib/hyperclient/link.rb
@@ -126,7 +126,8 @@ module Hyperclient
     # Internal: Delegate the method further down the API if the resource cannot serve it.
     def method_missing(method, *args, &block)
       if _resource.respond_to?(method.to_s)
-        _resource.send(method, *args, &block) || delegate_method(method, *args, &block)
+        result = _resource.send(method, *args, &block)
+        result.nil? ? delegate_method(method, *args, &block) : result
       else
         super
       end

But I was not entirely sure, hence I decided to open an issue first, instead of jumping straight to a PR. Any thoughts on this approach?

@dblock
Copy link
Collaborator

dblock commented Aug 26, 2017

Your fix looks right, make a PR!

@dblock dblock added the bug? label Aug 26, 2017
ivoanjo added a commit that referenced this issue Aug 26, 2017
Whenever a resource includes a field that is `false`, trying to access
it through a `Link` returned `nil`, rather than `false`.

This is because `Link` confused the `false` response with the resource
not actually handling the value, and then tried to use the delegation as
if a collection method was being called on the resource.

Fixes #126
ivoanjo added a commit that referenced this issue Aug 26, 2017
Whenever a resource includes a field that is `false`, trying to access
it through a `Link` returned `nil`, rather than `false`.

This is because `Link` confused the `false` response with the resource
not actually handling the value, and then tried to use the delegation as
if a collection method was being called on the resource.

Fixes #126
dblock pushed a commit that referenced this issue Aug 26, 2017
* Fix link delegation returning nil for field with value false

Whenever a resource includes a field that is `false`, trying to access
it through a `Link` returned `nil`, rather than `false`.

This is because `Link` confused the `false` response with the resource
not actually handling the value, and then tried to use the delegation as
if a collection method was being called on the resource.

Fixes #126

* Update CHANGELOG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants