adoroszlai commented on code in PR #8453:
URL: https://github.com/apache/ozone/pull/8453#discussion_r2200046856
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/OMClientRequest.java:
##########
@@ -521,17 +521,37 @@ public static String validateAndNormalizeKey(boolean
enableFileSystemPaths,
}
}
+ /**
+ * Normalizes the key path based on the bucket layout.
+ *
+ * @return normalized key path
+ */
+
Review Comment:
nit:
```suggestion
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/OMClientRequest.java:
##########
@@ -521,17 +521,37 @@ public static String validateAndNormalizeKey(boolean
enableFileSystemPaths,
}
}
+ /**
+ * Normalizes the key path based on the bucket layout.
+ *
+ * @return normalized key path
+ */
+
+ public static String normalizeKeyPath(boolean enableFileSystemPaths,
+ String keyPath, BucketLayout bucketLayout) throws OMException {
+ LOG.debug("Bucket Layout: {}", bucketLayout);
+ if (bucketLayout.shouldNormalizePaths(enableFileSystemPaths)) {
+ keyPath = OmUtils.normalizeKey(keyPath, false);
+ checkKeyName(keyPath);
Review Comment:
This is still validation.
```suggestion
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyDeleteRequestWithFSO.java:
##########
@@ -227,4 +236,63 @@ public void testRecursiveAccessCheck() throws Exception {
// false for file1.
assertFalse(pathViewer.isCheckRecursiveAccess());
}
+
+ @Test
+ public void testDeleteDirectoryWithColonInFSOBucket() throws Exception {
+
+ when(ozoneManager.getEnableFileSystemPaths()).thenReturn(true);
+
+ OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucketName,
omMetadataManager, getBucketLayout());
+
+ String dirName = "foo:dir/";
+
+ String dirKeyPath = addKeyToDirTable(volumeName, bucketName, dirName);
+
+ long parentObjectID = 0L;
+ long dirObjectID = 12345L;
+
+ OmDirectoryInfo omDirectoryInfo =
OMRequestTestUtils.createOmDirectoryInfo(dirName, dirObjectID, parentObjectID);
+
+ omMetadataManager.getDirectoryTable().put(dirKeyPath, omDirectoryInfo);
+
+ OmDirectoryInfo storedDirInfo =
omMetadataManager.getDirectoryTable().get(dirKeyPath);
+ assertNotNull(storedDirInfo);
+ assertEquals(dirName, storedDirInfo.getName());
+ assertEquals(dirObjectID, storedDirInfo.getObjectID());
+ assertEquals(parentObjectID, storedDirInfo.getParentObjectID());
+
+ OMRequest deleteRequest =
doPreExecuteCheck(createDeleteKeyRequest(dirName));
+
+ OMKeyDeleteRequest omKeyDeleteRequest = new
OMKeyDeleteRequestWithFSO(deleteRequest, getBucketLayout());
+
+ OMClientResponse response =
omKeyDeleteRequest.validateAndUpdateCache(ozoneManager, 100L);
+
+ assertEquals(OzoneManagerProtocolProtos.Status.OK,
response.getOMResponse().getStatus());
+
+ assertNull(omMetadataManager.getDirectoryTable().get(dirName));
+ }
+
+ private OMRequest createDeleteKeyRequest(String testKeyName) {
+ KeyArgs keyArgs = KeyArgs.newBuilder().setBucketName(bucketName)
+ .setVolumeName(volumeName).setKeyName(testKeyName).build();
+
+ DeleteKeyRequest deleteKeyRequest =
+ DeleteKeyRequest.newBuilder().setKeyArgs(keyArgs).build();
+
+ return OMRequest.newBuilder().setDeleteKeyRequest(deleteKeyRequest)
+ .setCmdType(OzoneManagerProtocolProtos.Type.DeleteKey)
+ .setClientId(UUID.randomUUID().toString()).build();
+ }
+
+ protected OMRequest doPreExecuteCheck(OMRequest originalOmRequest) throws
Exception {
+
+ OMKeyDeleteRequest omKeyDeleteRequest =
+ getOmKeyDeleteRequest(originalOmRequest);
+
+ OMRequest modifiedOmRequest = omKeyDeleteRequest.preExecute(ozoneManager);
+
+ assertNotEquals(originalOmRequest, modifiedOmRequest);
+
+ return modifiedOmRequest;
+ }
Review Comment:
Please do not duplicate these from `TestOMKeyDeleteRequest`.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/OMClientRequest.java:
##########
@@ -521,17 +521,37 @@ public static String validateAndNormalizeKey(boolean
enableFileSystemPaths,
}
}
+ /**
+ * Normalizes the key path based on the bucket layout.
+ *
+ * @return normalized key path
+ */
+
+ public static String normalizeKeyPath(boolean enableFileSystemPaths,
+ String keyPath, BucketLayout bucketLayout) throws OMException {
+ LOG.debug("Bucket Layout: {}", bucketLayout);
Review Comment:
I don't think this message is useful at all.
```suggestion
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/OMClientRequest.java:
##########
@@ -521,17 +521,37 @@ public static String validateAndNormalizeKey(boolean
enableFileSystemPaths,
}
}
+ /**
+ * Normalizes the key path based on the bucket layout.
Review Comment:
```suggestion
* Normalizes the key path based on the bucket layout. This should be
used for existing keys.
* For new key creation, please see {@link
#validateAndNormalizeKey(boolean, String, BucketLayout)}
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/OMClientRequest.java:
##########
@@ -521,17 +521,37 @@ public static String validateAndNormalizeKey(boolean
enableFileSystemPaths,
}
}
+ /**
+ * Normalizes the key path based on the bucket layout.
+ *
+ * @return normalized key path
+ */
+
+ public static String normalizeKeyPath(boolean enableFileSystemPaths,
+ String keyPath, BucketLayout bucketLayout) throws OMException {
+ LOG.debug("Bucket Layout: {}", bucketLayout);
+ if (bucketLayout.shouldNormalizePaths(enableFileSystemPaths)) {
+ keyPath = OmUtils.normalizeKey(keyPath, false);
+ checkKeyName(keyPath);
+ }
+ return keyPath;
+ }
+
+ private static void checkKeyName(String keyPath) throws OMException {
+ if (keyPath.endsWith("/")) {
+ throw new OMException(
+ "Invalid KeyPath, key names with trailing / "
+ + "are not allowed." + keyPath,
+ OMException.ResultCodes.INVALID_KEY_NAME);
+ }
+ }
+
public static String validateAndNormalizeKey(boolean enableFileSystemPaths,
String keyPath, BucketLayout bucketLayout) throws OMException {
LOG.debug("Bucket Layout: {}", bucketLayout);
if (bucketLayout.shouldNormalizePaths(enableFileSystemPaths)) {
keyPath = validateAndNormalizeKey(true, keyPath);
- if (keyPath.endsWith("/")) {
- throw new OMException(
- "Invalid KeyPath, key names with trailing / "
- + "are not allowed." + keyPath,
- OMException.ResultCodes.INVALID_KEY_NAME);
- }
+ checkKeyName(keyPath);
Review Comment:
Please restore original version, no need to extract method.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyDeleteRequestWithFSO.java:
##########
@@ -227,4 +236,63 @@ public void testRecursiveAccessCheck() throws Exception {
// false for file1.
assertFalse(pathViewer.isCheckRecursiveAccess());
}
+
+ @Test
+ public void testDeleteDirectoryWithColonInFSOBucket() throws Exception {
Review Comment:
I think the test case should be moved to the parent class.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyDeleteRequestWithFSO.java:
##########
@@ -227,4 +236,63 @@ public void testRecursiveAccessCheck() throws Exception {
// false for file1.
assertFalse(pathViewer.isCheckRecursiveAccess());
}
+
+ @Test
+ public void testDeleteDirectoryWithColonInFSOBucket() throws Exception {
+
+ when(ozoneManager.getEnableFileSystemPaths()).thenReturn(true);
+
+ OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucketName,
omMetadataManager, getBucketLayout());
+
+ String dirName = "foo:dir/";
+
+ String dirKeyPath = addKeyToDirTable(volumeName, bucketName, dirName);
+
+ long parentObjectID = 0L;
+ long dirObjectID = 12345L;
+
+ OmDirectoryInfo omDirectoryInfo =
OMRequestTestUtils.createOmDirectoryInfo(dirName, dirObjectID, parentObjectID);
+
+ omMetadataManager.getDirectoryTable().put(dirKeyPath, omDirectoryInfo);
+
+ OmDirectoryInfo storedDirInfo =
omMetadataManager.getDirectoryTable().get(dirKeyPath);
+ assertNotNull(storedDirInfo);
+ assertEquals(dirName, storedDirInfo.getName());
+ assertEquals(dirObjectID, storedDirInfo.getObjectID());
+ assertEquals(parentObjectID, storedDirInfo.getParentObjectID());
+
+ OMRequest deleteRequest =
doPreExecuteCheck(createDeleteKeyRequest(dirName));
+
+ OMKeyDeleteRequest omKeyDeleteRequest = new
OMKeyDeleteRequestWithFSO(deleteRequest, getBucketLayout());
Review Comment:
Use `getOmKeyDeleteRequest` instead of `new OMKeyDeleteRequestWithFSO`. It
creates different type for `TestOMKeyDeleteRequest` and
`TestOMKeyDeleteRequestWithFSO`.
--
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]
