absinthe-graphql / dataloader

DataLoader for Elixir
MIT License
489 stars 99 forks source link

Fix double wrapping({:ok,{:ok, value}}) problem on tuple policy with source KV #145

Closed altuntasfatih closed 2 years ago

altuntasfatih commented 2 years ago

Heys, We used the tuple policy to properly return the data to the caller for fine-gradient error handling. However, we faced this issue on the KV source. It wraps data by the ok tuple again without controlling whether the data is the ok tuple or not. So it wraps, then data becomes{: ok,{: ok, value}}.

I have added the following line code in two asterisks to the KV fetch function. It does not wrap again if the data is an ok tuple.

 case Map.fetch(batch, id) do
          :error -> {:error, "Unable to find id #{inspect(id)}"}
          {:ok, {:error, reason}} -> {:error, reason}
          ** {:ok, {:ok, item}} -> {:ok, item} **
          {:ok, item} -> {:ok, item}
        end

Actually, The code checks the error tuple but not the ok tuple case. I guess that was forgotten somehow.

In addition,

altuntasfatih commented 2 years ago

Could you review it? @akoutmos, @benwilson512, @jadlr, @mbuhot

mbuhot commented 2 years ago

It would be helpful to update the moduledoc: https://github.com/absinthe-graphql/dataloader/blob/master/lib/dataloader/kv.ex#L5-L7 to clarify the contract for the load_function.

Something along the lines of:

You must supply a function that accepts a batch key and list of ids, and returns a map of values keyed by id. Values may optionally be returned as :ok / :error tuples to indicate success of the operation.

altuntasfatih commented 2 years ago

Could you approve again to run workflows? benwilson512

altuntasfatih commented 2 years ago

@benwilson512, there is a conflict between .tool-versions and pipeline. The pipeline works on Elixir-1.10-11&OTP-22-23, whereas the .tool-versions works on Elixir-1.12-13&OTP-24. This causes problems. We should upgrade the pipeline.

For instance, the mix format works on both versions differently. that is the reason the pipeline says the files are not formatted.

At local, OTP24-Elixir1.13 says the format is okay, but Otp 23-elixir.1.11 says it is not formatted.

altuntasfatih commented 2 years ago

I have updated the pipeline as well. @benwilson512

altuntasfatih commented 2 years ago

I have amended it. Could you check again @benwilson512 ?

benwilson512 commented 2 years ago

@altuntasfatih the primary blocker here is that this is technically a breaking change. I'm not clear whether this was a bug or just bad design, but if anyone has code out there that relies on this it will break. We are already kicking off a 2.0 version now so I think we can include that with this.