Refactor XmlProcessor configuration handling and enhance factory tests for validation

Author: marc-gr

modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/XmlProcessor.java

@@ -419,51 +419,39 @@ public XmlProcessor create(
 
             // Parse XPath expressions map
             Map<String, String> xpathExpressions = new HashMap<>();
-            Object xpathConfig = config.get("xpath");
+            Map<String, Object> xpathConfig = ConfigurationUtils.readOptionalMap(TYPE, processorTag, config, "xpath");
             if (xpathConfig != null) {
-                if (xpathConfig instanceof Map) {
-                    @SuppressWarnings("unchecked")
-                    Map<String, Object> xpathMap = (Map<String, Object>) xpathConfig;
-                    for (Map.Entry<String, Object> entry : xpathMap.entrySet()) {
-                        if (entry.getValue() instanceof String str) {
-                            xpathExpressions.put(entry.getKey(), str);
-                        } else {
-                            throw new IllegalArgumentException(
-                                "XPath target field ["
-                                    + entry.getKey()
-                                    + "] must be a string, got ["
-                                    + entry.getValue().getClass().getSimpleName()
-                                    + "]"
-                            );
-                        }
+                for (Map.Entry<String, Object> entry : xpathConfig.entrySet()) {
+                    if (entry.getValue() instanceof String str) {
+                        xpathExpressions.put(entry.getKey(), str);
+                    } else {
+                        throw new IllegalArgumentException(
+                            "XPath target field ["
+                                + entry.getKey()
+                                + "] must be a string, got ["
+                                + entry.getValue().getClass().getSimpleName()
+                                + "]"
+                        );
                     }
-                } else {
-                    throw new IllegalArgumentException("XPath configuration must be a map of expressions to target fields");
                 }
             }
 
             // Parse namespaces map
             Map<String, String> namespaces = new HashMap<>();
-            Object namespaceConfig = config.get("namespaces");
+            Map<String, Object> namespaceConfig = ConfigurationUtils.readOptionalMap(TYPE, processorTag, config, "namespaces");
             if (namespaceConfig != null) {
-                if (namespaceConfig instanceof Map) {
-                    @SuppressWarnings("unchecked")
-                    Map<String, Object> namespaceMap = (Map<String, Object>) namespaceConfig;
-                    for (Map.Entry<String, Object> entry : namespaceMap.entrySet()) {
-                        if (entry.getValue() instanceof String str) {
-                            namespaces.put(entry.getKey(), str);
-                        } else {
-                            throw new IllegalArgumentException(
-                                "Namespace prefix ["
-                                    + entry.getKey()
-                                    + "] must have a string URI, got ["
-                                    + entry.getValue().getClass().getSimpleName()
-                                    + "]"
-                            );
-                        }
+                for (Map.Entry<String, Object> entry : namespaceConfig.entrySet()) {
+                    if (entry.getValue() instanceof String str) {
+                        namespaces.put(entry.getKey(), str);
+                    } else {
+                        throw new IllegalArgumentException(
+                            "Namespace prefix ["
+                                + entry.getKey()
+                                + "] must have a string URI, got ["
+                                + entry.getValue().getClass().getSimpleName()
+                                + "]"
+                        );
                     }
-                } else {
-                    throw new IllegalArgumentException("Namespaces configuration must be a map of prefixes to URIs");
                 }
             }
 

modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/XmlProcessorFactoryTests.java

@@ -12,6 +12,7 @@
 import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.test.ESTestCase;
 
+import java.util.Arrays;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -93,6 +94,32 @@ private XmlProcessor createProcessor(Map<String, Object> config) throws Exceptio
         return createProcessor(createFactory(), config);
     }
 
+    /**
+     * Creates a processor mimicking the production pipeline validation.
+     * This simulates what ConfigurationUtils.readProcessor() does.
+     */
+    private XmlProcessor createProcessorWithValidation(Map<String, Object> config) throws Exception {
+        XmlProcessor.Factory factory = createFactory();
+        String processorTag = randomAlphaOfLength(10);
+        
+        // Make a copy of the config to avoid modifying the original
+        Map<String, Object> configCopy = new HashMap<>(config);
+        
+        // Create the processor (this should consume config parameters)
+        XmlProcessor processor = factory.create(null, processorTag, null, configCopy, null);
+        
+        // Simulate the validation check from ConfigurationUtils.readProcessor()
+        if (configCopy.isEmpty() == false) {
+            throw new ElasticsearchParseException(
+                "processor [{}] doesn't support one or more provided configuration parameters {}",
+                "xml",
+                Arrays.toString(configCopy.keySet().toArray())
+            );
+        }
+        
+        return processor;
+    }
+
     /**
      * Helper method to create XPath configuration map.
      */
@@ -235,7 +262,7 @@ public void testCreateWithInvalidXPathConfig() throws Exception {
         Map<String, Object> config = createBaseConfig();
         config.put("xpath", "invalid_string"); // Should be a map
 
-        expectCreationFailure(config, IllegalArgumentException.class, "XPath configuration must be a map of expressions to target fields");
+        expectCreationFailure(config, ElasticsearchParseException.class, "[xpath] property isn't a map, but of type [java.lang.String]");
     }
 
     public void testCreateWithInvalidXPathTargetField() throws Exception {
@@ -271,7 +298,7 @@ public void testCreateWithInvalidNamespacesConfig() throws Exception {
         Map<String, Object> config = createBaseConfig();
         config.put("namespaces", "invalid_string"); // Should be a map
 
-        expectCreationFailure(config, IllegalArgumentException.class, "Namespaces configuration must be a map of prefixes to URIs");
+        expectCreationFailure(config, ElasticsearchParseException.class, "[namespaces] property isn't a map, but of type [java.lang.String]");
     }
 
     public void testCreateWithInvalidNamespaceURI() throws Exception {
@@ -384,4 +411,25 @@ public void testCreateWithXPathUsingNamespacesWithoutConfiguration() throws Exce
             "Invalid XPath expression [//book:title/text()]: contains namespace prefixes but no namespace configuration provided"
         );
     }
+
+    public void testConfigurationParametersAreProperlyRemoved() throws Exception {
+        // Test that demonstrates configuration validation works when using production-like validation
+        // This test verifies that ConfigurationUtils.readOptionalMap() properly removes parameters from config
+        // If any are left, the processor factory should throw an exception about unknown parameters
+        
+        Map<String, String> xpathConfig = createXPathConfig("//test", "test_field");
+        Map<String, Object> config = createConfigWithXPath(DEFAULT_FIELD, xpathConfig);
+        
+        // Add an intentionally unknown parameter to trigger the validation
+        config.put("unknown_parameter", "should_fail");
+        
+        // This should fail because "unknown_parameter" should remain in config after all valid params are removed
+        ElasticsearchParseException exception = expectThrows(
+            ElasticsearchParseException.class,
+            () -> createProcessorWithValidation(config)
+        );
+        
+        assertThat(exception.getMessage(), containsString("doesn't support one or more provided configuration parameters"));
+        assertThat(exception.getMessage(), containsString("unknown_parameter"));
+    }
 }