[PROPOSAL] Expose errors when getting data in Dataloader

[seddy]

NOTE - This is very much WIP, I'm pushing this commit up in order to get some
feedback on it before carrying on. Please don't review this as production-ready, as it's not and half the tests are failing 😄

Summary

Currently, there is no explicit handling of errors in Dataloader if we have (for example) a third party timeout, a database error or simple match error. The error is logged, but not otherwise exposed. This has a side-effect of potentially dropping Source structs and leading to unexpected errors where despite loading a source, Dataloader will raise with Source not found. I initially highlighted this in #35.

Possible approaches

As I see it, there are three possible approaches we can take when dealing with the errors here:

1. Return nil on error - current master behaviour, which has the shortcoming of hiding errors in production environments and potentially raising misleading errors (e.g. Demonstrate unexpected timeout behaviour when sources are slow #35)
2. Raise on error - that is the approach taken here, as I expect this would be the least breaking change; the upgrade path for most people would involve being aware that errors may now get raised. The only complex changes may arise for people who've implemented their own Source modules.
3. Change get and get_many to return :ok/:error tuples (a.k.a. let the developer decide) - This would be a simple extension of (2), as this is largely how I've implemented this internally. However, it's a more significant breaking change as anyone using Dataloader as they'd need to change everything to handle tuples instead of raw values.

It could be that we want a configurable approach here, but I'd be tempted to shy away from that simply because configuration will add complexity.

Where to raise on error

There are two places we could legitimately raise an error, either when we load the data or when we get the data. I've taken the approach of storing :ok/:error tuples internally on load, and then raising on get. My assumption here being that it's possible we load data we don't fetch, in which case the errors are irrelevant.

Work done so far

I've essentially done the following:

1. Refactor pmap to return :ok/:error tuples instead of swallowing errors
2. Refactor Dataloader.run/1 to manage the new pmap
3. Added "error" tests into the KV class and got them passing
4. Added some additional comments with thoughts on tidying up and possible further changes

I chose to implement the changes in KV rather than Ecto because it was a simpler interface and should demonstrate how sources can be updated to get the desired effect. I wanted to get feedback on the approach before carrying on in case I'm just chasing myself down a rabbit hole, so figured this was a good time to push something up.

Things to look at in this PR

Specifically, check out the tests; they outline the behaviour changes best. Also I'd like some validation on my comments around pmap please. I'm fairly certain it should be made less generic and behave differently depending on whether we're calling from Dataloader.run or a Source.run.

Next steps

• Decide on approach
• Refactor or update current implementation where necessary
• Extend this to Ecto
• Profit

[seddy]

Had a chat with @benwilson512 on slack, agreed to carry on with this primarily focusing on implementing option 2 (raising on error), and allowing the silent failure to be configurable for backwards compatibility. We'll make the raising the default behaviour.

Closing for now until I have a more complete PR.

[axelson]

@seddy will a behavior like #3 (:ok/:error) tuples be possible to configure?

[seddy]

Possibly @axelson - I expect that this is how I'll be maintaining state internally, so exposing this would hopefully be trivial. However, I'd be a bit hesitant about having three possible configurations for behaviour as it will almost certainly increase complexity, not just for code but for understanding the different configuration options.

That being said, I'll work on this more this week I hope, so we can take a view once some working code is up 😄