Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
swift: improve GetReadSeeker #51
Conversation
jameinel
reviewed
Jul 5, 2017
some naming things aside (readSeeker actually being a readSeekCloser, etc.)
I think the general changes are fine.
I'm a little concerned that just dropping the ErrUnexpectedEOF may work for a particular client, but not for all.
| +func (s *LiveTests) TestReadSeekerFileChangedUnderfoot(c *gc.C) { | ||
| + object := "test_obj2" | ||
| + data := "...some data..." | ||
| + err := s.swift.PutReader(s.containerName, object, bytes.NewReader([]byte(data)), int64(len(data))) |
| @@ -111,14 +116,19 @@ func (c *Client) GetReader(containerName, objectName string) (io.ReadCloser, htt | ||
| } | ||
| // GetReadSeeker creates a read seeker closer to the specified object's data. | ||
| -func (c *Client) GetReadSeeker(containerName, objectName string) (io.ReadSeeker, http.Header, error) { | ||
| +func (c *Client) GetReadSeeker(containerName, objectName string) (ReadSeekCloser, http.Header, error) { |
jameinel
Jul 5, 2017
Owner
Given the name of the type was in the method name (GetReadSeeker), it feels a bit odd to not have them match. That said, "GetReadSeeker" was not a good name to start with.
Just 'GetReader' comes to mind to indicate it is a type you can pull data out of, rather than concrete bytes (eg, GetReader vs GetBytes or GetString)
rogpeppe
Jul 5, 2017
Member
In a subsequent PR, I'm planning to remove GetReadSeeker (it's new and only used in one place) and add OpenObject that returns a ReadSeekCloser, and make GetReader use that.
Alternatively, I could change the signature of GetReader but I'd prefer not to break backward compatibility for a method that's been around for a long time.
(there is already a GetReader method)
| requestData := goosehttp.RequestData{} | ||
| err := c.touchObject(&requestData, client.HEAD, containerName, objectName) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| - o := &oreadseeker{c, containerName, objectName, requestData.RespLength, 0} | ||
| - return o, requestData.RespHeaders, err | ||
| + return &readSeeker{ |
jameinel
Jul 5, 2017
Owner
Having a 'readSeeker' actually be a ReadSeekCloser is also likely to add to confusion.
swiftFile perhaps?
rogpeppe
Jul 5, 2017
Member
sure. done-ish (named it "object" as swift has objects, not files, and the "swift" seems redundant given that we're in the swift package).
| + end := o.pos + int64(len(p)) | ||
| + if end > o.length { | ||
| + p = p[:o.length-o.pos] | ||
| + end = o.pos + int64(len(p)) |
jameinel
Jul 5, 2017
Owner
if len(p) is defined as o.length-o.pos
then o.pos + o.length - o.pos => o.length
Can you just do:
end = o.length
and have it be clearer?
| RespReader: &emptyReadCloser, | ||
| ReqHeaders: header, | ||
| - ExpectedStatus: []int{http.StatusOK, http.StatusPartialContent}, | ||
| + ExpectedStatus: acceptableReadStatuses, |
jameinel
Jul 5, 2017
Owner
Calling the value "acceptableReadStatuses" feels a bit off. Something like "supportedReadStatuses" might be clearer that we know we can handle X and Y.
I don't fully understand why it gets pulled into a package variable, given it seems very tied to exactly what this function can actually handle.
rogpeppe
Jul 5, 2017
Member
I pulled it into a package variable to avoid allocating a new slice every time.
I don't see too much difference in the names, so shrug done.
| + // Since we should be ensuring that the data never changes | ||
| + // underfoot because we're setting the If-Match header, | ||
| + // then the length should always be the same as when we | ||
| + // first opened it, so any short read justifies an ErrUnexpectedEOF. |
jameinel
Jul 5, 2017
Owner
You're not unconditionally setting If-Match, you're only doing so if you have an Etag. Are you sure you'll always have an etag?
rogpeppe
Jul 5, 2017
Member
We should always have an Etag, yes, but I think an ErrUnexpectedEOF in the unlikely event that we don't and the content changes underfoot seems OK to me.
| - } else { | ||
| - w.Write([]byte(objdata)) | ||
| - } | ||
| + // Note: even though the real Swift service does not |
|
$$merge$$ |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-goose |
rogpeppe commentedJul 4, 2017
We change the return value so that it returns
a ReadSeekCloser, opening the way for
having state inside the returned reader.
We also change it to ensure that the read
fails if the content changes underfoot,
so we can rely on the length remaining the same.
We also tidy up the live tests a little and fix a failure
(they now pass), and change the Swift test server
so that it deals with etags and If-Match headers
correctly by using http.ServeContent.