8784: Use arvados.Collection in cache.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curoverse.com>

diff --git a/services/keep-web/cache.go b/services/keep-web/cache.go
index ab7c65310b0abbf6b9ba1c5eb2b8a22142ab8347..2dfb542cf1d7033c068249a6b18be062ba086f0c 100644 (file)
--- a/services/keep-web/cache.go
+++ b/services/keep-web/cache.go
@@ -1,7 +1,6 @@
 package main
 
 import (
-       "fmt"
        "sync"
        "sync/atomic"
        "time"
@@ -42,7 +41,7 @@ type cachedPDH struct {
 
 type cachedCollection struct {
        expire     time.Time
-       collection map[string]interface{}
+       collection *arvados.Collection
 }
 
 type cachedPermission struct {
@@ -82,7 +81,7 @@ func (c *cache) Stats() cacheStats {
        }
 }
 
-func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceReload bool) (map[string]interface{}, error) {
+func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceReload bool) (*arvados.Collection, error) {
        c.setupOnce.Do(c.setup)
 
        atomic.AddUint64(&c.stats.Requests, 1)
@@ -114,7 +113,7 @@ func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceRelo
                }
        }
 
-       var collection map[string]interface{}
+       var collection *arvados.Collection
        if pdh != "" {
                collection = c.lookupCollection(pdh)
        }
@@ -127,14 +126,12 @@ func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceRelo
                // _and_ the current token has permission, we can
                // use our cached manifest.
                atomic.AddUint64(&c.stats.APICalls, 1)
-               var current map[string]interface{}
+               var current arvados.Collection
                err := arv.Get("collections", targetID, selectPDH, &current)
                if err != nil {
                        return nil, err
                }
-               if checkPDH, ok := current["portable_data_hash"].(string); !ok {
-                       return nil, fmt.Errorf("API response for %q had no PDH", targetID)
-               } else if checkPDH == pdh {
+               if current.PortableDataHash == pdh {
                        exp := time.Now().Add(time.Duration(c.TTL))
                        c.permissions.Add(permKey, &cachedPermission{
                                expire: exp,
@@ -150,7 +147,7 @@ func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceRelo
                        // PDH changed, but now we know we have
                        // permission -- and maybe we already have the
                        // new PDH in the cache.
-                       if coll := c.lookupCollection(checkPDH); coll != nil {
+                       if coll := c.lookupCollection(current.PortableDataHash); coll != nil {
                                return coll, nil
                        }
                }
@@ -162,23 +159,19 @@ func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceRelo
        if err != nil {
                return nil, err
        }
-       pdh, ok := collection["portable_data_hash"].(string)
-       if !ok {
-               return nil, fmt.Errorf("API response for %q had no PDH", targetID)
-       }
        exp := time.Now().Add(time.Duration(c.TTL))
        c.permissions.Add(permKey, &cachedPermission{
                expire: exp,
        })
        c.pdhs.Add(targetID, &cachedPDH{
                expire: exp,
-               pdh:    pdh,
+               pdh:    collection.PortableDataHash,
        })
-       c.collections.Add(pdh, &cachedCollection{
+       c.collections.Add(collection.PortableDataHash, &cachedCollection{
                expire:     exp,
                collection: collection,
        })
-       if int64(len(collection["manifest_text"].(string))) > c.MaxCollectionBytes/int64(c.MaxCollectionEntries) {
+       if int64(len(collection.ManifestText)) > c.MaxCollectionBytes/int64(c.MaxCollectionEntries) {
                go c.pruneCollections()
        }
        return collection, nil
@@ -203,7 +196,7 @@ func (c *cache) pruneCollections() {
                        continue
                }
                ent := v.(*cachedCollection)
-               n := len(ent.collection["manifest_text"].(string))
+               n := len(ent.collection.ManifestText)
                size += int64(n)
                entsize[i] = n
                expired[i] = ent.expire.Before(now)
@@ -236,12 +229,12 @@ func (c *cache) collectionBytes() uint64 {
                if !ok {
                        continue
                }
-               size += uint64(len(v.(*cachedCollection).collection["manifest_text"].(string)))
+               size += uint64(len(v.(*cachedCollection).collection.ManifestText))
        }
        return size
 }
 
-func (c *cache) lookupCollection(pdh string) map[string]interface{} {
+func (c *cache) lookupCollection(pdh string) *arvados.Collection {
        if pdh == "" {
                return nil
        } else if ent, cached := c.collections.Get(pdh); !cached {

diff --git a/services/keep-web/cache_test.go b/services/keep-web/cache_test.go
index f8aa2b1c60e095198719a7028de3a9cbde7fe12a..39ad1c37f6b0b626faa22a5821d6d5142927ff77 100644 (file)
--- a/services/keep-web/cache_test.go
+++ b/services/keep-web/cache_test.go
@@ -20,8 +20,8 @@ func (s *UnitSuite) TestCache(c *check.C) {
                coll, err := cache.Get(arv, arvadostest.FooCollection, false)
                c.Check(err, check.Equals, nil)
                c.Assert(coll, check.NotNil)
-               c.Check(coll["portable_data_hash"], check.Equals, arvadostest.FooPdh)
-               c.Check(coll["manifest_text"].(string)[:2], check.Equals, ". ")
+               c.Check(coll.PortableDataHash, check.Equals, arvadostest.FooPdh)
+               c.Check(coll.ManifestText[:2], check.Equals, ". ")
        }
        c.Check(cache.Stats().Requests, check.Equals, uint64(5))
        c.Check(cache.Stats().CollectionHits, check.Equals, uint64(4))
@@ -38,8 +38,8 @@ func (s *UnitSuite) TestCache(c *check.C) {
                coll, err := cache.Get(arv, arvadostest.FooPdh, false)
                c.Check(err, check.Equals, nil)
                c.Assert(coll, check.NotNil)
-               c.Check(coll["portable_data_hash"], check.Equals, arvadostest.FooPdh)
-               c.Check(coll["manifest_text"].(string)[:2], check.Equals, ". ")
+               c.Check(coll.PortableDataHash, check.Equals, arvadostest.FooPdh)
+               c.Check(coll.ManifestText[:2], check.Equals, ". ")
        }
        c.Check(cache.Stats().Requests, check.Equals, uint64(5+2))
        c.Check(cache.Stats().CollectionHits, check.Equals, uint64(4+2))

diff --git a/services/keep-web/handler.go b/services/keep-web/handler.go
index 846bdead7c5d85a18423e3d559986eeb37deaeed..17be60321f10a3ed89bd56c3e4bbde5d0bf8d4f5 100644 (file)
--- a/services/keep-web/handler.go
+++ b/services/keep-web/handler.go
@@ -251,15 +251,13 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) {
                forceReload = true
        }
 
-       var collection map[string]interface{}
+       var collection *arvados.Collection
        tokenResult := make(map[string]int)
-       found := false
        for _, arv.ApiToken = range tokens {
                var err error
                collection, err = h.Config.Cache.Get(arv, targetID, forceReload)
                if err == nil {
                        // Success
-                       found = true
                        break
                }
                if srvErr, ok := err.(arvadosclient.APIServerError); ok {
@@ -275,7 +273,7 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) {
                statusCode, statusText = http.StatusInternalServerError, err.Error()
                return
        }
-       if !found {
+       if collection == nil {
                if pathToken || !credentialsOK {
                        // Either the URL is a "secret sharing link"
                        // that didn't work out (and asking the client
@@ -316,16 +314,7 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) {
        basename := targetPath[len(targetPath)-1]
        applyContentDispositionHdr(w, r, basename, attachment)
 
-       j, err := json.Marshal(collection)
-       if err != nil {
-               panic(err)
-       }
-       var coll arvados.Collection
-       err = json.Unmarshal(j, &coll)
-       if err != nil {
-               panic(err)
-       }
-       fs := coll.FileSystem(&arvados.Client{
+       fs := collection.FileSystem(&arvados.Client{
                APIHost:   arv.ApiServer,
                AuthToken: arv.ApiToken,
                Insecure:  arv.ApiInsecure,
@@ -340,7 +329,7 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) {
        } else if stat.IsDir() && !strings.HasSuffix(r.URL.Path, "/") {
                h.seeOtherWithCookie(w, r, basename+"/", credentialsOK)
        } else if stat.IsDir() {
-               h.serveDirectory(w, r, &coll, fs, openPath, stripParts)
+               h.serveDirectory(w, r, collection, fs, openPath, stripParts)
        } else {
                http.ServeContent(w, r, basename, stat.ModTime(), f)
                if int64(w.WroteBodyBytes()) != stat.Size() {