GEODE-7266: change className validation

geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateRegionCommand.java

@@ -14,7 +14,9 @@
  */
 package org.apache.geode.management.internal.cli.commands;
 
-import static org.apache.geode.management.internal.ManagementHelper.isClassNameValid;
+
+
+import static org.apache.geode.management.configuration.ClassName.isClassNameValid;
 
 import java.util.ArrayList;
 import java.util.Arrays;

geode-core/src/test/java/org/apache/geode/management/internal/cli/converters/ClassNameConverterTest.java

@@ -49,7 +49,7 @@ public void convertClassAndEmptyProp() {
 
   @Test
   public void convertWithOnlyDelimiter() {
-    assertThat(converter.convertFromText("{", ClassName.class, "")).isEqualTo(ClassName.EMPTY);
+    assertThat(converter.convertFromText("{}", ClassName.class, "")).isEqualTo(ClassName.EMPTY);
   }
 
   @Test

geode-core/src/test/java/org/apache/geode/management/internal/cli/domain/ClassNameTest.java

@@ -15,18 +15,22 @@
 
 package org.apache.geode.management.internal.cli.domain;
 
+import static org.apache.geode.management.configuration.ClassName.isClassNameValid;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import java.util.Properties;
 
+import com.fasterxml.jackson.databind.ObjectMapper;
 import org.junit.Test;
 
 import org.apache.geode.management.configuration.ClassName;
 import org.apache.geode.management.internal.configuration.domain.DeclarableTypeInstantiator;
+import org.apache.geode.util.internal.GeodeJsonMapper;
 
 
 public class ClassNameTest {
+  private ObjectMapper mapper = GeodeJsonMapper.getMapper();
 
   @Test
   public void constructWithoutProps() {
@@ -114,4 +118,83 @@ public void getInstanceWithProps() {
     MyCacheWriter obj = DeclarableTypeInstantiator.newInstance(cacheWriter, null);
     assertThat(obj.getProperties()).containsEntry("k", "v").containsOnlyKeys("k");
   }
+
+  @Test
+  public void jsonMapper() throws Exception {
+    Properties properties = new Properties();
+    properties.put("key1", "value1");
+    ClassName className = new ClassName("abc", properties);
+    ClassName className1 = new ClassName("abc", mapper.writeValueAsString(properties));
+    assertThat(className).isEqualToComparingFieldByField(className1);
+    String json = mapper.writeValueAsString(className);
+    String json1 = mapper.writeValueAsString(className1);
+    assertThat(json).isEqualTo(json1);
+
+    ClassName className2 = mapper.readValue(json, ClassName.class);
+    assertThat(className2).isEqualToComparingFieldByField(className);
+  }
+
+  @Test
+  public void deserializeIllegalClassName() throws Exception {
+    assertThatThrownBy(() -> new ClassName("a b")).isInstanceOf(IllegalArgumentException.class);
+    String json = "{\"className\":\"a b\"}";
+    assertThatThrownBy(() -> mapper.readValue(json, ClassName.class))
+        .hasCauseInstanceOf(IllegalArgumentException.class);
+  }
+
+  @Test
+  public void isValidClassNameGivenEmptyStringReturnsFalse() {
+    assertThat(isClassNameValid("")).isFalse();
+  }
+
+  @Test
+  public void isValidClassNameGivenDashReturnsFalse() {
+    assertThat(isClassNameValid("-")).isFalse();
+  }
+
+  @Test
+  public void isValidClassNameGivenSpaceReturnsFalse() {
+    assertThat(isClassNameValid(" ")).isFalse();
+  }
+
+  @Test
+  public void isValidClassNameGivenCommaReturnsFalse() {
+    assertThat(isClassNameValid(",")).isFalse();
+  }
+
+  @Test
+  public void isValidClassNameGivenLeadingDotReturnsFalse() {
+    assertThat(isClassNameValid(".a")).isFalse();
+  }
+
+  @Test
+  public void isValidClassNameGivenTrailingDotReturnsFalse() {
+    assertThat(isClassNameValid("a.")).isFalse();
+  }
+
+  @Test
+  public void isValidClassNameGivenTwoDotsReturnsFalse() {
+    assertThat(isClassNameValid("a..a")).isFalse();
+  }
+
+  @Test
+  public void isValidClassNameGivenNameThatStartsWithDigitReturnsFalse() {
+    assertThat(isClassNameValid("9a")).isFalse();
+  }
+
+  @Test
+  public void isValidClassNameGivenNameReturnsTrue() {
+    assertThat(isClassNameValid("a9")).isTrue();
+  }
+
+  @Test
+  public void isValidClassNameGivenDotDelimitedNamesReturnsTrue() {
+    assertThat(isClassNameValid("$a1._b2.c3$")).isTrue();
+  }
+
+  @Test
+  public void isValidClassNameGivenMiddleNameThatStartsWithDigitReturnsFalse() {
+    assertThat(isClassNameValid("a1.2b.c3")).isFalse();
+  }
+
 }

geode-management/src/main/java/org/apache/geode/management/configuration/ClassName.java

@@ -21,10 +21,11 @@
 import java.util.Properties;
 import java.util.regex.Pattern;
 
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import org.apache.commons.lang3.StringUtils;
 
-import org.apache.geode.management.internal.ManagementHelper;
 import org.apache.geode.util.internal.GeodeJsonMapper;
 
 /**
@@ -35,8 +36,8 @@
 public class ClassName implements Serializable {
   private static final long serialVersionUID = 1L;
 
-  private String className = "";
-  private Properties initProperties = new Properties();
+  private String className;
+  private Properties initProperties;
   private static ObjectMapper mapper = GeodeJsonMapper.getMapper();
 
   /**
@@ -45,11 +46,6 @@ public class ClassName implements Serializable {
    */
   public static final ClassName EMPTY = new ClassName("");
 
-  /**
-   * Default constructor used for serialization.
-   */
-  public ClassName() {}
-
   /**
    * Object to be instantiated using the empty param constructor of the className
    *
@@ -60,22 +56,21 @@ public ClassName(String className) {
   }
 
   /**
+   * this is a convenient way to create a ClassName object using json represented properties
+   *
    * @param className this class needs to have a no-arg constructor
    * @param jsonInitProperties a json representation of the initialization properties
    *        that will be passed to {@link org.apache.geode.cache.Declarable#initialize}.
    * @throws IllegalArgumentException if the class name is not valid
    * @throws IllegalArgumentException if jsonInitProperties is invalid JSON
    */
   public ClassName(String className, String jsonInitProperties) {
-    if (StringUtils.isBlank(className)) {
-      return;
-    }
-    if (!ManagementHelper.isClassNameValid(className)) {
-      throw new IllegalArgumentException("Invalid className");
-    }
-    this.className = className;
+    this(className, createProperties(jsonInitProperties));
+  }
+
+  private static Properties createProperties(String jsonInitProperties) {
     try {
-      initProperties = mapper.readValue(jsonInitProperties, Properties.class);
+      return mapper.readValue(jsonInitProperties, Properties.class);
     } catch (IOException e) {
       throw new IllegalArgumentException("Invalid JSON: " + jsonInitProperties, e);
     }
@@ -86,7 +81,18 @@ public ClassName(String className, String jsonInitProperties) {
    * @param properties the initialization properties
    *        that will be passed to {@link org.apache.geode.cache.Declarable#initialize}.
    */
-  public ClassName(String className, Properties properties) {
+  @JsonCreator
+  public ClassName(@JsonProperty("className") String className,
+      @JsonProperty("initProperties") Properties properties) {
+    if (StringUtils.isBlank(className)) {
+      this.className = "";
+      this.initProperties = new Properties();
+      return;
+    }
+    // validate the className
+    if (!isClassNameValid(className)) {
+      throw new IllegalArgumentException("Invalid className");
+    }
     this.className = className;
     this.initProperties = properties;
   }
@@ -105,14 +111,6 @@ public Properties getInitProperties() {
     return initProperties;
   }
 
-  private static boolean isClassNameValid(String fqcn) {
-    if (StringUtils.isBlank(fqcn)) {
-      return false;
-    }
-    String regex = "([\\p{L}_$][\\p{L}\\p{N}_$]*\\.)*[\\p{L}_$][\\p{L}\\p{N}_$]*";
-    return Pattern.matches(regex, fqcn);
-  }
-
   @Override
   public int hashCode() {
     return Objects.hash(className, initProperties);
@@ -134,4 +132,12 @@ public boolean equals(Object o) {
         && this.getInitProperties().equals(that.getInitProperties());
   }
 
+  public static boolean isClassNameValid(String className) {
+    if (StringUtils.isBlank(className)) {
+      return false;
+    }
+    String regex = "([\\p{L}_$][\\p{L}\\p{N}_$]*\\.)*[\\p{L}_$][\\p{L}\\p{N}_$]*";
+    return Pattern.matches(regex, className);
+  }
+
 }

geode-web-management/src/integrationTest/java/org/apache/geode/management/internal/rest/PdxManagementTest.java

@@ -85,4 +85,16 @@ public void success() throws Exception {
     }
 
   }
+
+  @Test
+  public void invalidClassName() throws Exception {
+    String json = "{\"readSerialized\":true,\"pdxSerializer\":{\"className\":\"class name\"}}";
+
+    context.perform(post("/experimental/configurations/pdx")
+        .with(httpBasic("clusterManage", "clusterManage"))
+        .content(json))
+        .andExpect(status().isBadRequest())
+        .andExpect(jsonPath("$.statusCode", is("ILLEGAL_ARGUMENT")))
+        .andExpect(jsonPath("$.statusMessage", containsString("Invalid className")));
+  }
 }