OnError handler

[nicjohnson145]

returning an error if the validation fails is usually fine, but in some cases (like BadRequest) being able do some extra processing (like log.Error-ing the response body) would be quite handy. Currently in the returned error you lose access to the response body. Propose adding a OnError handler such that you could do something like

err := requests.
        URL("api.my-site.com").
        OnError(func(origErr error, resp *http.Response) error {
            data, err := io.ReadAll(resp.Body)
            if err != nil {
                return fmt.Errorf("error reading body: %w", err)
            }
            log.Errorf("error during request: %v", data)
            return origErr
        })

I understand this is possible by removing the default validator, and adding a handler that calls the default validator and then does this logging on error, and then returns the default error once logged, similar to:

err := requests.
        URL("api.my-site.com").
        AddValidator(nil).
        Handle(func(resp *http.Response) error {
            err := requests.DefaultValidator(resp)
            if err != nil {
                data, readErr := io.ReadAll(resp.Body)
                if readErr != nil {
                    return fmt.Errorf("error reading body: %w", readErr)
                }
                log.Errorf("error during request: %v", data)
                return err
            }
            <do some other processing, maybe using requests.ToJSON or something>
            return nil
        })

but it just seems like more boilerplate-y. 🤷

This is sort of a more general handling of #32, and would be useful in cases where the object returned by the API varies based on the status code

I'd be happy to take a stab at a PR if this is something you'd be interested in.

[earthboundkid]

That’s a really interesting idea. I like having a more general way to handle error JSON than #32. It would let you choose whether to return nil error or not, for example.

Let’s make an experimental branch and play with it for a while.

[earthboundkid]

Still thinking about how to do this. Current thoughts:

For logging, you really do want to be able to modify the error returned to add context, and you may also want to translate all errors into an app specific error domain. So the return an error design was good, but that screws up further error handlers in the chain.

One answer is to drop the chaining and just have a single error handler. I don't like that because it means you have to choose between logging and special handling validation. Even just one error handler per type feels like a weird, artificial limitation.

So maybe the better answer to change the error handler signature to OnError(f func(kind ErrorKind, cause error, req *http.Request, resp *http.Response) error)). That's a long signature and the ErrorKind is semi-redundant, but it solves the chaining problem.

[earthboundkid]

I am pretty happy with the current design. The only question I have left is whether it makes sense to pass a pointer to the Builder to the error handler. My instinct is no because you can’t actually extract much information from a Builder, but OTOH, it does seem like an important bit of context.

Maybe the answer is to have a requests.ErrorParams struct and I can add new fields to it later if I need to.

[earthboundkid]

The current version of the PR has requests.OnErrorParams which turns out to be handy because you can add methods to it, like URL() which always produces a URL, even if the actual request was nil.

[earthboundkid]

My remaining question about the current API is whether I should expand it out to some kind of On-whatever hook that will run after good requests too. My gut feeling is no because that's what transports are for. But it's not trivial to make a logging transport that measures the time of a request, so maybe it would be good to have a post-hook.

[earthboundkid]

Shower thoughts: the error handling API still kind of bugs me because it adds a lot of surface area to the code. I was thinking “how could you tell which status code triggered the error handler” and it seemed like the answer is that ErrorJSON should just be its own validator, and then it knows the status already. So maybe that’s the answer, just rip out the new code and add a validator instead.

[earthboundkid]

Okay, #63 is merged. Thanks for all the feedback on this.