diff --git a/changelog/unreleased/ocis-cache-displayname.md b/changelog/unreleased/ocis-cache-displayname.md new file mode 100644 index 0000000000..bfd9b72f9f --- /dev/null +++ b/changelog/unreleased/ocis-cache-displayname.md @@ -0,0 +1,5 @@ +Bugfix: Cache display names in ocs service + +The ocs list shares endpoint may need to fetch the displayname for multiple different users. We are now caching the lookup fo 60 seconds to save redundant RPCs to the users service. + +https://github.com/cs3org/reva/pull/1161 diff --git a/internal/http/services/owncloud/ocs/conversions/main.go b/internal/http/services/owncloud/ocs/conversions/main.go index 5d9734e957..4f74d924cf 100644 --- a/internal/http/services/owncloud/ocs/conversions/main.go +++ b/internal/http/services/owncloud/ocs/conversions/main.go @@ -305,30 +305,29 @@ func PublicShare2ShareData(share *link.PublicShare, r *http.Request, publicURL s expiration = "" } - shareWith := "" - if share.PasswordProtected { - shareWith = "***redacted***" + sd := &ShareData{ + // share.permissions are mapped below + // Displaynames are added later + ID: share.Id.OpaqueId, + ShareType: ShareTypePublicLink, + STime: share.Ctime.Seconds, // TODO CS3 api birth time = btime + Token: share.Token, + Expiration: expiration, + MimeType: share.Mtime.String(), + Name: share.DisplayName, + MailSend: 0, + URL: publicURL + path.Join("/", "#/s/"+share.Token), + Permissions: publicSharePermissions2OCSPermissions(share.GetPermissions()), + UIDOwner: LocalUserIDToString(share.Creator), + UIDFileOwner: LocalUserIDToString(share.Owner), } - return &ShareData{ - // share.permissions ar mapped below - // DisplaynameOwner: creator.DisplayName, - // DisplaynameFileOwner: share.GetCreator().String(), - ID: share.Id.OpaqueId, - ShareType: ShareTypePublicLink, - ShareWith: shareWith, - ShareWithDisplayname: shareWith, - STime: share.Ctime.Seconds, // TODO CS3 api birth time = btime - Token: share.Token, - Expiration: expiration, - MimeType: share.Mtime.String(), - Name: share.DisplayName, - MailSend: 0, - URL: publicURL + path.Join("/", "#/s/"+share.Token), - Permissions: publicSharePermissions2OCSPermissions(share.GetPermissions()), - UIDOwner: LocalUserIDToString(share.Creator), - UIDFileOwner: LocalUserIDToString(share.Owner), + if share.PasswordProtected { + sd.ShareWith = "***redacted***" + sd.ShareWithDisplayname = "***redacted***" } + + return sd // actually clients should be able to GET and cache the user info themselves ... // TODO check grantee type for user vs group } 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 index f64e3dc99d..fe170753a5 100644 --- a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go +++ b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go @@ -48,21 +48,22 @@ import ( "github.com/cs3org/reva/pkg/appctx" "github.com/cs3org/reva/pkg/rgrpc/todo/pool" "github.com/cs3org/reva/pkg/rhttp/router" + "github.com/cs3org/reva/pkg/ttlmap" "github.com/pkg/errors" ) // Handler implements the shares part of the ownCloud sharing API type Handler struct { - gatewayAddr string - publicURL string - ttlcache *TTLMap + gatewayAddr string + publicURL string + displayNameCache *ttlmap.TTLMap } // Init initializes this and any contained handlers func (h *Handler) Init(c *config.Config) error { h.gatewayAddr = c.GatewaySvc h.publicURL = c.Config.Host - h.ttlcache = New(1000, 60) + h.displayNameCache = ttlmap.New(1000, 60) return nil } @@ -312,6 +313,7 @@ func (h *Handler) createUserShare(w http.ResponseWriter, r *http.Request) { return } h.addDisplaynames(ctx, c, s) + response.WriteOCSSuccess(w, r, s) } @@ -427,11 +429,11 @@ func (h *Handler) createPublicLinkShare(w http.ResponseWriter, r *http.Request) s := conversions.PublicShare2ShareData(createRes.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.addDisplaynames(ctx, c, s) response.WriteOCSSuccess(w, r, s) } @@ -784,8 +786,8 @@ func (h *Handler) getShare(w http.ResponseWriter, r *http.Request, shareID strin 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) } - h.addDisplaynames(ctx, client, share) + response.WriteOCSSuccess(w, r, []*conversions.ShareData{share}) } @@ -907,8 +909,8 @@ func (h *Handler) updateShare(w http.ResponseWriter, r *http.Request, shareID st response.WriteOCSError(w, r, response.MetaServerError.StatusCode, err.Error(), err) return } - h.addDisplaynames(ctx, uClient, share) + response.WriteOCSSuccess(w, r, share) } @@ -1076,8 +1078,8 @@ func (h *Handler) listSharesWithMe(w http.ResponseWriter, r *http.Request) { response.WriteOCSError(w, r, response.MetaServerError.StatusCode, err.Error(), err) return } - h.addDisplaynames(r.Context(), gwc, data) + shares = append(shares, data) } @@ -1180,6 +1182,7 @@ func (h *Handler) listPublicShares(r *http.Request, filters []*link.ListPublicSh if h.addFileInfo(ctx, sData, statResponse.Info) != nil { return nil, err } + h.addDisplaynames(ctx, c, sData) log.Debug().Interface("share", share).Interface("info", statResponse.Info).Interface("shareData", share).Msg("mapped") @@ -1369,7 +1372,10 @@ func (h *Handler) addFileInfo(ctx context.Context, s *conversions.ShareData, inf func (h *Handler) getDisplayname(ctx context.Context, c gateway.GatewayAPIClient, userid string) string { log := appctx.GetLogger(ctx) - if dn := h.ttlcache.Get(userid); dn != "" { + if userid == "" { + return "" + } + if dn := h.displayNameCache.Get(userid); dn != "" { log.Debug().Str("userid", userid).Msg("cache hit") return dn } @@ -1410,15 +1416,21 @@ func (h *Handler) getDisplayname(ctx context.Context, c gateway.GatewayAPIClient return "" } - h.ttlcache.Put(userid, res.User.DisplayName) + h.displayNameCache.Put(userid, res.User.DisplayName) log.Debug().Str("userid", userid).Msg("cache update") return res.User.DisplayName } func (h *Handler) addDisplaynames(ctx context.Context, c gateway.GatewayAPIClient, s *conversions.ShareData) { - s.DisplaynameOwner = h.getDisplayname(ctx, c, s.UIDOwner) - s.DisplaynameFileOwner = h.getDisplayname(ctx, c, s.UIDFileOwner) - s.ShareWithDisplayname = h.getDisplayname(ctx, c, s.ShareWith) + if s.DisplaynameOwner == "" { + s.DisplaynameOwner = h.getDisplayname(ctx, c, s.UIDOwner) + } + if s.DisplaynameFileOwner == "" { + s.DisplaynameFileOwner = h.getDisplayname(ctx, c, s.UIDFileOwner) + } + if s.ShareWithDisplayname == "" { + s.ShareWithDisplayname = h.getDisplayname(ctx, c, s.ShareWith) + } } func (h *Handler) isPublicShare(r *http.Request, oid string) bool { @@ -1627,11 +1639,11 @@ 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.addDisplaynames(r.Context(), gwC, s) response.WriteOCSSuccess(w, r, s) } diff --git a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/ttlmap.go b/pkg/ttlmap/ttlmap.go similarity index 79% rename from internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/ttlmap.go rename to pkg/ttlmap/ttlmap.go index 3ee4a0905c..a559a37ea4 100644 --- a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/ttlmap.go +++ b/pkg/ttlmap/ttlmap.go @@ -16,24 +16,26 @@ // granted to it by virtue of its status as an Intergovernmental Organization // or submit itself to any jurisdiction. -package shares +package ttlmap import ( "sync" "time" ) -// below code copied from https://stackoverflow.com/a/25487392 -type item struct { - value string - lastAccess int64 -} - +// TTLMap is a simple kv cache, based on https://stackoverflow.com/a/25487392 +// The ttl of an item will be reset whenever it is read or written. type TTLMap struct { m map[string]*item l sync.Mutex } +type item struct { + value string + lastAccess int64 +} + +// New creates a new ttl cache, preallocating space for ln items and the given maxttl func New(ln int, maxTTL int) (m *TTLMap) { m = &TTLMap{m: make(map[string]*item, ln)} go func() { @@ -50,10 +52,12 @@ func New(ln int, maxTTL int) (m *TTLMap) { return } +// Len returns the current number of items in the cache func (m *TTLMap) Len() int { return len(m.m) } +// Put sets or overwrites an item, resetting the ttl func (m *TTLMap) Put(k, v string) { m.l.Lock() it, ok := m.m[k] @@ -65,6 +69,7 @@ func (m *TTLMap) Put(k, v string) { m.l.Unlock() } +// Get retrieves an item from the cache, resetting the ttl func (m *TTLMap) Get(k string) (v string) { m.l.Lock() if it, ok := m.m[k]; ok {