IngestDocument readability improvements (#124322)

server/src/main/java/org/elasticsearch/common/util/LocaleUtils.java

@@ -14,15 +14,15 @@ import java.util.Locale;
 import java.util.MissingResourceException;
 
 /**
- * Utilities for for dealing with {@link Locale} objects
+ * Utilities for dealing with {@link Locale} objects
  */
 public class LocaleUtils {
 
     /**
      * Parse the given locale as {@code language}, {@code language-country} or
      * {@code language-country-variant}.
-     * Either underscores or hyphens may be used as separators, but consistently, ie.
+     * Either underscores or hyphens may be used as separators, but consistently, i.e.
-     * you may not use an hyphen to separate the language from the country and an
+     * you may not use a hyphen to separate the language from the country and an
      * underscore to separate the country from the variant.
      * @throws IllegalArgumentException if there are too many parts in the locale string
      * @throws IllegalArgumentException if the language or country is not recognized

server/src/main/java/org/elasticsearch/ingest/ConfigurationUtils.java

@@ -543,18 +543,38 @@ public final class ConfigurationUtils {
                 Script script = new Script(ScriptType.INLINE, DEFAULT_TEMPLATE_LANG, propertyValue, Map.of());
                 return scriptService.compile(script, TemplateScript.CONTEXT);
             } else {
-                return (params) -> new TemplateScript(params) {
+                return new ConstantTemplateScriptFactory(propertyValue);
-                    @Override
-                    public String execute() {
-                        return propertyValue;
-                    }
-                };
             }
         } catch (Exception e) {
             throw ConfigurationUtils.newConfigurationException(processorType, processorTag, propertyName, e);
         }
     }
 
+    /**
+     * A 'template script' that ignores the model to which it is applied and just always returns a constant String.
+     * <p>
+     * Having a separate named class for this allows for some hot code paths to pre-apply the 'template script' statically,
+     * rather than bothering to invoke it per-document. Note that this is probably only useful if something expensive
+     * is being done with the result of calling the script, and the code can then avoid doing that thing per-document.
+     */
+    public static class ConstantTemplateScriptFactory implements TemplateScript.Factory {
+        final TemplateScript script;
+
+        private ConstantTemplateScriptFactory(String value) {
+            this.script = new TemplateScript(Map.of()) {
+                @Override
+                public String execute() {
+                    return value;
+                }
+            };
+        }
+
+        @Override
+        public TemplateScript newInstance(Map<String, Object> params) {
+            return script;
+        }
+    }
+
     private static void addMetadataToException(
         ElasticsearchException exception,
         String processorType,

server/src/main/java/org/elasticsearch/ingest/IngestDocument.java

@@ -83,7 +83,7 @@ public final class IngestDocument {
      * of the pipeline was that the _index value did not change and so only 'foo' would appear
      * in the index history.
      */
-    private Set<String> indexHistory = new LinkedHashSet<>();
+    private final Set<String> indexHistory = new LinkedHashSet<>();
 
     private boolean doNoSelfReferencesCheck = false;
     private boolean reroute = false;
@@ -238,9 +238,7 @@ public final class IngestDocument {
         } else if (object instanceof String string) {
             return Base64.getDecoder().decode(string);
         } else {
-            throw new IllegalArgumentException(
+            throw new IllegalArgumentException(Errors.notStringOrByteArray(path, object));
-                "Content field [" + path + "] of unknown type [" + object.getClass().getName() + "], must be string or byte array"
-            );
         }
     }
 
@@ -268,51 +266,42 @@ public final class IngestDocument {
             String pathElement = fieldPath.pathElements[i];
             if (context == null) {
                 return false;
-            }
+            } else if (context instanceof Map<?, ?> map) {
-            if (context instanceof Map<?, ?> map) {
                 context = map.get(pathElement);
             } else if (context instanceof List<?> list) {
+                int index;
                 try {
-                    int index = Integer.parseInt(pathElement);
+                    index = Integer.parseInt(pathElement);
-                    if (index < 0 || index >= list.size()) {
-                        if (failOutOfRange) {
-                            throw new IllegalArgumentException(
-                                "["
-                                    + index
-                                    + "] is out of bounds for array with length ["
-                                    + list.size()
-                                    + "] as part of path ["
-                                    + path
-                                    + "]"
-                            );
-                        } else {
-                            return false;
-                        }
-                    }
-                    context = list.get(index);
                 } catch (NumberFormatException e) {
                     return false;
                 }
-
+                if (index < 0 || index >= list.size()) {
+                    if (failOutOfRange) {
+                        throw new IllegalArgumentException(Errors.outOfBounds(path, index, list.size()));
+                    } else {
+                        return false;
+                    }
+                } else {
+                    context = list.get(index);
+                }
             } else {
                 return false;
             }
         }
 
         String leafKey = fieldPath.pathElements[fieldPath.pathElements.length - 1];
-        if (context instanceof Map<?, ?> map) {
+        if (context == null) {
+            return false;
+        } else if (context instanceof Map<?, ?> map) {
             return map.containsKey(leafKey);
-        }
+        } else if (context instanceof List<?> list) {
-        if (context instanceof List<?> list) {
             try {
                 int index = Integer.parseInt(leafKey);
                 if (index >= 0 && index < list.size()) {
                     return true;
                 } else {
                     if (failOutOfRange) {
-                        throw new IllegalArgumentException(
+                        throw new IllegalArgumentException(Errors.outOfBounds(path, index, list.size()));
-                            "[" + index + "] is out of bounds for array with length [" + list.size() + "] as part of path [" + path + "]"
-                        );
                     } else {
                         return false;
                     }
@@ -320,8 +309,9 @@ public final class IngestDocument {
             } catch (NumberFormatException e) {
                 return false;
             }
+        } else {
+            return false;
         }
-        return false;
     }
 
     /**
@@ -342,79 +332,58 @@ public final class IngestDocument {
         }
 
         String leafKey = fieldPath.pathElements[fieldPath.pathElements.length - 1];
-        if (context instanceof Map<?, ?> map) {
+        if (context == null) {
+            throw new IllegalArgumentException(Errors.cannotRemove(path, leafKey, null));
+        } else if (context instanceof Map<?, ?> map) {
             if (map.containsKey(leafKey)) {
                 map.remove(leafKey);
-                return;
+            } else {
+                throw new IllegalArgumentException(Errors.notPresent(path, leafKey));
             }
-            throw new IllegalArgumentException("field [" + leafKey + "] not present as part of path [" + path + "]");
+        } else if (context instanceof List<?> list) {
-        }
-        if (context instanceof List<?> list) {
             int index;
             try {
                 index = Integer.parseInt(leafKey);
             } catch (NumberFormatException e) {
-                throw new IllegalArgumentException(
+                throw new IllegalArgumentException(Errors.notInteger(path, leafKey), e);
-                    "[" + leafKey + "] is not an integer, cannot be used as an index as part of path [" + path + "]",
-                    e
-                );
             }
             if (index < 0 || index >= list.size()) {
-                throw new IllegalArgumentException(
+                throw new IllegalArgumentException(Errors.outOfBounds(path, index, list.size()));
-                    "[" + index + "] is out of bounds for array with length [" + list.size() + "] as part of path [" + path + "]"
+            } else {
-                );
+                list.remove(index);
             }
-            list.remove(index);
+        } else {
-            return;
+            throw new IllegalArgumentException(Errors.cannotRemove(path, leafKey, context));
         }
-
-        if (context == null) {
-            throw new IllegalArgumentException("cannot remove [" + leafKey + "] from null as part of path [" + path + "]");
-        }
-        throw new IllegalArgumentException(
-            "cannot remove [" + leafKey + "] from object of type [" + context.getClass().getName() + "] as part of path [" + path + "]"
-        );
     }
 
     private static ResolveResult resolve(String pathElement, String fullPath, Object context) {
         if (context == null) {
-            return ResolveResult.error("cannot resolve [" + pathElement + "] from null as part of path [" + fullPath + "]");
+            return ResolveResult.error(Errors.cannotResolve(fullPath, pathElement, null));
-        }
+        } else if (context instanceof Map<?, ?>) {
-        if (context instanceof Map<?, ?>) {
             @SuppressWarnings("unchecked")
             Map<String, Object> map = (Map<String, Object>) context;
             Object object = map.getOrDefault(pathElement, NOT_FOUND); // getOrDefault is faster than containsKey + get
             if (object == NOT_FOUND) {
-                return ResolveResult.error("field [" + pathElement + "] not present as part of path [" + fullPath + "]");
+                return ResolveResult.error(Errors.notPresent(fullPath, pathElement));
             } else {
                 return ResolveResult.success(object);
             }
-        }
+        } else if (context instanceof List<?> list) {
-        if (context instanceof List<?> list) {
             int index;
             try {
                 index = Integer.parseInt(pathElement);
             } catch (NumberFormatException e) {
-                return ResolveResult.error(
+                return ResolveResult.error(Errors.notInteger(fullPath, pathElement));
-                    "[" + pathElement + "] is not an integer, cannot be used as an index as part of path [" + fullPath + "]"
-                );
             }
             if (index < 0 || index >= list.size()) {
-                return ResolveResult.error(
+                return ResolveResult.error(Errors.outOfBounds(fullPath, index, list.size()));
-                    "[" + index + "] is out of bounds for array with length [" + list.size() + "] as part of path [" + fullPath + "]"
+            } else {
-                );
+                return ResolveResult.success(list.get(index));
             }
-            return ResolveResult.success(list.get(index));
+        } else {
+            return ResolveResult.error(Errors.cannotResolve(fullPath, pathElement, context));
         }
-        return ResolveResult.error(
-            "cannot resolve ["
-                + pathElement
-                + "] from object of type ["
-                + context.getClass().getName()
-                + "] as part of path ["
-                + fullPath
-                + "]"
-        );
     }
 
     /**
@@ -515,7 +484,6 @@ public final class IngestDocument {
                 return;
             }
         }
-
         setFieldValue(path, value);
     }
 
@@ -540,7 +508,6 @@ public final class IngestDocument {
                 }
             }
         }
-
         setFieldValue(path, value);
     }
 
@@ -550,9 +517,8 @@ public final class IngestDocument {
         for (int i = 0; i < fieldPath.pathElements.length - 1; i++) {
             String pathElement = fieldPath.pathElements[i];
             if (context == null) {
-                throw new IllegalArgumentException("cannot resolve [" + pathElement + "] from null as part of path [" + path + "]");
+                throw new IllegalArgumentException(Errors.cannotResolve(path, pathElement, null));
-            }
+            } else if (context instanceof Map<?, ?>) {
-            if (context instanceof Map) {
                 @SuppressWarnings("unchecked")
                 Map<String, Object> map = (Map<String, Object>) context;
                 Object object = map.getOrDefault(pathElement, NOT_FOUND); // getOrDefault is faster than containsKey + get
@@ -568,35 +534,22 @@ public final class IngestDocument {
                 try {
                     index = Integer.parseInt(pathElement);
                 } catch (NumberFormatException e) {
-                    throw new IllegalArgumentException(
+                    throw new IllegalArgumentException(Errors.notInteger(path, pathElement), e);
-                        "[" + pathElement + "] is not an integer, cannot be used as an index as part of path [" + path + "]",
-                        e
-                    );
                 }
                 if (index < 0 || index >= list.size()) {
-                    throw new IllegalArgumentException(
+                    throw new IllegalArgumentException(Errors.outOfBounds(path, index, list.size()));
-                        "[" + index + "] is out of bounds for array with length [" + list.size() + "] as part of path [" + path + "]"
+                } else {
-                    );
+                    context = list.get(index);
                 }
-                context = list.get(index);
             } else {
-                throw new IllegalArgumentException(
+                throw new IllegalArgumentException(Errors.cannotResolve(path, pathElement, context));
-                    "cannot resolve ["
-                        + pathElement
-                        + "] from object of type ["
-                        + context.getClass().getName()
-                        + "] as part of path ["
-                        + path
-                        + "]"
-                );
             }
         }
 
         String leafKey = fieldPath.pathElements[fieldPath.pathElements.length - 1];
         if (context == null) {
-            throw new IllegalArgumentException("cannot set [" + leafKey + "] with null parent as part of path [" + path + "]");
+            throw new IllegalArgumentException(Errors.cannotSet(path, leafKey, null));
-        }
+        } else if (context instanceof Map<?, ?>) {
-        if (context instanceof Map) {
             @SuppressWarnings("unchecked")
             Map<String, Object> map = (Map<String, Object>) context;
             if (append) {
@@ -614,42 +567,30 @@ public final class IngestDocument {
                 return;
             }
             map.put(leafKey, value);
-        } else if (context instanceof List) {
+        } else if (context instanceof List<?>) {
             @SuppressWarnings("unchecked")
             List<Object> list = (List<Object>) context;
             int index;
             try {
                 index = Integer.parseInt(leafKey);
             } catch (NumberFormatException e) {
-                throw new IllegalArgumentException(
+                throw new IllegalArgumentException(Errors.notInteger(path, leafKey), e);
-                    "[" + leafKey + "] is not an integer, cannot be used as an index as part of path [" + path + "]",
-                    e
-                );
             }
             if (index < 0 || index >= list.size()) {
-                throw new IllegalArgumentException(
+                throw new IllegalArgumentException(Errors.outOfBounds(path, index, list.size()));
-                    "[" + index + "] is out of bounds for array with length [" + list.size() + "] as part of path [" + path + "]"
+            } else {
-                );
+                if (append) {
-            }
+                    Object object = list.get(index);
-            if (append) {
+                    Object newList = appendValues(object, value, allowDuplicates);
-                Object object = list.get(index);
+                    if (newList != object) {
-                Object newList = appendValues(object, value, allowDuplicates);
+                        list.set(index, newList);
-                if (newList != object) {
+                    }
-                    list.set(index, newList);
+                    return;
                 }
-                return;
+                list.set(index, value);
             }
-            list.set(index, value);
         } else {
-            throw new IllegalArgumentException(
+            throw new IllegalArgumentException(Errors.cannotSet(path, leafKey, context));
-                "cannot set ["
-                    + leafKey
-                    + "] with parent object of type ["
-                    + context.getClass().getName()
-                    + "] as part of path ["
-                    + path
-                    + "]"
-            );
         }
     }
 
@@ -706,9 +647,7 @@ public final class IngestDocument {
         if (clazz.isInstance(object)) {
             return clazz.cast(object);
         }
-        throw new IllegalArgumentException(
+        throw new IllegalArgumentException(Errors.cannotCast(path, object, clazz));
-            "field [" + path + "] of type [" + object.getClass().getName() + "] cannot be cast to [" + clazz.getName() + "]"
-        );
     }
 
     /**
@@ -824,15 +763,12 @@ public final class IngestDocument {
     @SuppressWarnings("unchecked")
     private static Set<String> getAllFields(Map<String, Object> input, String prefix) {
         Set<String> allFields = Sets.newHashSet();
-
         input.forEach((k, v) -> {
             allFields.add(prefix + k);
-
             if (v instanceof Map<?, ?> mapValue) {
                 allFields.addAll(getAllFields((Map<String, Object>) mapValue, prefix + k + "."));
             }
         });
-
         return allFields;
     }
 
@@ -1055,24 +991,13 @@ public final class IngestDocument {
         }
     }
 
-    private static class ResolveResult {
+    private record ResolveResult(boolean wasSuccessful, Object resolvedObject, String errorMessage) {
-        boolean wasSuccessful;
-        String errorMessage;
-        Object resolvedObject;
-
         static ResolveResult success(Object resolvedObject) {
-            ResolveResult result = new ResolveResult();
+            return new ResolveResult(true, resolvedObject, null);
-            result.wasSuccessful = true;
-            result.resolvedObject = resolvedObject;
-            return result;
         }
 
         static ResolveResult error(String errorMessage) {
-            ResolveResult result = new ResolveResult();
+            return new ResolveResult(false, null, errorMessage);
-            result.wasSuccessful = false;
-            result.errorMessage = errorMessage;
-            return result;
-
         }
     }
 
@@ -1153,4 +1078,57 @@ public final class IngestDocument {
             throw new UnsupportedOperationException();
         }
     }
+
+    private static final class Errors {
+        private Errors() {
+            // utility class
+        }
+
+        private static String cannotCast(String path, Object value, Class<?> clazz) {
+            return "field [" + path + "] of type [" + value.getClass().getName() + "] cannot be cast to [" + clazz.getName() + "]";
+        }
+
+        private static String cannotRemove(String path, String key, Object value) {
+            if (value == null) {
+                return "cannot remove [" + key + "] from null as part of path [" + path + "]";
+            } else {
+                final String type = value.getClass().getName();
+                return "cannot remove [" + key + "] from object of type [" + type + "] as part of path [" + path + "]";
+            }
+        }
+
+        private static String cannotResolve(String path, String key, Object value) {
+            if (value == null) {
+                return "cannot resolve [" + key + "] from null as part of path [" + path + "]";
+            } else {
+                final String type = value.getClass().getName();
+                return "cannot resolve [" + key + "] from object of type [" + type + "] as part of path [" + path + "]";
+            }
+        }
+
+        private static String cannotSet(String path, String key, Object value) {
+            if (value == null) {
+                return "cannot set [" + key + "] with null parent as part of path [" + path + "]";
+            } else {
+                final String type = value.getClass().getName();
+                return "cannot set [" + key + "] with parent object of type [" + type + "] as part of path [" + path + "]";
+            }
+        }
+
+        private static String outOfBounds(String path, int index, int length) {
+            return "[" + index + "] is out of bounds for array with length [" + length + "] as part of path [" + path + "]";
+        }
+
+        private static String notInteger(String path, String key) {
+            return "[" + key + "] is not an integer, cannot be used as an index as part of path [" + path + "]";
+        }
+
+        private static String notPresent(String path, String key) {
+            return "field [" + key + "] not present as part of path [" + path + "]";
+        }
+
+        private static String notStringOrByteArray(String path, Object value) {
+            return "Content field [" + path + "] of unknown type [" + value.getClass().getName() + "], must be string or byte array";
+        }
+    }
 }

server/src/test/java/org/elasticsearch/ingest/ConfigurationUtilsTests.java

@@ -290,6 +290,7 @@ public class ConfigurationUtilsTests extends ESTestCase {
             propertyValue,
             scriptService
         );
+        assertThat(result, instanceOf(ConfigurationUtils.ConstantTemplateScriptFactory.class));
         assertThat(result.newInstance(null).execute(), equalTo(propertyValue));
         verify(scriptService, times(0)).compile(any(), any());
     }