Improve listing received shares (#3218)

* Get rid of unneeded etPath calls. Also route Stat calls more efficiently

* Add changelog

* ocs: path must always be prefixed by /

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* small refactoring to make code more readible

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* request info with full path

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* Add a TTL setting for the caches to prevent superfluous rapid syncs

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Co-authored-by: Jörn Friedrich Dreyer <jfd@butonic.de>

changelog/unreleased/improve-listing-received-shares.md

@@ -0,0 +1,7 @@
+Bugfix: Improve performance when listing received shares
+
+We improved the performance when listing received shares by getting rid of
+superfluous GetPath calls and sending stat request directly to the storage
+provider instead of the SharesStorageProvider.
+
+https://github.com/cs3org/reva/pull/3218

internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending.go

@@ -199,9 +199,7 @@ func (h *Handler) updateReceivedShare(w http.ResponseWriter, r *http.Request, sh
 
 	data.State = mapState(rs.GetState())
 
-	if err := h.addFileInfo(ctx, data, info); err != nil {
-		logger.Debug().Interface("received_share", rs).Interface("info", info).Interface("shareData", data).Err(err).Msg("could not add file info, skipping")
-	}
+	h.addFileInfo(ctx, data, info)
 	h.mapUserIds(r.Context(), client, data)
 
 	if data.State == ocsStateAccepted {

internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/public.go

@@ -219,10 +219,7 @@ func (h *Handler) listPublicShares(r *http.Request, filters []*link.ListPublicSh
 
 			sData.Name = share.DisplayName
 
-			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.addFileInfo(ctx, sData, info)
 			h.mapUserIds(ctx, client, sData)
 
 			log.Debug().Interface("share", share).Interface("info", info).Interface("shareData", share).Msg("mapped")
@@ -433,11 +430,7 @@ func (h *Handler) updatePublicShare(w http.ResponseWriter, r *http.Request, shar
 	}
 
 	s := conversions.PublicShare2ShareData(publicShare, r, h.publicURL)
-	err = h.addFileInfo(r.Context(), s, statRes.Info)
-	if err != nil {
-		response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error enhancing response with share data", err)
-		return
-	}
+	h.addFileInfo(r.Context(), s, statRes.Info)
 	h.mapUserIds(r.Context(), gwC, s)
 
 	response.WriteOCSSuccess(w, r, s)

internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go

@@ -272,11 +272,7 @@ func (h *Handler) CreateShare(w http.ResponseWriter, r *http.Request) {
 			return
 		}
 
-		err = h.addFileInfo(ctx, s, statRes.Info)
-		if err != nil {
-			response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error adding fileinfo to share", err)
-			return
-		}
+		h.addFileInfo(ctx, s, statRes.Info)
 
 		h.mapUserIds(ctx, client, s)
 
@@ -303,11 +299,7 @@ func (h *Handler) CreateShare(w http.ResponseWriter, r *http.Request) {
 		}
 
 		s := conversions.PublicShare2ShareData(share, r, h.publicURL)
-		err = h.addFileInfo(ctx, s, statRes.Info)
-		if err != nil {
-			response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error enhancing response with share data", err)
-			return
-		}
+		h.addFileInfo(ctx, s, statRes.Info)
 		h.mapUserIds(ctx, client, s)
 
 		response.WriteOCSSuccess(w, r, s)
@@ -608,11 +600,7 @@ func (h *Handler) GetShare(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	err = h.addFileInfo(ctx, share, info)
-	if err != nil {
-		sublog.Error().Err(err).Msg("error mapping share data")
-		response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error mapping share data", err)
-	}
+	h.addFileInfo(ctx, share, info)
 	h.mapUserIds(ctx, client, share)
 
 	if receivedshare != nil && receivedshare.State == collaboration.ShareState_SHARE_STATE_ACCEPTED {
@@ -776,11 +764,7 @@ func (h *Handler) updateShare(w http.ResponseWriter, r *http.Request, shareID st
 		return
 	}
 
-	err = h.addFileInfo(r.Context(), share, statRes.Info)
-	if err != nil {
-		response.WriteOCSError(w, r, response.MetaServerError.StatusCode, err.Error(), err)
-		return
-	}
+	h.addFileInfo(r.Context(), share, statRes.Info)
 	h.mapUserIds(ctx, client, share)
 
 	response.WriteOCSSuccess(w, r, share)
@@ -928,23 +912,10 @@ func (h *Handler) listSharesWithMe(w http.ResponseWriter, r *http.Request) {
 			info = pinfo
 		} else {
 			var status *rpc.Status
-			// FIXME the ResourceID is the id of the resource, but we want the id of the mount point so we can fetch that path, well we have the mountpoint path in the receivedshare
-			// first stat mount point, but the shares storage provider only handles accepted shares so we send the try to make the requests for only those
-			if rs.State == collaboration.ShareState_SHARE_STATE_ACCEPTED {
-				mountID := &provider.ResourceId{
-					StorageId: utils.ShareStorageProviderID,
-					SpaceId:   utils.ShareStorageSpaceID,
-					OpaqueId:  rs.Share.Id.OpaqueId,
-				}
-				info, status, err = h.getResourceInfoByID(ctx, client, mountID)
-			}
-			if rs.State != collaboration.ShareState_SHARE_STATE_ACCEPTED || err != nil || status.Code != rpc.Code_CODE_OK {
-				// fallback to unmounted resource
-				info, status, err = h.getResourceInfoByID(ctx, client, rs.Share.ResourceId)
-				if err != nil || status.Code != rpc.Code_CODE_OK {
-					h.logProblems(&sublog, status, err, "could not stat, skipping")
-					continue
-				}
+			info, status, err = h.getResourceInfoByID(ctx, client, rs.Share.ResourceId)
+			if err != nil || status.Code != rpc.Code_CODE_OK {
+				h.logProblems(&sublog, status, err, "could not stat, skipping")
+				continue
 			}
 		}
 
@@ -956,10 +927,7 @@ func (h *Handler) listSharesWithMe(w http.ResponseWriter, r *http.Request) {
 
 		data.State = mapState(rs.GetState())
 
-		if err := h.addFileInfo(ctx, data, info); err != nil {
-			sublog.Debug().Interface("received_share", rs.Share.Id).Err(err).Msg("could not add file info, skipping")
-			continue
-		}
+		h.addFileInfo(ctx, data, info)
 		h.mapUserIds(r.Context(), client, data)
 
 		if data.State == ocsStateAccepted {
@@ -1137,89 +1105,71 @@ func (h *Handler) addFilters(w http.ResponseWriter, r *http.Request, ref *provid
 	return collaborationFilters, linkFilters, nil
 }
 
-func (h *Handler) addFileInfo(ctx context.Context, s *conversions.ShareData, info *provider.ResourceInfo) error {
+func (h *Handler) addFileInfo(ctx context.Context, s *conversions.ShareData, info *provider.ResourceInfo) {
+	if info == nil {
+		return
+	}
+
 	sublog := appctx.GetLogger(ctx)
-	if info != nil {
-		// TODO The owner is not set in the storage stat metadata ...
-		parsedMt, _, err := mime.ParseMediaType(info.MimeType)
-		if err != nil {
-			// Should never happen. We log anyways so that we know if it happens.
-			sublog.Warn().Err(err).Msg("failed to parse mimetype")
-		}
-		s.MimeType = parsedMt
-		// TODO STime:     &types.Timestamp{Seconds: info.Mtime.Seconds, Nanos: info.Mtime.Nanos},
-		// TODO Storage: int
-		s.ItemSource = storagespace.FormatResourceID(*info.Id)
-		s.FileSource = s.ItemSource
-		switch {
-		case h.sharePrefix == "/":
-			s.FileTarget = info.Path
-			s.Path = info.Path
-			client, err := pool.GetGatewayServiceClient(h.gatewayAddr)
-			if err == nil {
-				gpRes, err := client.GetPath(ctx, &provider.GetPathRequest{
-					ResourceId: info.Id,
-				})
-				if err == nil && gpRes.Status.Code == rpc.Code_CODE_OK {
-					// TODO log error?
-					s.Path = gpRes.Path
-
-					// cut off configured home namespace, paths in ocs shares are relative to it
-					identifier := h.mustGetIdentifiers(ctx, client, info.GetOwner().GetOpaqueId(), false)
-					u := &userpb.User{
-						Id:          info.Owner,
-						Username:    identifier.Username,
-						DisplayName: identifier.DisplayName,
-						Mail:        identifier.Mail,
-					}
-					s.Path = strings.TrimPrefix(s.Path, h.getHomeNamespace(u))
-				}
-			}
-		default:
-			s.FileTarget = path.Join(h.sharePrefix, path.Base(info.Path))
-			if s.ShareType == conversions.ShareTypePublicLink {
-				s.FileTarget = path.Join("/", path.Base(info.Path))
-			}
-			s.Path = info.Path
-			client, err := pool.GetGatewayServiceClient(h.gatewayAddr)
-			if err == nil {
-				gpRes, err := client.GetPath(ctx, &provider.GetPathRequest{
-					ResourceId: info.Id,
-				})
-				if err == nil && gpRes.Status.Code == rpc.Code_CODE_OK {
-					// TODO log error?
-					s.Path = gpRes.Path
-				}
-				// on spaces, we could have no owner set
-				if info.Owner != nil {
-					// cut off configured home namespace, paths in ocs shares are relative to it
-					identifier := h.mustGetIdentifiers(ctx, client, info.GetOwner().GetOpaqueId(), false)
-					u := &userpb.User{
-						Id:          info.Owner,
-						Username:    identifier.Username,
-						DisplayName: identifier.DisplayName,
-						Mail:        identifier.Mail,
-					}
-					s.Path = strings.TrimPrefix(s.Path, h.getHomeNamespace(u))
+	// TODO The owner is not set in the storage stat metadata ...
+	parsedMt, _, err := mime.ParseMediaType(info.MimeType)
+	if err != nil {
+		// Should never happen. We log anyways so that we know if it happens.
+		sublog.Warn().Err(err).Msg("failed to parse mimetype")
+	}
+	s.MimeType = parsedMt
+	// TODO STime:     &types.Timestamp{Seconds: info.Mtime.Seconds, Nanos: info.Mtime.Nanos},
+	// TODO Storage: int
+	s.ItemSource = storagespace.FormatResourceID(*info.Id)
+	s.FileSource = s.ItemSource
+	s.Path = path.Join("/", info.Path)
+	switch {
+	case h.sharePrefix == "/":
+		s.FileTarget = info.Path
+		client, err := pool.GetGatewayServiceClient(h.gatewayAddr)
+		if err == nil {
+			gpRes, err := client.GetPath(ctx, &provider.GetPathRequest{
+				ResourceId: info.Id,
+			})
+			if err == nil && gpRes.Status.Code == rpc.Code_CODE_OK {
+				// TODO log error?
+
+				// cut off configured home namespace, paths in ocs shares are relative to it
+				identifier := h.mustGetIdentifiers(ctx, client, info.GetOwner().GetOpaqueId(), false)
+				u := &userpb.User{
+					Id:          info.Owner,
+					Username:    identifier.Username,
+					DisplayName: identifier.DisplayName,
+					Mail:        identifier.Mail,
 				}
+				s.Path = strings.TrimPrefix(gpRes.Path, h.getHomeNamespace(u))
 			}
 		}
-		s.StorageID = storageIDPrefix + s.FileTarget
-		// TODO FileParent:
-		// item type
-		s.ItemType = conversions.ResourceType(info.GetType()).String()
-
-		owner := info.GetOwner()
-		// file owner might not yet be set. Use file info
-		if s.UIDFileOwner == "" && owner != nil {
-			s.UIDFileOwner = owner.GetOpaqueId()
+	default:
+		name := info.Name
+		if name == "" {
+			// fall back to basename of path
+			name = path.Base(info.Path)
 		}
-		// share owner might not yet be set. Use file info
-		if s.UIDOwner == "" && owner != nil {
-			s.UIDOwner = owner.GetOpaqueId()
+		s.FileTarget = path.Join(h.sharePrefix, name)
+		if s.ShareType == conversions.ShareTypePublicLink {
+			s.FileTarget = path.Join("/", name)
 		}
 	}
-	return nil
+	s.StorageID = storageIDPrefix + s.FileTarget
+	// TODO FileParent:
+	// item type
+	s.ItemType = conversions.ResourceType(info.GetType()).String()
+
+	owner := info.GetOwner()
+	// file owner might not yet be set. Use file info
+	if s.UIDFileOwner == "" && owner != nil {
+		s.UIDFileOwner = owner.GetOpaqueId()
+	}
+	// share owner might not yet be set. Use file info
+	if s.UIDOwner == "" && owner != nil {
+		s.UIDOwner = owner.GetOpaqueId()
+	}
 }
 
 // mustGetIdentifiers always returns a struct with identifiers, if the user or group could not be found they will all be empty
@@ -1365,7 +1315,7 @@ func (h *Handler) getResourceInfoByReference(ctx context.Context, client gateway
 }
 
 func (h *Handler) getResourceInfoByID(ctx context.Context, client gateway.GatewayAPIClient, id *provider.ResourceId) (*provider.ResourceInfo, *rpc.Status, error) {
-	return h.getResourceInfo(ctx, client, storagespace.FormatResourceID(*id), &provider.Reference{ResourceId: id, Path: "."})
+	return h.getResourceInfo(ctx, client, storagespace.FormatResourceID(*id), &provider.Reference{ResourceId: id})
 }
 
 // getResourceInfo retrieves the resource info to a target.

internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/user.go

@@ -228,10 +228,7 @@ func (h *Handler) listUserShares(r *http.Request, filters []*collaboration.Filte
 				continue
 			}
 
-			if err := h.addFileInfo(ctx, data, info); err != nil {
-				log.Debug().Interface("share", s).Interface("info", info).Interface("shareData", data).Err(err).Msg("could not add file info, skipping")
-				continue
-			}
+			h.addFileInfo(ctx, data, info)
 			h.mapUserIds(ctx, client, data)
 
 			log.Debug().Interface("share", s).Interface("info", info).Interface("shareData", data).Msg("mapped")

pkg/share/manager/jsoncs3/jsoncs3.go

@@ -22,6 +22,7 @@ import (
 	"context"
 	"strings"
 	"sync"
+	"time"
 
 	"github.com/google/uuid"
 	"github.com/mitchellh/mapstructure"
@@ -152,10 +153,10 @@ func NewDefault(m map[string]interface{}) (share.Manager, error) {
 // New returns a new manager instance.
 func New(s metadata.Storage, gc gatewayv1beta1.GatewayAPIClient) (*Manager, error) {
 	return &Manager{
-		Cache:              providercache.New(s),
-		CreatedCache:       sharecache.New(s, "users", "created.json"),
-		UserReceivedStates: receivedsharecache.New(s),
-		GroupReceivedCache: sharecache.New(s, "groups", "received.json"),
+		Cache:              providercache.New(s, 0*time.Second),
+		CreatedCache:       sharecache.New(s, "users", "created.json", 0*time.Second),
+		UserReceivedStates: receivedsharecache.New(s, 0*time.Second),
+		GroupReceivedCache: sharecache.New(s, "groups", "received.json", 0*time.Second),
 		storage:            s,
 		gateway:            gc,
 	}, nil

pkg/share/manager/jsoncs3/providercache/providercache.go

@@ -40,6 +40,7 @@ type Cache struct {
 	Providers map[string]*Spaces
 
 	storage metadata.Storage
+	ttl     time.Duration
 }
 
 // Spaces holds the share information for provider
@@ -49,8 +50,9 @@ type Spaces struct {
 
 // Shares holds the share information of one space
 type Shares struct {
-	Shares map[string]*collaboration.Share
-	Mtime  time.Time
+	Shares   map[string]*collaboration.Share
+	Mtime    time.Time
+	nextSync time.Time
 }
 
 // UnmarshalJSON overrides the default unmarshaling
@@ -94,10 +96,11 @@ func (s *Shares) UnmarshalJSON(data []byte) error {
 }
 
 // New returns a new Cache instance
-func New(s metadata.Storage) Cache {
+func New(s metadata.Storage, ttl time.Duration) Cache {
 	return Cache{
 		Providers: map[string]*Spaces{},
 		storage:   s,
+		ttl:       ttl,
 	}
 }
 
@@ -190,7 +193,12 @@ func (c *Cache) Sync(ctx context.Context, storageID, spaceID string) error {
 	var mtime time.Time
 	if c.Providers[storageID] != nil && c.Providers[storageID].Spaces[spaceID] != nil {
 		mtime = c.Providers[storageID].Spaces[spaceID].Mtime
-		//    - y: set If-Modified-Since header to only download if it changed
+
+		if time.Now().Before(c.Providers[storageID].Spaces[spaceID].nextSync) {
+			log.Debug().Msg("Skipping provider cache sync, it was just recently synced...")
+			return nil
+		}
+		c.Providers[storageID].Spaces[spaceID].nextSync = time.Now().Add(c.ttl)
 	} else {
 		mtime = time.Time{} // Set zero time so that data from storage always takes precedence
 	}

pkg/share/manager/jsoncs3/providercache/providercache_test.go

@@ -62,7 +62,7 @@ var _ = Describe("Cache", func() {
 		storage, err = metadata.NewDiskStorage(tmpdir)
 		Expect(err).ToNot(HaveOccurred())
 
-		c = providercache.New(storage)
+		c = providercache.New(storage, 0*time.Second)
 		Expect(c).ToNot(BeNil())
 	})
 
@@ -160,7 +160,7 @@ var _ = Describe("Cache", func() {
 			BeforeEach(func() {
 				Expect(c.Persist(ctx, storageID, spaceID)).To(Succeed())
 				// reset in-memory cache
-				c = providercache.New(storage)
+				c = providercache.New(storage, 0*time.Second)
 			})
 
 			It("downloads if needed", func() {