From 0ccf258ca4db3b9801ed81dcf8ba146e26ed19d3 Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <brad@danga.com>
Date: Thu, 17 May 2018 17:56:04 -0400
Subject: [PATCH] blob: specify "oh, nevermind" error value for optional
 SubFetcher interface

To support composition, all optional interfaces in Go need a way to
say, at runtime, that "oh, nevermind, I defensively implemented this
interface but it turns out the thing I'm wrapping doesn't support it".

We learned that lesson only in the past few years, but SubFetcher
predates that.

So do that here and make proxycache implemented SubFetcher and make
everything respect the fallback oh-nevermind error value.

Fixes #1170

Change-Id: I3c0958cf6692a324e98f526571d10a352a8e18b4
---
 pkg/blob/fetcher.go                       |  7 ++++++-
 pkg/blobserver/blobpacked/subfetch.go     |  5 ++++-
 pkg/blobserver/proxycache/proxycache.go   | 17 +++++++++++++++++
 pkg/blobserver/storagetest/storagetest.go |  8 ++++++++
 4 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/pkg/blob/fetcher.go b/pkg/blob/fetcher.go
index e7971a19c..a5d4cb61d 100644
--- a/pkg/blob/fetcher.go
+++ b/pkg/blob/fetcher.go
@@ -49,6 +49,10 @@ type Fetcher interface {
 	Fetch(context.Context, Ref) (blob io.ReadCloser, size uint32, err error)
 }
 
+// ErrUnimplemented is returned by optional interfaces when their
+// wrapped values don't implemented the optional interface.
+var ErrUnimplemented = errors.New("optional method not implemented")
+
 // A SubFetcher is a Fetcher that can retrieve part of a blob.
 type SubFetcher interface {
 	// SubFetch returns part of a blob.
@@ -57,7 +61,8 @@ type SubFetcher interface {
 	// 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.
+	// the size of the blob. If the error is ErrUnimplemented, the caller should
+	// treat this Fetcher as if it doesn't implement SubFetcher.
 	SubFetch(ctx context.Context, ref Ref, offset, length int64) (io.ReadCloser, error)
 }
 
diff --git a/pkg/blobserver/blobpacked/subfetch.go b/pkg/blobserver/blobpacked/subfetch.go
index c2f949b26..af3eca4bf 100644
--- a/pkg/blobserver/blobpacked/subfetch.go
+++ b/pkg/blobserver/blobpacked/subfetch.go
@@ -47,7 +47,10 @@ func (s *storage) SubFetch(ctx context.Context, ref blob.Ref, offset, length int
 		return s.large.SubFetch(ctx, m.largeRef, int64(m.largeOff)+offset, length)
 	}
 	if sf, ok := s.small.(blob.SubFetcher); ok {
-		return sf.SubFetch(ctx, ref, offset, length)
+		rc, err := sf.SubFetch(ctx, ref, offset, length)
+		if err != blob.ErrUnimplemented {
+			return rc, err
+		}
 	}
 	rc, size, err := s.small.Fetch(ctx, ref)
 	if err != nil {
diff --git a/pkg/blobserver/proxycache/proxycache.go b/pkg/blobserver/proxycache/proxycache.go
index 89c3e221d..a12a5ed6a 100644
--- a/pkg/blobserver/proxycache/proxycache.go
+++ b/pkg/blobserver/proxycache/proxycache.go
@@ -67,6 +67,7 @@ type Storage struct {
 
 var (
 	_ blobserver.Storage = (*Storage)(nil)
+	_ blob.SubFetcher    = (*Storage)(nil)
 	// TODO:
 	// _ blobserver.Generationer = (*Storage)(nil)
 )
@@ -176,6 +177,22 @@ func (sto *Storage) Fetch(ctx context.Context, b blob.Ref) (rc io.ReadCloser, si
 	return ioutil.NopCloser(bytes.NewReader(all)), size, nil
 }
 
+func (sto *Storage) SubFetch(ctx context.Context, ref blob.Ref, offset, length int64) (io.ReadCloser, error) {
+	if sf, ok := sto.cache.(blob.SubFetcher); ok {
+		rc, err := sf.SubFetch(ctx, ref, offset, length)
+		if err == nil {
+			return rc, nil
+		}
+		if err != os.ErrNotExist && err != blob.ErrUnimplemented {
+			log.Printf("proxycache: error fetching from cache %T: %v", sto.cache, err)
+		}
+	}
+	if sf, ok := sto.origin.(blob.SubFetcher); ok {
+		return sf.SubFetch(ctx, ref, offset, length)
+	}
+	return nil, blob.ErrUnimplemented
+}
+
 type errList []error
 
 func (e errList) OrNil() error {
diff --git a/pkg/blobserver/storagetest/storagetest.go b/pkg/blobserver/storagetest/storagetest.go
index a965ea63d..61eea0b68 100644
--- a/pkg/blobserver/storagetest/storagetest.go
+++ b/pkg/blobserver/storagetest/storagetest.go
@@ -213,6 +213,10 @@ func (r *run) testSubFetcher() {
 	}
 	for _, tt := range regions {
 		r, err := sf.SubFetch(context.Background(), big.BlobRef(), tt.off, tt.limit)
+		if err == blob.ErrUnimplemented {
+			t.Logf("%T implements SubFetcher but its wrapped value doesn't", sto)
+			return
+		}
 		if err != nil {
 			t.Fatalf("Error fetching big blob for SubFetch: %v", err)
 		}
@@ -239,6 +243,10 @@ func (r *run) testSubFetcher() {
 	}
 	for _, tt := range invalids {
 		r, err := sf.SubFetch(context.Background(), big.BlobRef(), tt.off, tt.limit)
+		if err == blob.ErrUnimplemented {
+			t.Logf("%T implements SubFetcher but its wrapped value doesn't", sto)
+			return
+		}
 		if err == nil {
 			r.Close()
 			t.Errorf("No error fetching with off=%d limit=%d; wanted an error", tt.off, tt.limit)