[Enhancement] Error handling for illegal character usage in java regex named capture group (#4434)

RyanL1997 · Swiddis · web-flow · commit 0b7e86cd2030 · 2025-10-14T10:46:01.000-07:00
Co-authored-by: Simeon Widdis &lt;sawiddis@amazon.com&gt;
diff --git a/core/src/main/java/org/opensearch/sql/expression/parse/RegexCommonUtils.java b/core/src/main/java/org/opensearch/sql/expression/parse/RegexCommonUtils.java
@@ -22,6 +22,9 @@ public class RegexCommonUtils {
   private static final Pattern NAMED_GROUP_PATTERN =
       Pattern.compile("\\(\\?<([a-zA-Z][a-zA-Z0-9]*)>");
 
+  // Pattern to extract ANY named group (valid or invalid) for validation
+  private static final Pattern ANY_NAMED_GROUP_PATTERN = Pattern.compile("\\(\\?<([^>]+)>");
+
   private static final int MAX_CACHE_SIZE = 1000;
 
   private static final Map<String, Pattern> patternCache =
@@ -50,20 +53,52 @@ public static Pattern getCompiledPattern(String regex) {
   }
 
   /**
-   * Extract list of named group candidates from a regex pattern.
+   * Extract list of named group candidates from a regex pattern. Validates that all group names
+   * conform to Java regex named group requirements: must start with a letter and contain only
+   * letters and digits.
    *
    * @param pattern The regex pattern string
-   * @return List of named group names found in the pattern
+   * @return List of valid named group names found in the pattern
+   * @throws IllegalArgumentException if any named groups contain invalid characters
    */
   public static List<String> getNamedGroupCandidates(String pattern) {
     ImmutableList.Builder<String> namedGroups = ImmutableList.builder();
-    Matcher m = NAMED_GROUP_PATTERN.matcher(pattern);
-    while (m.find()) {
-      namedGroups.add(m.group(1));
+
+    Matcher anyGroupMatcher = ANY_NAMED_GROUP_PATTERN.matcher(pattern);
+    while (anyGroupMatcher.find()) {
+      String groupName = anyGroupMatcher.group(1);
+
+      if (!isValidJavaRegexGroupName(groupName)) {
+        throw new IllegalArgumentException(
+            String.format(
+                "Invalid capture group name '%s'. Java regex group names must start with a letter"
+                    + " and contain only letters and digits.",
+                groupName));
+      }
     }
+
+    Matcher validGroupMatcher = NAMED_GROUP_PATTERN.matcher(pattern);
+    while (validGroupMatcher.find()) {
+      namedGroups.add(validGroupMatcher.group(1));
+    }
+
     return namedGroups.build();
   }
 
+  /**
+   * Validates if a group name conforms to Java regex named group requirements. Java regex group
+   * names must: - Start with a letter (a-z, A-Z) - Contain only letters (a-z, A-Z) and digits (0-9)
+   *
+   * @param groupName The group name to validate
+   * @return true if valid, false otherwise
+   */
+  private static boolean isValidJavaRegexGroupName(String groupName) {
+    if (groupName == null || groupName.isEmpty()) {
+      return false;
+    }
+    return groupName.matches("^[A-Za-z][A-Za-z0-9]*$");
+  }
+
   /**
    * Match using find() for partial match semantics with string pattern.
    *
diff --git a/core/src/test/java/org/opensearch/sql/expression/parse/RegexCommonUtilsTest.java b/core/src/test/java/org/opensearch/sql/expression/parse/RegexCommonUtilsTest.java
@@ -194,12 +194,15 @@ public void testGetNamedGroupCandidatesWithNumericNames() {
   }
 
   @Test
-  public void testGetNamedGroupCandidatesSpecialCharacters() {
-    // Test that groups with special characters are not captured (only alphanumeric starting with
-    // letter)
-    String pattern = "(?<valid_group>[a-z]+)\\s+(?<123invalid>[0-9]+)\\s+(?<also-invalid>.*)";
-    List<String> groups = RegexCommonUtils.getNamedGroupCandidates(pattern);
-    assertEquals(0, groups.size());
+  public void testGetNamedGroupCandidatesWithInvalidCharactersThrowsException() {
+    // Test that groups with invalid characters throw exception (even if some are valid)
+    String pattern = "(?<validgroup>[a-z]+)\\s+(?<123invalid>[0-9]+)\\s+(?<also-invalid>.*)";
+    IllegalArgumentException exception =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> RegexCommonUtils.getNamedGroupCandidates(pattern));
+    // Should fail on the first invalid group name found
+    assertTrue(exception.getMessage().contains("Invalid capture group name"));
   }
 
   @Test
@@ -211,4 +214,100 @@ public void testGetNamedGroupCandidatesValidAlphanumeric() {
     assertEquals("groupA", groups.get(0));
     assertEquals("group2B", groups.get(1));
   }
+
+  @Test
+  public void testGetNamedGroupCandidatesWithUnderscore() {
+    // Test that underscores in named groups throw IllegalArgumentException
+    String patternWithUnderscore = ".+@(?<domain_name>.+)";
+    IllegalArgumentException exception =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> RegexCommonUtils.getNamedGroupCandidates(patternWithUnderscore));
+    assertTrue(exception.getMessage().contains("Invalid capture group name 'domain_name'"));
+    assertTrue(
+        exception
+            .getMessage()
+            .contains("must start with a letter and contain only letters and digits"));
+  }
+
+  @Test
+  public void testGetNamedGroupCandidatesWithHyphen() {
+    // Test that hyphens in named groups throw IllegalArgumentException
+    String patternWithHyphen = ".+@(?<domain-name>.+)";
+    IllegalArgumentException exception =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> RegexCommonUtils.getNamedGroupCandidates(patternWithHyphen));
+    assertTrue(exception.getMessage().contains("Invalid capture group name 'domain-name'"));
+    assertTrue(
+        exception
+            .getMessage()
+            .contains("must start with a letter and contain only letters and digits"));
+  }
+
+  @Test
+  public void testGetNamedGroupCandidatesWithDot() {
+    // Test that dots in named groups throw IllegalArgumentException
+    String patternWithDot = ".+@(?<domain.name>.+)";
+    IllegalArgumentException exception =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> RegexCommonUtils.getNamedGroupCandidates(patternWithDot));
+    assertTrue(exception.getMessage().contains("Invalid capture group name 'domain.name'"));
+  }
+
+  @Test
+  public void testGetNamedGroupCandidatesWithSpace() {
+    // Test that spaces in named groups throw IllegalArgumentException
+    String patternWithSpace = ".+@(?<domain name>.+)";
+    IllegalArgumentException exception =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> RegexCommonUtils.getNamedGroupCandidates(patternWithSpace));
+    assertTrue(exception.getMessage().contains("Invalid capture group name 'domain name'"));
+  }
+
+  @Test
+  public void testGetNamedGroupCandidatesStartingWithDigit() {
+    // Test that group names starting with digit throw IllegalArgumentException
+    String patternStartingWithDigit = ".+@(?<1domain>.+)";
+    IllegalArgumentException exception =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> RegexCommonUtils.getNamedGroupCandidates(patternStartingWithDigit));
+    assertTrue(exception.getMessage().contains("Invalid capture group name '1domain'"));
+  }
+
+  @Test
+  public void testGetNamedGroupCandidatesWithSpecialCharacters() {
+    // Test that special characters in named groups throw IllegalArgumentException
+    String patternWithSpecialChar = ".+@(?<domain@name>.+)";
+    IllegalArgumentException exception =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> RegexCommonUtils.getNamedGroupCandidates(patternWithSpecialChar));
+    assertTrue(exception.getMessage().contains("Invalid capture group name 'domain@name'"));
+  }
+
+  @Test
+  public void testGetNamedGroupCandidatesWithValidCamelCase() {
+    // Test that valid camelCase group names work correctly
+    String validPattern = "(?<userName>\\w+)@(?<domainName>\\w+)";
+    List<String> groups = RegexCommonUtils.getNamedGroupCandidates(validPattern);
+    assertEquals(2, groups.size());
+    assertEquals("userName", groups.get(0));
+    assertEquals("domainName", groups.get(1));
+  }
+
+  @Test
+  public void testGetNamedGroupCandidatesWithMixedInvalidValid() {
+    // Test that even one invalid group name fails the entire validation
+    String patternWithMixed =
+        "(?<validName>[a-z]+)\\s+(?<invalid_name>[0-9]+)\\s+(?<anotherValidName>.*)";
+    IllegalArgumentException exception =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> RegexCommonUtils.getNamedGroupCandidates(patternWithMixed));
+    assertTrue(exception.getMessage().contains("Invalid capture group name 'invalid_name'"));
+  }
 }
diff --git a/docs/user/ppl/cmd/parse.rst b/docs/user/ppl/cmd/parse.rst
@@ -114,3 +114,7 @@ There are a few limitations with parse command:
   For example, the following query will not display the parsed fields ``host`` unless the source field ``email`` is also explicitly included::
 
     source=accounts | parse email '.+@(?<host>.+)' | fields email, host ;
+
+- Named capture group must start with a letter and contain only letters and digits.
+
+  For detailed Java regex pattern syntax and usage, refer to the `official Java Pattern documentation <https://docs.oracle.com/javase/8/docs/api/java/util/regex/Pattern.html>`_
diff --git a/docs/user/ppl/cmd/rex.rst b/docs/user/ppl/cmd/rex.rst
@@ -166,7 +166,7 @@ Demonstrates naming restrictions for capture groups. Group names cannot contain
 Invalid PPL query with underscores::
 
     os> source=accounts | rex field=email "(?<user_name>[^@]+)@(?<email_domain>[^.]+)" | fields email, user_name, email_domain ;
-    {'reason': 'Invalid Query', 'details': 'Rex pattern must contain at least one named capture group', 'type': 'IllegalArgumentException'}
+    {'reason': 'Invalid Query', 'details': "Invalid capture group name 'user_name'. Java regex group names must start with a letter and contain only letters and digits.", 'type': 'IllegalArgumentException'}
     Error: Query returned no data
 
 Correct PPL query without underscores::
@@ -206,17 +206,17 @@ PPL query exceeding the configured limit results in an error::
 Comparison with Related Commands
 ================================
 
-============================= ============ ============
-Feature                        rex          parse
-============================= ============ ============
-Pattern Type                   Java Regex   Java Regex
-Named Groups Required          Yes          Yes
-Multiple Named Groups          Yes          No
-Multiple Matches               Yes          No
-Text Substitution              Yes          No
-Offset Tracking                Yes          No
-Underscores in Group Names     No           No
-============================= ============ ============
+================================== ============ ============
+Feature                             rex          parse
+================================== ============ ============
+Pattern Type                        Java Regex   Java Regex
+Named Groups Required               Yes          Yes
+Multiple Named Groups               Yes          No
+Multiple Matches                    Yes          No
+Text Substitution                   Yes          No
+Offset Tracking                     Yes          No
+Special Characters in Group Names   No           No
+================================== ============ ============
 
 
 Limitations
@@ -226,7 +226,6 @@ There are several important limitations with the rex command:
 
 **Named Capture Group Naming:**
 
-- Named capture groups cannot contain underscores due to Java regex limitations
 - Group names must start with a letter and contain only letters and digits
 - For detailed Java regex pattern syntax and usage, refer to the `official Java Pattern documentation <https://docs.oracle.com/javase/8/docs/api/java/util/regex/Pattern.html>`_
 
diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteParseCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteParseCommandIT.java
@@ -5,6 +5,10 @@
 
 package org.opensearch.sql.calcite.remote;
 
+import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_BANK;
+
+import java.io.IOException;
+import org.junit.Test;
 import org.opensearch.sql.ppl.ParseCommandIT;
 
 public class CalciteParseCommandIT extends ParseCommandIT {
@@ -13,4 +17,60 @@ public void init() throws Exception {
     super.init();
     enableCalcite();
   }
+
+  @Test
+  public void testParseErrorInvalidGroupNameUnderscore() throws IOException {
+    try {
+      executeQuery(
+          String.format(
+              "source=%s | parse email '.+@(?<host_name>.+)' | fields email", TEST_INDEX_BANK));
+      fail("Should have thrown an exception for underscore in named capture group");
+    } catch (Exception e) {
+      assertTrue(e.getMessage().contains("Invalid capture group name 'host_name'"));
+      assertTrue(
+          e.getMessage().contains("must start with a letter and contain only letters and digits"));
+    }
+  }
+
+  @Test
+  public void testParseErrorInvalidGroupNameHyphen() throws IOException {
+    try {
+      executeQuery(
+          String.format(
+              "source=%s | parse email '.+@(?<host-name>.+)' | fields email", TEST_INDEX_BANK));
+      fail("Should have thrown an exception for hyphen in named capture group");
+    } catch (Exception e) {
+      assertTrue(e.getMessage().contains("Invalid capture group name 'host-name'"));
+      assertTrue(
+          e.getMessage().contains("must start with a letter and contain only letters and digits"));
+    }
+  }
+
+  @Test
+  public void testParseErrorInvalidGroupNameStartingWithDigit() throws IOException {
+    try {
+      executeQuery(
+          String.format(
+              "source=%s | parse email '.+@(?<1host>.+)' | fields email", TEST_INDEX_BANK));
+      fail("Should have thrown an exception for group name starting with digit");
+    } catch (Exception e) {
+      assertTrue(e.getMessage().contains("Invalid capture group name '1host'"));
+      assertTrue(
+          e.getMessage().contains("must start with a letter and contain only letters and digits"));
+    }
+  }
+
+  @Test
+  public void testParseErrorInvalidGroupNameSpecialCharacter() throws IOException {
+    try {
+      executeQuery(
+          String.format(
+              "source=%s | parse email '.+@(?<host@name>.+)' | fields email", TEST_INDEX_BANK));
+      fail("Should have thrown an exception for special character in named capture group");
+    } catch (Exception e) {
+      assertTrue(e.getMessage().contains("Invalid capture group name 'host@name'"));
+      assertTrue(
+          e.getMessage().contains("must start with a letter and contain only letters and digits"));
+    }
+  }
 }
diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteRexCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteRexCommandIT.java
@@ -50,6 +50,70 @@ public void testRexErrorNoNamedGroups() throws IOException {
     }
   }
 
+  @Test
+  public void testRexErrorInvalidGroupNameUnderscore() throws IOException {
+    try {
+      executeQuery(
+          String.format(
+              "source=%s | rex field=email \\\"(?<user_name>[^@]+)@(?<domain>.+)\\\" | fields"
+                  + " email",
+              TEST_INDEX_ACCOUNT));
+      fail("Should have thrown an exception for underscore in named capture group");
+    } catch (Exception e) {
+      assertTrue(e.getMessage().contains("Invalid capture group name 'user_name'"));
+      assertTrue(
+          e.getMessage().contains("must start with a letter and contain only letters and digits"));
+    }
+  }
+
+  @Test
+  public void testRexErrorInvalidGroupNameHyphen() throws IOException {
+    try {
+      executeQuery(
+          String.format(
+              "source=%s | rex field=email \\\"(?<user-name>[^@]+)@(?<domain>.+)\\\" | fields"
+                  + " email",
+              TEST_INDEX_ACCOUNT));
+      fail("Should have thrown an exception for hyphen in named capture group");
+    } catch (Exception e) {
+      assertTrue(e.getMessage().contains("Invalid capture group name 'user-name'"));
+      assertTrue(
+          e.getMessage().contains("must start with a letter and contain only letters and digits"));
+    }
+  }
+
+  @Test
+  public void testRexErrorInvalidGroupNameStartingWithDigit() throws IOException {
+    try {
+      executeQuery(
+          String.format(
+              "source=%s | rex field=email \\\"(?<1user>[^@]+)@(?<domain>.+)\\\" | fields"
+                  + " email",
+              TEST_INDEX_ACCOUNT));
+      fail("Should have thrown an exception for group name starting with digit");
+    } catch (Exception e) {
+      assertTrue(e.getMessage().contains("Invalid capture group name '1user'"));
+      assertTrue(
+          e.getMessage().contains("must start with a letter and contain only letters and digits"));
+    }
+  }
+
+  @Test
+  public void testRexErrorInvalidGroupNameSpecialCharacter() throws IOException {
+    try {
+      executeQuery(
+          String.format(
+              "source=%s | rex field=email \\\"(?<user@name>[^@]+)@(?<domain>.+)\\\" | fields"
+                  + " email",
+              TEST_INDEX_ACCOUNT));
+      fail("Should have thrown an exception for special character in named capture group");
+    } catch (Exception e) {
+      assertTrue(e.getMessage().contains("Invalid capture group name 'user@name'"));
+      assertTrue(
+          e.getMessage().contains("must start with a letter and contain only letters and digits"));
+    }
+  }
+
   @Test
   public void testRexWithFiltering() throws IOException {
     JSONObject result =
