Merge pull request #6189 from owncloud/cache-special-drive-items

cache special drive items until space root changes

services/graph/pkg/config/defaults/defaultconfig.go

@@ -54,6 +54,8 @@ func DefaultConfig() *config.Config {
 			WebDavPath:   "/dav/spaces/",
 			DefaultQuota: "1000000000",
 			// 30 minutes
+			ExtendedSpacePropertiesCacheTTL: 1800,
+			// 30 minutes
 			GroupsCacheTTL: 1800,
 			// 30 minutes
 			UsersCacheTTL: 1800,

services/graph/pkg/service/v0/driveitems.go

@@ -13,9 +13,11 @@ import (
 	storageprovider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
 	types "github.com/cs3org/go-cs3apis/cs3/types/v1beta1"
 	"github.com/cs3org/reva/v2/pkg/storagespace"
+	"github.com/cs3org/reva/v2/pkg/utils"
 	"github.com/go-chi/render"
 	libregraph "github.com/owncloud/libre-graph-api-go"
 	"github.com/owncloud/ocis/v2/services/graph/pkg/service/v0/errorcode"
+	"golang.org/x/crypto/sha3"
 )
 
 // GetRootDriveChildren implements the Service interface.
@@ -77,18 +79,16 @@ func (g Graph) GetRootDriveChildren(w http.ResponseWriter, r *http.Request) {
 	render.JSON(w, r, &ListResponse{Value: files})
 }
 
-func (g Graph) getDriveItem(ctx context.Context, root storageprovider.ResourceId) (*libregraph.DriveItem, error) {
+func (g Graph) getDriveItem(ctx context.Context, ref storageprovider.Reference) (*libregraph.DriveItem, error) {
 	client := g.GetGatewayClient()
 
-	ref := &storageprovider.Reference{
-		ResourceId: &root,
-	}
-	res, err := client.Stat(ctx, &storageprovider.StatRequest{Ref: ref})
+	res, err := client.Stat(ctx, &storageprovider.StatRequest{Ref: &ref})
 	if err != nil {
 		return nil, err
 	}
 	if res.Status.Code != cs3rpc.Code_CODE_OK {
-		return nil, fmt.Errorf("could not stat %s: %s", ref, res.Status.Message)
+		refStr, _ := storagespace.FormatReference(&ref)
+		return nil, fmt.Errorf("could not stat %s: %s", refStr, res.Status.Message)
 	}
 	return cs3ResourceToDriveItem(res.Info)
 }
@@ -215,44 +215,107 @@ func (g Graph) getPathForResource(ctx context.Context, id storageprovider.Resour
 	return res.Path, err
 }
 
-// getExtendedSpaceProperties reads properties from the opaque and transforms them into driveItems
-func (g Graph) getExtendedSpaceProperties(ctx context.Context, baseURL *url.URL, space *storageprovider.StorageSpace) []libregraph.DriveItem {
-	var spaceItems []libregraph.DriveItem
+// getSpecialDriveItems reads properties from the opaque and transforms them into driveItems
+func (g Graph) getSpecialDriveItems(ctx context.Context, baseURL *url.URL, space *storageprovider.StorageSpace) []libregraph.DriveItem {
+	if space.GetRoot().GetStorageId() == utils.ShareStorageProviderID {
+		return nil // no point in stating the ShareStorageProvider
+	}
 	if space.Opaque == nil {
 		return nil
 	}
-	metadata := space.Opaque.Map
-	names := [2]string{SpaceImageSpecialFolderName, ReadmeSpecialFolderName}
 
-	for _, itemName := range names {
-		if itemID, ok := metadata[itemName]; ok {
-			rid, _ := storagespace.ParseID(string(itemID.Value))
-			// add the storageID of the space, all drive items of this space belong to the same storageID
-			rid.StorageId = space.GetRoot().GetStorageId()
-			spaceItem := g.getSpecialDriveItem(ctx, rid, itemName, baseURL, space)
-			if spaceItem != nil {
-				spaceItems = append(spaceItems, *spaceItem)
+	imageNode := utils.ReadPlainFromOpaque(space.Opaque, SpaceImageSpecialFolderName)
+	readmeNode := utils.ReadPlainFromOpaque(space.Opaque, ReadmeSpecialFolderName)
+
+	cachekey := spaceRootStatKey(space.Root, imageNode, readmeNode)
+	// if the root is older or equal to our cache we can reuse the cached extended spaces properties
+	if entry := g.specialDriveItemsCache.Get(cachekey); entry != nil {
+		if cached, ok := entry.Value().(specialDriveItemEntry); ok {
+			if cached.rootMtime != nil && space.Mtime != nil {
+				// beware, LaterTS does not handle equalness. it returns t1 if t1 > t2, else t2, so a >= check looks like this
+				if utils.LaterTS(space.Mtime, cached.rootMtime) == cached.rootMtime {
+					return cached.specialDriveItems
+				}
 			}
 		}
 	}
+
+	var spaceItems []libregraph.DriveItem
+
+	spaceItems = g.fetchSpecialDriveItem(ctx, spaceItems, SpaceImageSpecialFolderName, imageNode, space, baseURL)
+	spaceItems = g.fetchSpecialDriveItem(ctx, spaceItems, ReadmeSpecialFolderName, readmeNode, space, baseURL)
+
+	// cache properties
+	spacePropertiesEntry := specialDriveItemEntry{
+		specialDriveItems: spaceItems,
+		rootMtime:         space.Mtime,
+	}
+	g.specialDriveItemsCache.Set(cachekey, spacePropertiesEntry, time.Duration(g.config.Spaces.ExtendedSpacePropertiesCacheTTL))
+
+	return spaceItems
+}
+
+func (g Graph) fetchSpecialDriveItem(ctx context.Context, spaceItems []libregraph.DriveItem, itemName string, itemNode string, space *storageprovider.StorageSpace, baseURL *url.URL) []libregraph.DriveItem {
+	var ref storageprovider.Reference
+	if itemNode != "" {
+		rid, _ := storagespace.ParseID(itemNode)
+
+		rid.StorageId = space.GetRoot().GetStorageId()
+		ref = storageprovider.Reference{
+			ResourceId: &rid,
+		}
+		spaceItem := g.getSpecialDriveItem(ctx, ref, itemName, baseURL, space)
+		if spaceItem != nil {
+			spaceItems = append(spaceItems, *spaceItem)
+		}
+	}
 	return spaceItems
 }
 
-func (g Graph) getSpecialDriveItem(ctx context.Context, id storageprovider.ResourceId, itemName string, baseURL *url.URL, space *storageprovider.StorageSpace) *libregraph.DriveItem {
+// generates a space root stat cache key used to detect changes in a space
+// takes into account the special nodes because changing metadata does not affect the etag / mtime
+func spaceRootStatKey(id *storageprovider.ResourceId, imagenode, readmeNode string) string {
+	if id == nil {
+		return ""
+	}
+	sha3 := sha3.NewShake256()
+	_, _ = sha3.Write([]byte(id.GetStorageId()))
+	_, _ = sha3.Write([]byte(id.GetSpaceId()))
+	_, _ = sha3.Write([]byte(id.GetOpaqueId()))
+	_, _ = sha3.Write([]byte(imagenode))
+	_, _ = sha3.Write([]byte(readmeNode))
+	h := make([]byte, 64)
+	_, _ = sha3.Read(h)
+	return fmt.Sprintf("%x", h)
+}
+
+type specialDriveItemEntry struct {
+	specialDriveItems []libregraph.DriveItem
+	rootMtime         *types.Timestamp
+}
+
+func (g Graph) getSpecialDriveItem(ctx context.Context, ref storageprovider.Reference, itemName string, baseURL *url.URL, space *storageprovider.StorageSpace) *libregraph.DriveItem {
 	var spaceItem *libregraph.DriveItem
-	if id.SpaceId == "" && id.OpaqueId == "" {
+	if ref.GetResourceId().GetSpaceId() == "" && ref.GetResourceId().GetOpaqueId() == "" {
 		return nil
 	}
 
-	spaceItem, err := g.getDriveItem(ctx, id)
+	// FIXME we should send a fieldmask 'path' and return it as the Path property to save an additional call to the storage.
+	// To do that we need to align the useg of ResourceInfo.Name vs ResourceInfo.Path. By default, only the name should be set
+	// and Path should always be relative to the space root OR the resource the current user can access ...
+	spaceItem, err := g.getDriveItem(ctx, ref)
 	if err != nil {
-		g.logger.Error().Err(err).Str("ID", id.OpaqueId).Str("name", itemName).Msg("Could not get item info")
+		g.logger.Debug().Err(err).Str("ID", ref.GetResourceId().GetOpaqueId()).Str("name", itemName).Msg("Could not get item info")
 		return nil
 	}
-	itemPath, err := g.getPathForResource(ctx, id)
-	if err != nil {
-		g.logger.Error().Err(err).Str("ID", id.OpaqueId).Str("name", itemName).Msg("Could not get item path")
-		return nil
+	itemPath := ref.Path
+	if itemPath == "" {
+		// lookup by id
+		itemPath, err = g.getPathForResource(ctx, *ref.ResourceId)
+		if err != nil {
+			g.logger.Debug().Err(err).Str("ID", ref.GetResourceId().GetOpaqueId()).Str("name", itemName).Msg("Could not get item path")
+			return nil
+		}
 	}
 	spaceItem.SpecialFolder = &libregraph.SpecialFolder{Name: libregraph.PtrString(itemName)}
 	webdavURL := *baseURL

services/graph/pkg/service/v0/drives.go

@@ -536,7 +536,7 @@ func (g Graph) formatDrives(ctx context.Context, baseURL *url.URL, storageSpaces
 
 				// can't access disabled space
 				if utils.ReadPlainFromOpaque(storageSpace.Opaque, "trashed") != "trashed" {
-					res.Special = g.getExtendedSpaceProperties(ctx, baseURL, storageSpace)
+					res.Special = g.getSpecialDriveItems(ctx, baseURL, storageSpace)
 					quota, err := g.getDriveQuota(ctx, storageSpace)
 					res.Quota = &quota
 					if err != nil {

services/graph/pkg/service/v0/graph.go

@@ -65,7 +65,7 @@ type Graph struct {
 	gatewayClient            gateway.GatewayAPIClient
 	roleService              RoleService
 	permissionsService       Permissions
-	spacePropertiesCache     *ttlcache.Cache[string, interface{}]
+	specialDriveItemsCache   *ttlcache.Cache[string, interface{}]
 	usersCache               *ttlcache.Cache[string, libregraph.User]
 	groupsCache              *ttlcache.Cache[string, libregraph.Group]
 	eventsPublisher          events.Publisher

services/graph/pkg/service/v0/graph_test.go

@@ -202,6 +202,9 @@ var _ = Describe("Graph", func() {
 				gatewayClient.On("GetQuota", mock.Anything, mock.Anything).Return(&provider.GetQuotaResponse{
 					Status: status.NewUnimplemented(ctx, fmt.Errorf("not supported"), "not supported"),
 				}, nil)
+				gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(&provider.StatResponse{
+					Status: status.NewNotFound(ctx, "no special files found"),
+				}, nil)
 
 				r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives?$orderby=name%20asc", nil)
 				r = r.WithContext(ctx)
@@ -891,16 +894,30 @@ var _ = Describe("Graph", func() {
 				Status: status.NewOK(ctx),
 				Path:   "thepath",
 			}, nil)
-			gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(&provider.StatResponse{
-				Status: status.NewOK(ctx),
-				Info: &provider.ResourceInfo{
-					Id: &provider.ResourceId{
-						StorageId: "pro-1",
-						SpaceId:   "spaceID",
-						OpaqueId:  "specialID",
+			// no stat for the image
+			gatewayClient.On("Stat",
+				mock.Anything,
+				mock.MatchedBy(
+					func(req *provider.StatRequest) bool {
+						return req.Ref.Path == "/.space/logo.png"
+					})).
+				Return(&provider.StatResponse{
+					Status: status.NewNotFound(ctx, "not found"),
+				}, nil)
+			// mock readme stats
+			gatewayClient.On("Stat",
+				mock.Anything,
+				mock.Anything).
+				Return(&provider.StatResponse{
+					Status: status.NewOK(ctx),
+					Info: &provider.ResourceInfo{
+						Id: &provider.ResourceId{
+							StorageId: "pro-1",
+							SpaceId:   "spaceID",
+							OpaqueId:  "specialID",
+						},
 					},
-				},
-			}, nil)
+				}, nil)
 			gatewayClient.On("ListStorageSpaces",
 				mock.Anything,
 				mock.MatchedBy(
@@ -994,6 +1011,9 @@ var _ = Describe("Graph", func() {
 					StorageSpace: req.StorageSpace,
 				}
 			}, nil)
+			gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(&provider.StatResponse{
+				Status: status.NewNotFound(ctx, "no special files found"),
+			}, nil)
 
 			r := httptest.NewRequest(http.MethodPatch, "/graph/v1.0/drives/{driveID}/", bytes.NewBuffer(driveJson))
 			rctx := chi.NewRouteContext()

services/graph/pkg/service/v0/service.go

@@ -136,7 +136,7 @@ func NewService(opts ...Option) (Graph, error) {
 		config:                   options.Config,
 		mux:                      m,
 		logger:                   &options.Logger,
-		spacePropertiesCache:     spacePropertiesCache,
+		specialDriveItemsCache:   spacePropertiesCache,
 		usersCache:               usersCache,
 		groupsCache:              groupsCache,
 		eventsPublisher:          options.EventsPublisher,