fix: add ResolveRenameOp to handle directory renames

RenameNode was calling Resolve() which generated blob keys for
directories that don't have blobs, causing 'key not found' errors.

Added ResolveRenameOp to BlobKeyResolver interface:
- FlatKeyResolver returns nil (UUIDs don't change on rename)
- HierarchicalKeyResolver returns move op for files and directories

This allows directory renames to work correctly with flat storage,
and leverages os.Rename for atomic directory moves with hierarchical.

Co-authored-by: Ona <no-reply@ona.com>
This commit is contained in:
2026-01-04 23:58:43 +00:00
parent b8e1671248
commit d0c2a21ffd
5 changed files with 75 additions and 12 deletions

View File

@@ -301,8 +301,31 @@ func TestHTTP_CatalogEndpoints(t *testing.T) {
} }
}) })
// Note: Directory rename test skipped due to VFS bug where it tries to t.Run("rename directory", func(t *testing.T) {
// resolve blob keys for directories which don't have blobs. root, err := env.vfs.FindNode(ctx, env.db, env.rootDirID.String(), env.scope)
if err != nil {
t.Fatalf("FindNode root: %v", err)
}
renameDir, err := env.vfs.CreateDirectory(ctx, env.db, root.ID, "to-rename", env.scope)
if err != nil {
t.Fatalf("CreateDirectory to-rename: %v", err)
}
var resp struct {
Kind string `json:"kind"`
ID string `json:"id"`
Name string `json:"name"`
}
body := map[string]string{"name": "renamed-dir"}
doJSONAuth(t, env.app, http.MethodPatch, driveURL(fmt.Sprintf("/directories/%s", renameDir.PublicID)), env.accessToken, body, http.StatusOK, &resp)
if resp.Kind != "directory" {
t.Errorf("expected kind=directory, got %q", resp.Kind)
}
if resp.Name != "renamed-dir" {
t.Errorf("expected name=renamed-dir, got %q", resp.Name)
}
})
// File tests // File tests
t.Run("fetch file by ID", func(t *testing.T) { t.Run("fetch file by ID", func(t *testing.T) {

View File

@@ -45,3 +45,9 @@ func (r *FlatKeyResolver) ResolveBulkMoveOps(ctx context.Context, db bun.IDB, no
} }
return nil, nil return nil, nil
} }
// ResolveRenameOp returns nil for flat key storage since blob keys are UUIDs
// and don't change when nodes are renamed.
func (r *FlatKeyResolver) ResolveRenameOp(ctx context.Context, db bun.IDB, node *Node, newName string) (*BlobMoveOp, error) {
return nil, nil
}

View File

@@ -77,3 +77,34 @@ func (r *HierarchicalKeyResolver) ResolveBulkMoveOps(ctx context.Context, db bun
return ops, nil return ops, nil
} }
// ResolveRenameOp returns the blob move operation for renaming a node.
// For hierarchical storage, both files and directories need blob moves since
// the key is path-based. os.Rename handles directory moves atomically.
func (r *HierarchicalKeyResolver) ResolveRenameOp(ctx context.Context, db bun.IDB, node *Node, newName string) (*BlobMoveOp, error) {
oldPath, err := buildNodeAbsolutePathString(ctx, db, node)
if err != nil {
return nil, err
}
parentPath, err := buildPathFromNodeID(ctx, db, node.ParentID)
if err != nil {
return nil, err
}
var newPath string
if parentPath == "" {
newPath = newName
} else {
newPath = fmt.Sprintf("%s/%s", parentPath, newName)
}
oldKey := blob.Key(fmt.Sprintf("%s/%s", node.DriveID, oldPath))
newKey := blob.Key(fmt.Sprintf("%s/%s", node.DriveID, newPath))
return &BlobMoveOp{
Node: node,
OldKey: oldKey,
NewKey: newKey,
}, nil
}

View File

@@ -20,6 +20,12 @@ type BlobKeyResolver interface {
// Returns ErrBulkMoveRequiresSameParent if nodes don't all share the same parent. // Returns ErrBulkMoveRequiresSameParent if nodes don't all share the same parent.
// Returns nil, nil if no blob moves are needed (e.g., flat key storage where keys are UUIDs). // Returns nil, nil if no blob moves are needed (e.g., flat key storage where keys are UUIDs).
ResolveBulkMoveOps(ctx context.Context, db bun.IDB, nodes []*Node, newParentID uuid.UUID) ([]BlobMoveOp, error) ResolveBulkMoveOps(ctx context.Context, db bun.IDB, nodes []*Node, newParentID uuid.UUID) ([]BlobMoveOp, error)
// ResolveRenameOp returns the blob move operation needed when renaming a node.
// Returns nil, nil if no blob move is needed (e.g., flat key storage where keys are UUIDs).
// For hierarchical storage, returns a single move op for both files and directories
// (os.Rename handles directories atomically).
ResolveRenameOp(ctx context.Context, db bun.IDB, node *Node, newName string) (*BlobMoveOp, error)
} }
type DeletionPlan struct { type DeletionPlan struct {

View File

@@ -598,7 +598,8 @@ func (vfs *VirtualFS) RenameNode(ctx context.Context, db bun.IDB, node *Node, na
return ErrAccessDenied return ErrAccessDenied
} }
oldKey, err := vfs.keyResolver.Resolve(ctx, db, node) // Resolve rename operation before updating DB (needs old path)
renameOp, err := vfs.keyResolver.ResolveRenameOp(ctx, db, node, name)
if err != nil { if err != nil {
return err return err
} }
@@ -617,22 +618,18 @@ func (vfs *VirtualFS) RenameNode(ctx context.Context, db bun.IDB, node *Node, na
return err return err
} }
newKey, err := vfs.keyResolver.Resolve(ctx, db, node) // Execute blob move if needed
if err != nil { if renameOp != nil {
return err err = vfs.blobStore.Move(ctx, renameOp.OldKey, renameOp.NewKey)
}
if oldKey != newKey {
err = vfs.blobStore.Move(ctx, oldKey, newKey)
if err != nil { if err != nil {
return err return err
} }
if vfs.keyResolver.ShouldPersistKey() { if vfs.keyResolver.ShouldPersistKey() {
node.BlobKey = newKey node.BlobKey = renameOp.NewKey
_, err = db.NewUpdate().Model(node). _, err = db.NewUpdate().Model(node).
WherePK(). WherePK().
Set("blob_key = ?", newKey). Set("blob_key = ?", renameOp.NewKey).
Exec(ctx) Exec(ctx)
if err != nil { if err != nil {
return err return err