Improve listing received shares

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() {