Add error handling when sources fail to load data #41
Conversation
Previously, if any errors occurred when a source tried to load data, this would get logged and a `nil` would get returned. The danger of this is that this means errors may go unnoticed in production environments (e.g. timeouts to third parties) as a `nil` result may be a reasonable one depending on the `Source` and data. This provides two additional configurable methods of error handling: * `:raise_on_error` - this will raise a `Datlaoader.GetError` when one of the `get` methods are called. This is now the default behaviour for handling errors * `:tuples` - this changes the `get/4`/`get_many/4` methods to return `:ok`/`:error` tuples instead of just the value. This frees up the caller to handle errors any way they see fit
lib/dataloader.ex
Outdated
| file, e.g. to configure it to return nil on error: | ||
|
|
||
| config :dataloader, | ||
| get_policy: :return_nil_on_error |
There was a problem hiding this comment.
I'm not entirely sure top level config makes sense here.
lib/dataloader.ex
Outdated
| @type source_name :: any | ||
|
|
||
| @default_timeout 15_000 | ||
| def default_timeout, do: @default_timeout | ||
|
|
||
| @default_get_policy Application.get_env(:dataloader, :get_policy, :raise_on_error) |
There was a problem hiding this comment.
Ah so this pattern can lead to some fun behaviour for users. They'll do something like:
config :dataloader, get_policy: :return_nil_on_error
and then build their project from scratch. Then they change their mind and remove it. the @default_get_policy value will stay the same. The value was pulled when the dependency was compiled, and the dependency will not be recompiled just because the config changed.
If we're going to accept a mix config option here we need to always retrieve it at runtime.
There was a problem hiding this comment.
That's an absolutely fair point! I have been burned by that a couple of times before and clearly haven't learned my lesson.
I think you're right though, having this global config isn't really necessary so I'll just remove it 👍
|
@benwilson512 - thanks for the feedback! As suggested I've removed the global configuration for the |
|
This looks great, thank you! |
|
💯 |
|
Also, just realised I forgot to mention in the description but many thanks to @oneKelvinSmith who paired with me on this change 👍 |
I'm raising this following an initial PR (#39) a couple of weeks ago. I've had a couple of other guys take a look internally before raising and so hopefully it should pretty much be there 🤞. I ended up implementing all three ways mentioned in #39 (as this was trivial in the end; see the
do_get/2method). Happy to make changes, rename stuff etc if you want 😄!Previously, if any errors occurred when a source tried to load data,
this would get logged and a
nilwould get returned.The danger of this is that this means errors may go unnoticed in
production environments (e.g. timeouts to third parties) as a
nilresult may be a reasonable one depending on the
Sourceand data.This provides two additional configurable methods of error handling:
:raise_on_error- this will raise aDatlaoader.GetErrorwhen oneof the
getmethods are called. This is now the default behaviourfor handling errors
:tuples- this changes theget/4/get_many/4methods to return:ok/:errortuples instead of just the value. This frees up thecaller to handle errors any way they see fit