[refactor][api] rework requireOK(doRequest(r)) usage

[acpana]

Overview

During #11158, the team called out the odd ergonomics of requireOK() / requireHTTPCodes(). In particular, for doRequest() errors, we close the request body, but not for "require"/ http code errors.

There's some ~140 usage of requireOK()/ requireHTTPCodes() calls. Depending on the approach taken, a big diff may be generated.

Links and References

#11158 (comment)

[sidzi]

Can I pick this up ?

[jkirschner-hashicorp]

Hi @sidzi,

We would welcome a PR for this enhancement - thank you for offering to contribute!

Please review the conversation starting at the comment linked above to understand the direction the maintainers think the solution should take. Let us know if you have any follow-up questions.

I strongly recommend submitting a draft PR for review in which only 1-2 of the replacements have been made so we can align on approach early (before extending it to all ~140 usages). Perhaps that draft PR can include 1 instance of the common case in which requireOk is used, and 1 instance of the uncommon case in which requireOk is not used. That should be enough to align on approach early.

Thanks again!

[sidzi]

Hello,

I have two doubts:

1. Why are we delaying the call to closeResponseBody
   in case of a successful request ? (basically, why not closing it in doRequest) ?
   Why can't we do something like this :

func (c *Client) doRequest(r *request) (time.Duration, *http.Response, error) {
	req, err := r.toHTTP()
	if err != nil {
		return 0, nil, err
	}
	start := time.Now()
	resp, err := c.config.HttpClient.Do(req)
	diff := time.Since(start)
	defer closeResponseBody(resp)
	return diff, resp, err
}

2. in api.go:1107 if we do something like this:

func requireHttpCodes(d time.Duration, resp *http.Response, e error, httpCodes ...int) (time.Duration, *http.Response, error) {
	if e != nil {
		if resp != nil {
			closeResponseBody(resp)
		}
		return d, nil, fmt.Errorf("error making request: %w", e)
	}

^ here we are trying to short-circuit the call so that we can distinguish between doRequest and
requireHttpCode ( or requireOk ) ?

In a lot of cases we are directly returning the error without any specific handling. Ex :

func (a *Agent) ServiceDeregister(serviceID string) error {
	r := a.c.newRequest("PUT", "/v1/agent/service/deregister/"+serviceID)
	_, resp, err := requireOK(a.c.doRequest(r))
	if err != nil {
		return err
	}
	closeResponseBody(resp)
	return nil
}

[kisunji]

Hi @sidzi , thank you for taking this on!

Q1: We cannot close the body within doRequest because it is the caller's responsibility to handle *http.Response that is returned. Sometimes a caller will read the body (e.g. unmarshal the json from resp.Body) then close it. Your example would immediately close resp.Body when doRequest returns, so the caller will not have access to it.

Q2: If you take a look at the comment thread here (#11158) you can see that I propose to remove that "short-circuiting" from requireHttpCodes. I think a sensible start would be to refactor the signature to take:

func requireHttpCodes(resp *http.Response, httpCodes ...int) (*http.Response, error) 

and handle errors from doRequest outside this function.

[sidzi]

Thanks for resolving the doubts. I have submitted a draft. Please have a look at this : #11287