HDDS-13444. Unify isValidKeyPath implementation

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OzoneFSUtils.java

@@ -19,13 +19,15 @@
 
 import static org.apache.hadoop.ozone.OzoneConsts.OM_KEY_PREFIX;
 import static org.apache.hadoop.ozone.OzoneConsts.OZONE_URI_DELIMITER;
+import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.INVALID_KEY_NAME;
 
 import jakarta.annotation.Nonnull;
 import java.nio.file.Paths;
 import java.util.UUID;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.hdds.conf.ConfigurationSource;
 import org.apache.hadoop.ozone.OzoneConfigKeys;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
 import org.apache.hadoop.util.StringUtils;
 import org.apache.hadoop.util.Time;
 import org.slf4j.Logger;
@@ -97,34 +99,81 @@ public static boolean isFile(String keyName) {
   }
 
   /**
-   * Whether the pathname is valid.  Currently prohibits relative paths,
-   * names which contain a ":" or "//", or other non-canonical paths.
+   * Core validation logic for key path components.
+   * Checks for invalid characters: ".", "..", ":", "/", and "//" in the middle.
+   *
+   * @param path the path to validate
+   * @param allowLeadingSlash whether leading slash is allowed (true) or invalid (false)
+   * @return true if path components are valid, false otherwise
    */
-  public static boolean isValidName(String src) {
-    // Path must be absolute.
-    if (!src.startsWith(Path.SEPARATOR)) {
-      return false;
+  private static boolean validateKeyPathComponents(String path, boolean allowLeadingSlash) {
+    if (allowLeadingSlash) {
+      if (!path.startsWith(Path.SEPARATOR)) {
+        return false;
+      }
+    } else {
+      if (path.startsWith(Path.SEPARATOR)) {
+        return false;
+      }
     }
 
-    // Check for ".." "." ":" "/"
-    String[] components = StringUtils.split(src, '/');
+    String[] components = StringUtils.split(path, '/');
     for (int i = 0; i < components.length; i++) {
       String element = components[i];
-      if (element.equals(".")  ||
-          (element.contains(":"))  ||
-          (element.contains("/") || element.equals(".."))) {
+      if (element.equals(".") ||
+          element.contains(":") ||
+          element.contains("/") ||
+          element.equals("..")) {
         return false;
       }
-      // The string may start or end with a /, but not have
-      // "//" in the middle.
-      if (element.isEmpty() && i != components.length - 1 &&
-          i != 0) {
+
+      // The string may end with a /, but not have "//" in the middle.
+      if (element.isEmpty() && i != components.length - 1) {
+        // Allow empty at start for absolute paths (e.g., "/a/b" → ["", "a", "b"])
+        // This is needed for isValidName but not for isValidKeyPath (which rejects leading slashes)
+        if (allowLeadingSlash && i == 0) {
+          continue;
+        }
         return false;
       }
     }
     return true;
   }
 
+  /**
+   * Whether the pathname is valid.  Currently prohibits relative paths,
+   * names which contain a ":" or "//", or other non-canonical paths.
+   */
+  public static boolean isValidName(String path) {
+    return validateKeyPathComponents(path, true);
+  }
+
+  /**
+   * Whether the pathname is valid.  Check key names which contain a
+   * ":", ".", "..", "//", "". If it has any of these characters throws
+   * OMException, else return the path.
+   *
+   * @param path the path to validate
+   * @param throwOnEmpty whether to throw exception if path is empty
+   * @return the path if valid
+   * @throws OMException if path is invalid
+   */
+  public static String isValidKeyPath(String path, boolean throwOnEmpty) throws OMException {
+    if (path.isEmpty()) {
+      if (throwOnEmpty) {
+        throw new OMException("Invalid KeyPath, empty keyName" + path,
+            INVALID_KEY_NAME);
+      }
+      return path;
+    }
+
+    if (!validateKeyPathComponents(path, false)) {
+      throw new OMException("Invalid KeyPath " + path, INVALID_KEY_NAME);
+    }
+
+    return path;
+  }
+
   /**
    * Checks whether the bucket layout is valid for File System operations
    * otherwise throws IllegalArgumentException.

hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOzoneFsUtils.java

@@ -19,10 +19,12 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import org.apache.hadoop.hdds.conf.OzoneConfiguration;
 import org.apache.hadoop.ozone.OzoneConfigKeys;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.CsvSource;
@@ -67,4 +69,40 @@ void testCanEnableHsync(boolean canEnableHsync,
 
     assertEquals(canEnableHsync, OzoneFSUtils.canEnableHsync(conf, isClient));
   }
+
+  @Test
+  public void testIsValidKeyPath() throws OMException {
+    // Valid paths
+    assertEquals("a/b/c", OzoneFSUtils.isValidKeyPath("a/b/c", true));
+    assertEquals("a/b/c", OzoneFSUtils.isValidKeyPath("a/b/c", false));
+    assertEquals("file", OzoneFSUtils.isValidKeyPath("file", true));
+    assertEquals("dir/subdir/file", OzoneFSUtils.isValidKeyPath("dir/subdir/file", true));
+
+    // Empty path - throwOnEmpty=true should throw exception
+    assertThrows(OMException.class, () -> OzoneFSUtils.isValidKeyPath("", true));
+
+    // Empty path - throwOnEmpty=false should return empty string
+    assertEquals("", OzoneFSUtils.isValidKeyPath("", false));
+
+    // Invalid paths - leading slash
+    assertThrows(OMException.class, () -> OzoneFSUtils.isValidKeyPath("/a/b", true));
+    assertThrows(OMException.class, () -> OzoneFSUtils.isValidKeyPath("/a/b", false));
+
+    // Invalid paths - contains ".."
+    assertThrows(OMException.class, () -> OzoneFSUtils.isValidKeyPath("a/../b", true));
+    assertThrows(OMException.class, () -> OzoneFSUtils.isValidKeyPath("../a/b", true));
+
+    // Invalid paths - contains "."
+    assertThrows(OMException.class, () -> OzoneFSUtils.isValidKeyPath("a/./b", true));
+
+    // Invalid paths - contains ":"
+    assertThrows(OMException.class, () -> OzoneFSUtils.isValidKeyPath("a:b", true));
+    assertThrows(OMException.class, () -> OzoneFSUtils.isValidKeyPath("a/b:c", true));
+
+    // Invalid paths - contains "//" in the middle
+    assertThrows(OMException.class, () -> OzoneFSUtils.isValidKeyPath("a//b", true));
+
+    // Valid path ending with "/"
+    assertEquals("a/b/", OzoneFSUtils.isValidKeyPath("a/b/", true));
+  }
 }

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/OMClientRequest.java

@@ -17,7 +17,6 @@
 
 package org.apache.hadoop.ozone.om.request;
 
-import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.INVALID_KEY_NAME;
 import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.UNAUTHORIZED;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -45,6 +44,7 @@
 import org.apache.hadoop.ozone.om.execution.flowcontrol.ExecutionContext;
 import org.apache.hadoop.ozone.om.helpers.BucketLayout;
 import org.apache.hadoop.ozone.om.helpers.OMAuditLogger;
+import org.apache.hadoop.ozone.om.helpers.OzoneFSUtils;
 import org.apache.hadoop.ozone.om.lock.OMLockDetails;
 import org.apache.hadoop.ozone.om.protocolPB.grpc.GrpcClientConstants;
 import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerRatisUtils;
@@ -94,8 +94,7 @@ public enum Result {
   }
 
   public OMClientRequest(OMRequest omRequest) {
-    Objects.requireNonNull(omRequest, "omRequest == null");
-    this.omRequest = omRequest;
+    this.omRequest = Objects.requireNonNull(omRequest);
     this.omLockDetails.clear();
   }
 
@@ -566,39 +565,7 @@ public static String validateAndNormalizeKey(String keyName)
    * OMException, else return the path.
    */
   public static String isValidKeyPath(String path) throws OMException {
-    boolean isValid = true;
-
-    // If keyName is empty string throw error.
-    if (path.isEmpty()) {
-      throw new OMException("Invalid KeyPath, empty keyName" + path,
-          INVALID_KEY_NAME);
-    } else if (path.startsWith("/")) {
-      isValid = false;
-    } else {
-      // Check for ".." "." ":" "/"
-      String[] components = StringUtils.split(path, '/');
-      for (int i = 0; i < components.length; i++) {
-        String element = components[i];
-        if (element.equals(".") ||
-            (element.contains(":")) ||
-            (element.contains("/") || element.equals(".."))) {
-          isValid = false;
-          break;
-        }
-
-        // The string may end with a /, but not have
-        // "//" in the middle.
-        if (element.isEmpty() && i != components.length - 1) {
-          isValid = false;
-        }
-      }
-    }
-
-    if (isValid) {
-      return path;
-    } else {
-      throw new OMException("Invalid KeyPath " + path, INVALID_KEY_NAME);
-    }
+    return OzoneFSUtils.isValidKeyPath(path, true);
   }
 
   public OMLockDetails getOmLockDetails() {