absinthe-graphql / absinthe_ecto

DEPRECATED: Use dataloader
MIT License
130 stars 36 forks source link

Assoc helper, batch id and query fun #25

Closed tlvenn closed 6 years ago

tlvenn commented 7 years ago

With the introduction of the query fun support to tailor the batch query, I am wondering if it shouldnt be used to compose the batch id.

Suppose we have 2 fields which use the same ecto field but with 2 different queries. My understanding is that because they share the same batch id, only one of those 2 queries will run and those 2 fields will end up with the same value.

@benwilson512 what do you think ?

tlvenn commented 6 years ago

Hey @benwilson512 , whenever you get the chance, would appreciate your feedback on this, thanks in advance.

benwilson512 commented 6 years ago

Hey @tlvenn apologies for the delayed response, it seems this package isn't hooked up to our slack room properly so I was missing notifications.

Just to make sure I understand the question properly, can you illustrate a quick example of what you mean? The absinthe_ecto batch key includes all of https://github.com/absinthe-graphql/absinthe_ecto/blob/master/lib/absinthe/ecto.ex#L163 which actually does include the generated query.

benwilson512 commented 6 years ago

I will say, I'm not sure that this approach is sustainable. For example, this may already break with a query like:

assoc(fn query, args, info ->
  last_week = DateTime.utc_now |> subtract_one_week()
  query |> where([x], x.created_at > ^last_week)
end)

by recomputing the last_week time for every child there's a good chance the value will differ by some small amount by the time we've gone from the first child to the last, and we'll have multiple batches. Now, there are ways to write this via fragments that use NOW() in the DB or whatever but this quickly becomes a very subtle trap for folks.

It's also an issue for the DataLoader approach which, since you can ask for several things at once, requires that the user be able to supply the batch key in order to pull out the right result. But that may be really a different discussion.

tlvenn commented 6 years ago

Ha yes my bad, somehow got confused with the id variable above and my mind projected it as the batch id somehow ;)

Regarding your latest comment, I think it fails in the realm of limitation of the approach (similar to limit) and as far as the user is aware, it should not be too much of an issue but it definitely deserves to be documented.