Questions/ideas

[peaceful-james]

I am confused about the decision to always return a tuple with first element :error from defp handle_response({:ok, %Response{status_code: scode, body: body}}).

This is here: https://github.com/gvaughn/quest/blob/7f6dcaae9b4ef7cb5f2b7ed08c078801e405cc63/lib/quest/httpoison_dispatcher.ex#L43

It seems strange to always return an :error tuple. What is the reasoning behind it?

For the time being, I am using this modified version of the function (which seems simple and more fitting for every use case I have ):

# other http response code
 defp handle_response({:ok, %Response{status_code: scode, body: body}}) do
    case Jason.decode(body) do
      {:ok, json} ->
        {:ok, {scode, json}}

      # return unprocessed body
      _ ->
        {:error, {scode, body}}
    end
  end

[peaceful-james]

OK I have been playing with this even more and now I understand - we want an :error tuple when the response is not 200 or 204.

I have modified this code for my own purposes in two ways:

1. I have change handle_response to allow for more configurable "success_status_codes":

  @success_status_codes [200, 201]

# ...

  defp handle_response({:ok, %Response{body: body, status_code: scode} = resp}) when scode in @success_status_codes do
    case Jason.decode(body) do
      {:ok, resp} -> {:ok, {scode, resp}}
      {:error, _} -> {:error, {:json, {scode, resp}}}
    end
  end

#...

  # other http response code
  defp handle_response({:ok, %Response{status_code: scode, body: body}}) do
    case Jason.decode(body) do
      {:ok, json} ->
        {:error, {scode, json}}

      # return unprocessed body
      _ ->
        {:error, {:json, {scode, body}}}
    end
  end

2. I have changed my "service" module to auto-mock in test environment!

defp maybe_mock_dispatcher do
    if Application.get_env(:doorframe, :env) == :test and is_nil(System.get_env("TEST_WITH_REAL_API")) do
      [dispatcher: mocked_dispatcher]
    else
      []
    end
end

  def client(client_opts \\ []) do
    maybe_mock_dispatcher()
    |> Keyword.merge(client_opts)
    |> Enum.into(@default_q)
  end

This lets me do, .e.g user interaction testing without worrying about injecting mocks somehow. I can easily test with real API by either setting the env var TEST_WITH_REAL_API or by passing client_opts with [dispatcher: Quest.HTTPoisonDispatcher].

I will post more here as I keep altering this great little "lib" (I know it's not really a lib yet).

[gvaughn]

Hi @peaceful-james ! Thanks for your interest. I'm sorry for my delay replying, but I'm glad to see you figured things out.

Yes, the idea is if it's not a 20x status, then to our application logic it's an error, even though HTTPoison said it's :ok because it received a response from the server.

You're doing the right thing by customizing it for your needs. This is not exactly the code I run in production. I extracted this from there and improved on some naming that is not worth refactoring in my system.

There is one thing that I had considered initially, but talked myself out of, but in the past week I wish I'd really done it -- to have handle_response include the original Quest struct as a 3rd element of the return tuple. There's some cases I've been debugging due to a 3rd party API acting in undocumented ways that I really wanted the exact struct we sent plus the result in one place to log. That would be difficult for me to add given the size of my existing codebase, but if you're just starting out, it's something to consider.

That auto-mocking idea is interesting. Due to my "organically grown" codebase, it's not appropriate for my case, but if I were starting fresh I might consider something like that. I also have some vague notions of using the process dictionary like Mox, but until I delve into that, I'm not sure if the advantages are worth the complexity.

[peaceful-james]

Thanks for the reply @gvaughn and for the insight. I get the idea that if it's not 20x it should return :error but actually it returns error for 201 in its current state.

Feel free to close this issue. I just wrote my impressions for the sake of posterity.

[gvaughn]

You're absolutely right. The logic should look for all 20x statuses. It hasn't been a problem for me in practice because I find the other statuses to be rarely used, but for completeness, it's a good idea.

I'll leave the issue open for now to see if you have more questions or insight to share.

[peaceful-james]

The Stripe API requires x-www-form-urlencoded requests but sends back json responses.

Knowing this, the :encoding field should be changed to :request_encoding and :response_encoding. I have done this in projects that use the quest pattern and hit Stripe's API.

@gvaughn You can close this issue if you want, I'm just using it to record my ideas.

[gvaughn]

Yes. In my original project the encoding field was the request encoding. The response encoding should be detectable based on headers, but I never wrote that. It is a good idea to have a separate response encoding field in case the header is missing or wrong.

I don't mind having the issue open. It's a great discussion for anyone else who wants to adopt this pattern.

[peaceful-james]

should be detectable based on headers

In an ideal world! :)