Discussion: How to best report errors for the mailgun api?

[sam-falvo]

@mbanzon ,

I've run into situations where a request against mailgun API would fail, but errors are not propegated back to the caller of the API, at least through the err return. I've confirmed that simplehttp is not, seemingly by design, addressing 4xx or 5xx responses as errors.

Should we be inserting an abstraction layer between mailgun-go and simplehttp that does return code validation? I'm of the opinion that this is a good idea, as it'd simplify the interface from the caller's perspective. Thoughts?

[mbanzon]

The 4xx and 5xx responses from the Mailgun API - do they return valid JSON?

The .MakeRequest method returns a struct that contain the response code - but that seems to be insufficient?

If you can give just a small example (pseudo) on how the client code (mailgun-go) would like to use the simplehttp-package I am sure the changes needed would be small, quick and very appropriate.

[sam-falvo]

The 4xx and 5xx responses from the Mailgun API - do they return valid JSON?

Not always, but frequently, yes. Mailgun relies on a proxy front-end to the API, and sometimes the proxy will respond with a 404 on its own. It will return HTML instead of JSON. I'll ask to see how difficult it is to change that, but we shouldn't depend on errors giving meaningful responses in JSON.

The .MakeRequest method returns a struct that contain the response code - but that seems to be insufficient?

I think this is reasonable behavior from SimpleHTTP's perspective, and I think we should not change its behavior. It's just doing its job, and no network errors occurred from what it could see. However, a 404 or something from the perspective of a caller into mailgun-go would expect that to be treated as a real error.

So, the example I can give isn't relevant to simplehttp, but just to mailgun-go itself:

_, err := mg.CreateRoute(mailgun.Route{
    Priority: 1,
    Description: "Sample Route",
    Expression: "match_recipient(\".*@samples.mailgun.org\")",
    Actions: []string{
        "forward(\"http://example.com/messages/\")",
        "stop()",
    },
})
if err != nil {
    t.Fatal(err)
}

In this case, I'm creating a new route, but let's suppose I forget to pass in an Expression field. This is required for the create operation to work. If this field isn't present, the API will return a 4xx error of some flavor (I think 400 Bad Request, but I forget the details).

Again, from simplehttp's POV, that works fine -- it's making a request, and it's returning a valid response. But from Mailgun's POV, this is a genuine error.

As it stands now, if I make the above mistake, the err will always be nil, and no error can be caught (especially since no other means of returning the response code exists).

What I'm proposing is adding a small shim between mailgun-go and simplehttp that performs error detection, and populates an appropriate error object if necessary. I wanted to engage you in this because it changes the semantics of the interface somewhat, which may or may not be a breaking change from your existing library.

[mbanzon]

I have made a new branch (simplehttp-return-codes) on mailgun-go and a new branch (multiply-returns-values) on simplehttp. Would somthing like this work for you?

It allows the HTTP response code to be emitted to mailgun-go and handled there for proper error creation - but is that still too simple (with regards to mailgun-go)?

I am also toying with the idea of having a "Strict" value set to false in the request object in simplehttp to allow non 2xx response codes to return the current way and set to true to have it fail with a proper error on non 2xx codes - this will take some more work as it will break a lot of code.

[sam-falvo]

I have made a new branch (simplehttp-return-codes) on mailgun-go and a new branch (multiply-returns-values) on simplehttp. Would somthing like this work for you?

Actually, I don't think there's a need for this. The interface to simplehttp seems adequate. What I'm proposing is something like this inside of mailgun-go:

type UnexpectedResponseCodeError struct {
    Expected []int
    Got      int
    Message  []byte
}

func (e *UnexpectedResponseCodeError) Error() {
    return e.String()
}

func (e *UnexpectedResponseCodeError) String() {
    return fmt.Sprintf("Unexpected response code (%d) not in set %#v", e.got, e.expected)
}

func not_in(needle int, haystack []int) bool {
    for _, candidate := range haystack {
        if needle == candidate {
            return false
        }
    }
    return true
}

func PostResponseFromJSON(r *simplehttp.Request, p *simplehttp.Payload, v interface{}, okCodes []int) error {
    response, err := r.MakePostRequest(p)
    if err != nil {
        return err
    }
    if not_in(response.Code, okCodes) {
        return &UnexpectedResponseCodeError{
            Expected: okCodes,
            Got: response.Code,
            Message: response.Data,
        }
    }
    return response.ParseFromJSON(v)
}

I think that would be a lot easier.

[mbanzon]

Yes.

I think it would work out better. It won't really brake the other code I work with and it is easy to follow and understand.

[sam-falvo]

Awesom, code breakage was my concern! :) Thanks for the feedback; I'll start working on this shortly.