address review comments

davidkyle · Oct 6, 2023 · f52fb37 · f52fb37
1 parent 8e61ee8
commit f52fb37
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 22 deletions.
diff --git a/...ugin/ml/src/main/java/org/elasticsearch/xpack/ml/inference/ingest/InferenceProcessor.java b/...ugin/ml/src/main/java/org/elasticsearch/xpack/ml/inference/ingest/InferenceProcessor.java
@@ -373,7 +373,6 @@ public void accept(ClusterState state) {
         }
 
         @Override
-        @SuppressWarnings("unchecked")
         public InferenceProcessor create(
             Map<String, Processor.Factory> processorFactories,
             String tag,
@@ -405,25 +404,7 @@ public InferenceProcessor create(
                 inferenceConfigUpdate = inferenceConfigUpdateFromMap(inferenceConfigMap);
             }
 
-            Object inputOutputs = config.remove(INPUT_OUTPUT);
-            List<Map<String, Object>> inputs = null;
-            if (inputOutputs != null) {
-                // input_output may be a single map or a list of maps
-                if (inputOutputs instanceof List<?> inputOutputList) {
-                    if (inputOutputList.isEmpty() == false) {
-                        // check it is a list of maps
-                        if (inputOutputList.get(0) instanceof Map == false) {
-                            throw ConfigurationUtils.newConfigurationException(TYPE, tag, INPUT_FIELD, "property isn't a list of maps");
-                        }
-                    }
-                    inputs = (List<Map<String, Object>>) inputOutputList;
-                } else if (inputOutputs instanceof Map) {
-                    inputs = List.of((Map<String, Object>) inputOutputs);
-                } else {
-                    throw ConfigurationUtils.newConfigurationException(TYPE, tag, INPUT_FIELD, "property isn't a map or list of maps");
-                }
-            }
-
+            List<Map<String, Object>> inputs = readOptionalInputOutPutConfig(config, tag);
             boolean configuredWithInputFields = inputs != null;
             if (configuredWithInputFields) {
                 // new style input/output configuration
@@ -643,6 +624,29 @@ List<InputConfig> parseInputFields(String tag, List<Map<String, Object>> inputs)
             return parsedInputs;
         }
 
+        @SuppressWarnings("unchecked")
+        List<Map<String, Object>> readOptionalInputOutPutConfig(Map<String, Object> config, String tag) {
+            Object inputOutputs = config.remove(INPUT_OUTPUT);
+            if (inputOutputs == null) {
+                return null;
+            }
+
+            // input_output may be a single map or a list of maps
+            if (inputOutputs instanceof List<?> inputOutputList) {
+                if (inputOutputList.isEmpty() == false) {
+                    // check it is a list of maps
+                    if (inputOutputList.get(0) instanceof Map == false) {
+                        throw ConfigurationUtils.newConfigurationException(TYPE, tag, INPUT_OUTPUT, "property isn't a list of maps");
+                    }
+                }
+                return (List<Map<String, Object>>) inputOutputList;
+            } else if (inputOutputs instanceof Map) {
+                return List.of((Map<String, Object>) inputOutputs);
+            } else {
+                throw ConfigurationUtils.newConfigurationException(TYPE, tag, INPUT_OUTPUT, "property isn't a map or list of maps");
+            }
+        }
+
         private ElasticsearchException duplicatedFieldNameError(String property, String fieldName, String tag) {
             return newConfigurationException(TYPE, tag, property, "names must be unique but [" + fieldName + "] is repeated");
         }

diff --git a/...test/java/org/elasticsearch/xpack/ml/inference/ingest/InferenceProcessorFactoryTests.java b/...test/java/org/elasticsearch/xpack/ml/inference/ingest/InferenceProcessorFactoryTests.java
@@ -767,7 +767,7 @@ public void testCreateProcessorWithInputFieldWrongType() {
                 ElasticsearchParseException.class,
                 () -> processorFactory.create(Collections.emptyMap(), "processor_with_bad_config", null, config)
             );
-            assertThat(e.getMessage(), containsString("[input_field] property isn't a list of maps"));
+            assertThat(e.getMessage(), containsString("[input_output] property isn't a list of maps"));
         }
         {
             Map<String, Object> config = new HashMap<>();
@@ -778,7 +778,20 @@ public void testCreateProcessorWithInputFieldWrongType() {
                 ElasticsearchParseException.class,
                 () -> processorFactory.create(Collections.emptyMap(), "processor_with_bad_config", null, config)
             );
-            assertThat(e.getMessage(), containsString("[input_field] property isn't a map or list of maps"));
+            assertThat(e.getMessage(), containsString("[input_output] property isn't a map or list of maps"));
+        }
+        {
+            Map<Boolean, String> badMap = new HashMap<>();
+            badMap.put(Boolean.TRUE, "foo");
+            Map<String, Object> config = new HashMap<>();
+            config.put(InferenceProcessor.MODEL_ID, "my_model");
+            config.put(InferenceProcessor.INPUT_OUTPUT, badMap);
+
+            var e = expectThrows(
+                ElasticsearchParseException.class,
+                () -> processorFactory.create(Collections.emptyMap(), "processor_with_bad_config", null, config)
+            );
+            assertThat(e.getMessage(), containsString("[input_field] required property is missing"));
         }
         {
             // empty list