blob SubFetcher: explicitely states with errors the testSubFetcher constraints

testSubFetcher in blobserver/storagetest was already checking that we'd
get specific error messages in the case of negative input parameters or
an out of range offset.

This change rationalizes these constraints with named errors
(ErrNegativeSubFetch and ErrOutOfRangeOffsetSubFetch) specified
in the SubFetcher interface.

It also fixes the googlestorage and s3 implementations so that they pass
the aforementioned test.

Change-Id: I25b72b842855b90ee3cab44c90654581dccf4b8e

pkg/blob/fetcher.go

@@ -27,6 +27,11 @@ import (
 	"camlistore.org/pkg/types"
 )
 
+var (
+	ErrNegativeSubFetch         = errors.New("invalid negative subfetch parameters")
+	ErrOutOfRangeOffsetSubFetch = errors.New("subfetch offset greater than blob size")
+)
+
 // Fetcher is the minimal interface for retrieving a blob from storage.
 // The full storage interface is blobserver.Storage.
 type Fetcher interface {
@@ -47,8 +52,10 @@ type SubFetcher interface {
 	// SubFetch returns part of a blob.
 	// The caller must close the returned io.ReadCloser.
 	// The Reader may return fewer than 'length' bytes. Callers should
-	// check. The returned error should be os.ErrNotExist if the blob
-	// doesn't exist.
+	// check. The returned error should be: ErrNegativeSubFetch if any of
+	// offset or length is negative, or os.ErrNotExist if the blob
+	// doesn't exist, or ErrOutOfRangeOffsetSubFetch if offset goes over
+	// the size of the blob.
 	SubFetch(ref Ref, offset, length int64) (io.ReadCloser, error)
 }
 

pkg/blobserver/blobpacked/subfetch.go

@@ -17,7 +17,6 @@ limitations under the License.
 package blobpacked
 
 import (
-	"errors"
 	"io"
 	"io/ioutil"
 
@@ -71,10 +70,10 @@ func (s *storage) SubFetch(ref blob.Ref, offset, length int64) (io.ReadCloser, e
 
 func capOffsetLength(size uint32, offset, length int64) (newLength int64, err error) {
 	if offset < 0 || length < 0 {
-		return 0, errors.New("invalid negative subfetch parameters")
+		return 0, blob.ErrNegativeSubFetch
 	}
 	if offset > int64(size) {
-		return 0, errors.New("subfetch offset greater than blob size")
+		return 0, blob.ErrOutOfRangeOffsetSubFetch
 	}
 	if over := (offset + length) - int64(size); over > 0 {
 		length -= over

pkg/blobserver/interface.go

@@ -31,10 +31,12 @@ import (
 // MaxBlobSize is the size of a single blob in Camlistore.
 const MaxBlobSize = constants.MaxBlobSize
 
-var ErrCorruptBlob = errors.New("corrupt blob; digest doesn't match")
+var (
+	ErrCorruptBlob = errors.New("corrupt blob; digest doesn't match")
 
-// ErrNotImplemented should be returned in methods where the function is not implemented
-var ErrNotImplemented = errors.New("not implemented")
+	// ErrNotImplemented should be returned in methods where the function is not implemented
+	ErrNotImplemented = errors.New("not implemented")
+)
 
 // BlobReceiver is the interface for receiving
 type BlobReceiver interface {

pkg/blobserver/localdisk/localdisk.go

@@ -31,7 +31,6 @@ Example low-level config:
 package localdisk
 
 import (
-	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -131,7 +130,7 @@ func (ds *DiskStorage) Fetch(br blob.Ref) (io.ReadCloser, uint32, error) {
 
 func (ds *DiskStorage) SubFetch(br blob.Ref, offset, length int64) (io.ReadCloser, error) {
 	if offset < 0 || length < 0 {
-		return nil, errors.New("invalid offset or length")
+		return nil, blob.ErrNegativeSubFetch
 	}
 	rc, _, err := ds.fetch(br, offset, length)
 	return rc, err
@@ -158,7 +157,10 @@ func (ds *DiskStorage) fetch(br blob.Ref, offset, length int64) (rc io.ReadClose
 	}
 	// SubFetch:
 	if offset < 0 || offset > stat.Size() {
-		return nil, 0, errors.New("invalid offset")
+		if offset < 0 {
+			return nil, 0, blob.ErrNegativeSubFetch
+		}
+		return nil, 0, blob.ErrOutOfRangeOffsetSubFetch
 	}
 	return struct {
 		io.Reader

pkg/blobserver/memory/mem.go

@@ -20,7 +20,6 @@ package memory
 
 import (
 	"bytes"
-	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -109,7 +108,7 @@ func (s *Storage) Fetch(ref blob.Ref) (file io.ReadCloser, size uint32, err erro
 
 func (s *Storage) SubFetch(ref blob.Ref, offset, length int64) (io.ReadCloser, error) {
 	if offset < 0 || length < 0 {
-		return nil, errors.New("invalid negative subfetch parameters")
+		return nil, blob.ErrNegativeSubFetch
 	}
 	s.mu.RLock()
 	defer s.mu.RUnlock()
@@ -118,7 +117,7 @@ func (s *Storage) SubFetch(ref blob.Ref, offset, length int64) (io.ReadCloser, e
 		return nil, os.ErrNotExist
 	}
 	if offset > int64(len(b)) {
-		return nil, errors.New("subfetch offset greater than blob size")
+		return nil, blob.ErrOutOfRangeOffsetSubFetch
 	}
 	atomic.AddInt64(&s.blobsFetched, 1)
 	atomic.AddInt64(&s.bytesFetched, length)

pkg/blobserver/storagetest/storagetest.go

@@ -233,10 +233,10 @@ func (r *run) testSubFetcher() {
 		r, err := sf.SubFetch(big.BlobRef(), tt.off, tt.limit)
 		if err == nil {
 			r.Close()
-			t.Errorf("No error fetching with off=%d limit=%d; wanted and error", tt.off, tt.limit)
+			t.Errorf("No error fetching with off=%d limit=%d; wanted an error", tt.off, tt.limit)
 			continue
 		}
-		if v := err.Error(); !strings.Contains(v, "negative") && !strings.Contains(v, "offset") {
+		if err != blob.ErrNegativeSubFetch && err != blob.ErrOutOfRangeOffsetSubFetch {
 			t.Errorf("Unexpected error fetching with off=%d limit=%d: %v", tt.off, tt.limit, err)
 		}
 	}

pkg/googlestorage/googlestorage.go

@@ -33,10 +33,11 @@ import (
 	"strings"
 	"unicode/utf8"
 
+	"camlistore.org/pkg/blob"
 	"camlistore.org/pkg/httputil"
 	"camlistore.org/third_party/code.google.com/p/goauth2/oauth"
-	api "camlistore.org/third_party/google.golang.org/api/storage/v1"
 	"camlistore.org/third_party/github.com/bradfitz/gce"
+	api "camlistore.org/third_party/google.golang.org/api/storage/v1"
 )
 
 const (
@@ -186,8 +187,8 @@ func (c *Client) GetObject(obj *Object) (rc io.ReadCloser, size int64, err error
 // If length is negative, the rest of the object is returned.
 // The caller must close rc.
 func (c *Client) GetPartialObject(obj Object, offset, length int64) (rc io.ReadCloser, err error) {
-	if offset < 0 {
-		return nil, errors.New("invalid negative length")
+	if offset < 0 || length < 0 {
+		return nil, blob.ErrNegativeSubFetch
 	}
 	if err = obj.valid(); err != nil {
 		return
@@ -214,6 +215,9 @@ func (c *Client) GetPartialObject(obj Object, offset, length int64) (rc io.ReadC
 	}
 	if !(resp.StatusCode == http.StatusPartialContent || (offset == 0 && resp.StatusCode == http.StatusOK)) {
 		resp.Body.Close()
+		if resp.StatusCode == http.StatusRequestedRangeNotSatisfiable {
+			return nil, blob.ErrOutOfRangeOffsetSubFetch
+		}
 		return nil, fmt.Errorf("GS GET request failed status: %v\n", resp.Status)
 	}
 

pkg/misc/amazon/s3/client.go

@@ -36,6 +36,7 @@ import (
 	"strings"
 	"time"
 
+	"camlistore.org/pkg/blob"
 	"camlistore.org/pkg/httputil"
 )
 
@@ -324,6 +325,9 @@ func (c *Client) GetPartial(bucket, key string, offset, length int64) (rc io.Rea
 	case http.StatusNotFound:
 		res.Body.Close()
 		return nil, os.ErrNotExist
+	case http.StatusRequestedRangeNotSatisfiable:
+		res.Body.Close()
+		return nil, blob.ErrOutOfRangeOffsetSubFetch
 	default:
 		res.Body.Close()
 		return nil, fmt.Errorf("Amazon HTTP error on GET: %d", res.StatusCode)