exAspArk
`BatchLoader.for().batch {}` evaluated as Truthy but should be Falsey
I think this bug is a serious problem. every time BatchLoader.for().batch is called, a new object of nil will be returned by default. this can cause some weird bugs in Ruby and is a waste of memory. you can reproduce it as the following:
➜ ~ irb
irb(main):001:0> require 'batch-loader'
=> true
irb(main):002:0> a = nil
=> nil
irb(main):003:0> b = BatchLoader.for(1).batch do |ids, loader|
ids.each { |id| loader.call(id, nil) }
end
=> nil
irb(main):004:0> a.object_id
=> 8
irb(main):005:0> b.object_id # why 'b' is a new object of `nil`?
=> 200
irb(main):006:0> b = BatchLoader.for(2).batch do |ids, loader|
ids.each { |id| loader.call(id, nil) }
end
=> nil
irb(main):007:0> b.object_id # a new object of `nil` again
=> 220
irb(main):008:1* unless b
irb(main):009:1* puts 1 # this should be called, but it was skipped because of a new object of `nil`
irb(main):010:0> end
=> nil
irb(main):011:1* unless a
irb(main):012:1* puts 1
irb(main):013:0> end
1
=> nil
env: batch-loader (2.0.1)
this bug result in some weird behaviors that unless b cannot be evaluated as true and it's hard to debug, it confused me a lot.
I know why. it's because b is not an actual NilClass object. it's just an instance of BatchLoader. method_missing is patched and be proxied to the underlying __loaded_value so it looks like an underlying object but it's actually not. But I'm doubting if that can work for every case. just like what I showed above, unless b is an edge case that should be evaluated as true but is false right now.
given another case, some unexpected behavior:
> nil == b
=> false
> b == nil
=> true
> [nil].include?(b) # because `includes?` internally uses '=='
=> false
> b || 1
=> nil # should be 1
FYI BatchLoader undefines several methods (see batch_loader.rb:144) including is_a? and PP's pretty_print which messes with IRB's inspector logic.
If you try it outside of IRB, you'll see the actual inspect value, e.g.
$ ruby -rbatch-loader -e "p BatchLoader.for(1).batch {}"
#<BatchLoader:0x120>
The delegation only works for / relies on method calls. Here's another example:
require 'batch-loader'
class Foo; end
foo = BatchLoader.for(1).batch do |ids, loader|
ids.each { |id| loader.call(id, Foo.new) }
end
foo.class
#=> Foo
but:
case foo
when Foo
puts 'a Foo'
else
puts 'not a Foo'
end
# not a Foo
This is because case calls Foo === foo under the hood (instead of foo.is_a?(Foo)), thus bypassing the proxy object and its delegation.
As a workaround, you could call itself on the proxy which returns the actual object:
case foo.itself
when Foo
puts 'a Foo'
else
puts 'not a Foo'
end
# a Foo
Applied to your examples:
b = BatchLoader.for(1).batch do |ids, loader|
ids.each { |id| loader.call(id, nil) }
end
b.itself.object_id
#=> 8
puts 1 unless b.itself
# prints 1
nil == b.itself
#=> true
[nil].include?(b.itself)
#=> true
b.itself || 1
#=> 1
@sos4nt Thanks for your detailed explanation. I totally understand what you mean. But I still think this should be a problem. We're trying to use batch-loader to fix our N+1 issues. I thought it can be integrated seamlessly without significant changes. For example:
class UserService
def self.find_by_user_id(id)
User.find_by(id: id)
end
def self.lazy_find_by_user_id(id)
BatchLoader.for(id).batch do |ids, loader|
User.where(id: ids).each { |user| loader.call(user.id, user) }
end
end
end
class OrderService
def create(user_id)
user = UserService.find_by_user_id(user_id)
unless user
raise "user not found"
end
...
end
end
I thought we can just replace UserService.find_by_user_id(user_id) with UserService.lazy_find_by_user_id(user_id). Nothing else we need to change. But seems like we also need to change unless user to unless !!user. Of course we can simply do that. but please just imagine that if there are many places that write code like if user or user_ids.map {|id| UserService.lazy_find_by_user_id(id)}.include?(nil) or user || create_user(). We need to carefully check the each of them and correct them one by one. And tell people to use if !!user rather than if user because user is not an actual nil but a delegator. IMHO, It's counter-intuitive and error-prone. How can we integrate it without significant changes?