Cache resources from share getter methods in OCS (#1643)

cs3org · Apr 26, 2021 · d1d113e · d1d113e
1 parent 98b5863
commit d1d113e
Show file tree

Hide file tree

Showing 11 changed files with 240 additions and 133 deletions.
diff --git a/changelog/unreleased/ocs-resource-cache.md b/changelog/unreleased/ocs-resource-cache.md
@@ -0,0 +1,8 @@
+Enhancement: Cache resources from share getter methods in OCS
+
+In OCS, once we retrieve the shares from the shareprovider service, we stat each
+of those separately to obtain the required info, which introduces a lot of
+latency. This PR introduces a resoource info cache in OCS, which would prevent
+this latency.
+
+https://github.com/cs3org/reva/pull/1643
diff --git a/go.mod b/go.mod
@@ -9,6 +9,7 @@ require (
 	github.com/Masterminds/sprig v2.22.0+incompatible
 	github.com/ReneKroon/ttlcache/v2 v2.4.0
 	github.com/aws/aws-sdk-go v1.38.13
+	github.com/bluele/gcache v0.0.2 // indirect
 	github.com/c-bata/go-prompt v0.2.5
 	github.com/cheggaaa/pb v1.0.29
 	github.com/coreos/go-oidc v2.2.1+incompatible

diff --git a/go.sum b/go.sum
@@ -92,6 +92,8 @@ github.com/beorn7/perks v1.0.0/go.mod h1:KWe93zE9D1o94FZ5RNwFwVgaQK1VOXiVxmqh+Ce
 github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM=
 github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw=
 github.com/bgentry/speakeasy v0.1.0/go.mod h1:+zsyZBPWlz7T6j88CTgSN5bM796AkVf0kBD4zp0CCIs=
+github.com/bluele/gcache v0.0.2 h1:WcbfdXICg7G/DGBh1PFfcirkWOQV+v077yF1pSy3DGw=
+github.com/bluele/gcache v0.0.2/go.mod h1:m15KV+ECjptwSPxKhOhQoAFQVtUFjTVkc3H8o0t/fp0=
 github.com/bmatcuk/doublestar/v2 v2.0.3/go.mod h1:QMmcs3H2AUQICWhfzLXz+IYln8lRQmTZRptLie8RgRw=
 github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869/go.mod h1:Ekp36dRnpXw/yCqJaO+ZrUyxD+3VXMFFr56k5XYrpB4=
 github.com/bmizerany/pat v0.0.0-20170815010413-6226ea591a40 h1:y4B3+GPxKlrigF1ha5FFErxK+sr6sWxQovRMzwMhejo=

diff --git a/internal/grpc/services/gateway/usershareprovider.go b/internal/grpc/services/gateway/usershareprovider.go
@@ -337,6 +337,9 @@ func (s *svc) UpdateReceivedShare(ctx context.Context, req *collaboration.Update
 			return &collaboration.UpdateReceivedShareResponse{
 				Status: createRefStatus,
 			}, err
+		} else if req.Field.GetState() == collaboration.ShareState_SHARE_STATE_REJECTED {
+			// Nothing more to do, return the original result
+			return res, nil
 		}
 	}
 

diff --git a/internal/http/services/owncloud/ocs/config/config.go b/internal/http/services/owncloud/ocs/config/config.go
@@ -34,6 +34,8 @@ type Config struct {
 	SharePrefix             string                `mapstructure:"share_prefix"`
 	HomeNamespace           string                `mapstructure:"home_namespace"`
 	AdditionalInfoAttribute string                `mapstructure:"additional_info_attribute"`
+	ResourceInfoCacheSize   int                   `mapstructure:"resource_info_cache_size"`
+	ResourceInfoCacheTTL    int                   `mapstructure:"resource_info_cache_ttl"`
 }
 
 // Init sets sane defaults
@@ -58,5 +60,9 @@ func (c *Config) Init() {
 		c.AdditionalInfoAttribute = "{{.Mail}}"
 	}
 
+	if c.ResourceInfoCacheSize == 0 {
+		c.ResourceInfoCacheSize = 1000000
+	}
+
 	c.GatewaySvc = sharedconf.GetGatewaySVC(c.GatewaySvc)
 }
diff --git a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/public.go b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/public.go
@@ -23,6 +23,7 @@ import (
 	"fmt"
 	"net/http"
 	"strconv"
+	"time"
 
 	rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
 	collaboration "github.com/cs3org/go-cs3apis/cs3/sharing/collaboration/v1beta1"
@@ -159,33 +160,43 @@ func (h *Handler) listPublicShares(r *http.Request, filters []*link.ListPublicSh
 			return ocsDataPayload, res.Status, nil
 		}
 
+		var info *provider.ResourceInfo
 		for _, share := range res.GetShare() {
-
-			statRequest := &provider.StatRequest{
-				Ref: &provider.Reference{
-					Spec: &provider.Reference_Id{
-						Id: share.ResourceId,
+			key := wrapResourceID(share.ResourceId)
+			if infoIf, err := h.resourceInfoCache.Get(key); h.resourceInfoCacheTTL > 0 && err == nil {
+				log.Debug().Msgf("cache hit for resource %+v", share.ResourceId)
+				info = infoIf.(*provider.ResourceInfo)
+			} else {
+				statRequest := &provider.StatRequest{
+					Ref: &provider.Reference{
+						Spec: &provider.Reference_Id{
+							Id: share.ResourceId,
+						},
 					},
-				},
-			}
-
-			statResponse, err := c.Stat(ctx, statRequest)
-			if err != nil || res.Status.Code != rpc.Code_CODE_OK {
-				log.Debug().Interface("share", share).Interface("response", statResponse).Err(err).Msg("could not stat share, skipping")
-				continue
+				}
+
+				statResponse, err := c.Stat(ctx, statRequest)
+				if err != nil || res.Status.Code != rpc.Code_CODE_OK {
+					log.Debug().Interface("share", share).Interface("response", statResponse).Err(err).Msg("could not stat share, skipping")
+					continue
+				}
+				info = statResponse.Info
+				if h.resourceInfoCacheTTL > 0 {
+					_ = h.resourceInfoCache.SetWithExpire(key, info, time.Second*h.resourceInfoCacheTTL)
+				}
 			}
 
 			sData := conversions.PublicShare2ShareData(share, r, h.publicURL)
 
 			sData.Name = share.DisplayName
 
-			if err := h.addFileInfo(ctx, sData, statResponse.Info); err != nil {
-				log.Debug().Interface("share", share).Interface("info", statResponse.Info).Err(err).Msg("could not add file info, skipping")
+			if err := h.addFileInfo(ctx, sData, info); err != nil {
+				log.Debug().Interface("share", share).Interface("info", info).Err(err).Msg("could not add file info, skipping")
 				continue
 			}
 			h.mapUserIds(ctx, c, sData)
 
-			log.Debug().Interface("share", share).Interface("info", statResponse.Info).Interface("shareData", share).Msg("mapped")
+			log.Debug().Interface("share", share).Interface("info", info).Interface("shareData", share).Msg("mapped")
 
 			ocsDataPayload = append(ocsDataPayload, sData)
 

diff --git a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go
@@ -42,6 +42,7 @@ import (
 	"github.com/rs/zerolog/log"
 
 	"github.com/ReneKroon/ttlcache/v2"
+	"github.com/bluele/gcache"
 	"github.com/cs3org/reva/internal/http/services/owncloud/ocdav"
 	"github.com/cs3org/reva/internal/http/services/owncloud/ocs/config"
 	"github.com/cs3org/reva/internal/http/services/owncloud/ocs/conversions"
@@ -58,8 +59,10 @@ type Handler struct {
 	publicURL              string
 	sharePrefix            string
 	homeNamespace          string
+	resourceInfoCacheTTL   time.Duration
 	additionalInfoTemplate *template.Template
 	userIdentifierCache    *ttlcache.Cache
+	resourceInfoCache      gcache.Cache
 }
 
 // we only cache the minimal set of data instead of the full user metadata
@@ -75,12 +78,15 @@ func (h *Handler) Init(c *config.Config) error {
 	h.publicURL = c.Config.Host
 	h.sharePrefix = c.SharePrefix
 	h.homeNamespace = c.HomeNamespace
+	h.resourceInfoCacheTTL = time.Duration(c.ResourceInfoCacheTTL)
 
 	h.additionalInfoTemplate, _ = template.New("additionalInfo").Parse(c.AdditionalInfoAttribute)
 
 	h.userIdentifierCache = ttlcache.NewCache()
 	_ = h.userIdentifierCache.SetTTL(60 * time.Second)
 
+	h.resourceInfoCache = gcache.New(c.ResourceInfoCacheSize).LFU().Build()
+
 	return nil
 }
 
@@ -391,31 +397,42 @@ func (h *Handler) getShare(w http.ResponseWriter, r *http.Request, shareID strin
 		return
 	}
 
-	// prepare the stat request
-	statReq := &provider.StatRequest{
-		// prepare the reference
-		Ref: &provider.Reference{
-			// using ResourceId from the share
-			Spec: &provider.Reference_Id{Id: resourceID},
-		},
-	}
+	var info *provider.ResourceInfo
+	key := wrapResourceID(resourceID)
+	if infoIf, err := h.resourceInfoCache.Get(key); h.resourceInfoCacheTTL > 0 && err == nil {
+		logger.Debug().Msgf("cache hit for resource %+v", resourceID)
+		info = infoIf.(*provider.ResourceInfo)
+	} else {
+		// prepare the stat request
+		statReq := &provider.StatRequest{
+			// prepare the reference
+			Ref: &provider.Reference{
+				// using ResourceId from the share
+				Spec: &provider.Reference_Id{Id: resourceID},
+			},
+		}
 
-	statResponse, err := client.Stat(ctx, statReq)
-	if err != nil {
-		log.Error().Err(err).Msg("error mapping share data")
-		response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error mapping share data", err)
-		return
-	}
+		statResponse, err := client.Stat(ctx, statReq)
+		if err != nil {
+			log.Error().Err(err).Msg("error mapping share data")
+			response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error mapping share data", err)
+			return
+		}
 
-	if statResponse.Status.Code != rpc.Code_CODE_OK {
-		log.Error().Err(err).Str("status", statResponse.Status.Code.String()).Msg("error mapping share data")
-		response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error mapping share data", err)
-		return
+		if statResponse.Status.Code != rpc.Code_CODE_OK {
+			log.Error().Err(err).Str("status", statResponse.Status.Code.String()).Msg("error mapping share data")
+			response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error mapping share data", err)
+			return
+		}
+		info = statResponse.Info
+		if h.resourceInfoCacheTTL > 0 {
+			_ = h.resourceInfoCache.SetWithExpire(key, info, time.Second*h.resourceInfoCacheTTL)
+		}
 	}
 
-	err = h.addFileInfo(ctx, share, statResponse.Info)
+	err = h.addFileInfo(ctx, share, info)
 	if err != nil {
-		log.Error().Err(err).Str("status", statResponse.Status.Code.String()).Msg("error mapping share data")
+		log.Error().Err(err).Msg("error mapping share data")
 		response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error mapping share data", err)
 	}
 	h.mapUserIds(ctx, client, share)
@@ -585,6 +602,7 @@ func (h *Handler) listSharesWithMe(w http.ResponseWriter, r *http.Request) {
 	}
 
 	ctx := r.Context()
+	logger := appctx.GetLogger(ctx)
 
 	var pinfo *provider.ResourceInfo
 	p := r.URL.Query().Get("path")
@@ -593,33 +611,41 @@ func (h *Handler) listSharesWithMe(w http.ResponseWriter, r *http.Request) {
 		// prefix the path with the owners home, because ocs share requests are relative to the home dir
 		target := path.Join(h.homeNamespace, r.FormValue("path"))
 
-		statReq := &provider.StatRequest{
-			Ref: &provider.Reference{
-				Spec: &provider.Reference_Path{
-					Path: target,
+		if infoIf, err := h.resourceInfoCache.Get(target); h.resourceInfoCacheTTL > 0 && err == nil {
+			logger.Debug().Msgf("cache hit for resource %+v", target)
+			pinfo = infoIf.(*provider.ResourceInfo)
+		} else {
+			statReq := &provider.StatRequest{
+				Ref: &provider.Reference{
+					Spec: &provider.Reference_Path{
+						Path: target,
+					},
 				},
-			},
-		}
+			}
 
-		statRes, err := gwc.Stat(ctx, statReq)
-		if err != nil {
-			response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error sending a grpc stat request", err)
-			return
-		}
+			statRes, err := gwc.Stat(ctx, statReq)
+			if err != nil {
+				response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error sending a grpc stat request", err)
+				return
+			}
 
-		if statRes.Status.Code != rpc.Code_CODE_OK {
-			switch statRes.Status.Code {
-			case rpc.Code_CODE_NOT_FOUND:
-				response.WriteOCSError(w, r, response.MetaNotFound.StatusCode, "path not found", nil)
-			case rpc.Code_CODE_PERMISSION_DENIED:
-				response.WriteOCSError(w, r, response.MetaUnauthorized.StatusCode, "permission denied", nil)
-			default:
-				response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "grpc stat request failed", nil)
+			if statRes.Status.Code != rpc.Code_CODE_OK {
+				switch statRes.Status.Code {
+				case rpc.Code_CODE_NOT_FOUND:
+					response.WriteOCSError(w, r, response.MetaNotFound.StatusCode, "path not found", nil)
+				case rpc.Code_CODE_PERMISSION_DENIED:
+					response.WriteOCSError(w, r, response.MetaUnauthorized.StatusCode, "permission denied", nil)
+				default:
+					response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "grpc stat request failed", nil)
+				}
+				return
 			}
-			return
-		}
 
-		pinfo = statRes.GetInfo()
+			pinfo = statRes.GetInfo()
+			if h.resourceInfoCacheTTL > 0 {
+				_ = h.resourceInfoCache.SetWithExpire(target, pinfo, time.Second*h.resourceInfoCacheTTL)
+			}
+		}
 	}
 
 	lrsReq := collaboration.ListReceivedSharesRequest{}
@@ -659,22 +685,31 @@ func (h *Handler) listSharesWithMe(w http.ResponseWriter, r *http.Request) {
 			// we can reuse the stat info
 			info = pinfo
 		} else {
-			// we need to do a stat call
-			statRequest := provider.StatRequest{
-				Ref: &provider.Reference{
-					Spec: &provider.Reference_Id{
-						Id: rs.Share.ResourceId,
+			key := wrapResourceID(rs.Share.ResourceId)
+			if infoIf, err := h.resourceInfoCache.Get(key); h.resourceInfoCacheTTL > 0 && err == nil {
+				logger.Debug().Msgf("cache hit for resource %+v", rs.Share.ResourceId)
+				info = infoIf.(*provider.ResourceInfo)
+			} else {
+				// we need to do a stat call
+				statRequest := provider.StatRequest{
+					Ref: &provider.Reference{
+						Spec: &provider.Reference_Id{
+							Id: rs.Share.ResourceId,
+						},
 					},
-				},
-			}
+				}
 
-			statRes, err := gwc.Stat(r.Context(), &statRequest)
-			if err != nil || statRes.Status.Code != rpc.Code_CODE_OK {
-				h.logProblems(statRes.GetStatus(), err, "could not stat, skipping")
-				continue
-			}
+				statRes, err := gwc.Stat(r.Context(), &statRequest)
+				if err != nil || statRes.Status.Code != rpc.Code_CODE_OK {
+					h.logProblems(statRes.GetStatus(), err, "could not stat, skipping")
+					continue
+				}
 
-			info = statRes.GetInfo()
+				info = statRes.GetInfo()
+				if h.resourceInfoCacheTTL > 0 {
+					_ = h.resourceInfoCache.SetWithExpire(key, info, time.Second*h.resourceInfoCacheTTL)
+				}
+			}
 		}
 
 		data, err := conversions.CS3Share2ShareData(r.Context(), rs.Share)
@@ -773,32 +808,38 @@ func (h *Handler) addFilters(w http.ResponseWriter, r *http.Request, prefix stri
 	}
 
 	target := path.Join(prefix, r.FormValue("path"))
-
-	statReq := &provider.StatRequest{
-		Ref: &provider.Reference{
-			Spec: &provider.Reference_Path{
-				Path: target,
+	if infoIf, err := h.resourceInfoCache.Get(target); h.resourceInfoCacheTTL > 0 && err == nil {
+		info = infoIf.(*provider.ResourceInfo)
+	} else {
+		statReq := &provider.StatRequest{
+			Ref: &provider.Reference{
+				Spec: &provider.Reference_Path{
+					Path: target,
+				},
 			},
-		},
-	}
+		}
 
-	res, err := gwClient.Stat(ctx, statReq)
-	if err != nil {
-		response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error sending a grpc stat request", err)
-		return nil, nil, err
-	}
+		res, err := gwClient.Stat(ctx, statReq)
+		if err != nil {
+			response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error sending a grpc stat request", err)
+			return nil, nil, err
+		}
 
-	if res.Status.Code != rpc.Code_CODE_OK {
-		err = errors.New(res.Status.Message)
-		if res.Status.Code == rpc.Code_CODE_NOT_FOUND {
-			response.WriteOCSError(w, r, response.MetaNotFound.StatusCode, "not found", err)
+		if res.Status.Code != rpc.Code_CODE_OK {
+			err = errors.New(res.Status.Message)
+			if res.Status.Code == rpc.Code_CODE_NOT_FOUND {
+				response.WriteOCSError(w, r, response.MetaNotFound.StatusCode, "not found", err)
+				return nil, nil, err
+			}
+			response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "grpc stat request failed", err)
 			return nil, nil, err
 		}
-		response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "grpc stat request failed", err)
-		return nil, nil, err
-	}
 
-	info = res.Info
+		info = res.Info
+		if h.resourceInfoCacheTTL > 0 {
+			_ = h.resourceInfoCache.SetWithExpire(target, info, time.Second*h.resourceInfoCacheTTL)
+		}
+	}
 
 	collaborationFilters = append(collaborationFilters, &collaboration.ListSharesRequest_Filter{
 		Type: collaboration.ListSharesRequest_Filter_TYPE_RESOURCE_ID,