[GitHub] [geode] DonalEvans commented on a change in pull request #7282: GEODE-9969: Fix unescaping the region name with underscore

[GitBox <gi...@apache.org>]

DonalEvans commented on a change in pull request #7282:
URL: https://github.com/apache/geode/pull/7282#discussion_r787954570



##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionHelper.java
##########
@@ -768,9 +768,11 @@ public static String escapePRPath(String prFullPath) {
 
 
   public static final String TWO_SEPARATORS = SEPARATOR + SEPARATOR;
+  public static final String THREE_SEPARATORS = TWO_SEPARATORS + SEPARATOR;
 
   public static String unescapePRPath(String escapedPath) {
     String path = escapedPath.replace('_', SEPARATOR_CHAR);
+    path = path.replace(THREE_SEPARATORS, "/_");

Review comment:
       Could this be:
   ```
   path = path.replace(THREE_SEPARATORS, SEPARATOR + "_");
   ```
   to maintain consistent use of the `SEPARATOR` constant please?

##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionHelperJUnitTest.java
##########
@@ -41,4 +41,22 @@ public void testEscapeUnescape() {
     }
   }
 
+  @Test
+  public void testEscapeUnescapeWhenRegionHasUnderscoreInTheName() {
+    {

Review comment:
       Can these unnecessary braces be removed please?

##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionHelperJUnitTest.java
##########
@@ -41,4 +41,22 @@ public void testEscapeUnescape() {
     }
   }
 
+  @Test
+  public void testEscapeUnescapeWhenRegionHasUnderscoreInTheName() {
+    {
+      String bucketName =
+          PartitionedRegionHelper.getBucketName(SEPARATOR + "_region", 5);
+      assertEquals("Name = " + bucketName, -1, bucketName.indexOf(SEPARATOR));
+      assertEquals(SEPARATOR + "_region",
+          PartitionedRegionHelper.getPRPath(bucketName));

Review comment:
       Could the assertions in this test be changed to AssertJ rather than JUnit please? The assertions here would then become:
   ```
   assertThat(bucketName).as("Name = " + bucketName).doesNotContain(SEPARATOR);
   assertThat(PartitionedRegionHelper.getPRPath(bucketName))
             .isEqualTo(SEPARATOR + "_region");
   ```




-- 
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: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org