modify based on comments

Signed-off-by: Ruirui Zhang <mariazrr@amazon.com>

Author: ruai0511

server/src/main/java/org/opensearch/autotagging/Attribute.java

@@ -16,7 +16,7 @@
 
 /**
  * Represents an attribute within the auto-tagging feature. Attributes define characteristics that can
- * be used for tagging and classification. Implementations Implementations must ensure that attributes
+ * be used for tagging and classification. Implementations must ensure that attributes
  * are uniquely identifiable by their name. Attributes should be singletons and managed centrally to
  * avoid duplicates.
  *
@@ -25,11 +25,29 @@
 public interface Attribute extends Writeable {
     String getName();
 
+    /**
+     * Ensure that `validateAttribute` is called in the constructor of attribute implementations
+     * to prevent potential serialization issues.
+     */
+    default void validateAttribute() {
+        String name = getName();
+        if (name == null || name.isEmpty()) {
+            throw new IllegalArgumentException("Attribute name cannot be null or empty");
+        }
+    }
+
     @Override
     default void writeTo(StreamOutput out) throws IOException {
         out.writeString(getName());
     }
 
+    /**
+     * Retrieves an attribute from the given feature type based on its name.
+     * Implementations of `FeatureType.getAttributeFromName` must be thread-safe as this method
+     * may be called concurrently.
+     * @param in - the {@link StreamInput} from which the attribute name is read
+     * @param featureType - the FeatureType used to look up the attribute
+     */
     static Attribute from(StreamInput in, FeatureType featureType) throws IOException {
         String attributeName = in.readString();
         Attribute attribute = featureType.getAttributeFromName(attributeName);

server/src/main/java/org/opensearch/autotagging/AutoTaggingRegistry.java

@@ -21,6 +21,10 @@
  * @opensearch.experimental
  */
 public class AutoTaggingRegistry {
+    /**
+     * featureTypesRegistryMap should be concurrently readable but not concurrently writable.
+     * The registration of FeatureType should only be done during boot-up.
+     */
     public static final Map<String, FeatureType> featureTypesRegistryMap = new HashMap<>();
     public static final int MAX_FEATURE_TYPE_NAME_LENGTH = 30;
 

server/src/main/java/org/opensearch/autotagging/FeatureType.java

@@ -33,6 +33,10 @@ public interface FeatureType extends Writeable {
 
     String getName();
 
+    /**
+     * Returns the registry of allowed attributes for this feature type.
+     * Implementations must ensure that access to this registry is thread-safe.
+     */
     Map<String, Attribute> getAllowedAttributesRegistry();
 
     default int getMaxNumberOfValuesPerAttribute() {
@@ -49,6 +53,11 @@ default boolean isValidAttribute(Attribute attribute) {
         return getAllowedAttributesRegistry().containsValue(attribute);
     }
 
+    /**
+     * Retrieves an attribute by its name from the allowed attributes' registry.
+     * Implementations must ensure that this method is thread-safe.
+     * @param name The name of the attribute.
+     */
     default Attribute getAttributeFromName(String name) {
         return getAllowedAttributesRegistry().get(name);
     }

server/src/main/java/org/opensearch/autotagging/Rule.java

@@ -43,6 +43,7 @@ public class Rule implements Writeable, ToXContentObject {
     private final Map<Attribute, Set<String>> attributeMap;
     private final String featureValue;
     private final String updatedAt;
+    private RuleValidator ruleValidator;
     public static final String _ID_STRING = "_id";
     public static final String DESCRIPTION_STRING = "description";
     public static final String UPDATED_AT_STRING = "updated_at";
@@ -72,8 +73,8 @@ public Rule(StreamInput in) throws IOException {
     }
 
     private void validateRule() {
-        RuleValidator validator = new RuleValidator(description, attributeMap, featureValue, updatedAt, featureType);
-        validator.validate();
+        this.ruleValidator = new RuleValidator(description, attributeMap, featureValue, updatedAt, featureType);
+        this.ruleValidator.validate();
     }
 
     @Override

server/src/main/java/org/opensearch/autotagging/RuleValidator.java

@@ -11,6 +11,8 @@
 import org.opensearch.common.ValidationException;
 import org.joda.time.Instant;
 
+import java.util.ArrayList;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
@@ -28,7 +30,6 @@ public class RuleValidator {
     private final String featureValue;
     private final String updatedAt;
     private final FeatureType featureType;
-    private final ValidationException validationException = new ValidationException();
     public static final int MAX_DESCRIPTION_LENGTH = 256;
 
     public RuleValidator(
@@ -46,90 +47,106 @@ public RuleValidator(
     }
 
     public void validate() {
-        validateStringFields();
-        validateFeatureType();
-        validateUpdatedAtEpoch();
-        validateAttributeMap();
-        if (!validationException.validationErrors().isEmpty()) {
+        List<String> errorMessages = new ArrayList<>();
+        errorMessages.addAll(validateStringFields());
+        errorMessages.addAll(validateFeatureType());
+        errorMessages.addAll(validateUpdatedAtEpoch());
+        errorMessages.addAll(validateAttributeMap());
+        if (!errorMessages.isEmpty()) {
+            ValidationException validationException = new ValidationException();
+            validationException.addValidationErrors(errorMessages);
             throw new IllegalArgumentException(validationException);
         }
     }
 
-    private void validateStringFields() {
-        requireNonNullOrEmpty(description, "Rule description can't be null or empty");
-        if (description != null && description.length() > MAX_DESCRIPTION_LENGTH) {
-            validationException.addValidationError("Rule description cannot exceed " + MAX_DESCRIPTION_LENGTH + " characters.");
+    private List<String> validateStringFields() {
+        List<String> errors = new ArrayList<>();
+        if (isNullOrEmpty(description)) {
+            errors.add("Rule description can't be null or empty");
+        } else if (description.length() > MAX_DESCRIPTION_LENGTH) {
+            errors.add("Rule description cannot exceed " + MAX_DESCRIPTION_LENGTH + " characters.");
         }
-        requireNonNullOrEmpty(featureValue, "Rule featureValue can't be null or empty");
-        requireNonNullOrEmpty(updatedAt, "Rule update time can't be null or empty");
+        if (isNullOrEmpty(featureValue)) {
+            errors.add("Rule featureValue can't be null or empty");
+        }
+        if (isNullOrEmpty(updatedAt)) {
+            errors.add("Rule update time can't be null or empty");
+        }
+        return errors;
     }
 
-    private void validateFeatureType() {
+    private boolean isNullOrEmpty(String str) {
+        return str == null || str.isEmpty();
+    }
+
+    private List<String> validateFeatureType() {
         if (featureType == null) {
-            validationException.addValidationError("Couldn't identify which feature the rule belongs to. Rule feature can't be null.");
+            return List.of("Couldn't identify which feature the rule belongs to. Rule feature can't be null.");
         }
+        return new ArrayList<>();
     }
 
-    private void validateUpdatedAtEpoch() {
+    private List<String> validateUpdatedAtEpoch() {
         if (updatedAt != null && !isValid(Instant.parse(updatedAt).getMillis())) {
-            validationException.addValidationError("Rule update time is not a valid epoch");
+            return List.of("Rule update time is not a valid epoch");
         }
+        return new ArrayList<>();
     }
 
-    private void validateAttributeMap() {
+    private List<String> validateAttributeMap() {
+        List<String> errors = new ArrayList<>();
         if (attributeMap == null || attributeMap.isEmpty()) {
-            validationException.addValidationError("Rule should have at least 1 attribute requirement");
+            errors.add("Rule should have at least 1 attribute requirement");
         }
 
         if (attributeMap != null && featureType != null) {
             for (Map.Entry<Attribute, Set<String>> entry : attributeMap.entrySet()) {
                 Attribute attribute = entry.getKey();
                 Set<String> attributeValues = entry.getValue();
-                validateAttributeExistence(attribute);
-                validateMaxAttributeValues(attribute, attributeValues);
-                validateAttributeValuesLength(attributeValues);
+                errors.addAll(validateAttributeExistence(attribute));
+                errors.addAll(validateMaxAttributeValues(attribute, attributeValues));
+                errors.addAll(validateAttributeValuesLength(attributeValues));
             }
         }
+        return errors;
     }
 
-    private void validateAttributeExistence(Attribute attribute) {
+    private List<String> validateAttributeExistence(Attribute attribute) {
         if (featureType.getAttributeFromName(attribute.getName()) == null) {
-            validationException.addValidationError(
-                attribute.getName() + " is not a valid attribute within the " + featureType.getName() + " feature."
-            );
+            return List.of(attribute.getName() + " is not a valid attribute within the " + featureType.getName() + " feature.");
         }
+        return new ArrayList<>();
     }
 
-    private void validateMaxAttributeValues(Attribute attribute, Set<String> attributeValues) {
+    private List<String> validateMaxAttributeValues(Attribute attribute, Set<String> attributeValues) {
+        List<String> errors = new ArrayList<>();
+        String attributeName = attribute.getName();
+        if (attributeValues.isEmpty()) {
+            errors.add("Attribute values for " + attributeName + " cannot be empty.");
+        }
         int maxSize = featureType.getMaxNumberOfValuesPerAttribute();
         int actualSize = attributeValues.size();
         if (actualSize > maxSize) {
-            validationException.addValidationError(
+            errors.add(
                 "Each attribute can only have a maximum of "
                     + maxSize
                     + " values. The input attribute "
-                    + attribute
+                    + attributeName
                     + " has length "
                     + attributeValues.size()
                     + ", which exceeds this limit."
             );
         }
+        return errors;
     }
 
-    private void validateAttributeValuesLength(Set<String> attributeValues) {
+    private List<String> validateAttributeValuesLength(Set<String> attributeValues) {
         int maxValueLength = featureType.getMaxCharLengthPerAttributeValue();
         for (String attributeValue : attributeValues) {
             if (attributeValue.isEmpty() || attributeValue.length() > maxValueLength) {
-                validationException.addValidationError(
-                    "Attribute value [" + attributeValue + "] is invalid (empty or exceeds " + maxValueLength + " characters)"
-                );
+                return List.of("Attribute value [" + attributeValue + "] is invalid (empty or exceeds " + maxValueLength + " characters)");
             }
         }
-    }
-
-    private void requireNonNullOrEmpty(String value, String errorMessage) {
-        if (value == null || value.isEmpty()) {
-            validationException.addValidationError(errorMessage);
-        }
+        return new ArrayList<>();
     }
 }