From 8373a7da3ac3b8b7effb16c22b74252036e2ef3d Mon Sep 17 00:00:00 2001 From: jkoberg Date: Fri, 13 May 2022 12:27:35 +0200 Subject: [PATCH 1/5] move all children to bin when trashing Signed-off-by: jkoberg --- pkg/storage/utils/decomposedfs/tree/tree.go | 44 ++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/pkg/storage/utils/decomposedfs/tree/tree.go b/pkg/storage/utils/decomposedfs/tree/tree.go index d239275ce1..a939268a5e 100644 --- a/pkg/storage/utils/decomposedfs/tree/tree.go +++ b/pkg/storage/utils/decomposedfs/tree/tree.go @@ -384,8 +384,50 @@ func (t *Tree) ListFolder(ctx context.Context, n *node.Node) ([]*node.Node, erro return nodes, nil } -// Delete deletes a node in the tree by moving it to the trash +// Delete deletes a node and all its children in the tree by moving them to the trash func (t *Tree) Delete(ctx context.Context, n *node.Node) (err error) { + nodes, err := appendChildren(ctx, n, nil) + if err != nil { + return err + } + + // traverse tree from leave to root + for i := len(nodes) - 1; i >= 0; i-- { + if err := t.delete(ctx, nodes[i]); err != nil { + return err + } + } + return nil +} + +// appendChildren appends `n` and all its children to `nodes` +func appendChildren(ctx context.Context, n *node.Node, nodes []*node.Node) ([]*node.Node, error) { + nodes = append(nodes, n) + + children, err := os.ReadDir(n.InternalPath()) + if err != nil { + // TODO: How to differentiate folders from files? + return nodes, nil + } + + for _, c := range children { + cn, err := n.Child(ctx, c.Name()) + if err != nil { + // continue? + return nil, err + } + nodes, err = appendChildren(ctx, cn, nodes) + if err != nil { + // continue? + return nil, err + } + } + + return nodes, nil +} + +// Delete deletes one node in the tree by moving it to the trash +func (t *Tree) delete(ctx context.Context, n *node.Node) (err error) { deletingSharedResource := ctx.Value(appctx.DeletingSharedResource) if deletingSharedResource != nil && deletingSharedResource.(bool) { From c112c59a6c108adeb4f33cf4e9d6f26b430943db Mon Sep 17 00:00:00 2001 From: jkoberg Date: Fri, 13 May 2022 13:25:14 +0200 Subject: [PATCH 2/5] revert trashing logic but use it when purging Signed-off-by: jkoberg --- pkg/storage/utils/decomposedfs/recycle.go | 14 ++- pkg/storage/utils/decomposedfs/tree/tree.go | 101 +++++++++++--------- 2 files changed, 71 insertions(+), 44 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/recycle.go b/pkg/storage/utils/decomposedfs/recycle.go index 36e3fea073..b6695532b9 100644 --- a/pkg/storage/utils/decomposedfs/recycle.go +++ b/pkg/storage/utils/decomposedfs/recycle.go @@ -288,11 +288,12 @@ func (fs *Decomposedfs) RestoreRecycleItem(ctx context.Context, ref *provider.Re return restoreFunc() } -// PurgeRecycleItem purges the specified item +// PurgeRecycleItem purges the specified item and all its children func (fs *Decomposedfs) PurgeRecycleItem(ctx context.Context, ref *provider.Reference, key, relativePath string) error { if ref == nil { return errtypes.BadRequest("missing reference, needs a space id") } + rn, purgeFunc, err := fs.tp.PurgeRecycleItemFunc(ctx, ref.ResourceId.OpaqueId, key, relativePath) if err != nil { if errors.Is(err, iofs.ErrNotExist) { @@ -321,6 +322,17 @@ func (fs *Decomposedfs) EmptyRecycle(ctx context.Context, ref *provider.Referenc if ref == nil || ref.ResourceId == nil || ref.ResourceId.OpaqueId == "" { return errtypes.BadRequest("spaceid must be set") } + + items, err := fs.ListRecycle(ctx, ref, "", "/") + if err != nil { + return err + } + + for _, i := range items { + if err := fs.PurgeRecycleItem(ctx, ref, i.Key, ""); err != nil { + return err + } + } // TODO what permission should we check? we could check the root node of the user? or the owner permissions on his home root node? // The current impl will wipe your own trash. or when no user provided the trash of 'root' return os.RemoveAll(fs.getRecycleRoot(ctx, ref.ResourceId.StorageId)) diff --git a/pkg/storage/utils/decomposedfs/tree/tree.go b/pkg/storage/utils/decomposedfs/tree/tree.go index a939268a5e..70cadde288 100644 --- a/pkg/storage/utils/decomposedfs/tree/tree.go +++ b/pkg/storage/utils/decomposedfs/tree/tree.go @@ -384,50 +384,8 @@ func (t *Tree) ListFolder(ctx context.Context, n *node.Node) ([]*node.Node, erro return nodes, nil } -// Delete deletes a node and all its children in the tree by moving them to the trash +// Delete deletes a node in the tree by moving it to the trash func (t *Tree) Delete(ctx context.Context, n *node.Node) (err error) { - nodes, err := appendChildren(ctx, n, nil) - if err != nil { - return err - } - - // traverse tree from leave to root - for i := len(nodes) - 1; i >= 0; i-- { - if err := t.delete(ctx, nodes[i]); err != nil { - return err - } - } - return nil -} - -// appendChildren appends `n` and all its children to `nodes` -func appendChildren(ctx context.Context, n *node.Node, nodes []*node.Node) ([]*node.Node, error) { - nodes = append(nodes, n) - - children, err := os.ReadDir(n.InternalPath()) - if err != nil { - // TODO: How to differentiate folders from files? - return nodes, nil - } - - for _, c := range children { - cn, err := n.Child(ctx, c.Name()) - if err != nil { - // continue? - return nil, err - } - nodes, err = appendChildren(ctx, cn, nodes) - if err != nil { - // continue? - return nil, err - } - } - - return nodes, nil -} - -// Delete deletes one node in the tree by moving it to the trash -func (t *Tree) delete(ctx context.Context, n *node.Node) (err error) { deletingSharedResource := ctx.Value(appctx.DeletingSharedResource) if deletingSharedResource != nil && deletingSharedResource.(bool) { @@ -580,6 +538,20 @@ func (t *Tree) PurgeRecycleItemFunc(ctx context.Context, spaceid, key string, pa return nil, nil, err } + // only the root node is trashed, the rest is still in normal file system + children, err := os.ReadDir(deletedNodePath) + var nodes []*node.Node + for _, c := range children { + n, _, _, _, err := t.readRecycleItem(ctx, spaceid, key, filepath.Join(path, c.Name())) + if err != nil { + return nil, nil, err + } + nodes, err = appendChildren(ctx, n, nodes) + if err != nil { + return nil, nil, err + } + } + fn := func() error { // delete the actual node // TODO recursively delete children @@ -602,6 +574,23 @@ func (t *Tree) PurgeRecycleItemFunc(ctx context.Context, spaceid, key string, pa return err } + // delete children + for i := len(nodes) - 1; i >= 0; i-- { + n := nodes[i] + if err := os.RemoveAll(n.InternalPath()); err != nil { + log.Error().Err(err).Str("deletedNodePath", deletedNodePath).Msg("error deleting trash node") + return err + } + if n.BlobID != "" { + if err = t.DeleteBlob(n); err != nil { + log.Error().Err(err).Str("trashItem", trashItem).Msg("error deleting item blob") + return err + } + + } + + } + return nil } @@ -908,3 +897,29 @@ func (t *Tree) readRecycleItem(ctx context.Context, spaceID, key, path string) ( return } + +// appendChildren appends `n` and all its children to `nodes` +func appendChildren(ctx context.Context, n *node.Node, nodes []*node.Node) ([]*node.Node, error) { + nodes = append(nodes, n) + + children, err := os.ReadDir(n.InternalPath()) + if err != nil { + // TODO: How to differentiate folders from files? + return nodes, nil + } + + for _, c := range children { + cn, err := n.Child(ctx, c.Name()) + if err != nil { + // continue? + return nil, err + } + nodes, err = appendChildren(ctx, cn, nodes) + if err != nil { + // continue? + return nil, err + } + } + + return nodes, nil +} From 82b7489433ee6bcc6ce510e7c845120832a1c60b Mon Sep 17 00:00:00 2001 From: jkoberg Date: Fri, 13 May 2022 13:33:34 +0200 Subject: [PATCH 3/5] changelog Signed-off-by: jkoberg --- changelog/unreleased/fix-blob-deletion.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/unreleased/fix-blob-deletion.md diff --git a/changelog/unreleased/fix-blob-deletion.md b/changelog/unreleased/fix-blob-deletion.md new file mode 100644 index 0000000000..5d24c94936 --- /dev/null +++ b/changelog/unreleased/fix-blob-deletion.md @@ -0,0 +1,6 @@ +Bugfix: Actually remove blobs when purging + +Blobs were not being deleted properly on purge. +Now if a folder gets purged all its children will be deleted + +https://github.com/cs3org/reva/pull/2868 From 192c3ffc5b1604741b14be2e509f36960d04ea81 Mon Sep 17 00:00:00 2001 From: jkoberg Date: Fri, 13 May 2022 14:30:50 +0200 Subject: [PATCH 4/5] remove empty parent folders when purging nodes Signed-off-by: jkoberg --- pkg/storage/fs/ocis/blobstore/blobstore.go | 4 ++-- pkg/storage/utils/decomposedfs/tree/tree.go | 4 ++-- pkg/utils/utils.go | 19 +++++++++++++++++++ 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/pkg/storage/fs/ocis/blobstore/blobstore.go b/pkg/storage/fs/ocis/blobstore/blobstore.go index 97dc6ca111..8fbdf30b44 100644 --- a/pkg/storage/fs/ocis/blobstore/blobstore.go +++ b/pkg/storage/fs/ocis/blobstore/blobstore.go @@ -26,6 +26,7 @@ import ( "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/lookup" "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/node" + "github.com/cs3org/reva/v2/pkg/utils" "github.com/pkg/errors" ) @@ -79,8 +80,7 @@ func (bs *Blobstore) Download(node *node.Node) (io.ReadCloser, error) { // Delete deletes a blob from the blobstore func (bs *Blobstore) Delete(node *node.Node) error { - err := os.Remove(bs.path(node)) - if err != nil { + if err := utils.RemoveItem(bs.path(node)); err != nil { return errors.Wrapf(err, "could not delete blob '%s'", bs.path(node)) } return nil diff --git a/pkg/storage/utils/decomposedfs/tree/tree.go b/pkg/storage/utils/decomposedfs/tree/tree.go index 70cadde288..22642b81d4 100644 --- a/pkg/storage/utils/decomposedfs/tree/tree.go +++ b/pkg/storage/utils/decomposedfs/tree/tree.go @@ -555,7 +555,7 @@ func (t *Tree) PurgeRecycleItemFunc(ctx context.Context, spaceid, key string, pa fn := func() error { // delete the actual node // TODO recursively delete children - if err := os.RemoveAll(deletedNodePath); err != nil { + if err := utils.RemoveItem(deletedNodePath); err != nil { log.Error().Err(err).Str("deletedNodePath", deletedNodePath).Msg("error deleting trash node") return err } @@ -577,7 +577,7 @@ func (t *Tree) PurgeRecycleItemFunc(ctx context.Context, spaceid, key string, pa // delete children for i := len(nodes) - 1; i >= 0; i-- { n := nodes[i] - if err := os.RemoveAll(n.InternalPath()); err != nil { + if err := utils.RemoveItem(n.InternalPath()); err != nil { log.Error().Err(err).Str("deletedNodePath", deletedNodePath).Msg("error deleting trash node") return err } diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index 9b3c7ffb29..3f049c9d45 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -23,6 +23,7 @@ import ( "net" "net/http" "net/url" + "os" "os/user" "path" "path/filepath" @@ -368,3 +369,21 @@ func ExistsInOpaque(o *types.Opaque, key string) bool { _, ok := o.Map[key] return ok } + +// RemoveItem removes the given item, its children and all empty parent folders +func RemoveItem(path string) error { + if err := os.RemoveAll(path); err != nil { + return err + } + + for { + path = filepath.Dir(path) + if err := os.Remove(path); err != nil { + // remove will fail when the dir is not empty. + // We can exit in that case + return nil + } + + } + +} From 1947a90c4918ac644ea4e425f75c8499c5652c2e Mon Sep 17 00:00:00 2001 From: jkoberg Date: Wed, 18 May 2022 16:32:32 +0200 Subject: [PATCH 5/5] delete revisions also Signed-off-by: jkoberg --- pkg/storage/utils/decomposedfs/node/node.go | 9 +++ pkg/storage/utils/decomposedfs/recycle.go | 2 +- pkg/storage/utils/decomposedfs/tree/tree.go | 68 ++++++++++++++------- 3 files changed, 57 insertions(+), 22 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/node/node.go b/pkg/storage/utils/decomposedfs/node/node.go index 0df5047d41..48645be677 100644 --- a/pkg/storage/utils/decomposedfs/node/node.go +++ b/pkg/storage/utils/decomposedfs/node/node.go @@ -1055,6 +1055,15 @@ func ReadBlobSizeAttr(path string) (int64, error) { return blobSize, nil } +// ReadBlobIDAttr reads the blobsize from the xattrs +func ReadBlobIDAttr(path string) (string, error) { + attr, err := xattrs.Get(path, xattrs.BlobIDAttr) + if err != nil { + return "", errors.Wrapf(err, "error reading blobid xattr") + } + return attr, nil +} + func (n *Node) hasUserShares(ctx context.Context) bool { g, err := n.ListGrantees(ctx) if err != nil { diff --git a/pkg/storage/utils/decomposedfs/recycle.go b/pkg/storage/utils/decomposedfs/recycle.go index b6695532b9..bc25cb4a01 100644 --- a/pkg/storage/utils/decomposedfs/recycle.go +++ b/pkg/storage/utils/decomposedfs/recycle.go @@ -288,7 +288,7 @@ func (fs *Decomposedfs) RestoreRecycleItem(ctx context.Context, ref *provider.Re return restoreFunc() } -// PurgeRecycleItem purges the specified item and all its children +// PurgeRecycleItem purges the specified item, all its children and all their revisions func (fs *Decomposedfs) PurgeRecycleItem(ctx context.Context, ref *provider.Reference, key, relativePath string) error { if ref == nil { return errtypes.BadRequest("missing reference, needs a space id") diff --git a/pkg/storage/utils/decomposedfs/tree/tree.go b/pkg/storage/utils/decomposedfs/tree/tree.go index 22642b81d4..e3461d78d8 100644 --- a/pkg/storage/utils/decomposedfs/tree/tree.go +++ b/pkg/storage/utils/decomposedfs/tree/tree.go @@ -553,21 +553,10 @@ func (t *Tree) PurgeRecycleItemFunc(ctx context.Context, spaceid, key string, pa } fn := func() error { - // delete the actual node - // TODO recursively delete children - if err := utils.RemoveItem(deletedNodePath); err != nil { - log.Error().Err(err).Str("deletedNodePath", deletedNodePath).Msg("error deleting trash node") + if err := t.removeNode(deletedNodePath, rn); err != nil { return err } - // delete blob from blobstore - if rn.BlobID != "" { - if err = t.DeleteBlob(rn); err != nil { - log.Error().Err(err).Str("trashItem", trashItem).Msg("error deleting trash item blob") - return err - } - } - // delete item link in trash if err = os.Remove(trashItem); err != nil { log.Error().Err(err).Str("trashItem", trashItem).Msg("error deleting trash item") @@ -577,17 +566,9 @@ func (t *Tree) PurgeRecycleItemFunc(ctx context.Context, spaceid, key string, pa // delete children for i := len(nodes) - 1; i >= 0; i-- { n := nodes[i] - if err := utils.RemoveItem(n.InternalPath()); err != nil { - log.Error().Err(err).Str("deletedNodePath", deletedNodePath).Msg("error deleting trash node") + if err := t.removeNode(n.InternalPath(), n); err != nil { return err } - if n.BlobID != "" { - if err = t.DeleteBlob(n); err != nil { - log.Error().Err(err).Str("trashItem", trashItem).Msg("error deleting item blob") - return err - } - - } } @@ -597,6 +578,51 @@ func (t *Tree) PurgeRecycleItemFunc(ctx context.Context, spaceid, key string, pa return rn, fn, nil } +func (t *Tree) removeNode(path string, n *node.Node) error { + // delete the actual node + if err := utils.RemoveItem(path); err != nil { + log.Error().Err(err).Str("path", path).Msg("error node") + return err + } + + // delete blob from blobstore + if n.BlobID != "" { + if err := t.DeleteBlob(n); err != nil { + log.Error().Err(err).Str("blobID", n.BlobID).Msg("error deleting nodes blob") + return err + } + } + + // delete revisions + revs, err := filepath.Glob(n.InternalPath() + node.RevisionIDDelimiter + "*") + if err != nil { + log.Error().Err(err).Str("path", n.InternalPath()+node.RevisionIDDelimiter+"*").Msg("glob failed badly") + return err + } + for _, rev := range revs { + bID, err := node.ReadBlobIDAttr(rev) + if err != nil { + log.Error().Err(err).Str("revision", rev).Msg("error reading blobid attribute") + return err + } + + if err := utils.RemoveItem(rev); err != nil { + log.Error().Err(err).Str("revision", rev).Msg("error removing revision node") + return err + } + + if bID != "" { + if err := t.DeleteBlob(&node.Node{SpaceID: n.SpaceID, BlobID: bID}); err != nil { + log.Error().Err(err).Str("revision", rev).Str("blobID", bID).Msg("error removing revision node blob") + return err + } + } + + } + + return nil +} + // Propagate propagates changes to the root of the tree func (t *Tree) Propagate(ctx context.Context, n *node.Node) (err error) { sublog := appctx.GetLogger(ctx).With().Interface("node", n).Logger()