sreejasahithi commented on code in PR #9521:
URL: https://github.com/apache/ozone/pull/9521#discussion_r2665080166
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OzoneFSUtils.java:
##########
@@ -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,
Review Comment:
```suggestion
throw new OMException("Invalid KeyPath, empty keyName " + path,
```
or we could instead remove path from the message as it is empty and the
message already says empty KeyName.
##########
hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOzoneFsUtils.java:
##########
@@ -67,4 +69,40 @@ void testCanEnableHsync(boolean canEnableHsync,
assertEquals(canEnableHsync, OzoneFSUtils.canEnableHsync(conf, isClient));
}
+
+ @Test
+ public void testIsValidKeyPath() throws OMException {
Review Comment:
you can add some more test cases for key paths like`file.txt`, `./file`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
