Catch an object named after the root with a trailing / & carefully …

…handle trailing `/` files on sync (#2847)
Azure · Oct 29, 2024 · 572a58d · 572a58d
1 parent 7a50237
commit 572a58d
Show file tree

Hide file tree

Showing 3 changed files with 114 additions and 16 deletions.
diff --git a/cmd/syncProcessor.go b/cmd/syncProcessor.go
@@ -278,6 +278,18 @@ func (b *remoteResourceDeleter) delete(object StoredObject) error {
 		b.clientOptions.PerCallPolicies = append([]policy.Policy{common.NewRecursivePolicy()}, b.clientOptions.PerCallPolicies...)
 	}
 	*/
+	objectPath := path.Join(b.rootPath, object.relativePath)
+	if object.relativePath == "\x00" && b.targetLocation != common.ELocation.Blob() {
+		return nil // Do nothing, we don't want to accidentally delete the root.
+	} else if object.relativePath == "\x00" { // this is acceptable on blob, though. Dir stubs are a thing, and they aren't necessary for normal function.
+		objectPath = b.rootPath
+	}
+
+	if strings.HasSuffix(object.relativePath, "/") && !strings.HasSuffix(objectPath, "/") && b.targetLocation == common.ELocation.Blob() {
+		// If we were targeting a directory, we still need to be. path.join breaks that.
+		// We also want to defensively code around this, and make sure we are not putting folder// or trying to put a weird URI in to an endpoint that can't do this.
+		objectPath += "/"
+	}
 
 	sc := b.remoteClient
 	if object.entityType == common.EEntityType.File() {
@@ -294,7 +306,7 @@ func (b *remoteResourceDeleter) delete(object StoredObject) error {
 		switch b.targetLocation {
 		case common.ELocation.Blob():
 			bsc, _ := sc.BlobServiceClient()
-			var blobClient *blob.Client = bsc.NewContainerClient(b.containerName).NewBlobClient(path.Join(b.rootPath, object.relativePath))
+			var blobClient *blob.Client = bsc.NewContainerClient(b.containerName).NewBlobClient(objectPath)
 
 			objURL, err = b.getObjectURL(blobClient.URL())
 			if err != nil {
@@ -306,7 +318,7 @@ func (b *remoteResourceDeleter) delete(object StoredObject) error {
 			_, err = blobClient.Delete(b.ctx, nil)
 		case common.ELocation.File():
 			fsc, _ := sc.FileServiceClient()
-			fileClient := fsc.NewShareClient(b.containerName).NewRootDirectoryClient().NewFileClient(path.Join(b.rootPath, object.relativePath))
+			fileClient := fsc.NewShareClient(b.containerName).NewRootDirectoryClient().NewFileClient(objectPath)
 
 			objURL, err = b.getObjectURL(fileClient.URL())
 			if err != nil {
@@ -320,7 +332,7 @@ func (b *remoteResourceDeleter) delete(object StoredObject) error {
 			}, fileClient, b.forceIfReadOnly)
 		case common.ELocation.BlobFS():
 			dsc, _ := sc.DatalakeServiceClient()
-			fileClient := dsc.NewFileSystemClient(b.containerName).NewFileClient(path.Join(b.rootPath, object.relativePath))
+			fileClient := dsc.NewFileSystemClient(b.containerName).NewFileClient(objectPath)
 
 			objURL, err = b.getObjectURL(fileClient.DFSURL())
 			if err != nil {
@@ -356,7 +368,7 @@ func (b *remoteResourceDeleter) delete(object StoredObject) error {
 		switch b.targetLocation {
 		case common.ELocation.Blob():
 			bsc, _ := sc.BlobServiceClient()
-			blobClient := bsc.NewContainerClient(b.containerName).NewBlobClient(path.Join(b.rootPath, object.relativePath))
+			blobClient := bsc.NewContainerClient(b.containerName).NewBlobClient(objectPath)
 			// HNS endpoint doesn't like delete snapshots on a directory
 			objURL, err = b.getObjectURL(blobClient.URL())
 			if err != nil {
@@ -369,7 +381,7 @@ func (b *remoteResourceDeleter) delete(object StoredObject) error {
 			}
 		case common.ELocation.File():
 			fsc, _ := sc.FileServiceClient()
-			dirClient := fsc.NewShareClient(b.containerName).NewDirectoryClient(path.Join(b.rootPath, object.relativePath))
+			dirClient := fsc.NewShareClient(b.containerName).NewDirectoryClient(objectPath)
 			objURL, err = b.getObjectURL(dirClient.URL())
 			if err != nil {
 				return err
@@ -383,7 +395,7 @@ func (b *remoteResourceDeleter) delete(object StoredObject) error {
 			}
 		case common.ELocation.BlobFS():
 			dsc, _ := sc.DatalakeServiceClient()
-			directoryClient := dsc.NewFileSystemClient(b.containerName).NewDirectoryClient(path.Join(b.rootPath, object.relativePath))
+			directoryClient := dsc.NewFileSystemClient(b.containerName).NewDirectoryClient(objectPath)
 			objURL, err = b.getObjectURL(directoryClient.DFSURL())
 			if err != nil {
 				return err

diff --git a/cmd/zc_traverser_blob.go b/cmd/zc_traverser_blob.go
@@ -419,6 +419,11 @@ func (t *blobTraverser) parallelList(containerClient *container.Client, containe
 
 				storedObject := t.createStoredObjectForBlob(preprocessor, blobInfo, strings.TrimPrefix(*blobInfo.Name, searchPrefix), containerName)
 
+				// edge case, blob name happens to be the same as root and ends in /
+				if storedObject.relativePath == "" && strings.HasSuffix(storedObject.name, "/") {
+					storedObject.relativePath = "\x00" // Short circuit, letting the backend know we *really* meant root/.
+				}
+
 				if t.s2sPreserveSourceTags && blobInfo.BlobTags != nil {
 					blobTagsMap := common.BlobTags{}
 					for _, blobTag := range blobInfo.BlobTags.BlobTagSet {
@@ -554,6 +559,11 @@ func (t *blobTraverser) serialList(containerClient *container.Client, containerN
 
 			storedObject := t.createStoredObjectForBlob(preprocessor, blobInfo, relativePath, containerName)
 
+			// edge case, blob name happens to be the same as root and ends in /
+			if storedObject.relativePath == "" && strings.HasSuffix(storedObject.name, "/") {
+				storedObject.relativePath = "\x00" // Short circuit, letting the backend know we *really* meant root/.
+			}
+
 			// Setting blob tags
 			if t.s2sPreserveSourceTags && blobInfo.BlobTags != nil {
 				blobTagsMap := common.BlobTags{}

diff --git a/e2etest/zt_newe2e_fns_dir_test.go b/e2etest/zt_newe2e_fns_dir_test.go
@@ -15,15 +15,16 @@ func init() {
 }
 
 func (*FNSSuite) Scenario_CopyToOverlappableDirectoryMarker(a *ScenarioVariationManager) {
+	DirMeta := ResolveVariation(a, []string{"", common.POSIXFolderMeta})
 	tgtVerb := ResolveVariation(a, []AzCopyVerb{AzCopyVerbCopy, AzCopyVerbSync})
 
 	// Target a fns account
 	destRm := ObjectResourceMappingFlat{
 		"foobar/": ResourceDefinitionObject{
 			ObjectProperties: ObjectProperties{
-				Metadata: common.Metadata{
+				Metadata: common.Iff(DirMeta != "", common.Metadata{
 					common.POSIXFolderMeta: pointerTo("true"),
-				},
+				}, nil),
 			},
 			Body: NewZeroObjectContentContainer(0),
 		},
@@ -71,9 +72,9 @@ func (*FNSSuite) Scenario_CopyToOverlappableDirectoryMarker(a *ScenarioVariation
 			},
 			"foobar/": ResourceDefinitionObject{
 				ObjectProperties: ObjectProperties{
-					Metadata: common.Metadata{
+					Metadata: common.Iff(DirMeta != "", common.Metadata{
 						common.POSIXFolderMeta: pointerTo("true"),
-					},
+					}, nil),
 				},
 				ObjectShouldExist: pointerTo(true),
 			},
@@ -83,15 +84,22 @@ func (*FNSSuite) Scenario_CopyToOverlappableDirectoryMarker(a *ScenarioVariation
 
 // Scenario_IncludeRootDirectoryStub tests that the root directory (and sub directories) appropriately get their files picked up.
 func (*FNSSuite) Scenario_IncludeRootDirectoryStub(a *ScenarioVariationManager) {
+	DirMeta := ResolveVariation(a, []string{"", common.POSIXFolderMeta})
+
 	dst := CreateResource[ContainerResourceManager](a, GetRootResource(a, common.ELocation.Blob()), ResourceDefinitionContainer{})
 	src := CreateResource[ContainerResourceManager](a, GetRootResource(a, common.ELocation.Blob()), ResourceDefinitionContainer{
 		Objects: ObjectResourceMappingFlat{
-			"foobar":               ResourceDefinitionObject{Body: NewRandomObjectContentContainer(512), ObjectProperties: ObjectProperties{Metadata: common.Metadata{"dontcopyme": pointerTo("")}}}, // Object w/ same name as root dir
-			"foobar/":              ResourceDefinitionObject{ObjectProperties: ObjectProperties{EntityType: common.EEntityType.Folder(), Metadata: common.Metadata{"asdf": pointerTo("qwerty")}}},    // Folder w/ same name as object, add special prop to ensure
+			"foobar": ResourceDefinitionObject{Body: NewRandomObjectContentContainer(512), ObjectProperties: ObjectProperties{Metadata: common.Metadata{"dontcopyme": pointerTo("")}}}, // Object w/ same name as root dir
+			"foobar/": ResourceDefinitionObject{
+				ObjectProperties: ObjectProperties{
+					EntityType: common.Iff(DirMeta != "", common.EEntityType.Folder(), common.EEntityType.File()),
+					Metadata:   common.Metadata{"asdf": pointerTo("qwerty")},
+				},
+			}, // Folder w/ same name as object, add special prop to ensure
 			"foobar/foo":           ResourceDefinitionObject{Body: NewZeroObjectContentContainer(0)},
 			"foobar/bar":           ResourceDefinitionObject{Body: NewZeroObjectContentContainer(0)},
 			"foobar/baz":           ResourceDefinitionObject{Body: NewZeroObjectContentContainer(0)},
-			"foobar/folder/":       ResourceDefinitionObject{ObjectProperties: ObjectProperties{EntityType: common.EEntityType.Folder()}},
+			"foobar/folder/":       ResourceDefinitionObject{ObjectProperties: ObjectProperties{EntityType: common.Iff(DirMeta != "", common.EEntityType.Folder(), common.EEntityType.File())}},
 			"foobar/folder/foobar": ResourceDefinitionObject{Body: NewZeroObjectContentContainer(0)},
 		},
 	})
@@ -116,13 +124,81 @@ func (*FNSSuite) Scenario_IncludeRootDirectoryStub(a *ScenarioVariationManager)
 
 	ValidateResource(a, dst, ResourceDefinitionContainer{
 		Objects: ObjectResourceMappingFlat{
-			"foobar":               ResourceDefinitionObject{ObjectShouldExist: pointerTo(false)}, // We shouldn't have captured foobar, but foobar/ should exist as a directory.
-			"foobar/":              ResourceDefinitionObject{ObjectProperties: ObjectProperties{EntityType: common.EEntityType.Folder(), Metadata: common.Metadata{common.POSIXFolderMeta: pointerTo("true"), "asdf": pointerTo("qwerty")}}},
+			"foobar": ResourceDefinitionObject{ObjectShouldExist: pointerTo(false)}, // We shouldn't have captured foobar, but foobar/ should exist as a directory.
+			"foobar/": ResourceDefinitionObject{ObjectProperties: ObjectProperties{
+				EntityType: common.Iff(DirMeta != "", common.EEntityType.Folder(), common.EEntityType.File()),
+				Metadata: common.Metadata{
+					"asdf": pointerTo("qwerty"),
+				},
+			},
+			},
 			"foobar/foo":           ResourceDefinitionObject{Body: NewZeroObjectContentContainer(0)},
 			"foobar/bar":           ResourceDefinitionObject{Body: NewZeroObjectContentContainer(0)},
 			"foobar/baz":           ResourceDefinitionObject{Body: NewZeroObjectContentContainer(0)},
-			"foobar/folder/":       ResourceDefinitionObject{ObjectProperties: ObjectProperties{EntityType: common.EEntityType.Folder()}},
+			"foobar/folder/":       ResourceDefinitionObject{ObjectProperties: ObjectProperties{EntityType: common.Iff(DirMeta != "", common.EEntityType.Folder(), common.EEntityType.File())}},
 			"foobar/folder/foobar": ResourceDefinitionObject{Body: NewZeroObjectContentContainer(0)},
 		},
 	}, false)
 }
+
+/*
+Scenario_SyncTrailingSlashDeletion tests against a potential accidental deletion bug that could occur when `folder/` exists at the destination, but not the source
+and `folder/` happens to have an overlapping file at `folder`.
+*/
+func (*FNSSuite) Scenario_SyncTrailingSlashDeletion(a *ScenarioVariationManager) {
+	folderStyle := ResolveVariation(a, []common.EntityType{common.EEntityType.File(), common.EEntityType.Folder()})
+
+	dest := CreateResource[ContainerResourceManager](a, GetRootResource(a, common.ELocation.Blob()), ResourceDefinitionContainer{
+		Objects: ObjectResourceMappingFlat{
+			"foobar": ResourceDefinitionObject{
+				Body: NewRandomObjectContentContainer(1024),
+			},
+			"foobar/": ResourceDefinitionObject{
+				ObjectProperties: ObjectProperties{
+					EntityType: folderStyle,
+				},
+			},
+			"foobar/bar/": ResourceDefinitionObject{
+				Body: NewRandomObjectContentContainer(1024),
+			},
+		},
+	})
+
+	src := CreateResource[ContainerResourceManager](a, GetRootResource(a, common.ELocation.Blob()), ResourceDefinitionContainer{
+		Objects: ObjectResourceMappingFlat{
+			"foobar": ResourceDefinitionObject{
+				Body: NewRandomObjectContentContainer(1024),
+			}, // We don't care about anything other than the overlap. We merely want to trigger a delete op against dest's folder/.
+		},
+	})
+
+	RunAzCopy(a, AzCopyCommand{
+		Verb: AzCopyVerbSync,
+		Targets: []ResourceManager{
+			src.GetObject(a, "foobar/", common.EEntityType.Folder()),
+			dest.GetObject(a, "foobar/", common.EEntityType.Folder()),
+		},
+		Flags: SyncFlags{
+			CopySyncCommonFlags: CopySyncCommonFlags{
+				Recursive: pointerTo(true),
+				GlobalFlags: GlobalFlags{
+					OutputType: pointerTo(common.EOutputFormat.Text()),
+				},
+				IncludeDirectoryStubs: pointerTo(true),
+			},
+			DeleteDestination: pointerTo(true),
+		},
+	})
+
+	ValidateResource(a, dest, ResourceDefinitionContainer{
+		Objects: ObjectResourceMappingFlat{
+			"foobar": ResourceDefinitionObject{}, // We just care this guy exists
+			"foobar/": ResourceDefinitionObject{ // and this guy doesn't.
+				ObjectShouldExist: pointerTo(false),
+			},
+			"foobar/bar/": ResourceDefinitionObject{
+				ObjectShouldExist: pointerTo(false),
+			},
+		},
+	}, false)
+}