Better error handling for API errors

[jacobbednarz]

This introduces a change to how we raise and interact with exceptions
raised by the API responses.

For quite some time, interacting with the API response errors has been
difficult due to them spitting out parsed JSON (as a string). This means
if you need to check the type of exception in order to recover or retry,
you need to use an unreliable strings.Contains check. This also
extends to internally when we want to show the user an error, we had a
errorFromResponse function which would return the HTTP status code and
whatever was in the errors array.

To improve the situation I'm proposing a revamp of our error handling by
exposing various methods on APIRequestError struct as helpers to get
the information clients may need to better handle errors. Here are some
examples:

Return the whole error message

fmt.Println(err.Error())

// => "HTTP status 400: Authentication error (10000)"

// with multiple errors returned

fmt.Println(err.Error())

// => "HTTP status 303: test (10000), test1 (10001)"

Return only the HTTP status

fmt.Println(err.(*cloudflare.APIRequestError).HTTPStatusCode())

// => 400

Check if the response was rate limited

fmt.Println(err.(*cloudflare.APIRequestError).ClientRateLimited())

// => false

Fetch just the internal error codes

e := APIRequestError{
  StatusCode: 123,
  Errors: []ResponseInfo{
    {
      Message: "test",
      Code:    10000,
    },
    {
      Message: "test1",
      Code:    10001,
    },
  },
}

fmt.Printf("%#v", e.InternalErrorCodes())

// => []int{10000, 10001}

// or

fmt.Println(err.(*cloudflare.APIRequestError).InternalErrorCodes())

// => []int{10000, 10001}

In order to achieve this, we'll need to update almost every method that
makes an API call to reflect the following (example from
createAccessRule in firewall.go):

@@ -218,7 +218,7 @@ func (api *API) createAccessRule(prefix string, accessRule AccessRule) (*AccessR
        uri := prefix + "/firewall/access_rules/rules"
        res, err := api.makeRequest("POST", uri, accessRule)
        if err != nil {
-               return nil, errors.Wrap(err, errMakeRequestError)
+               return nil, err
        }

        response := &AccessRuleResponse{}

Personally, I think this is a better situation for the maintainers and
consumers of this library as we'll be better equipped to handle
differing error scenarios. The Terraform provider will also benefit from
these changes for things like retries.

Recent issues related to this (more upstream in Terraform):

• Surface JSON error code in Error type #587
• Error on duplicate firewall rules - no way of checking the error code #574
• Unclear error message from using cloudflare API #502
• api.DeleteDNSRecord cause error HTTP status 401: invalid credentials #414

I haven't gone ahead and fixed any tests yet but if we agree this is
a good move, I'll take the usual steps with CI.

[jacobbednarz]

@jsha @firefart would you folks mind chucking your 2 cents in here as well whether this jives for your uses?

[patryk]

Love the idea of concatenating multiple messages while stringifying.

[jacobbednarz]

Awesome, let me add some test coverage and update the methods themselves and we can get this shipped.

[firefart]

Thanks for the quick implementation @jacobbednarz !
Left you some comments on the changes

[jacobbednarz]

@firefart for your specific use case raised in #574, I envision your code looking something like this:

resp, err := c.client.CreateAccountAccessRule(accountID, cloudflare.AccessRule{
  Mode:  "block",
  Notes: comment,
  Configuration: cloudflare.AccessRuleConfiguration{
    Target: target,
    Value:  ip,
  },
})

// BYO contains function 
if err != nil {
  if contains(err.(*APIRequestError).InternalErrorCodes(), 10009) {
    // fine to ignore
  } else {
    // better look into the error
  }
}

// ...or just access the index position directly
if err != nil {
  if err.(*APIRequestError).InternalErrorCodes()[0] == 10009 {
    // fine to ignore
  } else {
    // better look into the error
  }
}