Prevent panic when batchFunc returns nil in his results

[jpastoor]

Currently when the batchloader returns some nil values in his output list of Results, a panic occurs. This case is possible because the return type is by reference []*Result.

The programmer should fix his batchFunc obviously, but the problem might be hard to debug since the panic does not give information on what went wrong.

The proposed change replaces the nil values with &Result{Error: fmt.Errorf("no value for key")}.

[codecov-commenter]

Codecov Report

Merging #64 into master will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
+ Coverage   86.94%   87.07%   +0.13%     
==========================================
  Files           6        6              
  Lines         291      294       +3     
==========================================
+ Hits          253      256       +3     
  Misses         38       38              

[tonyghita]

I don't quite understand why this would be desirable. I don't think it's an error for a value not to exist at a key. Am I missing something?

[jpastoor]

In the case that the batchFuncs actually misses keys (and length of result is different than the length of keys) the dataloader injects errors for the thunk() method to read - everything keeps working in that case.

But in the case the length is correct, but the output of the batchFunc contain nils, the thunk() method receives a nil in result.value while retrieving that key.

thunk := func() (interface{}, error) {
		result.mu.RLock()
		resultNotSet := result.value == nil // resultNotSet: true
		result.mu.RUnlock()

		if resultNotSet {
			result.mu.Lock()
			if v, ok := <-c; ok { // v: nil   ok: true
				result.value = v
			}
			result.mu.Unlock()
		}
		result.mu.RLock()
		defer result.mu.RUnlock()

                // At this point result.value = nil, so accessing .Data panics
		return result.value.Data, result.value.Error
	}

Because the panic() happens in the thunk() it might be hard for developers to reason it back to a mistake in their dataloader batchFunc.

And this programming mistake is probable, when you set up your batchFunc like I did

1. First create an output slice of len(keys)
2. Query the db for records who match the keys
3. Loop over the returned db records and match them to the keys. Assign the db records to the index of the matched keys to return them in the correct order.

here I forgot to do
4. Loop over the keys, check if the corresponding results entry already has a value filled from the db, if not, fill it with a Result{Data: nil} or Result{Error: fmt.Errorf("no data")}

Forgetting step 4 triggers a panic.

[nicksrandall]

I'm inclined to agree with Tony, I don't think we can assume that nil is an error -- it could be an expected value.

[frederikhors]

I'm in the case explained by @jpastoor now.

Can we please print something in console or maybe add some docs about this?

It's very hard to debug (I'm not a pro).

[tobias-kuendig]

For anyone running into this same issue:

In your batchFn, you probably have something like this:

result := make([]*dataloader.Result[T], len(keys))

This initializes a slice of dataloader.Result for each given key. These Results are nil by default. Everywhere where you return the result from the batchFn, make sure all of the values are properly set and no longer nil.

I've had an exit condition that would only set the first item to an error, but leave the rest of the slice nil.

This triggers this problem.