OAK-11555 Elastic: support dot in property and function names

thomasmueller · thomasmueller · commit 86987050d57a · 2025-03-07T11:17:54.000+01:00
diff --git a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/util/ElasticIndexUtils.java b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/util/ElasticIndexUtils.java
@@ -35,27 +35,106 @@ public class ElasticIndexUtils {
      * Convert a JCR property name to a Elasticsearch field name.
      * Notice that "|" is not allowed in JCR names.
      *
-     * "." is converted to "|dot|"
-     * "/" is converted to "||"
-     *
      * @param propertyName the property name
      * @return the field name
      */
     public static String fieldName(String propertyName) {
+        if(propertyName.startsWith(":")) {
+            // there are some hardcoded field names
+            return propertyName;
+        }
         String fieldName = propertyName;
-        // 99% property names don't contain a slash or dot,
-        // so for performance reason use indexOf
-        int slashIndex = fieldName.indexOf('|');
-        if (slashIndex >= 0) {
-            fieldName = fieldName.replaceAll("\\|", "\\|\\|");
+        boolean escape = false;
+        if (fieldName.isBlank()) {
+            // empty field name or field names that only consist of spaces
+            escape = true;
+        } else {
+            // 99.99% property names are OK,
+            // so we loop over the characters first
+            for (int i = 0; i < fieldName.length(); i++) {
+                switch (fieldName.charAt(i)) {
+                case '|':
+                case '.':
+                case '^':
+                case '_':
+                    escape = true;
+                }
+            }
         }
-        int dotIndex = fieldName.indexOf('.');
-        if (dotIndex >= 0) {
-            fieldName = fieldName.replaceAll("\\.", "|dot|");
+        if (escape) {
+            StringBuilder buff = new StringBuilder(fieldName.length());
+            for (int i = 0; i < fieldName.length(); i++) {
+                char c = fieldName.charAt(i);
+                switch (c) {
+                case '|':
+                    buff.append("||");
+                    break;
+                case '.':
+                case '^':
+                    buff.append('|').append(Integer.toHexString(c)).append('|');
+                    break;
+                default:
+                    buff.append(c);
+                }
+            }
+            fieldName = buff.toString();
+            // internal field start with a _
+            // we also support empty or just spaces
+            if (fieldName.startsWith("_") || fieldName.isBlank()) {
+                fieldName = "|" + fieldName;
+            }
         }
         return fieldName;
     }
 
+    /**
+     * Convert an elasticsearch field name to a JCR property name.
+     *
+     * @param fieldName the field name
+     * @return the property name
+     */
+    public static String propertyNameFromFieldName(String fieldName) {
+        if (fieldName.indexOf('|') < 0) {
+            return fieldName;
+        }
+        if (fieldName.startsWith("|")) {
+            if (fieldName.equals("|")) {
+                return "";
+            } if (fieldName.startsWith("|_") || fieldName.substring(1).isBlank()) {
+                fieldName = fieldName.substring(1);
+            }
+        }
+        StringBuilder buff = new StringBuilder(fieldName.length());
+        for (int i = 0; i < fieldName.length(); i++) {
+            char c = fieldName.charAt(i);
+            switch (c) {
+            case '|':
+                String next = fieldName.substring(i + 1);
+                if (next.startsWith("|")) {
+                    buff.append('|');
+                    i++;
+                } else {
+                    int end = next.indexOf('|');
+                    if (end < 0) {
+                        buff.append(next);
+                        break;
+                    }
+                    String code = next.substring(0, end);
+                    try {
+                        buff.append((char) Integer.parseInt(code, 16));
+                    } catch (NumberFormatException e) {
+                        buff.append(code);
+                    }
+                    i += code.length() + 1;
+                }
+                break;
+            default:
+                buff.append(c);
+            }
+        }
+        return buff.toString();
+    }
+
     /**
      * Transforms a path into an _id compatible with Elasticsearch specification. The path cannot be larger than 512
      * bytes. For performance reasons paths that are already compatible are returned untouched. Otherwise, SHA-256
diff --git a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticConnectionRule.java b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticConnectionRule.java
@@ -61,7 +61,7 @@ public ElasticConnectionRule() {
     public ElasticConnectionRule(String elasticConnectionString) {
         this(elasticConnectionString,
                 "elastic_test_" +
-                        RandomStringUtils.random(5, true, false).toLowerCase() +
+                        RandomStringUtils.insecure().next(5, true, false).toLowerCase() +
                         System.currentTimeMillis()
         );
     }
diff --git a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexSuggestionCommonTest.java b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexSuggestionCommonTest.java
@@ -63,7 +63,7 @@ public void explain() throws Exception {
         String expected = "{\"_source\":{\"includes\":[\":path\"]},\"query\":{\"bool\":{\"must\":[{\"nested\":{\"inner_hits\":" +
                 "{\"size\":100},\"path\":\":suggest\",\"query\":{\"match_bool_prefix\":{\":suggest.value\":{\"operator\":\"and\",\"query\":\"boo\"}}},\"score_mode\":\"max\"}}]}},\"size\":100}";
 
-        Query q = qm.createQuery(sql, Query.SQL);
+        Query q = qm.createQuery(sql, Query.JCR_SQL2);
         Row row = q.execute().getRows().nextRow();
         MatcherAssert.assertThat(row.getValue("plan").getString(), CoreMatchers.containsString(expected));
     }
diff --git a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticPropertyIndexTest.java b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticPropertyIndexTest.java
@@ -22,6 +22,7 @@
 import org.apache.jackrabbit.oak.api.Tree;
 import org.apache.jackrabbit.oak.commons.junit.LogCustomizer;
 import org.apache.jackrabbit.oak.plugins.index.elastic.util.ElasticIndexDefinitionBuilder;
+import org.apache.jackrabbit.oak.plugins.index.elastic.util.ElasticIndexUtils;
 import org.apache.jackrabbit.oak.plugins.index.search.FulltextIndexConstants;
 import org.apache.jackrabbit.oak.plugins.index.search.util.IndexDefinitionBuilder;
 import org.apache.jackrabbit.oak.plugins.index.search.util.IndexDefinitionBuilder.PropertyRule;
@@ -235,7 +236,9 @@ public void propertyWithDot() throws Exception {
             String explanation = explain(query);
             assertThat(explanation, containsString("/oak:index/test1"));
             assertThat(explanation, containsString(
-                    "{\"term\":{\"first|dot|name\":{\"value\":\"Antonio\""));
+                    "{\"term\":{\"" +
+                            ElasticIndexUtils.fieldName("first.name") +
+                            "\":{\"value\":\"Antonio\""));
             assertQuery(query, List.of("/test"));
         });
 
@@ -246,7 +249,9 @@ public void propertyWithDot() throws Exception {
             String explanation = explain(lowerQuery);
             assertThat(explanation, containsString("/oak:index/test1"));
             assertThat(explanation, containsString(
-                    "{\"term\":{\"function*lower*@first|dot|name\":{\"value\":\"antonio\""));
+                    "{\"term\":{\"" +
+                            ElasticIndexUtils.fieldName("function*lower*@first.name") +
+                            "\":{\"value\":\"antonio\""));
             assertQuery(lowerQuery, List.of("/test"));
         });
     }
diff --git a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticRegexPropertyIndexTest.java b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticRegexPropertyIndexTest.java
@@ -25,6 +25,7 @@
 
 import org.apache.jackrabbit.oak.api.CommitFailedException;
 import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.plugins.index.elastic.util.ElasticIndexUtils;
 import org.apache.jackrabbit.oak.plugins.index.search.FulltextIndexConstants;
 import org.apache.jackrabbit.oak.plugins.index.search.util.IndexDefinitionBuilder;
 import org.apache.jackrabbit.oak.plugins.index.search.util.IndexDefinitionBuilder.PropertyRule;
@@ -65,7 +66,10 @@ public void regexPropertyWithFlattened() throws Exception {
         assertEventually(() -> {
             String explain = explain(propaQuery);
             assertThat(explain, containsString("elasticsearch:test1"));
-            assertThat(explain, containsString("[{\"term\":{\"flat:allProperties.propa\":{\"value\":\"foo\"}}}]"));
+            assertThat(explain, containsString("[{\"term\":{\"flat:" +
+                    ElasticIndexUtils.fieldName("allProperties") + "." +
+                    ElasticIndexUtils.fieldName("propa") +
+                    "\":{\"value\":\"foo\"}}}]"));
             assertQuery(propaQuery, List.of("/test/a", "/test/b"));
         });
 
@@ -74,8 +78,14 @@ public void regexPropertyWithFlattened() throws Exception {
         assertEventually(() -> {
             String explain = explain(propaOrderQuery);
             assertThat(explain, containsString("elasticsearch:test1"));
-            assertThat(explain, containsString("\"query\":{\"bool\":{\"filter\":[{\"prefix\":{\"flat:allProperties.propd\":{\"value\":\"foo\"}}}]}}"));
-            assertThat(explain, containsString("\"sort\":[{\"flat:allProperties.propd\":{\"order\":\"asc\"}},{\":path\":{\"order\":\"asc\"}}]"));
+            assertThat(explain, containsString("\"query\":{\"bool\":{\"filter\":[{\"prefix\":{\"flat:" +
+                    ElasticIndexUtils.fieldName("allProperties") + "." +
+                    ElasticIndexUtils.fieldName("propd") +
+                    "\":{\"value\":\"foo\"}}}]}}"));
+            assertThat(explain, containsString("\"sort\":[{\"flat:" +
+                    ElasticIndexUtils.fieldName("allProperties") + "." +
+                    ElasticIndexUtils.fieldName("propd") +
+                    "\":{\"order\":\"asc\"}},{\":path\":{\"order\":\"asc\"}}]"));
             assertThat(explain, containsString("sortOrder: [{ propertyName : propd, propertyType : UNDEFINED, order : ASCENDING }]"));
             assertQuery(propaOrderQuery, List.of("/test/f", "/test/e"));
         });
diff --git a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocumentMakerLargeStringPropertiesLogTest.java b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocumentMakerLargeStringPropertiesLogTest.java
@@ -73,7 +73,7 @@ private void setThresholdLimit(String threshold) {
         System.setProperty(FulltextDocumentMaker.WARN_LOG_STRING_SIZE_THRESHOLD_KEY, threshold);
     }
 
-    private ElasticDocumentMaker addPropertyAccordingToType(NodeBuilder test, Type type, String... str) {
+    private <T> ElasticDocumentMaker addPropertyAccordingToType(NodeBuilder test, Type<T> type, String... str) {
         NodeState root = INITIAL_CONTENT;
         ElasticIndexDefinitionBuilder builder = new ElasticIndexDefinitionBuilder();
         builder.indexRule("nt:base")
diff --git a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/util/ElasticIndexUtilsTest.java b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/util/ElasticIndexUtilsTest.java
@@ -21,16 +21,47 @@
 import java.net.URLEncoder;
 import java.nio.charset.StandardCharsets;
 import java.util.List;
+import java.util.Random;
 
 import org.junit.Test;
 
 public class ElasticIndexUtilsTest {
 
     @Test
     public void fieldName() {
-        assertEquals("a", ElasticIndexUtils.fieldName("a"));
-        assertEquals("first|dot|name", ElasticIndexUtils.fieldName("first.name"));
-        assertEquals("first||name", ElasticIndexUtils.fieldName("first|name"));
+        assertEquals("regular", ElasticIndexUtils.fieldName("regular"));
+        assertEquals(":nodeName", ElasticIndexUtils.fieldName(":nodeName"));
+        assertEquals("first|2e|name", ElasticIndexUtils.fieldName("first.name"));
+        assertEquals("weird|5e|", ElasticIndexUtils.fieldName("weird^"));
+        assertEquals("embedded_is_fine", ElasticIndexUtils.fieldName("embedded_is_fine"));
+        assertEquals("|_id", ElasticIndexUtils.fieldName("_id"));
+        assertEquals("|", ElasticIndexUtils.fieldName(""));
+        assertEquals("| ", ElasticIndexUtils.fieldName(" "));
+        assertEquals("||", ElasticIndexUtils.fieldName("|"));
+        assertEquals("||test||", ElasticIndexUtils.fieldName("|test|"));
+    }
+
+    @Test
+    public void randomFieldNames() {
+        ElasticIndexUtils.propertyNameFromFieldName("");
+        Random r = new Random(1);
+        for (int i = 0; i < 1000; i++) {
+            StringBuilder buff = new StringBuilder();
+            int len = 1 + r.nextInt(5);
+            String chars = "|^._ 25ex";
+            for (int j = 0; j < len; j++) {
+                buff.append(chars.charAt(r.nextInt(chars.length())));
+            }
+            String p = buff.toString();
+            String f = ElasticIndexUtils.fieldName(p);
+            String p2 = ElasticIndexUtils.propertyNameFromFieldName(f);
+            if (!p.equals(p2)) {
+                p2 = ElasticIndexUtils.propertyNameFromFieldName(f);
+                assertEquals(p, p2);
+            }
+            // just to make sure there are no exceptions (within some limits)
+            ElasticIndexUtils.propertyNameFromFieldName(p);
+        }
     }
     
     @Test

Original file line number	Diff line number	Diff line change
`@@ -61,7 +61,7 @@ public ElasticConnectionRule() {`
`61`	`61`	`public ElasticConnectionRule(String elasticConnectionString) {`
`62`	`62`	`this(elasticConnectionString,`
`63`	`63`	`"elastic_test_" +`
`64`		`- RandomStringUtils.random(5, true, false).toLowerCase() +`
	`64`	`+ RandomStringUtils.insecure().next(5, true, false).toLowerCase() +`
`65`	`65`	`System.currentTimeMillis()`
`66`	`66`	`);`
`67`	`67`	`}`
Original file line number	Diff line number	Diff line change
`@@ -63,7 +63,7 @@ public void explain() throws Exception {`
`63`	`63`	`String expected = "{\"_source\":{\"includes\":[\":path\"]},\"query\":{\"bool\":{\"must\":[{\"nested\":{\"inner_hits\":" +`
`64`	`64`	`"{\"size\":100},\"path\":\":suggest\",\"query\":{\"match_bool_prefix\":{\":suggest.value\":{\"operator\":\"and\",\"query\":\"boo\"}}},\"score_mode\":\"max\"}}]}},\"size\":100}";`
`65`	`65`
`66`		`- Query q = qm.createQuery(sql, Query.SQL);`
	`66`	`+ Query q = qm.createQuery(sql, Query.JCR_SQL2);`
`67`	`67`	`Row row = q.execute().getRows().nextRow();`
`68`	`68`	`MatcherAssert.assertThat(row.getValue("plan").getString(), CoreMatchers.containsString(expected));`
`69`	`69`	`}`
Original file line number	Diff line number	Diff line change
`@@ -73,7 +73,7 @@ private void setThresholdLimit(String threshold) {`
`73`	`73`	`System.setProperty(FulltextDocumentMaker.WARN_LOG_STRING_SIZE_THRESHOLD_KEY, threshold);`
`74`	`74`	`}`
`75`	`75`
`76`		`- private ElasticDocumentMaker addPropertyAccordingToType(NodeBuilder test, Type type, String... str) {`
	`76`	`+ private <T> ElasticDocumentMaker addPropertyAccordingToType(NodeBuilder test, Type<T> type, String... str) {`
`77`	`77`	`NodeState root = INITIAL_CONTENT;`
`78`	`78`	`ElasticIndexDefinitionBuilder builder = new ElasticIndexDefinitionBuilder();`
`79`	`79`	`builder.indexRule("nt:base")`