From d5c2ef4af3639a7af4860cb371e12f49964f0137 Mon Sep 17 00:00:00 2001 From: James Baiera Date: Mon, 25 Aug 2025 15:53:10 -0400 Subject: [PATCH 1/8] Update FieldPath to parse array syntax --- .../elasticsearch/ingest/IngestDocument.java | 102 +++++++++++++++--- 1 file changed, 88 insertions(+), 14 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java index a9a738bfdb806..97a04a94bbb73 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java @@ -43,6 +43,7 @@ import java.util.Set; import java.util.function.BiConsumer; import java.util.stream.Collectors; +import java.util.stream.Stream; /** * Represents a single document being captured before indexing and holds the source and metadata (like id, type and index). @@ -201,7 +202,7 @@ public T getFieldValue(String path, Class clazz) { * or if the field that is found at the provided path is not of the expected type. */ public T getFieldValue(String path, Class clazz, boolean ignoreMissing) { - final FieldPath fieldPath = FieldPath.of(path); + final FieldPath fieldPath = FieldPath.of(path, getCurrentAccessPattern()); Object context = fieldPath.initialContext(this); ResolveResult result = resolve(fieldPath.pathElements, fieldPath.pathElements.length, path, context, getCurrentAccessPatternSafe()); if (result.wasSuccessful) { @@ -270,7 +271,7 @@ public boolean hasField(String path) { * @throws IllegalArgumentException if the path is null, empty or invalid. */ public boolean hasField(String path, boolean failOutOfRange) { - final FieldPath fieldPath = FieldPath.of(path); + final FieldPath fieldPath = FieldPath.of(path, getCurrentAccessPattern()); Object context = fieldPath.initialContext(this); int leafKeyIndex = fieldPath.pathElements.length - 1; int lastContainerIndex = fieldPath.pathElements.length - 2; @@ -424,7 +425,7 @@ public void removeField(String path) { * @throws IllegalArgumentException if the path is null, empty, or invalid; or if the field doesn't exist (and ignoreMissing is false). */ public void removeField(String path, boolean ignoreMissing) { - final FieldPath fieldPath = FieldPath.of(path); + final FieldPath fieldPath = FieldPath.of(path, getCurrentAccessPattern()); Object context = fieldPath.initialContext(this); String leafKey = fieldPath.pathElements[fieldPath.pathElements.length - 1]; ResolveResult result = resolve( @@ -734,7 +735,7 @@ public void setFieldValue(String path, Object value, boolean ignoreEmptyValue) { } private void setFieldValue(String path, Object value, boolean append, boolean allowDuplicates) { - final FieldPath fieldPath = FieldPath.of(path); + final FieldPath fieldPath = FieldPath.of(path, getCurrentAccessPattern()); Object context = fieldPath.initialContext(this); int leafKeyIndex = fieldPath.pathElements.length - 1; int lastContainerIndex = fieldPath.pathElements.length - 2; @@ -1288,8 +1289,37 @@ public String getFieldName() { private static final class FieldPath { + private record Element(String fieldName, Integer arrayIndex) { + private static final String EMPTY_STRING = ""; + + static Element field(String fieldName) { + Objects.requireNonNull(fieldName, "fieldName cannot be null"); + if (fieldName.isEmpty()) { + throw new IllegalArgumentException("fieldName cannot be empty"); + } + return new Element(fieldName, null); + } + + static Element index(int arrayIndex) { + if (arrayIndex < 0) { + throw new IndexOutOfBoundsException(arrayIndex); + } + return new Element(EMPTY_STRING, arrayIndex); + } + + boolean isFieldName() { + return fieldName.isEmpty() == false && arrayIndex == null; + } + + boolean isArrayIndex() { + return fieldName.isEmpty() && + } + } + + private record CacheKey(String path, IngestPipelineFieldAccessPattern accessPattern) {} + private static final int MAX_SIZE = 512; - private static final Map CACHE = ConcurrentCollections.newConcurrentMapWithAggressiveConcurrency(); + private static final Map CACHE = ConcurrentCollections.newConcurrentMapWithAggressiveConcurrency(); // constructing a new FieldPath requires that we parse a String (e.g. "foo.bar.baz") into an array // of path elements (e.g. ["foo", "bar", "baz"]). Calling String#split results in the allocation @@ -1298,27 +1328,28 @@ private static final class FieldPath { // do some processing ourselves on the path and path elements to validate and prepare them. // the above CACHE and the below 'FieldPath.of' method allow us to almost always avoid this work. - static FieldPath of(String path) { + static FieldPath of(String path, IngestPipelineFieldAccessPattern accessPattern) { if (Strings.isEmpty(path)) { throw new IllegalArgumentException("path cannot be null nor empty"); } - FieldPath res = CACHE.get(path); + CacheKey cacheKey = new CacheKey(path, accessPattern); + FieldPath res = CACHE.get(cacheKey); if (res != null) { return res; } - res = new FieldPath(path); + res = new FieldPath(path, accessPattern); if (CACHE.size() > MAX_SIZE) { CACHE.clear(); } - CACHE.put(path, res); + CACHE.put(cacheKey, res); return res; } - private final String[] pathElements; + private final Element[] pathElements; private final boolean useIngestContext; // you shouldn't call this directly, use the FieldPath.of method above instead! - private FieldPath(String path) { + private FieldPath(String path, IngestPipelineFieldAccessPattern accessPattern) { String newPath; if (path.startsWith(INGEST_KEY_PREFIX)) { useIngestContext = true; @@ -1331,10 +1362,53 @@ private FieldPath(String path) { newPath = path; } } - this.pathElements = newPath.split("\\."); - if (pathElements.length == 1 && pathElements[0].isEmpty()) { - throw new IllegalArgumentException("path [" + path + "] is not valid"); + String[] pathParts = newPath.split("\\."); + this.pathElements = processPathParts(path, pathParts, accessPattern); + } + + private static Element[] processPathParts(String fullPath, String[] pathParts, IngestPipelineFieldAccessPattern accessPattern) { + if (pathParts.length == 1 && pathParts[0].isEmpty()) { + throw new IllegalArgumentException("path [" + fullPath + "] is not valid"); } + return Arrays.stream(pathParts) + .flatMap(pathPart -> { + int openBracket = pathPart.indexOf('['); + if (openBracket == -1) { + return Stream.of(Element.field(pathPart)); + } else if (openBracket == 0) { + throw new IllegalArgumentException("path [" + fullPath + "] is not valid"); + } else { + List resultElements = new ArrayList<>(); + String rootField = pathPart.substring(0, openBracket); + resultElements.add(Element.field(rootField)); + + boolean elementsRemain = true; + while (elementsRemain) { + int closeBracket = pathPart.indexOf(']', openBracket); + if (closeBracket <= openBracket) { + throw new IllegalArgumentException("path [" + fullPath + "] is not valid"); + } + + String rawIndex = pathPart.substring(openBracket + 1, closeBracket); + try { + resultElements.add(Element.index(Integer.parseInt(rawIndex))); + } catch (NumberFormatException numberFormatException) { + throw new IllegalArgumentException("path [" + fullPath + "] is not valid"); + } + + if (closeBracket == pathPart.length() - 1) { + elementsRemain = false; + } else { + if (pathPart.charAt(closeBracket + 1) != '[') { + throw new IllegalArgumentException("path [" + fullPath + "] is not valid"); + } + openBracket = closeBracket + 1; + } + } + return resultElements.stream(); + } + }) + .toArray(Element[]::new); } public Object initialContext(IngestDocument document) { From 5a8478e1687fb27988b0f0f606c6c975911ec707 Mon Sep 17 00:00:00 2001 From: James Baiera Date: Tue, 26 Aug 2025 15:50:18 -0400 Subject: [PATCH 2/8] Only parse the paths in the new way when flexible is enabled --- .../elasticsearch/ingest/IngestDocument.java | 43 ++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java index 97a04a94bbb73..ed85098fdff13 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java @@ -1292,6 +1292,11 @@ private static final class FieldPath { private record Element(String fieldName, Integer arrayIndex) { private static final String EMPTY_STRING = ""; + /** + * Creates a new FieldPath Element which corresponds to a regular part of a field path. + * @param fieldName name of the field to access, or a string to use for array indexing if running in CLASSIC access pattern. + * @return A field name element + */ static Element field(String fieldName) { Objects.requireNonNull(fieldName, "fieldName cannot be null"); if (fieldName.isEmpty()) { @@ -1300,6 +1305,12 @@ static Element field(String fieldName) { return new Element(fieldName, null); } + /** + * Creates a new FieldPath Element which corresponds to an array index specified by the square bracket syntax available + * when using the {@link IngestPipelineFieldAccessPattern#FLEXIBLE} access pattern. + * @param arrayIndex array index specified in square brackets + * @return An array index element + */ static Element index(int arrayIndex) { if (arrayIndex < 0) { throw new IndexOutOfBoundsException(arrayIndex); @@ -1307,15 +1318,31 @@ static Element index(int arrayIndex) { return new Element(EMPTY_STRING, arrayIndex); } + /** + * @return true if this element is for accessing a regular field + */ boolean isFieldName() { return fieldName.isEmpty() == false && arrayIndex == null; } + /** + * @return true if this element is for an array index used for the FLEXIBLE access pattern + */ boolean isArrayIndex() { - return fieldName.isEmpty() && + return isFieldName() == false; + } + + @Override + public String toString() { + return isFieldName() ? fieldName : "[" + arrayIndex + "]"; } } + /** + * A compound cache key for tracking previously parsed field paths + * @param path The field path as given by the caller + * @param accessPattern The access pattern used to parse the field path + */ private record CacheKey(String path, IngestPipelineFieldAccessPattern accessPattern) {} private static final int MAX_SIZE = 512; @@ -1370,6 +1397,20 @@ private static Element[] processPathParts(String fullPath, String[] pathParts, I if (pathParts.length == 1 && pathParts[0].isEmpty()) { throw new IllegalArgumentException("path [" + fullPath + "] is not valid"); } + return switch (accessPattern) { + case CLASSIC -> Arrays.stream(pathParts).map(Element::field).toArray(Element[]::new); + case FLEXIBLE -> parseFlexibleFields(fullPath, pathParts); + }; + } + + /** + * Parses path syntax that is specific to the {@link IngestPipelineFieldAccessPattern#FLEXIBLE} ingest doc access pattern. Supports + * syntax like square bracket array access, which is the only way to index arrays in flexible mode. + * @param fullPath The un-split path to use for error messages + * @param pathParts The tokenized field path to parse + * @return An array of Elements + */ + private static Element[] parseFlexibleFields(String fullPath, String[] pathParts) { return Arrays.stream(pathParts) .flatMap(pathPart -> { int openBracket = pathPart.indexOf('['); From d3dc2a9f650ba40cb2575e2f1bf0a9cab948ca81 Mon Sep 17 00:00:00 2001 From: James Baiera Date: Fri, 29 Aug 2025 03:22:28 -0400 Subject: [PATCH 3/8] Make access pattern optional --- .../elasticsearch/ingest/IngestDocument.java | 17 +++++++++-------- .../ingest/IngestDocumentTests.java | 9 +++++---- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java index ed85098fdff13..adbf107319b94 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java @@ -40,6 +40,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.function.BiConsumer; import java.util.stream.Collectors; @@ -202,7 +203,7 @@ public T getFieldValue(String path, Class clazz) { * or if the field that is found at the provided path is not of the expected type. */ public T getFieldValue(String path, Class clazz, boolean ignoreMissing) { - final FieldPath fieldPath = FieldPath.of(path, getCurrentAccessPattern()); + final FieldPath fieldPath = FieldPath.of(path, getCurrentAccessPatternSafe()); Object context = fieldPath.initialContext(this); ResolveResult result = resolve(fieldPath.pathElements, fieldPath.pathElements.length, path, context, getCurrentAccessPatternSafe()); if (result.wasSuccessful) { @@ -271,7 +272,7 @@ public boolean hasField(String path) { * @throws IllegalArgumentException if the path is null, empty or invalid. */ public boolean hasField(String path, boolean failOutOfRange) { - final FieldPath fieldPath = FieldPath.of(path, getCurrentAccessPattern()); + final FieldPath fieldPath = FieldPath.of(path, getCurrentAccessPatternSafe()); Object context = fieldPath.initialContext(this); int leafKeyIndex = fieldPath.pathElements.length - 1; int lastContainerIndex = fieldPath.pathElements.length - 2; @@ -425,7 +426,7 @@ public void removeField(String path) { * @throws IllegalArgumentException if the path is null, empty, or invalid; or if the field doesn't exist (and ignoreMissing is false). */ public void removeField(String path, boolean ignoreMissing) { - final FieldPath fieldPath = FieldPath.of(path, getCurrentAccessPattern()); + final FieldPath fieldPath = FieldPath.of(path, getCurrentAccessPatternSafe()); Object context = fieldPath.initialContext(this); String leafKey = fieldPath.pathElements[fieldPath.pathElements.length - 1]; ResolveResult result = resolve( @@ -735,7 +736,7 @@ public void setFieldValue(String path, Object value, boolean ignoreEmptyValue) { } private void setFieldValue(String path, Object value, boolean append, boolean allowDuplicates) { - final FieldPath fieldPath = FieldPath.of(path, getCurrentAccessPattern()); + final FieldPath fieldPath = FieldPath.of(path, getCurrentAccessPatternSafe()); Object context = fieldPath.initialContext(this); int leafKeyIndex = fieldPath.pathElements.length - 1; int lastContainerIndex = fieldPath.pathElements.length - 2; @@ -1154,10 +1155,10 @@ List getPipelineStack() { } /** - * @return The access pattern for any currently executing pipelines, or null if no pipelines are in progress for this doc + * @return The access pattern for any currently executing pipelines, or empty if no pipelines are in progress for this doc */ - public IngestPipelineFieldAccessPattern getCurrentAccessPattern() { - return accessPatternStack.peek(); + public Optional getCurrentAccessPattern() { + return Optional.ofNullable(accessPatternStack.peek()); } /** @@ -1165,7 +1166,7 @@ public IngestPipelineFieldAccessPattern getCurrentAccessPattern() { * pipelines are in progress for this doc for the sake of backwards compatibility */ private IngestPipelineFieldAccessPattern getCurrentAccessPatternSafe() { - return Objects.requireNonNullElse(getCurrentAccessPattern(), IngestPipelineFieldAccessPattern.CLASSIC); + return getCurrentAccessPattern().orElse(IngestPipelineFieldAccessPattern.CLASSIC); } /** diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java index 2e308b91c58df..fc76ee90ba6d4 100644 --- a/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java @@ -2034,7 +2034,7 @@ public void testNestedAccessPatternPropagation() { // At the end of the test, there should be neither pipeline ids nor access patterns left in the stack. assertThat(document.getPipelineStack(), is(empty())); - assertThat(document.getCurrentAccessPattern(), is(nullValue())); + assertThat(document.getCurrentAccessPattern().isEmpty(), is(true)); } /** @@ -2082,7 +2082,8 @@ void doTestNestedAccessPatternPropagation(int level, int maxCallDepth, IngestDoc // Assert expected state assertThat(document.getPipelineStack().getFirst(), is(expectedPipelineId)); - assertThat(document.getCurrentAccessPattern(), is(expectedAccessPattern)); + assertThat(document.getCurrentAccessPattern().isPresent(), is(true)); + assertThat(document.getCurrentAccessPattern().get(), is(expectedAccessPattern)); // Randomly recurse: We recurse only one time per level to avoid hogging test time, but we randomize which // pipeline to recurse on, eventually requiring a recursion on the last pipeline run if one hasn't happened yet. @@ -2099,11 +2100,11 @@ void doTestNestedAccessPatternPropagation(int level, int maxCallDepth, IngestDoc assertThat(document.getPipelineStack().size(), is(equalTo(level))); if (level == 0) { // Top level means access pattern should be empty - assertThat(document.getCurrentAccessPattern(), is(nullValue())); + assertThat(document.getCurrentAccessPattern().isEmpty(), is(true)); } else { // If we're nested below the top level we should still have an access // pattern on the document for the pipeline above us - assertThat(document.getCurrentAccessPattern(), is(not(nullValue()))); + assertThat(document.getCurrentAccessPattern().isPresent(), is(true)); } } logger.debug("LEVEL {}/{}: COMPLETE", level, maxCallDepth); From 4f4e7a48673e1ab0e2913b7703a4702dcd8152a9 Mon Sep 17 00:00:00 2001 From: James Baiera Date: Thu, 4 Sep 2025 15:43:16 -0400 Subject: [PATCH 4/8] Throw on square brackets in field path --- .../elasticsearch/ingest/IngestDocument.java | 106 +++--------------- 1 file changed, 13 insertions(+), 93 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java index adbf107319b94..f0e9ec0a24b09 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java @@ -44,7 +44,6 @@ import java.util.Set; import java.util.function.BiConsumer; import java.util.stream.Collectors; -import java.util.stream.Stream; /** * Represents a single document being captured before indexing and holds the source and metadata (like id, type and index). @@ -1290,55 +1289,6 @@ public String getFieldName() { private static final class FieldPath { - private record Element(String fieldName, Integer arrayIndex) { - private static final String EMPTY_STRING = ""; - - /** - * Creates a new FieldPath Element which corresponds to a regular part of a field path. - * @param fieldName name of the field to access, or a string to use for array indexing if running in CLASSIC access pattern. - * @return A field name element - */ - static Element field(String fieldName) { - Objects.requireNonNull(fieldName, "fieldName cannot be null"); - if (fieldName.isEmpty()) { - throw new IllegalArgumentException("fieldName cannot be empty"); - } - return new Element(fieldName, null); - } - - /** - * Creates a new FieldPath Element which corresponds to an array index specified by the square bracket syntax available - * when using the {@link IngestPipelineFieldAccessPattern#FLEXIBLE} access pattern. - * @param arrayIndex array index specified in square brackets - * @return An array index element - */ - static Element index(int arrayIndex) { - if (arrayIndex < 0) { - throw new IndexOutOfBoundsException(arrayIndex); - } - return new Element(EMPTY_STRING, arrayIndex); - } - - /** - * @return true if this element is for accessing a regular field - */ - boolean isFieldName() { - return fieldName.isEmpty() == false && arrayIndex == null; - } - - /** - * @return true if this element is for an array index used for the FLEXIBLE access pattern - */ - boolean isArrayIndex() { - return isFieldName() == false; - } - - @Override - public String toString() { - return isFieldName() ? fieldName : "[" + arrayIndex + "]"; - } - } - /** * A compound cache key for tracking previously parsed field paths * @param path The field path as given by the caller @@ -1373,7 +1323,7 @@ static FieldPath of(String path, IngestPipelineFieldAccessPattern accessPattern) return res; } - private final Element[] pathElements; + private final String[] pathElements; private final boolean useIngestContext; // you shouldn't call this directly, use the FieldPath.of method above instead! @@ -1394,12 +1344,12 @@ private FieldPath(String path, IngestPipelineFieldAccessPattern accessPattern) { this.pathElements = processPathParts(path, pathParts, accessPattern); } - private static Element[] processPathParts(String fullPath, String[] pathParts, IngestPipelineFieldAccessPattern accessPattern) { + private static String[] processPathParts(String fullPath, String[] pathParts, IngestPipelineFieldAccessPattern accessPattern) { if (pathParts.length == 1 && pathParts[0].isEmpty()) { throw new IllegalArgumentException("path [" + fullPath + "] is not valid"); } return switch (accessPattern) { - case CLASSIC -> Arrays.stream(pathParts).map(Element::field).toArray(Element[]::new); + case CLASSIC -> pathParts; case FLEXIBLE -> parseFlexibleFields(fullPath, pathParts); }; } @@ -1411,46 +1361,16 @@ private static Element[] processPathParts(String fullPath, String[] pathParts, I * @param pathParts The tokenized field path to parse * @return An array of Elements */ - private static Element[] parseFlexibleFields(String fullPath, String[] pathParts) { - return Arrays.stream(pathParts) - .flatMap(pathPart -> { - int openBracket = pathPart.indexOf('['); - if (openBracket == -1) { - return Stream.of(Element.field(pathPart)); - } else if (openBracket == 0) { - throw new IllegalArgumentException("path [" + fullPath + "] is not valid"); - } else { - List resultElements = new ArrayList<>(); - String rootField = pathPart.substring(0, openBracket); - resultElements.add(Element.field(rootField)); - - boolean elementsRemain = true; - while (elementsRemain) { - int closeBracket = pathPart.indexOf(']', openBracket); - if (closeBracket <= openBracket) { - throw new IllegalArgumentException("path [" + fullPath + "] is not valid"); - } - - String rawIndex = pathPart.substring(openBracket + 1, closeBracket); - try { - resultElements.add(Element.index(Integer.parseInt(rawIndex))); - } catch (NumberFormatException numberFormatException) { - throw new IllegalArgumentException("path [" + fullPath + "] is not valid"); - } - - if (closeBracket == pathPart.length() - 1) { - elementsRemain = false; - } else { - if (pathPart.charAt(closeBracket + 1) != '[') { - throw new IllegalArgumentException("path [" + fullPath + "] is not valid"); - } - openBracket = closeBracket + 1; - } - } - return resultElements.stream(); - } - }) - .toArray(Element[]::new); + private static String[] parseFlexibleFields(String fullPath, String[] pathParts) { + boolean invalidPath = Arrays.stream(pathParts).anyMatch(pathPart -> { + int openBracket = pathPart.indexOf('['); + int closedBracket = pathPart.indexOf(']'); + return openBracket != -1 && closedBracket != -1; + }); + if (invalidPath) { + throw new IllegalArgumentException("path [" + fullPath + "] is not valid"); + } + return pathParts; } public Object initialContext(IngestDocument document) { From c405ce2c291c8bb4f80737ea3718c4139f256aaf Mon Sep 17 00:00:00 2001 From: James Baiera Date: Thu, 4 Sep 2025 16:24:49 -0400 Subject: [PATCH 5/8] Add tests --- .../ingest/IngestDocumentTests.java | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java index fc76ee90ba6d4..81995a535f48f 100644 --- a/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java @@ -189,6 +189,90 @@ private void doWithRandomAccessPattern(Consumer action) throws E doWithAccessPattern(randomFrom(IngestPipelineFieldAccessPattern.values()), action); } + private void assertPathValid(IngestDocument doc, String path) { + // The fields being checked do not exist, so they all return false when running hasField + assertFalse(doc.hasField(path)); + } + + private void assertPathInvalid(IngestDocument doc, String path, String errorMessage) { + IllegalArgumentException expected = expectThrows(IllegalArgumentException.class, () -> doc.hasField(path)); + assertThat(expected.getMessage(), equalTo(errorMessage)); + } + + public void testPathParsingLogic() throws Exception { + // Force a blank document for this test + document = new IngestDocument("index", "id", 1, null, null, new HashMap<>()); + + doWithRandomAccessPattern((doc) -> { + assertPathInvalid(doc, null, "path cannot be null nor empty"); + assertPathInvalid(doc, "", "path cannot be null nor empty"); + assertPathValid(doc, "a"); + assertPathValid(doc, "ab"); + assertPathValid(doc, "abc"); + assertPathValid(doc, "a.b"); + assertPathValid(doc, "a.b.c"); + // Trailing empty strings are trimmed by field path parsing logic + assertPathValid(doc, "a."); + assertPathValid(doc, "a.."); + assertPathValid(doc, "a..."); + // Empty field names are not allowed in the beginning or middle of the path though + assertPathInvalid(doc, ".a.b", "fieldName cannot be empty"); + assertPathInvalid(doc, "a..b", "fieldName cannot be empty"); + }); + + doWithAccessPattern(CLASSIC, (doc) -> { + // Classic allows number fields because they are treated as either field names or array indices depending on context + assertPathValid(doc, "a.0"); + // Classic allows square brackets because it is not part of it's syntax + assertPathValid(doc, "a[0]"); + assertPathValid(doc, "a[]"); + assertPathValid(doc, "a]["); + assertPathValid(doc, "["); + assertPathValid(doc, "a["); + assertPathValid(doc, "[a"); + assertPathValid(doc, "]"); + assertPathValid(doc, "a]"); + assertPathValid(doc, "]a"); + assertPathValid(doc, "[]"); + assertPathValid(doc, "]["); + assertPathValid(doc, "[a]"); + assertPathValid(doc, "]a["); + assertPathValid(doc, "[]a"); + assertPathValid(doc, "][a"); + }); + + doWithAccessPattern(FLEXIBLE, (doc) -> { + // Flexible has specific handling of square brackets + assertPathInvalid(doc, "a[0]", "path [a[0]] is not valid"); + assertPathInvalid(doc, "a[]", "path [a[]] is not valid"); + assertPathInvalid(doc, "a][", "path [a][] is not valid"); + assertPathInvalid(doc, "[", "path [[] is not valid"); + assertPathInvalid(doc, "a[", "path [a[] is not valid"); + assertPathInvalid(doc, "[a", "path [[a] is not valid"); + assertPathInvalid(doc, "]", "path []] is not valid"); + assertPathInvalid(doc, "a]", "path [a]] is not valid"); + assertPathInvalid(doc, "]a", "path []a] is not valid"); + assertPathInvalid(doc, "[]", "path [[]] is not valid"); + assertPathInvalid(doc, "][", "path [][] is not valid"); + assertPathInvalid(doc, "[a]", "path [[a]] is not valid"); + assertPathInvalid(doc, "]a[", "path []a[] is not valid"); + assertPathInvalid(doc, "[]a", "path [[]a] is not valid"); + assertPathInvalid(doc, "][a", "path [][a] is not valid"); + + assertPathInvalid(doc, "a[0].b", "path [a[0].b] is not valid"); + assertPathInvalid(doc, "a[0].b[1]", "path [a[0].b[1]] is not valid"); + assertPathInvalid(doc, "a[0].b[1].c", "path [a[0].b[1].c] is not valid"); + assertPathInvalid(doc, "a[0].b[1].c[2]", "path [a[0].b[1].c[2]] is not valid"); + assertPathInvalid(doc, "a[0][1].c[2]", "path [a[0][1].c[2]] is not valid"); + assertPathInvalid(doc, "a[0].b[1][2]", "path [a[0].b[1][2]] is not valid"); + assertPathInvalid(doc, "a[0][1][2]", "path [a[0][1][2]] is not valid"); + + assertPathInvalid(doc, "a[0][", "path [a[0][] is not valid"); + assertPathInvalid(doc, "a[0]]", "path [a[0]]] is not valid"); + assertPathInvalid(doc, "a[0]blahblah", "path [a[0]blahblah] is not valid"); + }); + } + public void testSimpleGetFieldValue() throws Exception { doWithRandomAccessPattern((doc) -> { assertThat(doc.getFieldValue("foo", String.class), equalTo("bar")); From 646405a1547ef27c225fcab88bf4062b30b7e4c9 Mon Sep 17 00:00:00 2001 From: James Baiera Date: Fri, 5 Sep 2025 02:21:15 -0400 Subject: [PATCH 6/8] Fix test issue --- .../elasticsearch/ingest/IngestDocument.java | 31 +++++++++++++------ .../ingest/IngestDocumentTests.java | 4 +-- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java index f0e9ec0a24b09..01bda018bf46d 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java @@ -1349,26 +1349,39 @@ private static String[] processPathParts(String fullPath, String[] pathParts, In throw new IllegalArgumentException("path [" + fullPath + "] is not valid"); } return switch (accessPattern) { - case CLASSIC -> pathParts; + case CLASSIC -> validateClassicFields(fullPath, pathParts); case FLEXIBLE -> parseFlexibleFields(fullPath, pathParts); }; } + /** + * Parses path syntax that is specific to the {@link IngestPipelineFieldAccessPattern#CLASSIC} ingest doc access pattern. Supports + * syntax like context aware array access. + * @param fullPath The un-split path to use for error messages + * @param pathParts The tokenized field path to parse + * @return An array of Strings + */ + private static String[] validateClassicFields(String fullPath, String[] pathParts) { + for (String pathPart : pathParts) { + if (pathPart.isEmpty()) { + throw new IllegalArgumentException("path [" + fullPath + "] is not valid"); + } + } + return pathParts; + } + /** * Parses path syntax that is specific to the {@link IngestPipelineFieldAccessPattern#FLEXIBLE} ingest doc access pattern. Supports * syntax like square bracket array access, which is the only way to index arrays in flexible mode. * @param fullPath The un-split path to use for error messages * @param pathParts The tokenized field path to parse - * @return An array of Elements + * @return An array of Strings */ private static String[] parseFlexibleFields(String fullPath, String[] pathParts) { - boolean invalidPath = Arrays.stream(pathParts).anyMatch(pathPart -> { - int openBracket = pathPart.indexOf('['); - int closedBracket = pathPart.indexOf(']'); - return openBracket != -1 && closedBracket != -1; - }); - if (invalidPath) { - throw new IllegalArgumentException("path [" + fullPath + "] is not valid"); + for (String pathPart : pathParts) { + if (pathPart.isEmpty() || pathPart.indexOf('[') >= 0 || pathPart.indexOf(']') >= 0) { + throw new IllegalArgumentException("path [" + fullPath + "] is not valid"); + } } return pathParts; } diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java index 81995a535f48f..4d09ed8cae9ea 100644 --- a/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java @@ -216,8 +216,8 @@ public void testPathParsingLogic() throws Exception { assertPathValid(doc, "a.."); assertPathValid(doc, "a..."); // Empty field names are not allowed in the beginning or middle of the path though - assertPathInvalid(doc, ".a.b", "fieldName cannot be empty"); - assertPathInvalid(doc, "a..b", "fieldName cannot be empty"); + assertPathInvalid(doc, ".a.b", "path [.a.b] is not valid"); + assertPathInvalid(doc, "a..b", "path [a..b] is not valid"); }); doWithAccessPattern(CLASSIC, (doc) -> { From 7de9e84d725cc63ef740dba794f474312fbb1b26 Mon Sep 17 00:00:00 2001 From: James Baiera Date: Fri, 12 Sep 2025 02:24:24 -0400 Subject: [PATCH 7/8] Code cleanup --- .../src/main/java/org/elasticsearch/ingest/IngestDocument.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java index 3597808252325..1cc12cc0c85e5 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java @@ -1381,7 +1381,7 @@ private static String[] validateClassicFields(String fullPath, String[] pathPart */ private static String[] parseFlexibleFields(String fullPath, String[] pathParts) { for (String pathPart : pathParts) { - if (pathPart.isEmpty() || pathPart.indexOf('[') >= 0 || pathPart.indexOf(']') >= 0) { + if (pathPart.isEmpty() || pathPart.contains("[") || pathPart.contains("]")) { throw new IllegalArgumentException("path [" + fullPath + "] is not valid"); } } From 7a454dd3c7f81ead87ea1cec3ff945f93ada0a87 Mon Sep 17 00:00:00 2001 From: James Baiera Date: Fri, 12 Sep 2025 02:25:47 -0400 Subject: [PATCH 8/8] Remove redundant code --- .../src/main/java/org/elasticsearch/ingest/IngestDocument.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java index 1cc12cc0c85e5..b97968de8da14 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java @@ -1347,9 +1347,6 @@ private FieldPath(String path, IngestPipelineFieldAccessPattern accessPattern) { } private static String[] processPathParts(String fullPath, String[] pathParts, IngestPipelineFieldAccessPattern accessPattern) { - if (pathParts.length == 1 && pathParts[0].isEmpty()) { - throw new IllegalArgumentException("path [" + fullPath + "] is not valid"); - } return switch (accessPattern) { case CLASSIC -> validateClassicFields(fullPath, pathParts); case FLEXIBLE -> parseFlexibleFields(fullPath, pathParts);