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

Dataloader.KV: load new items when results are non-empty #75

Merged
merged 4 commits into from
Apr 16, 2019

Conversation

alex-knowles
Copy link
Contributor

@alex-knowles alex-knowles commented Apr 15, 2019

Fixes issue #74.

Map.get/2 will either return some value() or nil.

case Map.get(batch, id) do
{:error, reason} -> {:error, reason}
item -> {:ok, item}
end

The above case statement does not specifically handle nil, because it (incorrectly) expects to find either an :error 2-tuple or a cached result.

However, when fetch/3 is called by load/3, it is possible that:

  1. results is a non-empty map, and
  2. there is no results entry matching id

In that case, Map.get/2 would return nil, but it was being pattern-matched as a positive result instead of an error, which caused the incorrect path to be followed in:

case fetch(source, batch_key, id) do
{:error, _message} ->
update_in(source.batches, fn batches ->
Map.update(batches, batch_key, MapSet.new([id]), &MapSet.put(&1, id))
end)
_ ->
source
end

This prevented new data from being loaded into a Dataloader.KV source that contained cached results.

The fix proposed here is to use Map.get/3 with a third argument of :error, which can be explicitly matched in the case statement:

        case Map.get(batch, id, :error) do
          {:error, reason} -> {:error, reason}
          :error -> {:error, "Unable to find id #{inspect(id)}"}
          item -> {:ok, item}
        end

Add a Dataloader.KV test to assert that the loader can
load new data in subsequent rounds.

See issue absinthe-graphql#74
Handle case where Dataloader.KV has some results and is asked
to load an id that is neither present in 'results' or 'batches'.

In this case Map.get/2 would return `nil`, which was falling
through and being treated as if the id was found in the results.

This replaces Map.get/2 with Map.get/3 so that we can provide
an explicit value to pattern-match on (:error).
@alex-knowles alex-knowles marked this pull request as ready for review April 15, 2019 01:55
Calling Map.get/3 with :error as the third argument is
almost the same as calling Map.fetch/2 except that non-errors
are wrapped in :ok tuples.

This more clearly delinates the cases where a result is found
and the case where a result is not found.  It also shows that
{:error, reason} is expected to be mapped to 'id' in some
situations.
@benwilson512 benwilson512 merged commit 4c4d536 into absinthe-graphql:master Apr 16, 2019
@benwilson512
Copy link
Contributor

Thank you!

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

Successfully merging this pull request may close these issues.

2 participants