geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kl...@apache.org
Subject [geode] branch develop updated: GEODE-5609: Extract validateRegionName and improve testing (#2353)
Date Tue, 21 Aug 2018 17:31:29 GMT
This is an automated email from the ASF dual-hosted git repository.

klund pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git


The following commit(s) were added to refs/heads/develop by this push:
     new 49e1c18  GEODE-5609: Extract validateRegionName and improve testing (#2353)
49e1c18 is described below

commit 49e1c18ad4d20f3d5c468f4b69a2debe2a6382f3
Author: Kirk Lund <klund@apache.org>
AuthorDate: Tue Aug 21 10:31:22 2018 -0700

    GEODE-5609: Extract validateRegionName and improve testing (#2353)
    
    * Extract validateRegionName from LocalRegion to RegionNameValidation
    * Move and rename RegionNameValidationJUnitTest
    * Rewrite RegionNameValidationTest with AssertJ and modern testing
---
 .../geode/internal/cache/GemFireCacheImpl.java     |   2 +-
 .../apache/geode/internal/cache/LocalRegion.java   |  46 +----
 .../geode/internal/cache/RegionNameValidation.java |  78 ++++++++
 .../geode/cache/RegionNameValidationJUnitTest.java |  89 ---------
 .../internal/cache/RegionNameValidationTest.java   | 212 +++++++++++++++++++++
 5 files changed, 292 insertions(+), 135 deletions(-)

diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
index ae2d925..046237b 100755
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
@@ -3000,7 +3000,7 @@ public class GemFireCacheImpl implements InternalCache, InternalClientCache,
Has
       }
     }
     this.stopper.checkCancelInProgress(null);
-    LocalRegion.validateRegionName(name, internalRegionArgs);
+    RegionNameValidation.validate(name, internalRegionArgs);
     RegionAttributes<K, V> attrs = p_attrs;
     attrs = invokeRegionBefore(null, name, attrs, internalRegionArgs);
     if (attrs == null) {
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java b/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
index 5e2a491..482bf0c 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
@@ -46,7 +46,6 @@ import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
-import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 import javax.transaction.RollbackException;
@@ -936,7 +935,7 @@ public class LocalRegion extends AbstractRegion implements LoaderHelperFactory,
           }
           throw new RegionDestroyedException(toString(), getFullPath());
         }
-        validateRegionName(subregionName, internalRegionArgs);
+        RegionNameValidation.validate(subregionName, internalRegionArgs);
 
         validateSubregionAttributes(regionAttributes);
 
@@ -7388,49 +7387,6 @@ public class LocalRegion extends AbstractRegion implements LoaderHelperFactory,
     }
   }
 
-  public static void validateRegionName(String name, InternalRegionArguments internalRegionArgs)
{
-    if (name == null) {
-      throw new IllegalArgumentException(
-          LocalizedStrings.LocalRegion_NAME_CANNOT_BE_NULL.toLocalizedString());
-    }
-    if (name.isEmpty()) {
-      throw new IllegalArgumentException(
-          LocalizedStrings.LocalRegion_NAME_CANNOT_BE_EMPTY.toLocalizedString());
-    }
-    if (name.contains(SEPARATOR)) {
-      throw new IllegalArgumentException(
-          LocalizedStrings.LocalRegion_NAME_CANNOT_CONTAIN_THE_SEPARATOR_0
-              .toLocalizedString(SEPARATOR));
-    }
-
-    // Validate the name of the region only if it isn't an internal region
-    if (internalRegionArgs.isInternalRegion()) {
-      return;
-    }
-    if (internalRegionArgs.isUsedForMetaRegion()) {
-      return;
-    }
-    if (internalRegionArgs.isUsedForPartitionedRegionAdmin()) {
-      return;
-    }
-    if (internalRegionArgs.isUsedForPartitionedRegionBucket()) {
-      return;
-    }
-
-    if (name.startsWith("__")) {
-      throw new IllegalArgumentException(
-          "Region names may not begin with a double-underscore: " + name);
-    }
-
-    final Pattern NAME_PATTERN = Pattern.compile("[aA-zZ0-9-_.]+");
-    // Ensure the region only contains valid characters
-    Matcher matcher = NAME_PATTERN.matcher(name);
-    if (!matcher.matches()) {
-      throw new IllegalArgumentException(
-          "Region names may only be alphanumeric and may contain hyphens or underscores:
" + name);
-    }
-  }
-
   private void checkCacheClosed() {
     if (this.cache.isClosed()) {
       throw this.cache.getCacheClosedException(null, null);
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/RegionNameValidation.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/RegionNameValidation.java
new file mode 100644
index 0000000..e5273d4
--- /dev/null
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/RegionNameValidation.java
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.geode.internal.cache;
+
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.internal.i18n.LocalizedStrings;
+
+class RegionNameValidation {
+
+  private static final Pattern NAME_PATTERN = Pattern.compile("[aA-zZ0-9-_.]+");
+
+  static Pattern getNamePattern() {
+    return NAME_PATTERN;
+  }
+
+  static void validate(String name) {
+    validate(name, new InternalRegionArguments());
+  }
+
+  static void validate(String name, InternalRegionArguments internalRegionArguments) {
+    if (name == null) {
+      throw new IllegalArgumentException(
+          LocalizedStrings.LocalRegion_NAME_CANNOT_BE_NULL.toLocalizedString());
+    }
+    if (name.isEmpty()) {
+      throw new IllegalArgumentException(
+          LocalizedStrings.LocalRegion_NAME_CANNOT_BE_EMPTY.toLocalizedString());
+    }
+    if (name.contains(Region.SEPARATOR)) {
+      throw new IllegalArgumentException(
+          LocalizedStrings.LocalRegion_NAME_CANNOT_CONTAIN_THE_SEPARATOR_0
+              .toLocalizedString(Region.SEPARATOR));
+    }
+
+    // Validate the name of the region only if it isn't an internal region
+    if (internalRegionArguments.isInternalRegion()) {
+      return;
+    }
+    if (internalRegionArguments.isUsedForMetaRegion()) {
+      return;
+    }
+    if (internalRegionArguments.isUsedForPartitionedRegionAdmin()) {
+      return;
+    }
+    if (internalRegionArguments.isUsedForPartitionedRegionBucket()) {
+      return;
+    }
+
+    if (name.startsWith("__")) {
+      throw new IllegalArgumentException(
+          "Region names may not begin with a double-underscore: " + name);
+    }
+
+    // Ensure the region only contains valid characters
+    Matcher matcher = NAME_PATTERN.matcher(name);
+    if (!matcher.matches()) {
+      throw new IllegalArgumentException(
+          "Region names may only be alphanumeric and may contain hyphens or underscores:
" + name);
+    }
+  }
+}
diff --git a/geode-core/src/test/java/org/apache/geode/cache/RegionNameValidationJUnitTest.java
b/geode-core/src/test/java/org/apache/geode/cache/RegionNameValidationJUnitTest.java
deleted file mode 100644
index 64c1076..0000000
--- a/geode-core/src/test/java/org/apache/geode/cache/RegionNameValidationJUnitTest.java
+++ /dev/null
@@ -1,89 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
- * agreements. See the NOTICE file distributed with this work for additional information
regarding
- * copyright ownership. The ASF licenses this file to You under the Apache License, Version
2.0 (the
- * "License"); you may not use this file except in compliance with the License. You may obtain
a
- * copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software distributed under
the License
- * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
express
- * or implied. See the License for the specific language governing permissions and limitations
under
- * the License.
- */
-package org.apache.geode.cache;
-
-import static org.junit.Assert.fail;
-
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
-
-import org.junit.Test;
-
-import org.apache.geode.internal.cache.InternalRegionArguments;
-import org.apache.geode.internal.cache.LocalRegion;
-
-public class RegionNameValidationJUnitTest {
-  private static final Pattern NAME_PATTERN = Pattern.compile("[aA-zZ0-9-_.]+");
-  private static final String REGION_NAME = "MyRegion";
-
-
-  @Test
-  public void testInvalidNames() {
-    InternalRegionArguments ira = new InternalRegionArguments();
-    ira.setInternalRegion(false);
-    try {
-      LocalRegion.validateRegionName(null, ira);
-      fail();
-    } catch (IllegalArgumentException ignore) {
-    }
-    try {
-      LocalRegion.validateRegionName("", ira);
-      fail();
-    } catch (IllegalArgumentException ignore) {
-    }
-    try {
-      LocalRegion.validateRegionName("FOO" + Region.SEPARATOR, ira);
-      fail();
-    } catch (IllegalArgumentException ignore) {
-    }
-
-  }
-
-  @Test
-  public void testExternalRegionNames() {
-    InternalRegionArguments ira = new InternalRegionArguments();
-    ira.setInternalRegion(false);
-    validateCharacters(ira);
-    try {
-      LocalRegion.validateRegionName("__InvalidInternalRegionName", ira);
-      fail();
-    } catch (IllegalArgumentException ignore) {
-    }
-  }
-
-  @Test
-  public void testInternalRegionNames() {
-    InternalRegionArguments ira = new InternalRegionArguments();
-    ira.setInternalRegion(true);
-    LocalRegion.validateRegionName("__ValidInternalRegionName", ira);
-  }
-
-  private void validateCharacters(InternalRegionArguments ira) {
-    for (int x = 0; x < Character.MAX_VALUE; x++) {
-      String name = (char) x + REGION_NAME;
-      Matcher matcher = NAME_PATTERN.matcher(name);
-      if (matcher.matches()) {
-        LocalRegion.validateRegionName(name, ira);
-      } else {
-        try {
-          LocalRegion.validateRegionName(name, ira);
-          fail("Should have received an IllegalArgumentException for character: " + (char)
x + "["
-              + x + "]");
-        } catch (IllegalArgumentException ignore) {
-        }
-      }
-    }
-  }
-}
diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/RegionNameValidationTest.java
b/geode-core/src/test/java/org/apache/geode/internal/cache/RegionNameValidationTest.java
new file mode 100644
index 0000000..b2ba9f5
--- /dev/null
+++ b/geode-core/src/test/java/org/apache/geode/internal/cache/RegionNameValidationTest.java
@@ -0,0 +1,212 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.geode.internal.cache;
+
+import static java.lang.Character.MAX_VALUE;
+import static org.apache.geode.cache.Region.SEPARATOR;
+import static org.apache.geode.internal.cache.RegionNameValidation.getNamePattern;
+import static org.apache.geode.internal.cache.RegionNameValidation.validate;
+import static org.apache.geode.internal.i18n.LocalizedStrings.LocalRegion_NAME_CANNOT_BE_EMPTY;
+import static org.apache.geode.internal.i18n.LocalizedStrings.LocalRegion_NAME_CANNOT_BE_NULL;
+import static org.apache.geode.internal.i18n.LocalizedStrings.LocalRegion_NAME_CANNOT_CONTAIN_THE_SEPARATOR_0;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.regex.Matcher;
+
+import org.junit.Test;
+
+public class RegionNameValidationTest {
+
+  @Test
+  public void nullThrows() {
+    assertThatThrownBy(() -> validate(null)).isInstanceOf(IllegalArgumentException.class)
+        .hasMessageContaining(LocalRegion_NAME_CANNOT_BE_NULL.toLocalizedString());
+  }
+
+  @Test
+  public void emptyThrows() {
+    assertThatThrownBy(() -> validate("")).isInstanceOf(IllegalArgumentException.class)
+        .hasMessageContaining(LocalRegion_NAME_CANNOT_BE_EMPTY.toLocalizedString());
+  }
+
+  @Test
+  public void endingWithSeparatorThrows() {
+    assertThatThrownBy(() -> validate("foo" + SEPARATOR))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessageContaining(LocalRegion_NAME_CANNOT_CONTAIN_THE_SEPARATOR_0.toLocalizedString(
+            SEPARATOR));
+  }
+
+  @Test
+  public void startingWithSeparatorThrows() {
+    assertThatThrownBy(() -> validate(SEPARATOR + "foo"))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessageContaining(LocalRegion_NAME_CANNOT_CONTAIN_THE_SEPARATOR_0.toLocalizedString(
+            SEPARATOR));
+  }
+
+  @Test
+  public void containingSeparatorThrows() {
+    assertThatThrownBy(() -> validate("foo" + SEPARATOR + "bar"))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessageContaining(LocalRegion_NAME_CANNOT_CONTAIN_THE_SEPARATOR_0.toLocalizedString(
+            SEPARATOR));
+  }
+
+  @Test
+  public void startingWithDoubleUnderscoreThrows() {
+    assertThatThrownBy(() -> validate("__InvalidInternalRegionName"))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessageContaining("Region names may not begin with a double-underscore");
+  }
+
+  @Test
+  public void startingWithDoubleUnderscoreIsOkForInternalRegion() {
+    InternalRegionArguments internalRegionArguments = new InternalRegionArguments();
+    internalRegionArguments.setInternalRegion(true);
+
+    assertThatCode(() -> validate("__ValidInternalRegionName", internalRegionArguments))
+        .doesNotThrowAnyException();
+  }
+
+  @Test
+  public void startingWithUnderscoreIsOk() {
+    validate("_validRegionName");
+  }
+
+  @Test
+  public void containingUnderscoreIsOk() {
+    validate("valid_regionName");
+  }
+
+  @Test
+  public void containingMultipleUnderscoresIsOk() {
+    validate("valid_region_name");
+  }
+
+  @Test
+  public void endingWithUnderscoreIsOk() {
+    validate("validRegionName_");
+  }
+
+  @Test
+  public void containingDoubleUnderscoreIsOk() {
+    validate("valid__regionName");
+  }
+
+  @Test
+  public void endingWithDoubleUnderscoreIsOk() {
+    validate("validRegionName__");
+  }
+
+  @Test
+  public void startingWithHyphenIsOk() {
+    validate("-validRegionName");
+  }
+
+  @Test
+  public void startingWithDoubleHyphenIsOk() {
+    validate("--validRegionName");
+  }
+
+  @Test
+  public void containingHyphenIsOk() {
+    validate("valid-regionName");
+  }
+
+  @Test
+  public void containingDoubleHyphenIsOk() {
+    validate("valid--regionName");
+  }
+
+  @Test
+  public void endingWithHyphenIsOk() {
+    validate("validRegionName-");
+  }
+
+  @Test
+  public void endingWithDoubleHyphenIsOk() {
+    validate("validRegionName--");
+  }
+
+  @Test
+  public void startingWithMatchingCharactersAreOk() {
+    for (char character = 0; character < MAX_VALUE; character++) {
+      String name = character + "ValidRegion";
+      Matcher matcher = getNamePattern().matcher(name);
+      if (matcher.matches()) {
+        validate(name);
+      }
+    }
+  }
+
+  @Test
+  public void containingMatchingCharactersAreOk() {
+    for (char character = 0; character < MAX_VALUE; character++) {
+      String name = "Valid" + character + "Region";
+      Matcher matcher = getNamePattern().matcher(name);
+      if (matcher.matches()) {
+        validate(name);
+      }
+    }
+  }
+
+  @Test
+  public void endingMatchingCharactersAreOk() {
+    for (char character = 0; character < MAX_VALUE; character++) {
+      String name = "ValidRegion" + character;
+      Matcher matcher = getNamePattern().matcher(name);
+      if (matcher.matches()) {
+        validate(name);
+      }
+    }
+  }
+
+  @Test
+  public void startingWithNonMatchingCharactersThrow() {
+    for (char character = 0; character < MAX_VALUE; character++) {
+      String name = character + "InvalidRegion";
+      Matcher matcher = getNamePattern().matcher(name);
+      if (!matcher.matches()) {
+        assertThatThrownBy(() -> validate(name)).isInstanceOf(IllegalArgumentException.class);
+      }
+    }
+  }
+
+  @Test
+  public void containingNonMatchingCharactersThrow() {
+    for (char character = 0; character < MAX_VALUE; character++) {
+      String name = "Invalid" + character + "Region";
+      Matcher matcher = getNamePattern().matcher(name);
+      if (!matcher.matches()) {
+        assertThatThrownBy(() -> validate(name)).isInstanceOf(IllegalArgumentException.class);
+      }
+    }
+  }
+
+  @Test
+  public void endingWithNonMatchingCharactersThrow() {
+    for (char character = 0; character < MAX_VALUE; character++) {
+      String name = "InvalidRegion" + character;
+      Matcher matcher = getNamePattern().matcher(name);
+      if (!matcher.matches()) {
+        assertThatThrownBy(() -> validate(name)).isInstanceOf(IllegalArgumentException.class);
+      }
+    }
+  }
+}


Mime
View raw message