Extract config validation class into separate module (apache#7734)

* Extract config validation class into separate module * fix tests * adding example * fixing Co-authored-by: Jerry Peng <[email protected]>
abhiQmar · Aug 4, 2020 · e915b8e · e915b8e
1 parent 6e5d66c
commit e915b8e
Show file tree

Hide file tree

Showing 20 changed files with 171 additions and 88 deletions.
diff --git a/conf/functions_worker.yml b/conf/functions_worker.yml
@@ -235,3 +235,6 @@ tlsCertRefreshCheckDurationSec: 300
 
 connectorsDirectory: ./connectors
 functionsDirectory: ./functions
+
+# Should connector config be validated during during submission
+validateConnectorConfig: false
diff --git a/pom.xml b/pom.xml
@@ -23,7 +23,6 @@
   xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
   <modelVersion>4.0.0</modelVersion>
   <packaging>pom</packaging>
-
   <parent>
     <groupId>org.apache</groupId>
     <artifactId>apache</artifactId>
@@ -1687,6 +1686,7 @@ flexible messaging model and an intuitive client API.</description>
         <module>dashboard</module>
         <module>pulsar-broker-auth-sasl</module>
         <module>pulsar-client-auth-sasl</module>
+        <module>pulsar-config-validation</module>
 
         <!-- transaction related modules -->
         <module>pulsar-transaction</module>
@@ -1740,6 +1740,7 @@ flexible messaging model and an intuitive client API.</description>
         <module>pulsar-zookeeper</module>
         <module>pulsar-broker-auth-sasl</module>
         <module>pulsar-client-auth-sasl</module>
+        <module>pulsar-config-validation</module>
 
         <!-- transaction related modules -->
         <module>pulsar-transaction</module>

diff --git a/pulsar-config-validation/pom.xml b/pulsar-config-validation/pom.xml
@@ -0,0 +1,42 @@
+<?xml version="1.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.
+
+-->
+<project
+        xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"
+        xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
+    <modelVersion>4.0.0</modelVersion>
+    <parent>
+        <groupId>org.apache.pulsar</groupId>
+        <artifactId>pulsar</artifactId>
+        <version>2.7.0-SNAPSHOT</version>
+        <relativePath>..</relativePath>
+    </parent>
+
+    <artifactId>pulsar-config-validation</artifactId>
+    <description>Annotation based config validation for Pulsar</description>
+
+    <dependencies>
+        <dependency>
+            <groupId>org.slf4j</groupId>
+            <artifactId>slf4j-api</artifactId>
+        </dependency>
+    </dependencies>
+</project>
diff --git a/...ar/common/validator/ConfigValidation.java → ...r/config/validation/ConfigValidation.java b/...ar/common/validator/ConfigValidation.java → ...r/config/validation/ConfigValidation.java
@@ -16,7 +16,8 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.pulsar.common.validator;
+package org.apache.pulsar.config.validation;
+
 
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Field;
@@ -25,15 +26,19 @@
 import java.util.HashMap;
 import java.util.Map;
 
-import lombok.extern.slf4j.Slf4j;
-
 /**
  * The class that does the validation of all the members of a given object.
  */
-@Slf4j
 public class ConfigValidation {
 
-    public static void validateConfig(Object config) {
+    private static final Class DEFAULT_ANNOTATION_CLASS = ConfigValidationAnnotations.class;
+
+    /**
+     * Validate the config object with annotations from annotationClass
+     * @param config config object
+     * @param annotationClass class with annotations to use
+     */
+    public static void validateConfig(Object config, Class annotationClass) {
         for (Field field : config.getClass().getDeclaredFields()) {
             Object value = null;
             field.setAccessible(true);
@@ -42,25 +47,33 @@ public static void validateConfig(Object config) {
             } catch (IllegalAccessException e) {
                 throw new RuntimeException(e);
             }
-            validateField(field, value);
+            validateField(field, value, annotationClass);
         }
-        validateClass(config);
+        validateClass(config, annotationClass);
+    }
+
+    /**
+     * Validate the config object with default annotation class
+     * @param config config object
+     */
+    public static void validateConfig(Object config) {
+        validateConfig(config, DEFAULT_ANNOTATION_CLASS);
     }
 
-    private static void validateClass(Object config) {
-        processAnnotations(config.getClass().getAnnotations(), config.getClass().getName(), config);
+    private static void validateClass(Object config, Class annotationClass) {
+        processAnnotations(config.getClass().getAnnotations(), config.getClass().getName(), config, annotationClass);
     }
 
-    private static void validateField(Field field, Object value) {
-        processAnnotations(field.getAnnotations(), field.getName(), value);
+    private static void validateField(Field field, Object value, Class annotationClass) {
+        processAnnotations(field.getAnnotations(), field.getName(), value, annotationClass);
     }
 
-    private static void processAnnotations(Annotation[] annotations, String fieldName, Object value) {
+    private static void processAnnotations(Annotation[] annotations, String fieldName, Object value, Class annotationClass) {
         try {
             for (Annotation annotation : annotations) {
                 String type = annotation.annotationType().getName();
                 Class<?> validatorClass = null;
-                Class<?>[] classes = ConfigValidationAnnotations.class.getDeclaredClasses();
+                Class<?>[] classes = annotationClass.getDeclaredClasses();
                 //check if annotation is one of our
                 for (Class<?> clazz : classes) {
                     if (clazz.getName().equals(type)) {

diff --git a/...alidator/ConfigValidationAnnotations.java → ...lidation/ConfigValidationAnnotations.java b/...alidator/ConfigValidationAnnotations.java → ...lidation/ConfigValidationAnnotations.java
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.pulsar.common.validator;
+package org.apache.pulsar.config.validation;
 
 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
@@ -131,15 +131,6 @@ public class ConfigValidationAnnotations {
         Class<?>[] valueValidatorClasses();
     }
 
-    /**
-     * checks if the topic name is valid.
-     */
-    @Retention(RetentionPolicy.RUNTIME)
-    @Target(ElementType.FIELD)
-    public @interface TopicName {
-        Class<?> validatorClass() default ValidatorImpls.TopicNameValidator.class;
-    }
-
     /**
      * Field names for annotations.
      */

diff --git a/...mmon/validator/ConfigValidationUtils.java → ...fig/validation/ConfigValidationUtils.java b/...mmon/validator/ConfigValidationUtils.java → ...fig/validation/ConfigValidationUtils.java
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.pulsar.common.validator;
+package org.apache.pulsar.config.validation;
 
 import java.util.Map;
 

diff --git a/...he/pulsar/common/validator/Validator.java → ...e/pulsar/config/validation/Validator.java b/...he/pulsar/common/validator/Validator.java → ...e/pulsar/config/validation/Validator.java
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.pulsar.common.validator;
+package org.apache.pulsar.config.validation;
 
 import java.util.Map;
 

diff --git a/...lsar/common/validator/ValidatorImpls.java → ...sar/config/validation/ValidatorImpls.java b/...lsar/common/validator/ValidatorImpls.java → ...sar/config/validation/ValidatorImpls.java
@@ -16,22 +16,23 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.pulsar.common.validator;
+package org.apache.pulsar.config.validation;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.lang.reflect.InvocationTargetException;
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.Map;
 
-import lombok.NoArgsConstructor;
-import lombok.extern.slf4j.Slf4j;
-import org.apache.pulsar.common.naming.TopicName;
-
 /**
  * System defined Validator Annotations.
  */
-@Slf4j
 public class ValidatorImpls {
+
+    private static final Logger log = LoggerFactory.getLogger(ValidatorImpls.class);
+
     /**
      * Validates a positive number.
      */
@@ -272,9 +273,12 @@ public void validateField(String name, Object o) {
     /**
      * validates that the string is equal to one of the specified ones in the list.
      */
-    @NoArgsConstructor
     public static class StringValidator extends Validator {
 
+        public StringValidator() {
+
+        }
+
         private HashSet<String> acceptedValues = null;
 
         public StringValidator(Map<String, Object> params) {
@@ -349,26 +353,6 @@ public void validateField(String name, Object o) {
         }
     }
 
-    /**
-     * Validates that the field is a valid topic name.
-     */
-    @NoArgsConstructor
-    public static class TopicNameValidator extends Validator {
-
-        @Override
-        public void validateField(String name, Object o) {
-            if (o == null) {
-                return;
-            }
-            new StringValidator().validateField(name, o);
-            String topic = (String) o;
-            if (!TopicName.isValid(topic)) {
-                throw new IllegalArgumentException(
-                        String.format("The topic name %s is invalid for field '%s'", topic, name));
-            }
-        }
-    }
-
     /**
      * Validates basic types.
      */

diff --git a/...pulsar/common/validator/package-info.java → ...ulsar/config/validation/package-info.java b/...pulsar/common/validator/package-info.java → ...ulsar/config/validation/package-info.java
@@ -19,4 +19,4 @@
 /**
  * Implementation of Validator interfaces and annotations.
  */
-package org.apache.pulsar.common.validator;
+package org.apache.pulsar.config.validation;
diff --git a/...ommon/validator/ConfigValidationTest.java → ...nfig/validation/ConfigValidationTest.java b/...ommon/validator/ConfigValidationTest.java → ...nfig/validation/ConfigValidationTest.java
@@ -16,12 +16,17 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.pulsar.common.validator;
+package org.apache.pulsar.config.validation;
 
 import org.testng.annotations.Test;
 
-import java.util.*;
-import static org.testng.Assert.*;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.expectThrows;
 
 public class ConfigValidationTest {
 
@@ -66,8 +71,6 @@ class TestConfig {
         public Map stringIntegerMap;
         @ConfigValidationAnnotations.StringList
         public List stringList;
-        @ConfigValidationAnnotations.TopicName
-        public String topic;
         @ConfigValidationAnnotations.CustomType(validatorClass = TestValidator.class)
         public String customString;
     }
@@ -118,14 +121,6 @@ public void testStringList() {
         assertTrue(e.getMessage().contains("stringList"));
     }
 
-    @Test
-    public void testTopicName() {
-        TestConfig testConfig = createGoodConfig();
-        testConfig.topic = "http://google.com";
-        Exception e = expectThrows(IllegalArgumentException.class, () -> ConfigValidation.validateConfig(testConfig));
-        assertTrue(e.getMessage().contains("topic"));
-    }
-
     @Test
     public void testCustomString() {
         TestConfig testConfig = createGoodConfig();
@@ -141,7 +136,6 @@ private TestConfig createGoodConfig() {
         testConfig.integerList = testIntegerList;
         testConfig.stringIntegerMap = testStringIntegerMap;
         testConfig.stringList = testStringList;
-        testConfig.topic = topic;
         testConfig.customString = "ABCDEabcde";
         return testConfig;
     }

diff --git a/.../common/validator/ValidatorImplsTest.java → ...config/validation/ValidatorImplsTest.java b/.../common/validator/ValidatorImplsTest.java → ...config/validation/ValidatorImplsTest.java
@@ -16,9 +16,8 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.pulsar.common.validator;
+package org.apache.pulsar.config.validation;
 
-import org.apache.pulsar.common.naming.TopicName;
 import org.testng.annotations.Test;
 
 import java.net.InetSocketAddress;
@@ -60,7 +59,6 @@ public void testNotNullValidator() {
         ValidatorImpls.NotNullValidator validator = new ValidatorImpls.NotNullValidator();
         validator.validateField("fieldname", 2);
         validator.validateField("fieldname", "Something");
-        validator.validateField("fieldname", TopicName.get("default"));
         assertThrows(IllegalArgumentException.class, () -> validator.validateField("field", null));
     }
 
@@ -140,15 +138,6 @@ public void testStringValidator() {
         assertThrows(IllegalArgumentException.class, () -> validator.validateField("fieldname", "beautiful"));
     }
 
-    @Test
-    public void testTopicNameValidator() {
-        ValidatorImpls.TopicNameValidator validator = new ValidatorImpls.TopicNameValidator();
-        validator.validateField("fieldname", "topicname");
-        validator.validateField("fieldname", "public/default/topicname");
-        validator.validateField("fieldname", "persistent://public/default/topicname");
-        assertThrows(IllegalArgumentException.class, () -> validator.validateField("fieldname", "http://google.com"));
-    }
-
     @Test
     public void testSimpleTypeValidator() {
         Map<String, Object> config = new HashMap<>();

diff --git a/pulsar-functions/utils/pom.xml b/pulsar-functions/utils/pom.xml
@@ -56,6 +56,12 @@
       <version>${project.version}</version>
     </dependency>
 
+    <dependency>
+      <groupId>${project.groupId}</groupId>
+      <artifactId>pulsar-config-validation</artifactId>
+      <version>${project.version}</version>
+    </dependency>
+
     <dependency>
       <groupId>com.fasterxml.jackson.dataformat</groupId>
       <artifactId>jackson-dataformat-yaml</artifactId>

diff --git a/pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/SinkConfigUtils.java b/pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/SinkConfigUtils.java
@@ -37,7 +37,7 @@
 import org.apache.pulsar.common.nar.NarClassLoader;
 import org.apache.pulsar.common.util.ClassLoaderUtils;
 import org.apache.pulsar.common.util.ObjectMapperFactory;
-import org.apache.pulsar.common.validator.ConfigValidation;
+import org.apache.pulsar.config.validation.ConfigValidation;
 import org.apache.pulsar.functions.api.utils.IdentityFunction;
 import org.apache.pulsar.functions.proto.Function;
 import org.apache.pulsar.functions.proto.Function.FunctionDetails;

diff --git a/...ar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/SourceConfigUtils.java b/...ar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/SourceConfigUtils.java
@@ -37,7 +37,7 @@
 import org.apache.pulsar.common.nar.NarClassLoader;
 import org.apache.pulsar.common.util.ClassLoaderUtils;
 import org.apache.pulsar.common.util.ObjectMapperFactory;
-import org.apache.pulsar.common.validator.ConfigValidation;
+import org.apache.pulsar.config.validation.ConfigValidation;
 import org.apache.pulsar.functions.api.utils.IdentityFunction;
 import org.apache.pulsar.functions.proto.Function;
 import org.apache.pulsar.functions.proto.Function.FunctionDetails;

diff --git a/...-functions/utils/src/test/java/org/apache/pulsar/functions/utils/SinkConfigUtilsTest.java b/...-functions/utils/src/test/java/org/apache/pulsar/functions/utils/SinkConfigUtilsTest.java
@@ -28,7 +28,7 @@
 import org.apache.pulsar.common.io.ConnectorDefinition;
 import org.apache.pulsar.common.io.SinkConfig;
 import org.apache.pulsar.common.util.Reflections;
-import org.apache.pulsar.common.validator.ConfigValidationAnnotations;
+import org.apache.pulsar.config.validation.ConfigValidationAnnotations;
 import org.apache.pulsar.functions.api.utils.IdentityFunction;
 import org.apache.pulsar.functions.proto.Function;
 import org.apache.pulsar.functions.utils.io.ConnectorUtils;