Grok returns a list of matches for repeated pattern names elastic#92092 (elastic#92586)

Grok returns a list of matches for repeated pattern names

This change makes the Elasticsearch Grok processor behaves in the 
same way that Logstash's grok, when handling repeated pattern 
names, returning a list of matches instead only the first only

Closes elastic#92092

Author: Pablo Alcantar Morales

docs/changelog/92586.yaml

@@ -0,0 +1,6 @@
+pr: 92586
+summary: "Grok returns a list of matches for repeated pattern names #92092"
+area: Ingest Node
+type: bug
+issues:
+ - 92092

libs/grok/src/main/java/org/elasticsearch/grok/GrokCaptureExtracter.java

@@ -20,34 +20,55 @@
 /**
  * How to extract matches.
  */
-public abstract class GrokCaptureExtracter {
+public interface GrokCaptureExtracter {
+
     /**
      * Extract {@link Map} results. This implementation of {@link GrokCaptureExtracter}
      * is mutable and should be discarded after collecting a single result.
      */
-    static class MapExtracter extends GrokCaptureExtracter {
+    class MapExtracter implements GrokCaptureExtracter {
         private final Map<String, Object> result;
         private final List<GrokCaptureExtracter> fieldExtracters;
 
+        @SuppressWarnings("unchecked")
         MapExtracter(List<GrokCaptureConfig> captureConfig) {
             result = captureConfig.isEmpty() ? emptyMap() : new HashMap<>();
             fieldExtracters = new ArrayList<>(captureConfig.size());
             for (GrokCaptureConfig config : captureConfig) {
-                fieldExtracters.add(config.objectExtracter(v -> result.put(config.name(), v)));
+                fieldExtracters.add(config.objectExtracter(value -> {
+                    var key = config.name();
+
+                    // Logstash's Grok processor flattens the list of values to a single value in case there's only 1 match,
+                    // so we have to do the same to be compatible.
+                    // e.g.:
+                    // pattern = `%{SINGLEDIGIT:name}(%{SINGLEDIGIT:name})?`
+                    // - GROK(pattern, "1") => { name: 1 }
+                    // - GROK(pattern, "12") => { name: [1, 2] }
+                    if (result.containsKey(key)) {
+                        if (result.get(key)instanceof List<?> values) {
+                            ((ArrayList<Object>) values).add(value);
+                        } else {
+                            var values = new ArrayList<>();
+                            values.add(result.get(key));
+                            values.add(value);
+                            result.put(key, values);
+                        }
+                    } else {
+                        result.put(key, value);
+                    }
+                }));
             }
         }
 
         @Override
-        void extract(byte[] utf8Bytes, int offset, Region region) {
-            for (GrokCaptureExtracter extracter : fieldExtracters) {
-                extracter.extract(utf8Bytes, offset, region);
-            }
+        public void extract(byte[] utf8Bytes, int offset, Region region) {
+            fieldExtracters.forEach(extracter -> extracter.extract(utf8Bytes, offset, region));
         }
 
         Map<String, Object> result() {
             return result;
         }
     }
 
-    abstract void extract(byte[] utf8Bytes, int offset, Region region);
+    void extract(byte[] utf8Bytes, int offset, Region region);
 }

libs/grok/src/main/java/org/elasticsearch/grok/GrokCaptureType.java

@@ -9,7 +9,6 @@
 package org.elasticsearch.grok;
 
 import org.elasticsearch.grok.GrokCaptureConfig.NativeExtracterMap;
-import org.joni.Region;
 
 import java.nio.charset.StandardCharsets;
 import java.util.function.Consumer;
@@ -70,16 +69,12 @@ static GrokCaptureType fromString(String str) {
     }
 
     protected final GrokCaptureExtracter rawExtracter(int[] backRefs, Consumer<? super String> emit) {
-        return new GrokCaptureExtracter() {
-            @Override
-            void extract(byte[] utf8Bytes, int offset, Region region) {
-                for (int number : backRefs) {
-                    if (region.beg[number] >= 0) {
-                        int matchOffset = offset + region.beg[number];
-                        int matchLength = region.end[number] - region.beg[number];
-                        emit.accept(new String(utf8Bytes, matchOffset, matchLength, StandardCharsets.UTF_8));
-                        return; // Capture only the first value.
-                    }
+        return (utf8Bytes, offset, region) -> {
+            for (int number : backRefs) {
+                if (region.beg[number] >= 0) {
+                    int matchOffset = offset + region.beg[number];
+                    int matchLength = region.end[number] - region.beg[number];
+                    emit.accept(new String(utf8Bytes, matchOffset, matchLength, StandardCharsets.UTF_8));
                 }
             }
         };

libs/grok/src/test/java/org/elasticsearch/grok/GrokTests.java

@@ -801,9 +801,15 @@ public void testMultipleNamedCapturesWithSameName() {
         Grok grok = new Grok(bank, "%{SINGLEDIGIT:num}%{SINGLEDIGIT:num}", logger::warn);
         assertCaptureConfig(grok, Map.of("num", STRING));
 
-        Map<String, Object> expected = new HashMap<>();
-        expected.put("num", "1");
-        assertThat(grok.captures("12"), equalTo(expected));
+        assertThat(grok.captures("12"), equalTo(Map.of("num", List.of("1", "2"))));
+
+        grok = new Grok(bank, "%{SINGLEDIGIT:num:int}(%{SINGLEDIGIT:num:int})?", logger::warn);
+        assertCaptureConfig(grok, Map.of("num", INTEGER));
+        assertEquals(grok.captures("1"), Map.of("num", 1));
+        assertEquals(grok.captures("1a"), Map.of("num", 1));
+        assertEquals(grok.captures("a1"), Map.of("num", 1));
+        assertEquals(grok.captures("12"), Map.of("num", List.of(1, 2)));
+        assertEquals(grok.captures("123"), Map.of("num", List.of(1, 2)));
     }
 
     public void testExponentialExpressions() {

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

@@ -333,7 +333,7 @@ public void testCombineSamePatternNameAcrossPatterns() throws Exception {
         assertThat(doc.getFieldValue("second", String.class), equalTo("3"));
     }
 
-    public void testFirstWinNamedCapture() throws Exception {
+    public void testShouldCaptureAllSameNameGroupsAsList() throws Exception {
         String fieldName = RandomDocumentPicks.randomFieldName(random());
         IngestDocument doc = RandomDocumentPicks.randomIngestDocument(random(), new HashMap<>());
         doc.setFieldValue(fieldName, "12");
@@ -350,7 +350,7 @@ public void testFirstWinNamedCapture() throws Exception {
             MatcherWatchdog.noop()
         );
         processor.execute(doc);
-        assertThat(doc.getFieldValue("first", String.class), equalTo("1"));
+        assertEquals(doc.getFieldValue("first", List.class), List.of("1", "2"));
     }
 
     public void testUnmatchedNamesNotIncludedInDocument() throws Exception {