HADOOP-19445: ABFS: [FnsOverBlob][Tests] Add Tests For Negative Scena…

…rios Identified for Rename Operation (#7386) Contributed by Manish Bhatt Reviewed by Anmol Asrani, Anuj Modi Signed off by: Anuj Modi <[email protected]>
apache · Mar 5, 2025 · d552bb0 · d552bb0
1 parent 6b561d5
commit d552bb0
Show file tree

Hide file tree

Showing 7 changed files with 960 additions and 76 deletions.
diff --git a/...src/main/java/org/apache/hadoop/fs/azurebfs/contracts/services/AzureServiceErrorCode.java b/...src/main/java/org/apache/hadoop/fs/azurebfs/contracts/services/AzureServiceErrorCode.java
@@ -56,7 +56,6 @@ public enum AzureServiceErrorCode {
   OTHER_SERVER_THROTTLING("ServerBusy", HttpURLConnection.HTTP_UNAVAILABLE,
           "The server is currently unable to receive requests. Please retry your request."),
   INVALID_QUERY_PARAMETER_VALUE("InvalidQueryParameterValue", HttpURLConnection.HTTP_BAD_REQUEST, null),
-  INVALID_RENAME_DESTINATION("InvalidRenameDestinationPath", HttpURLConnection.HTTP_BAD_REQUEST, null),
   AUTHORIZATION_PERMISSION_MISS_MATCH("AuthorizationPermissionMismatch", HttpURLConnection.HTTP_FORBIDDEN, null),
   ACCOUNT_REQUIRES_HTTPS("AccountRequiresHttps", HttpURLConnection.HTTP_BAD_REQUEST, null),
   MD5_MISMATCH("Md5Mismatch", HttpURLConnection.HTTP_BAD_REQUEST,

diff --git a/...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java b/...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java
@@ -83,8 +83,8 @@
 import static java.net.HttpURLConnection.HTTP_NOT_FOUND;
 import static java.net.HttpURLConnection.HTTP_OK;
 import static java.net.HttpURLConnection.HTTP_PRECON_FAILED;
-import static org.apache.hadoop.fs.azurebfs.AzureBlobFileSystemStore.extractEtagHeader;
 import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_GET_FILE_STATUS;
+import static org.apache.hadoop.fs.azurebfs.AzureBlobFileSystemStore.extractEtagHeader;
 import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.ACQUIRE_LEASE_ACTION;
 import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.AND_MARK;
 import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.APPEND_BLOB_TYPE;
@@ -424,7 +424,7 @@ private void fixAtomicEntriesInListResults(final AbfsRestOperation op,
     }
     List<BlobListResultEntrySchema> filteredEntries = new ArrayList<>();
     for (BlobListResultEntrySchema entry : listResultSchema.paths()) {
-      if (!takeListPathAtomicRenameKeyAction(entry.path(),
+      if (!takeListPathAtomicRenameKeyAction(entry.path(), entry.isDirectory(),
           entry.contentLength().intValue(), tracingContext)) {
         filteredEntries.add(entry);
       }
@@ -442,17 +442,19 @@ public void createNonRecursivePreCheck(Path parentPath,
       if (isAtomicRenameKey(parentPath.toUri().getPath())) {
         takeGetPathStatusAtomicRenameKeyAction(parentPath, tracingContext);
       }
-      getPathStatus(parentPath.toUri().getPath(), false,
-          tracingContext, null);
+      try {
+        getPathStatus(parentPath.toUri().getPath(), false,
+            tracingContext, null);
+      } finally {
+        getAbfsCounters().incrementCounter(CALL_GET_FILE_STATUS, 1);
+      }
     } catch (AbfsRestOperationException ex) {
       if (ex.getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) {
         throw new FileNotFoundException("Cannot create file "
             + parentPath.toUri().getPath()
             + " because parent folder does not exist.");
       }
       throw ex;
-    } finally {
-      getAbfsCounters().incrementCounter(CALL_GET_FILE_STATUS, 1);
     }
   }
 
@@ -807,23 +809,26 @@ public AbfsClientRenameResult renamePath(final String source,
     BlobRenameHandler blobRenameHandler = getBlobRenameHandler(source,
         destination, sourceEtag, isAtomicRenameKey(source), tracingContext
     );
-    incrementAbfsRenamePath();
-    if (blobRenameHandler.execute()) {
-      final AbfsUriQueryBuilder abfsUriQueryBuilder
-          = createDefaultUriQueryBuilder();
-      final URL url = createRequestUrl(destination,
-          abfsUriQueryBuilder.toString());
-      final List<AbfsHttpHeader> requestHeaders = createDefaultHeaders();
-      final AbfsRestOperation successOp = getSuccessOp(
-          AbfsRestOperationType.RenamePath, HTTP_METHOD_PUT,
-          url, requestHeaders);
-      return new AbfsClientRenameResult(successOp, true, false);
-    } else {
-      throw new AbfsRestOperationException(HTTP_INTERNAL_ERROR,
-          AzureServiceErrorCode.UNKNOWN.getErrorCode(),
-          ERR_RENAME_BLOB + source + SINGLE_WHITE_SPACE + AND_MARK
-              + SINGLE_WHITE_SPACE + destination,
-          null);
+    try {
+      if (blobRenameHandler.execute()) {
+        final AbfsUriQueryBuilder abfsUriQueryBuilder
+            = createDefaultUriQueryBuilder();
+        final URL url = createRequestUrl(destination,
+            abfsUriQueryBuilder.toString());
+        final List<AbfsHttpHeader> requestHeaders = createDefaultHeaders();
+        final AbfsRestOperation successOp = getSuccessOp(
+            AbfsRestOperationType.RenamePath, HTTP_METHOD_PUT,
+            url, requestHeaders);
+        return new AbfsClientRenameResult(successOp, true, false);
+      } else {
+        throw new AbfsRestOperationException(HTTP_INTERNAL_ERROR,
+            AzureServiceErrorCode.UNKNOWN.getErrorCode(),
+            ERR_RENAME_BLOB + source + SINGLE_WHITE_SPACE + AND_MARK
+                + SINGLE_WHITE_SPACE + destination,
+            null);
+      }
+    } finally {
+      incrementAbfsRenamePath();
     }
   }
 
@@ -1801,11 +1806,11 @@ public void takeGetPathStatusAtomicRenameKeyAction(final Path path,
    * @throws AzureBlobFileSystemException server error
    */
   private boolean takeListPathAtomicRenameKeyAction(final Path path,
-      final int renamePendingJsonLen,
+      final boolean isDirectory, final int renamePendingJsonLen,
       final TracingContext tracingContext)
       throws AzureBlobFileSystemException {
     if (path == null || path.isRoot() || !isAtomicRenameKey(
-        path.toUri().getPath()) || !path.toUri()
+        path.toUri().getPath()) || isDirectory || !path.toUri()
         .getPath()
         .endsWith(RenameAtomicity.SUFFIX)) {
       return false;
@@ -1833,7 +1838,7 @@ private boolean takeListPathAtomicRenameKeyAction(final Path path,
   }
 
   @VisibleForTesting
-  RenameAtomicity getRedoRenameAtomicity(final Path renamePendingJsonPath,
+  public RenameAtomicity getRedoRenameAtomicity(final Path renamePendingJsonPath,
       int fileLen,
       final TracingContext tracingContext) {
     return new RenameAtomicity(renamePendingJsonPath,

diff --git a/.../hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/BlobRenameHandler.java b/.../hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/BlobRenameHandler.java
@@ -31,7 +31,6 @@
 
 import org.apache.hadoop.classification.VisibleForTesting;
 import org.apache.hadoop.fs.Path;
-import org.apache.hadoop.fs.PathIOException;
 import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AbfsRestOperationException;
 import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AzureBlobFileSystemException;
 import org.apache.hadoop.fs.azurebfs.contracts.exceptions.TimeoutException;
@@ -257,36 +256,14 @@ private boolean containsColon(Path p) {
   private boolean preCheck(final Path src, final Path dst,
       final PathInformation pathInformation)
       throws AzureBlobFileSystemException {
-    validateDestinationPath(src, dst);
+    validateDestinationIsNotSubDir(src, dst);
     validateSourcePath(pathInformation);
     validateDestinationPathNotExist(src, dst, pathInformation);
     validateDestinationParentExist(src, dst, pathInformation);
 
     return true;
   }
 
-  /**
-   * Validate if the format of the destination path is correct and if the destination
-   * path is not a sub-directory of the source path.
-   *
-   * @param src source path
-   * @param dst destination path
-   *
-   * @throws AbfsRestOperationException if the destination path is invalid
-   */
-  private void validateDestinationPath(final Path src, final Path dst)
-      throws AbfsRestOperationException {
-    if (containsColon(dst)) {
-      throw new AbfsRestOperationException(
-          HttpURLConnection.HTTP_BAD_REQUEST,
-          AzureServiceErrorCode.INVALID_RENAME_DESTINATION.getErrorCode(), null,
-          new PathIOException(dst.toUri().getPath(),
-              "Destination path contains colon"));
-    }
-
-    validateDestinationIsNotSubDir(src, dst);
-  }
-
   /**
    * Validate if the destination path is not a sub-directory of the source path.
    *

diff --git a/...ls/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ListActionTaker.java b/...ls/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ListActionTaker.java
@@ -35,6 +35,7 @@
 import org.apache.hadoop.fs.azurebfs.AbfsConfiguration;
 import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AbfsRestOperationException;
 import org.apache.hadoop.fs.azurebfs.contracts.exceptions.AzureBlobFileSystemException;
+import org.apache.hadoop.fs.azurebfs.contracts.exceptions.FileSystemOperationUnhandledException;
 import org.apache.hadoop.fs.azurebfs.contracts.exceptions.InvalidConfigurationValueException;
 import org.apache.hadoop.fs.azurebfs.contracts.services.BlobListResultSchema;
 import org.apache.hadoop.fs.azurebfs.contracts.services.ListResultEntrySchema;
@@ -119,7 +120,14 @@ private boolean takeAction(List<Path> paths)
         LOG.debug("Thread interrupted while taking action on path: {}",
             path.toUri().getPath());
       } catch (ExecutionException e) {
-        executionException = (AzureBlobFileSystemException) e.getCause();
+        LOG.debug("Execution exception while taking action on path: {}",
+            path.toUri().getPath());
+        if (e.getCause() instanceof AzureBlobFileSystemException) {
+          executionException = (AzureBlobFileSystemException) e.getCause();
+        } else {
+          executionException =
+              new FileSystemOperationUnhandledException(executionException);
+        }
       }
     }
     if (executionException != null) {
@@ -261,7 +269,7 @@ protected String listAndEnqueue(final ListBlobQueue listBlobQueue,
   protected void addPaths(final List<Path> paths,
       final ListResultSchema retrievedSchema) {
     for (ListResultEntrySchema entry : retrievedSchema.paths()) {
-      Path entryPath = new Path(ROOT_PATH, entry.name());
+      Path entryPath = new Path(ROOT_PATH + entry.name());
       if (!entryPath.equals(this.path)) {
         paths.add(entryPath);
       }

diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/utils/UriUtils.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/utils/UriUtils.java
@@ -248,8 +248,13 @@ private static String replacedUrl(String baseUrl, String oldString, String newSt
    */
   public static boolean isKeyForDirectorySet(String key, Set<String> dirSet) {
     for (String dir : dirSet) {
-      if (dir.isEmpty() || key.startsWith(
-          dir + AbfsHttpConstants.FORWARD_SLASH)) {
+      // Ensure the directory ends with a forward slash
+      if (StringUtils.isNotEmpty(dir)
+          && !dir.endsWith(AbfsHttpConstants.FORWARD_SLASH)) {
+        dir += AbfsHttpConstants.FORWARD_SLASH;
+      }
+      // Return true if the directory is empty or the key starts with the directory
+      if (dir.isEmpty() || key.startsWith(dir)) {
         return true;
       }