Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Migrated v4 signing process to streaming. #37
Conversation
axw
reviewed
Mar 12, 2015
| @@ -309,16 +311,10 @@ func canonicalHeaders(sortedHeaderNames []string, host string, hdr http.Header) | ||
| // lowercase hexadecimal string. | ||
| func payloadHash(req *http.Request, hasher hasher) (string, error) { |
axw
Mar 12, 2015
Member
Probably should add a comment to this saying that it'll eat the request body.
axw
Mar 12, 2015
Member
On second thoughts, I think a much simpler and less error prone approach would be to check in payloadHash if req.Body is an io.ReadSeeker. If it is, pass to hasher as-is and then seek back to the original pos, if it's not, use the old approach of reading into memory and then replacing req.Body.
kat-co
Mar 12, 2015
Member
As discussed in IRC, http.NewRequest wraps all body parameters in an ioutil.NopCloser, so this type assertion won't work :(
axw
reviewed
Mar 12, 2015
| @@ -201,14 +206,14 @@ func (b *Bucket) GetReader(path string) (rc io.ReadCloser, err error) { | ||
| // PutReader inserts an object into the S3 bucket by consuming data | ||
| // from r until EOF. | ||
| -func (b *Bucket) PutReader(path string, r io.Reader, length int64, contType string, perm ACL) error { | ||
| +func (b *Bucket) PutReader(path string, r io.ReadSeeker, length int64, contType string, perm ACL) error { |
axw
Mar 12, 2015
Member
Hrm. This is an API break, because someone could be relying on the function signature (e.g. taking a method value).
dimitern
Mar 12, 2015
Member
+1 also the name doesn't match if it's a ReadSeeker. How about adding separate method?
kat-co
Mar 12, 2015
Member
I cannot; the underlying object's stream must seek back to the beginning or there will be an error.
|
Looks mostly OK, however I can't honestly approve it without some benchmarking before and after this change to see what's the impact. |
kat-co commentedMar 12, 2015