Do not depend on #method_missing for #respond_to?

[ZJvandeWeg]

After the object is batch loaded, the original method_missing
implementation is not called anymore after a __replace_with! call.

When respond_to? is called on the loaded object afterwords this will
call #method_missing on the loaded object. However, the loaded object
can itself have an implementation of method_missing, in this case only
to determine if it responds to a symbol.

Other than the current implementation, the #method_missing could be
listed in IMPLEMENTED_INSTANCE_METHODS, so it won't be rerouted to the
lazy loaded object. This however calls __sync! again, with the
overhead that entails.

Detected this behaviour first for #4, but that was the wrong patch.

[coveralls]

Coverage increased (+0.002%) to 99.721% when pulling 37a408e on ZJvandeWeg:zj-method-missing-bug into 2cd0fa7 on exAspArk:master.

[ZJvandeWeg]

@exAspArk let me know if you have questions on the description. Its quite an edge case, but one we at GitLab ran into.

Please let me know what you think about the patch.

[exAspArk]

@ZJvandeWeg hey, thanks a lot for opening the PR!

After the object is batch loaded, the original method_missing implementation is not called anymore after a __replace_with! call.
When respond_to? is called on the loaded object afterwords this will call #method_missing on the loaded object.

I think that method_missing(:respond_to?, method_name, include_private) doesn't call the original method_missing , it calls respond_to?(method_name, include_private) through public_send? Or there is another issue which can be reproduced with a test?

However, reusing already loaded value for respond_to? make perfect sense to me.

Currently, __sync! is designed to "sync" and "replace" the BatchLoader instance (with the cache option true by default). And "replacing" the object is quite overhead because __replace_with! isn't the fastest method :)

For example, we could make __sync! idempotent and smarter. So, if we call it multiple times and it was already executed – don't do anything. Something like:

   def __sync!
+    return self if @replaced
     loaded_value = __sync

     if @cache
+      @replaced = true
       __replace_with!(loaded_value)
     else
       loaded_value
     end
   end

This way it'll also respect the cache flag. I.e. if user decided not to cache loaded value, it always will be loaded over and over again and returned to the user.

[ZJvandeWeg]

I think that method_missing(:respond_to?, method_name, include_private) doesn't call the original method_missing, it calls respond_to?(method_name, include_private) through public_send? Or there is another issue which can be reproduced with a test?

Fair enough, I'll write a test to show what happens.

[ZJvandeWeg]

@exAspArk I'll update this branch in a bit to have a failing test case. The first commit will be the failing test case, where the last commit will rectify this.

On the __sync! idea, we can't call :respond_to? on it, given it will keep recursively calling itself and thus fail when the stack gets too deep.

I've checked out if I can refactor to create a loaded_value method. But first wanted to get your view on it, because you have a better idea how this library works. 😄

[coveralls]

Coverage increased (+0.005%) to 99.725% when pulling 80ec48c on ZJvandeWeg:zj-method-missing-bug into 2cd0fa7 on exAspArk:master.

[ZJvandeWeg]

I'll rebase the test case commit away, but for now it might be good for you to checkout what I mean.

[ZJvandeWeg]

@exAspArk Is there something I can do to move this along? 😄

[exAspArk]

@ZJvandeWeg hey, sorry for the delay, busy end of the year :) I'll review it on Friday.

[exAspArk]

@ZJvandeWeg I see what you mean! We replace the method_missing method on which we depend in the respond_to?.

Just one suggestion:

# Instead of:
loaded_value = defined?(@loaded_value) ? @loaded_value : __sync!
loaded_value.respond_to?(method_name, include_private)

# Use:
__sync!.respond_to?(method_name, include_private)

Since there is a cache option which can be set to true or false, it makes sense to always rely on __sync! which takes the flag into account automatically.

Here is an example of the test which will be fixed with the changes:

it 'syncs the object on every call' do
  loaded_user = post.user_lazy(cache: false)

  expect(User).to receive(:where).with(id: [1]).once.and_call_original
  loaded_user.respond_to?(:id)

  expect(User).to receive(:where).with(id: [1]).once.and_call_original
  loaded_user.respond_to?(:id)
end

Thanks a lot for the test and awesome commit messages!

[ZJvandeWeg]

# Use:
__sync!.respond_to?(method_name, include_private)

@exAspArk This won't work, as self that is returned will call itself recursively, until the stack is too deep. That's why I wanted to wrap @loaded_value in #loaded_value, but that has downsides too when replacing the value.

I also disagree with the test which you propose, actually. Because this will be testing the caching being turned of. However, the bug I ran into was the dependency of method_missing in #respond_to?. The proposed test is not covering that from happening again. I'll try to take a look the coming days to improve the test, but briefly looking at it again today I think I'll create a new class for that test.

let me know what you think! 😄

[exAspArk]

@ZJvandeWeg oh, yeah, you're right. There will be a recursion =/

What do you think about:

def respond_to?(method_name, include_private = false)
  LEFT_INSTANCE_METHODS.include?(method_name) || __loaded_value.respond_to?(method_name, include_private)
end

private

def __loaded_value
  result = __sync!
  @cache ? @loaded_value : result
end

?

It'll take the cache flag into account, so all the tests will pass:

# no recursion
it 'syncs the object just once' do
  loaded_user = post.user_lazy

  expect(loaded_user.respond_to?(:id)).to eq(true)
end

# no cache
it 'syncs the object on every call with cache false flag' do
  loaded_user = post.user_lazy(cache: false)

  expect(User).to receive(:where).with(id: [1]).once.and_call_original
  loaded_user.respond_to?(:id)

  expect(User).to receive(:where).with(id: [1]).once.and_call_original
  loaded_user.respond_to?(:id)
end

[coveralls]

Coverage increased (+0.01%) to 99.73% when pulling 5394d70 on ZJvandeWeg:zj-method-missing-bug into 2cd0fa7 on exAspArk:master.

[ZJvandeWeg]

@exAspArk Thanks for pointing me to the cache feature about twice now 😅 Its not something we use at the moment, so I keep forgetting about it.

Your suggestions work, and I think I better understand the gem now to have more confidence in my own code too. Thanks for taking the time anyway. Much appreciated.

I've squashed the commits into one, and looked over the diff once more, it seems all good to me. A bit noisy on the spec/ because the use of other rspec features, hope its ok.

[exAspArk]

@ZJvandeWeg awesome!

cache feature

Yeah, it is not so useful in production environment but may be helpful in test environment or REPL to avoid any side effects. However, it currently plays a dual role:

• no caching between BatchLoader instances
• no caching between method executions for the same instances – this one is less useful and probably can be removed in the future

A bit noisy on the spec/ because the use of other rspec features, hope its ok

Absolutely. Even though I personally prefer Four-Phase Test practice to make each test more "independent" and readable over DRY and overusing RSpec features like let, subject, etc. Previously, I wrote a lot of tests with just 1 line in the body, it was clean but not really easily understandable and maintainable. However, it's all very subjective and seems fine to me in this case :)

Thanks a lot for your contribution! And sorry that it took me so long to understand the problem and review the PR. I'll release it in v1.2.1.