Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Oct 31, 2022

Conversation

altuntasfatih
Copy link
Contributor

@altuntasfatih altuntasfatih commented Sep 26, 2022

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,

  • I have replaced the then/2 function with an anonymous function to fix the pipeline
  • I have replaced the use Mix.Config statement with import Config to remove the deprecated warning.

@altuntasfatih
Copy link
Contributor Author

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

config/config.exs Outdated Show resolved Hide resolved
@altuntasfatih altuntasfatih requested review from benwilson512 and kdawgwilk and removed request for kdawgwilk and benwilson512 September 30, 2022 07:05
test/dataloader_test.exs Outdated Show resolved Hide resolved
@altuntasfatih altuntasfatih requested review from benwilson512 and kdawgwilk and removed request for kdawgwilk and benwilson512 October 4, 2022 08:10
@altuntasfatih altuntasfatih requested review from benwilson512 and kdawgwilk and removed request for kdawgwilk and benwilson512 October 7, 2022 11:23
@altuntasfatih
Copy link
Contributor Author

altuntasfatih commented Oct 7, 2022

Could you approve again to run workflows? benwilson512

@altuntasfatih altuntasfatih requested review from benwilson512 and mbuhot and removed request for kdawgwilk, benwilson512 and mbuhot October 10, 2022 09:31
@altuntasfatih
Copy link
Contributor Author

altuntasfatih commented Oct 11, 2022

@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
Copy link
Contributor Author

altuntasfatih commented Oct 11, 2022

I have updated the pipeline as well. @benwilson512

@altuntasfatih altuntasfatih requested review from kdawgwilk and benwilson512 and removed request for benwilson512 and kdawgwilk October 14, 2022 11:08
test/dataloader_test.exs Outdated Show resolved Hide resolved
test/dataloader_test.exs Outdated Show resolved Hide resolved
test/dataloader_test.exs Outdated Show resolved Hide resolved
@altuntasfatih
Copy link
Contributor Author

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

@benwilson512
Copy link
Contributor

@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.

@benwilson512 benwilson512 merged commit 5c47f24 into absinthe-graphql:master Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants