Optimize error message and check if field name is empty string

Signed-off-by: Gao Binlong <gbinlong@amazon.com>
gaobinlong · Jun 6, 2024 · cff2bcf · cff2bcf
1 parent 7cad2e0
commit cff2bcf
Show file tree

Hide file tree

Showing 4 changed files with 36 additions and 22 deletions.
diff --git a/modules/ingest-common/src/main/java/org/opensearch/ingest/common/FingerprintProcessor.java b/modules/ingest-common/src/main/java/org/opensearch/ingest/common/FingerprintProcessor.java
@@ -10,6 +10,7 @@
 
 import org.opensearch.common.Nullable;
 import org.opensearch.common.hash.MessageDigests;
+import org.opensearch.core.common.Strings;
 import org.opensearch.ingest.AbstractProcessor;
 import org.opensearch.ingest.ConfigurationUtils;
 import org.opensearch.ingest.IngestDocument;
@@ -23,7 +24,6 @@
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Set;
 import java.util.stream.Collectors;
 
@@ -57,15 +57,15 @@ public final class FingerprintProcessor extends AbstractProcessor {
     ) {
         super(tag, description);
         if (fields != null && !fields.isEmpty()) {
-            if (fields.stream().anyMatch(Objects::isNull)) {
-                throw new IllegalArgumentException("field path cannot be null nor empty");
+            if (fields.stream().anyMatch(Strings::isNullOrEmpty)) {
+                throw new IllegalArgumentException("field name in [fields] cannot be null nor empty");
             }
             if (excludeFields != null && !excludeFields.isEmpty()) {
                 throw new IllegalArgumentException("either fields or exclude_fields can be set");
             }
         }
-        if (excludeFields != null && !excludeFields.isEmpty() && excludeFields.stream().anyMatch(Objects::isNull)) {
-            throw new IllegalArgumentException("field path cannot be null nor empty");
+        if (excludeFields != null && !excludeFields.isEmpty() && excludeFields.stream().anyMatch(Strings::isNullOrEmpty)) {
+            throw new IllegalArgumentException("field name in [exclude_fields] cannot be null nor empty");
         }
 
         if (!HASH_METHODS.contains(hashMethod.toUpperCase(Locale.ROOT))) {
@@ -247,16 +247,15 @@ public FingerprintProcessor create(
             List<String> fields = ConfigurationUtils.readOptionalList(TYPE, processorTag, config, "fields");
             List<String> excludeFields = ConfigurationUtils.readOptionalList(TYPE, processorTag, config, "exclude_fields");
             if (fields != null && !fields.isEmpty()) {
-
-                if (fields.stream().anyMatch(Objects::isNull)) {
-                    throw newConfigurationException(TYPE, processorTag, "fields", "field path cannot be null nor empty");
+                if (fields.stream().anyMatch(Strings::isNullOrEmpty)) {
+                    throw newConfigurationException(TYPE, processorTag, "fields", "field name cannot be null nor empty");
                 }
                 if (excludeFields != null && !excludeFields.isEmpty()) {
                     throw newConfigurationException(TYPE, processorTag, "fields", "either fields or exclude_fields can be set");
                 }
             }
-            if (excludeFields != null && !excludeFields.isEmpty() && excludeFields.stream().anyMatch(Objects::isNull)) {
-                throw newConfigurationException(TYPE, processorTag, "exclude_fields", "field path cannot be null nor empty");
+            if (excludeFields != null && !excludeFields.isEmpty() && excludeFields.stream().anyMatch(Strings::isNullOrEmpty)) {
+                throw newConfigurationException(TYPE, processorTag, "exclude_fields", "field name cannot be null nor empty");
             }
 
             String targetField = ConfigurationUtils.readStringProperty(TYPE, processorTag, config, "target_field", "fingerprint");

diff --git a/...t-common/src/test/java/org/opensearch/ingest/common/FingerprintProcessorFactoryTests.java b/...t-common/src/test/java/org/opensearch/ingest/common/FingerprintProcessorFactoryTests.java
@@ -32,7 +32,6 @@ public void init() {
     public void testCreate() throws Exception {
         Map<String, Object> config = new HashMap<>();
 
-        boolean includeAllFields = randomBoolean();
         List<String> fields = null;
         List<String> excludeFields = null;
         if (randomBoolean()) {
@@ -73,24 +72,32 @@ public void testCreateWithFields() throws Exception {
 
         config = new HashMap<>();
         List<String> fields = new ArrayList<>();
-        fields.add(null);
+        if (randomBoolean()) {
+            fields.add(null);
+        } else {
+            fields.add("");
+        }
         config.put("fields", fields);
         try {
             factory.create(null, null, null, config);
             fail("factory create should have failed");
         } catch (OpenSearchParseException e) {
-            assertThat(e.getMessage(), equalTo("[fields] field path cannot be null nor empty"));
+            assertThat(e.getMessage(), equalTo("[fields] field name cannot be null nor empty"));
         }
 
         config = new HashMap<>();
         List<String> excludeFields = new ArrayList<>();
-        excludeFields.add(null);
+        if (randomBoolean()) {
+            excludeFields.add(null);
+        } else {
+            excludeFields.add("");
+        }
         config.put("exclude_fields", excludeFields);
         try {
             factory.create(null, null, null, config);
             fail("factory create should have failed");
         } catch (OpenSearchParseException e) {
-            assertThat(e.getMessage(), equalTo("[exclude_fields] field path cannot be null nor empty"));
+            assertThat(e.getMessage(), equalTo("[exclude_fields] field name cannot be null nor empty"));
         }
     }
 

diff --git a/...s/ingest-common/src/test/java/org/opensearch/ingest/common/FingerprintProcessorTests.java b/...s/ingest-common/src/test/java/org/opensearch/ingest/common/FingerprintProcessorTests.java
@@ -53,21 +53,29 @@ public void testGenerateFingerprint() throws Exception {
 
     public void testCreateFingerprintProcessorFailed() {
         List<String> fields = new ArrayList<>();
-        fields.add(null);
+        if (randomBoolean()) {
+            fields.add(null);
+        } else {
+            fields.add("");
+        }
         fields.add(randomAlphaOfLength(10));
 
         assertThrows(
-            "field path cannot be null nor empty",
+            "field name in [fields] cannot be null nor empty",
             IllegalArgumentException.class,
             () -> createFingerprintProcessor(fields, null, null, randomFrom(List.of("MD5", "SHA-1", "SHA-256", "SHA3-256")), false)
         );
 
         List<String> excludeFields = new ArrayList<>();
-        excludeFields.add(null);
+        if (randomBoolean()) {
+            excludeFields.add(null);
+        } else {
+            excludeFields.add("");
+        }
         excludeFields.add(randomAlphaOfLength(10));
 
         assertThrows(
-            "field path cannot be null nor empty",
+            "field name in [exclude_fields] cannot be null nor empty",
             IllegalArgumentException.class,
             () -> createFingerprintProcessor(null, excludeFields, null, randomFrom(List.of("MD5", "SHA-1", "SHA-256", "SHA3-256")), false)
         );

diff --git a/...common/src/yamlRestTest/resources/rest-api-spec/test/ingest/340_fingerprint_processor.yml b/...common/src/yamlRestTest/resources/rest-api-spec/test/ingest/340_fingerprint_processor.yml
@@ -11,7 +11,7 @@ teardown:
       version: " - 2.14.99"
       reason: "introduced in 2.15"
   - do:
-      catch: /field path cannot be null nor empty/
+      catch: /field name in \[fields\] cannot be null nor empty/
       ingest.put_pipeline:
         id: "1"
         body:  >
@@ -25,15 +25,15 @@ teardown:
             ]
           }
   - do:
-      catch: /field path cannot be null nor empty/
+      catch: /field name in \[exclude_fields\] cannot be null nor empty/
       ingest.put_pipeline:
         id: "1"
         body:  >
           {
             "processors": [
               {
                 "fingerprint" : {
-                  "exclude_fields": [null]
+                  "exclude_fields": [""]
                 }
               }
             ]