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 runs once, never again #74

Closed
alex-knowles opened this issue Apr 10, 2019 · 2 comments
Closed

Dataloader.KV runs once, never again #74

alex-knowles opened this issue Apr 10, 2019 · 2 comments

Comments

@alex-knowles
Copy link
Contributor

alex-knowles commented Apr 10, 2019

Summary

  • Version: Dataloader 1.0.6
  • Expected behavior: You can call Dataloader.run/1, then Dataloader.load/4 with new items and then Dataloder.run/1 again to fetch results for the new items
  • Observed behavior: When Dataloader.KV is the source, the second call to Dataloader.run/1 does not fetch results for new items

Details

It's possible that the :load_function I'm passing to Dataloader.KV.new/1 is flawed.

However, it should be easy to reproduce this behavior with a proven :load_function of your own.

Try this at home

With Dataloader.KV as the source:

iex(1)> loader = Dataloader.new() |> Dataloader.add_source(Data, kv_source)
iex(2)> loader = Dataloader.load(loader, Data, kv_batch, kv_item_1)
iex(3)> Dataloader.pending_batches?(loader)
true
iex(4)> loader = Dataloader.run(loader)
iex(5)> Dataloader.pending_batches?(loader)
false
iex(6)> loader = Dataloader.load(loader, Data, kv_batch, kv_item_2)
iex(7)> Dataloader.pending_batches?(loader)
false

On the other hand, if I use Dataloader.Ecto, I get a different result:

iex(1)> loader = Dataloader.new() |> Dataloader.add_source(EctoData, ecto_source)
iex(2)> loader = Dataloader.load(loader, EctoData, ecto_batch, ecto_item_1)
iex(3)> Dataloader.pending_batches?(loader)
true
iex(4)> loader = Dataloader.run(loader)
iex(5)> Dataloader.pending_batches?(loader)
false
iex(6)> loader = Dataloader.load(loader, EctoData, ecto_batch, ecto_item_2)
iex(7)> Dataloader.pending_batches?(loader)
true

Why this matters

When you run a GraphQL query like:

query StarWarsHuman($humanID:ID!){
  human(id: $humanID) {
    name
    friends {
      name
      friends {
        name
      }
    }
  }
}

You get back:

{
  "data": {
    "human": {
      "name": "Luke Skywalker",
      "friends": [
        {
          "name": "Han Solo",
          "friends": null
        },
        {
          "name": "Leia Organa",
          "friends": null
        },
        {
          "name": "C-3PO",
          "friends": null
        },
        {
          "name": "R2-D2",
          "friends": null
        }
      ]
    }
  }
}

You can get traverse one depth level of a graph, but no further 🙁

alex-knowles added a commit to alex-knowles/dataloader that referenced this issue Apr 12, 2019
Add a Dataloader.KV test to assert that the loader can
load new data in subsequent rounds.

See issue absinthe-graphql#74
@alex-knowles
Copy link
Contributor Author

I wrote a test to illustrate. In commit ef5c978:

test "loading something not in the cache does change the loader", %{loader: loader} do
round1_loader =
loader
|> Dataloader.load(Test, :users, "ben")
|> Dataloader.run()
round2_loader =
round1_loader
|> Dataloader.load(Test, :users, "bruce")
|> Dataloader.run()
refute round2_loader == round1_loader
refute loader == round1_loader
end

The test currently fails:

  1) test loading something not in the cache does change the loader (Dataloader.KVTest)
     test/dataloader/kv_test.exs:63
     Refute with == failed, both sides are exactly equal
     code: refute round2_loader == round1_loader
     left: %Dataloader{
             options: [get_policy: :raise_on_error],
             sources: %{
               Test => %Dataloader.KV{
                 batches: %{},
                 load_function: #Function<0.66469107/2 in Dataloader.KVTest.__ex_unit_setup_0/1>,
                 opts: [max_concurrency: 16, timeout: 30000],
                 results: %{
                   users: %{"ben" => [id: "ben", username: "Ben Wilson"]}
                 }
               }
             }
           }
     stacktrace:
       test/dataloader/kv_test.exs:74: (test)

Ben is in the results from round 1, but Bruce didn't make it in round 2.

alex-knowles added a commit to alex-knowles/dataloader that referenced this issue Apr 15, 2019
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 added a commit to alex-knowles/dataloader that referenced this issue Apr 15, 2019
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).
@benwilson512
Copy link
Contributor

Fixed by PR, 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

No branches or pull requests

2 participants