Ability to inject middleware functions to manipulate requests and responses #78
Conversation
| @@ -85,22 +78,25 @@ type Client struct { | |||
| DisableRetries bool | |||
|
|
|||
| // RequestMiddlewares is a slice of functions that are called in order before a request is sent | |||
| RequestMiddlewares []RequestMiddleware | |||
| RequestMiddlewares *[]RequestMiddleware | |||
There was a problem hiding this comment.
I think this wants to become:
| RequestMiddlewares *[]RequestMiddleware | |
| RequestMiddlewares []*RequestMiddleware |
which'd mean the middleware functions themselves would be pointers?
|
|
||
| // ResponseMiddlewares is a slice of functions that are called in order before a response is parsed and returned | ||
| ResponseMiddlewares []ResponseMiddleware | ||
| ResponseMiddlewares *[]ResponseMiddleware |
| continue | ||
| } | ||
| } | ||
| status = resp.StatusCode |
There was a problem hiding this comment.
what if resp is nil here (for example a dropped connection?)?
| errText = fmt.Sprintf("OData error: %s", o.Error) | ||
| default: | ||
| defer resp.Body.Close() | ||
| respBody, err := io.ReadAll(resp.Body) |
There was a problem hiding this comment.
these two lines assume there's a body, but we're not checking that it has any length here?
| if attempts > 0 { | ||
| time.Sleep(time.Duration(backoff)) | ||
| } | ||
| c.retryableClient.CheckRetry = func(ctx context.Context, resp *http.Response, err error) (bool, error) { |
There was a problem hiding this comment.
because this function sits on the instance of the object (Client) and not (*Client) this change to retryableClient won't be retained, which seems fine but just double-checking
There was a problem hiding this comment.
This was the intent actually, as the ConsistencyFailureFunc is defined per operation / request type, e.g. a consistency error looks different for say GET /foo/1 to PUT /foo/1
| func NewClient(apiVersion ApiVersion, tenantId string) Client { | ||
| r := retryablehttp.NewClient() | ||
| return Client{ | ||
| Endpoint: environments.MsGraphGlobal.Endpoint, | ||
| ApiVersion: apiVersion, | ||
| TenantId: tenantId, | ||
| UserAgent: "Hamilton (Go-http-client/1.1)", | ||
| httpClient: &http.Client{}, | ||
| Endpoint: environments.MsGraphGlobal.Endpoint, | ||
| ApiVersion: apiVersion, | ||
| TenantId: tenantId, | ||
| UserAgent: "Hamilton (Go-http-client/1.1)", | ||
| HttpClient: r.StandardClient(), | ||
| retryableClient: r, | ||
| } | ||
| } |
There was a problem hiding this comment.
unrelated to this PR, but a though that it'd be convenient to have a NewClientWithEndpoint constructor that this calls with environments.MsGraphGlobal.Endpoint?
There was a problem hiding this comment.
Nice, I like that. It has been asked for, but I was hesitant to overly clutter the NewClient function signature
There was a problem hiding this comment.
Although actually, to be used anywhere except corner cases, this would also have to be cascaded to every resource client
| if err != nil { | ||
| return nil, status, nil, err | ||
| } | ||
| c.retryableClient.CheckRetry = func(ctx context.Context, resp *http.Response, err error) (bool, error) { |
There was a problem hiding this comment.
Just a thought - is it possible / worthwhile to export this default retry function somehow? I'm just thinking, what if I want to pass in a custom HTTP client, but I still want to use the same retry behaviour? That way maybe I could do something along these lines:
r := retryablehttp.NewClient()
r.HTTPClient = wrappedClient
r.CheckRetry = msgraph.DefaultCheckRetry
graphClient := msgraph.NewApplicationsClient(tenantID, r.StandardClient())There was a problem hiding this comment.
That would be a nice feature, but the problem is that the default retry behavior sources a ConsistencyFailureFunc directly from the input struct for each request, so it can be determined whether the failure was due to replication delay within Azure AD. If you wanted to get the generic retry behavior (e.g. 429 backoff, 5xx retries) you can just use retryablehttp as it's providing that functionality here.
msgraph.Client{}.HttpClientso callers can additionally supply their own http.Client