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

Default value + Callable syntax #8

Merged
merged 6 commits into from
Nov 1, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ that you can set version constraints properly.

#### [Unreleased](https://github.com/exAspArk/batch-loader/compare/v1.0.4...HEAD)

* WIP
* `Added`: `default_value` override option.
* `Added`: `loader.call {}` block syntax, for memoizing repeat calls to the same item.

#### [v1.0.4](https://github.com/exAspArk/batch-loader/compare/v1.0.3...v1.0.4)

Expand Down
49 changes: 47 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ This gem provides a generic lazy batching mechanism to avoid N+1 DB queries, HTT
* [Highlights](#highlights)
* [Usage](#usage)
* [Why?](#why)
* [Basic example](#basic-example)
* [Basic examples](#basic-examples)
* [How it works](#how-it-works)
* [RESTful API example](#restful-api-example)
* [GraphQL example](#graphql-example)
Expand Down Expand Up @@ -97,7 +97,7 @@ puts users # Users

But the problem here is that `load_posts` now depends on the child association and knows that it has to preload data for future use. And it'll do it every time, even if it's not necessary. Can we do better? Sure!

### Basic example
### Basic examples
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to update the Contents section as well :)


With `BatchLoader` we can rewrite the code above:

Expand Down Expand Up @@ -125,6 +125,51 @@ puts users # Users SELECT * FROM users WHERE id IN

As we can see, batching is isolated and described right in a place where it's needed.

For batches where there is no item in response to a call, we normally return nil. However, you can use `default_value:` to return something else instead. This is particularly useful for 1:Many relationships, where you

```ruby
def load_posts(ids)
Post.where(id: ids)
end

def load_user(post)
BatchLoader.for(post.user_id).batch(default_value: NullUser.new) do |user_ids, loader|
User.where(id: user_ids).each { |user| loader.call(user.id, user) }
end
end

posts = load_posts([1, 2, 3])


users = posts.map do |post|
load_user(post)
end

puts users
```

For batches where the value is some kind of collection, such as an Array or Hash, `loader` also supports being called with a block, which yields the _current_ value, and returns the _next_ value. This is extremely useful for 1:Many relationships:

```ruby
def load_users(ids)
User.where(id: ids)
end

def load_comments(user)
BatchLoader.for(user.id).batch(default_value: []) do |comment_ids, loader|
Comment.where(user_id: user_ids).each do |comment|
loader.call(user.id) { |memo| memo.push(comment) }
end
end
end

users = load_users([1, 2, 3])

comments = users.map do |user|
load_comments(user)
end
```

### How it works

In general, `BatchLoader` returns a lazy object. Each lazy object knows which data it needs to load and how to batch the query. As soon as you need to use the lazy objects, they will be automatically loaded once without N+1 queries.
Expand Down
18 changes: 14 additions & 4 deletions lib/batch_loader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ class BatchLoader
IMPLEMENTED_INSTANCE_METHODS = %i[object_id __id__ __send__ singleton_method_added __sync respond_to? batch inspect].freeze
REPLACABLE_INSTANCE_METHODS = %i[batch inspect].freeze
LEFT_INSTANCE_METHODS = (IMPLEMENTED_INSTANCE_METHODS - REPLACABLE_INSTANCE_METHODS).freeze
NULL_VALUE = :batch_loader_null

NoBatchError = Class.new(StandardError)

Expand All @@ -22,7 +23,8 @@ def initialize(item:)
@item = item
end

def batch(cache: true, &batch_block)
def batch(default_value: nil, cache: true, &batch_block)
@default_value = default_value
@cache = cache
@batch_block = batch_block
__executor_proxy.add(item: @item)
Expand Down Expand Up @@ -75,12 +77,20 @@ def __ensure_batched
return if __executor_proxy.value_loaded?(item: @item)

items = __executor_proxy.list_items
loader = ->(item, value) { __executor_proxy.load(item: item, value: value) }
loader = -> (item, value = NULL_VALUE, &block) {
Copy link
Owner

@exAspArk exAspArk Oct 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not very common but there is another way to check whether the optional argument was passed or not:

-> (item, value = (value_set = true; nil), &block) {
  if block
    raise ... if value_set
    ...
  else
    raise ... unless value_set
    ...
  end
}

This way you can leave your argument checks (raise) without touching and preloading the value to check it's value.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's neat, i didn't know that was possible in that context, although I don't find it particularly legible.

I also consider (*item_and_maybe_value, &block) followed by decomposition in the block, but didn't like that it changes the arity to be incredibly permissive.

I think the existing implementation is the most readable of the 3, if not necessarily the most clever.

if block
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had no idea you could pass a block to a lambda, that was the only reason I extracted this, so I have re-inlined it.

raise ArgumentError, "Please pass a value or a block, not both" if value != NULL_VALUE
next_value = block.call(__executor_proxy.loaded_value(item: item))
else
next_value = value
end
__executor_proxy.load(item: item, value: next_value)
}

@batch_block.call(items, loader)
items.each do |item|
next if __executor_proxy.value_loaded?(item: item)
loader.call(item, nil) # use "nil" for not loaded item after succesfull batching
loader.call(item, @default_value)
end
__executor_proxy.delete(items: items)
end
Expand Down Expand Up @@ -109,7 +119,7 @@ def __purge_cache
def __executor_proxy
@__executor_proxy ||= begin
raise NoBatchError.new("Please provide a batch block first") unless @batch_block
BatchLoader::ExecutorProxy.new(&@batch_block)
BatchLoader::ExecutorProxy.new(@default_value, &@batch_block)
end
end

Expand Down
11 changes: 8 additions & 3 deletions lib/batch_loader/executor_proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@

class BatchLoader
class ExecutorProxy
attr_reader :block, :global_executor
attr_reader :default_value, :block, :global_executor

def initialize(&block)
def initialize(default_value, &block)
@default_value = default_value
@block = block
@block_hash_key = block.source_location
@global_executor = BatchLoader::Executor.ensure_current
Expand All @@ -29,7 +30,11 @@ def load(item:, value:)
end

def loaded_value(item:)
loaded[item]
if value_loaded?(item: item)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that with the changes the source code became not thread-safe :)

It's possible to have race-conditions when Thread1 read the value_loaded?(item: item) value and Thread2 loaded value after that, before Thread1 finished executing loader. Need to think how to avoid such situations, probably with some locking mechanisms (hope they won't affect performance a lot).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was able to reproduce the race-condition in #10. Will think what's the best way to fix it.

loaded[item]
else
@default_value.dup
end
end

def value_loaded?(item:)
Expand Down
32 changes: 32 additions & 0 deletions spec/batch_loader_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,38 @@

expect(lazy).to eq(2)
end

it 'supports alternative default values' do
lazy = BatchLoader.for(1).batch(default_value: 123) do |nums, loader|
# No-op, so default is returned
end

expect(lazy).to eq(123)
end

it 'supports memoizing repeated calls to the same item, via a block' do
lazy = BatchLoader.for(1).batch(default_value: []) do |nums, loader|
nums.each do |num|
loader.call(num) { |memo| memo.push(num) }
loader.call(num) { |memo| memo.push(num + 1) }
loader.call(num) { |memo| memo.push(num + 2) }
end
end

expect(lazy).to eq([1,2,3])
end

context "called with block and value syntax" do
it 'raises ArgumentError' do
lazy = BatchLoader.for(1).batch(default_value: {}) do |nums, loader|
nums.each do |num|
loader.call(num, "one value") { "too many values" }
end
end

expect { lazy.sync }.to raise_error(ArgumentError)
end
end
end

describe '#inspect' do
Expand Down