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

Handle timeout and other possible HTTPoison errors #37

Closed
vladimir-e opened this issue May 29, 2018 · 1 comment
Closed

Handle timeout and other possible HTTPoison errors #37

vladimir-e opened this issue May 29, 2018 · 1 comment

Comments

@vladimir-e
Copy link
Contributor

vladimir-e commented May 29, 2018

Hello. First of all thanks a lot for making this lib!

I faced a problem while using the lib and would like to discuss a solution, so I can submit a PR fixing it.

During long-running process of importing products via Shopify API, we occasionally get the following error:

Shopify.Product.create(session, payload)

%FunctionClauseError{args: nil, arity: 2, clauses: nil, function: :handle_response, kind: nil, module: Shopify.Adapters.HTTP}

It happens here:

https://github.com/nsweeting/shopify/blob/master/lib/shopify/adapters/http.ex

defmodule Shopify.Adapters.HTTP do
  # ...
  def handle_response({:ok, %HTTPoison.Response{status_code: code, body: body}}, resource) do
    Shopify.Response.new(code, body, resource)
  end

I believe we could add another clause for handle_response catching everything else and return something like:

  def handle_response(response, resource) do
    {:error, response, resource}
  end

Maybe it would be worthwhile creating Shopify.Error to "standardize" the error format.

Please let me know what you think? If I at least start on adding a clause for handle_response figuring out the details of implementation as I go, would you accept such PR?

Currently I use this workaround:

    try do
      result = Product.create(session, payload)

    rescue
      # Catch unhandled timeout error in Shopify lib
      e in FunctionClauseError ->
        {:error, record, e}
    end
@Ninigi
Copy link
Collaborator

Ninigi commented May 30, 2018

First of all, thanks for the work you already put into thinking about this issue, really appreciated!

You are absolutely right, there are unhandled errors that will sometimes break an app with a pretty unclear error message... I would love to put some work into that and handle known errors more gracefully.

Right now, according to the documentation, the resource methods are supposed to return {:ok, %Shopify.Response{}} or {:error, %Shopify.Response{}}, I would like to stick to this format of a 2 element tuple and either :ok or :error, but we can introduce a %Shopify.Error{} struct.

Regarding the implementation:
I think returning {:error, %Shopify.Error{}} would bubble up, so guess your approach would work 👍
We would have to put some thought into the error fields, since we might want to use it for the errors returned by the shopify api as well, for that we would need a data and a code field to make it backwards compatible.

If you want to take a shot at it, I will be more than happy to review and merge 🔥

EDIT: @vladimir-e just to make sure you get notified

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants