Skip to content
This repository has been archived by the owner on Sep 22, 2021. It is now read-only.

Fix logging crash. #1

Merged
merged 1 commit into from
Sep 9, 2020
Merged

Fix logging crash. #1

merged 1 commit into from
Sep 9, 2020

Conversation

jimsynz
Copy link

@jimsynz jimsynz commented Sep 8, 2020

When Stripe.request/2 returns an error tuple containing a value which does not implement String.Chars we'd get an exception because we were trying to interpolate it straight into the log message.

According to dialyzer the error term is always a struct:

  @spec do_call(:delete | :get | :post, any, any, keyword) ::
          {:error,
           %{
             :__exception__ => true,
             :__struct__ =>
               Stripe.APIError
               | Stripe.AuthenticationError
               | Stripe.CardError
               | Stripe.InvalidRequestError
               | Stripe.RateLimitError,
             :message => any,
             :type => <<_::64, _::_*8>>,
             optional(:code) => any,
             optional(:param) => any
           }}
          | {:ok, any}
          | Stripe.APIConnectionError.t()

I note that we also don't handle the Stripe.APIConnectionError type. Should I fix that too I wonder?

This was obscuring a hackney crash and made it that little bit harder to
track down.
Copy link

@d-staehler d-staehler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@steffanlevet
Copy link

@jimsynz I think that APIConnectionError is used for things like the stipe key being wrong???? maybe that we can't establish with Stripe???
Would be good to handle it in any case

@jimsynz
Copy link
Author

jimsynz commented Sep 8, 2020

@steffanlevet agreed. Will update.

@jimsynz
Copy link
Author

jimsynz commented Sep 8, 2020

@steffanlevet no, my bad. The APIConnectionError.t is there because I was looking at the typings for do_call/4 and not request/2. Request always returns a Tesla.Env.result, which types as {:ok, t()} | {:error, any()} meaning that my PR is fine as is.

@steffanlevet
Copy link

@jimsynz happy for you to merge this when you want

@jimsynz jimsynz merged commit 8745041 into master Sep 9, 2020
@jimsynz jimsynz deleted the issue_logging_crash branch September 9, 2020 21:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants