lfsapi: teach (*Client) Do() how to handle 307 redirections#1791
lfsapi: teach (*Client) Do() how to handle 307 redirections#1791technoweenie merged 3 commits intoapi-masterfrom
(*Client) Do() how to handle 307 redirections#1791Conversation
ttaylorr
left a comment
There was a problem hiding this comment.
Looks good, just a few comments to consider 😄
| } | ||
|
|
||
| func (c *Client) doWithRedirects(cli *http.Client, req *http.Request, via []*http.Request) (*http.Response, error) { | ||
| if seeker, ok := req.Body.(io.Seeker); ok { |
There was a problem hiding this comment.
Should this be an error if len(via) > 0 && !ok?
There was a problem hiding this comment.
Possibly, but not in this PR. My experiments with re-introducing the http trace code gave me a better way to deal with it.
| return res, err | ||
| } | ||
|
|
||
| if res.StatusCode != 307 { |
There was a problem hiding this comment.
One of our roadmap items is to respond to 301s as well. Do you think we could add an || res.StatusCode == 301, and maintain a cache of permanent redirections? I'm thinking something like a map[*url.URL]*url.URL map of from -> to.
There was a problem hiding this comment.
Yes, but not as part of this refactoring effort.
lfsapi/client.go
Outdated
| redirectedReq.ContentLength = req.ContentLength | ||
|
|
||
| if err = checkRedirect(redirectedReq, via); err != nil { | ||
| return res, errors.Wrapf(err, err.Error()) |
There was a problem hiding this comment.
What does errors.Wrapf(err, err.Error()) do? I think this would just turn an error "foo" into "foo: foo".
There was a problem hiding this comment.
Yeah, I think this is a relic of the older error wrapping, which was totally different.
lfsapi/client.go
Outdated
| } | ||
|
|
||
| func checkRedirect(req *http.Request, via []*http.Request) error { | ||
| if len(via) >= 3 { |
There was a problem hiding this comment.
Do you think we should make this a configurable value?
There was a problem hiding this comment.
Perhaps, but not in this refactoring PR.
lfsapi/client.go
Outdated
| return errors.New("stopped after 3 redirects") | ||
| } | ||
|
|
||
| oldest := via[0] |
There was a problem hiding this comment.
I think this line will cause a panic if len(via) == 0.
lfsapi/client.go
Outdated
| continue | ||
| } | ||
| } | ||
| req.Header.Set(key, oldest.Header.Get(key)) |
There was a problem hiding this comment.
I'm not sure this behavior fits in a check<T>() function. This seems side-effect-y, so I think we should either document it's behavior, or extract it out to another function.
There was a problem hiding this comment.
That's fair. Need more tests first.
| Test string | ||
| } | ||
|
|
||
| func TestClientRedirect(t *testing.T) { |
ttaylorr
left a comment
There was a problem hiding this comment.
Sorry, I think I misunderstood the intent of this pull-request. From your comments, I think this is just moving some code over from the httputil package, so I think this is fine to go ahead with for now.
|
I did clean up the redundant error wrapping, and removed |
Thanks so much! ✨ |
No description provided.