You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by th...@apache.org on 2026/09/16 14:21:39 UTC
(jackrabbit-oak) branch OAK-12370 updated: OAK-12370: warn when a query uses a leading '/' in a property or column name (#3127)
This is an automated email from the ASF dual-hosted git repository.
thomasmueller pushed a commit to branch OAK-12370
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
The following commit(s) were added to refs/heads/OAK-12370 by this push:
new 06c2229074 OAK-12370: warn when a query uses a leading '/' in a property or column name (#3127)
06c2229074 is described below
commit 06c2229074db3e970dfa943d0d4e0d3167d952af
Author: Theia Vlad <39...@users.noreply.github.com>
AuthorDate: Wed Sep 16 16:21:32 2026 +0200
OAK-12370: warn when a query uses a leading '/' in a property or column name (#3127)
A leading '/' on a property/column name is not a valid relative path: it silently matches nothing and defeats indexes. Emit a WARN (on by default, kill-switch FT_OAK-12370) so real usage surfaces in logs before any future hard rejection.
---
.../main/java/org/apache/jackrabbit/oak/Oak.java | 8 +
.../jackrabbit/oak/query/QueryEngineSettings.java | 12 ++
.../apache/jackrabbit/oak/query/SQL2Parser.java | 73 +++++++--
.../jackrabbit/oak/query/SQL2ParserTest.java | 172 ++++++++++++++++++++-
4 files changed, 250 insertions(+), 15 deletions(-)
diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/Oak.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/Oak.java
index 407e208384..068cbfd7ed 100644
--- a/oak-core/src/main/java/org/apache/jackrabbit/oak/Oak.java
+++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/Oak.java
@@ -589,6 +589,10 @@ public class Oak {
LOG.info("Registered ignore limit in index selection feature: " + QueryEngineSettings.FT_IGNORE_LIMIT_IN_INDEX_SELECTION);
closer.register(ignoreLimitInIndexSelection);
queryEngineSettings.setIgnoreLimitInIndexSelectionFeature(ignoreLimitInIndexSelection);
+ Feature disableWarnSlashPropertyName = newFeature(QueryEngineSettings.FT_DISABLE_WARN_SLASH_PROPERTY_NAME, whiteboard);
+ LOG.info("Registered disable warn slash property name feature: " + QueryEngineSettings.FT_DISABLE_WARN_SLASH_PROPERTY_NAME);
+ closer.register(disableWarnSlashPropertyName);
+ queryEngineSettings.setDisableWarnSlashPropertyNameFeature(disableWarnSlashPropertyName);
}
return this;
@@ -1009,6 +1013,10 @@ public class Oak {
settings.setIgnoreLimitInIndexSelectionFeature(feature);
}
+ public void setDisableWarnSlashPropertyNameFeature(@Nullable Feature feature) {
+ settings.setDisableWarnSlashPropertyNameFeature(feature);
+ }
+
@Override
public void setQueryValidatorPattern(String key, String pattern, String comment, boolean failQuery) {
settings.getQueryValidator().setPattern(key, pattern, comment, failQuery);
diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettings.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettings.java
index 93d6eebda7..def77c7004 100644
--- a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettings.java
+++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettings.java
@@ -67,6 +67,8 @@ public class QueryEngineSettings implements QueryEngineSettingsMBean, QueryLimit
public static final String FT_IGNORE_LIMIT_IN_INDEX_SELECTION = "FT_OAK-12057";
+ public static final String FT_DISABLE_WARN_SLASH_PROPERTY_NAME = "FT_OAK-12370";
+
public static final int DEFAULT_PREFETCH_COUNT = Integer.getInteger(OAK_QUERY_PREFETCH_COUNT, -1);
public static final String OAK_QUERY_FAIL_TRAVERSAL = "oak.queryFailTraversal";
@@ -125,6 +127,7 @@ public class QueryEngineSettings implements QueryEngineSettingsMBean, QueryLimit
private Feature sortUnionQueryLegacyModeFeature;
private Feature optimizeXPathUnion;
private Feature ignoreLimitInIndexSelectionFeature;
+ private Feature disableWarnSlashPropertyNameFeature;
private String autoOptionsMappingJson = "{}";
private QueryOptions.AutomaticQueryOptionsMapping autoOptionsMapping = new QueryOptions.AutomaticQueryOptionsMapping(autoOptionsMappingJson);
@@ -257,6 +260,15 @@ public class QueryEngineSettings implements QueryEngineSettingsMBean, QueryLimit
return ignoreLimitInIndexSelectionFeature == null || ignoreLimitInIndexSelectionFeature.isEnabled();
}
+ public void setDisableWarnSlashPropertyNameFeature(@Nullable Feature feature) {
+ this.disableWarnSlashPropertyNameFeature = feature;
+ }
+
+ public boolean isWarnOnSlashInPropertyNameEnabled() {
+ // The warning is on by default; enabling this opt-out toggle suppresses it.
+ return disableWarnSlashPropertyNameFeature == null || !disableWarnSlashPropertyNameFeature.isEnabled();
+ }
+
public String getStrictPathRestriction() {
return strictPathRestriction.name();
}
diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/SQL2Parser.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/SQL2Parser.java
index 11409c9509..e0c0953de5 100644
--- a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/SQL2Parser.java
+++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/SQL2Parser.java
@@ -26,6 +26,8 @@ import java.util.HashMap;
import java.util.Locale;
import java.util.Map;
import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
import javax.jcr.PropertyType;
import javax.jcr.RepositoryException;
@@ -114,6 +116,22 @@ public class SQL2Parser {
private boolean literalUsageLogged;
+ /**
+ * Property/column names (with a leading '/') already warned about. Cleared periodically to
+ * bound memory and to keep surfacing ongoing usage; see IndexName for the same idiom.
+ */
+ private static final Set<String> LOGGED_SLASH_PROPERTY_NAMES = ConcurrentHashMap.newKeySet();
+
+ /**
+ * Upper bound on {@link #LOGGED_SLASH_PROPERTY_NAMES}. Unlike index names (a bounded, config-defined
+ * set), property names come from arbitrary queries and can be unbounded (e.g. paths with dynamic
+ * segments), so the set is capped to bound memory; the periodic clear frees capacity again.
+ */
+ static final int MAX_LOGGED_SLASH_PROPERTY_NAMES = 1000;
+
+ /** When {@link #LOGGED_SLASH_PROPERTY_NAMES} will next be cleared. */
+ private static volatile long nextSlashPropertyLogClear;
+
private final QueryExecutionStats stats;
/**
@@ -349,6 +367,33 @@ public class SQL2Parser {
return s;
}
+ private String checkPropertyName(String propertyName) {
+ if (propertyName != null && propertyName.startsWith("/")
+ && settings.isWarnOnSlashInPropertyNameEnabled()) {
+ long now = System.currentTimeMillis();
+ if (nextSlashPropertyLogClear < now) {
+ LOGGED_SLASH_PROPERTY_NAMES.clear();
+ // clear again in 5 minutes, so ongoing usage keeps being surfaced
+ nextSlashPropertyLogClear = now + 5 * 60 * 1000;
+ }
+ // warn at most once per distinct offending name per interval, and bound the set size,
+ // to avoid flooding logs / unbounded memory for user-supplied names
+ if (LOGGED_SLASH_PROPERTY_NAMES.size() < MAX_LOGGED_SLASH_PROPERTY_NAMES
+ && LOGGED_SLASH_PROPERTY_NAMES.add(propertyName)) {
+ LOG.warn("Query uses a leading '/' in a property or column name, which is not valid " +
+ "and will be rejected in a future version: [{}] (should be [{}]). Query: {}",
+ propertyName, propertyName.substring(1), statement);
+ }
+ }
+ return propertyName;
+ }
+
+ // Visible for testing: reset the leading-'/' warning de-duplication state.
+ static void resetSlashPropertyNameWarnings() {
+ LOGGED_SLASH_PROPERTY_NAMES.clear();
+ nextSlashPropertyLogClear = 0;
+ }
+
private SourceImpl parseSource() throws ParseException {
SelectorImpl selector = parseSelector();
selectors.put(selector.getSelectorName(), selector);
@@ -406,11 +451,11 @@ public class SQL2Parser {
} else {
String selector1 = name;
read(".");
- String property1 = readName();
+ String property1 = checkPropertyName(readName());
read("=");
String selector2 = readName();
read(".");
- return factory.equiJoinCondition(selector1, property1, selector2, readName());
+ return factory.equiJoinCondition(selector1, property1, selector2, checkPropertyName(readName()));
}
}
@@ -446,16 +491,16 @@ public class SQL2Parser {
a = parseCondition(op);
}
} else if (readIf(".")) {
- a = parseCondition(factory.propertyValue(identifier, readName()));
+ a = parseCondition(factory.propertyValue(identifier, checkPropertyName(readName())));
} else {
- a = parseCondition(factory.propertyValue(getOnlySelectorName(), identifier));
+ a = parseCondition(factory.propertyValue(getOnlySelectorName(), checkPropertyName(identifier)));
}
} else if ("[".equals(currentToken)) {
String name = readName();
if (readIf(".")) {
- a = parseCondition(factory.propertyValue(name, readName()));
+ a = parseCondition(factory.propertyValue(name, checkPropertyName(readName())));
} else {
- a = parseCondition(factory.propertyValue(getOnlySelectorName(), name));
+ a = parseCondition(factory.propertyValue(getOnlySelectorName(), checkPropertyName(name)));
}
} else if (supportSQL1) {
StaticOperandImpl left = parseStaticOperand();
@@ -579,7 +624,7 @@ public class SQL2Parser {
name, null, parseStaticOperand());
} else {
String selector = name;
- name = readName();
+ name = checkPropertyName(readName());
read(",");
c = factory.fullTextSearch(
selector, name, parseStaticOperand());
@@ -587,7 +632,7 @@ public class SQL2Parser {
} else {
read(",");
c = factory.fullTextSearch(
- getOnlySelectorName(), name,
+ getOnlySelectorName(), checkPropertyName(name),
parseStaticOperand());
}
}
@@ -626,7 +671,7 @@ public class SQL2Parser {
name, null, parseStaticOperand());
} else {
String selector = name;
- name = readName();
+ name = checkPropertyName(readName());
read(",");
c = factory.fullTextSearch(
selector, name, parseStaticOperand());
@@ -634,7 +679,7 @@ public class SQL2Parser {
} else {
read(",");
c = factory.fullTextSearch(
- getOnlySelectorName(), name,
+ getOnlySelectorName(), checkPropertyName(name),
parseStaticOperand());
}
}
@@ -749,9 +794,9 @@ public class SQL2Parser {
private PropertyValueImpl parsePropertyValue(String name) throws ParseException {
if (readIf(".")) {
- return factory.propertyValue(name, readName());
+ return factory.propertyValue(name, checkPropertyName(readName()));
} else {
- return factory.propertyValue(getOnlySelectorName(), name);
+ return factory.propertyValue(getOnlySelectorName(), checkPropertyName(name));
}
}
@@ -955,7 +1000,7 @@ public class SQL2Parser {
}
readOptionalAlias(column);
} else {
- column.propertyName = readName();
+ column.propertyName = checkPropertyName(readName());
if (column.propertyName.equals("rep:spellcheck")) {
if (readIf("(")) {
read(")");
@@ -967,7 +1012,7 @@ public class SQL2Parser {
if (readIf("*")) {
column.propertyName = null;
} else {
- column.propertyName = readName();
+ column.propertyName = checkPropertyName(readName());
if (!readOptionalAlias(column)) {
column.columnName =
column.selectorName
diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/query/SQL2ParserTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/query/SQL2ParserTest.java
index 2709a2cd0b..6b384e5f56 100644
--- a/oak-core/src/test/java/org/apache/jackrabbit/oak/query/SQL2ParserTest.java
+++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/query/SQL2ParserTest.java
@@ -26,8 +26,10 @@ import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock;
import java.text.ParseException;
+import java.util.ArrayList;
import java.util.List;
+import org.apache.jackrabbit.oak.commons.junit.LogCustomizer;
import org.apache.jackrabbit.oak.namepath.NamePathMapper;
import org.apache.jackrabbit.oak.query.ast.NodeTypeInfoProvider;
import org.apache.jackrabbit.oak.query.stats.QueryStatsData;
@@ -35,9 +37,14 @@ import org.apache.jackrabbit.oak.query.xpath.XPathToSQL2Converter;
import org.apache.jackrabbit.oak.spi.query.QueryIndex;
import org.apache.jackrabbit.oak.spi.query.QueryIndexProvider;
import org.apache.jackrabbit.oak.spi.state.NodeState;
+import org.apache.jackrabbit.oak.spi.toggle.Feature;
+import org.apache.jackrabbit.oak.spi.toggle.FeatureToggle;
+import org.apache.jackrabbit.oak.spi.whiteboard.DefaultWhiteboard;
import org.jetbrains.annotations.NotNull;
+import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;
+import org.slf4j.event.Level;
/**
* Tests the SQL-2 parser.
@@ -53,11 +60,21 @@ public class SQL2ParserTest {
}
public static SQL2Parser createTestSQL2Parser(NamePathMapper mappings, NodeTypeInfoProvider nodeTypes2) {
+ return createTestSQL2Parser(mappings, nodeTypes2, new QueryEngineSettings());
+ }
+
+ public static SQL2Parser createTestSQL2Parser(NamePathMapper mappings, NodeTypeInfoProvider nodeTypes2,
+ QueryEngineSettings settings) {
QueryStatsData data = new QueryStatsData("", "");
- return new SQL2Parser(mappings, nodeTypes2, new QueryEngineSettings(), data.new QueryExecutionStats());
+ return new SQL2Parser(mappings, nodeTypes2, settings, data.new QueryExecutionStats());
}
+ @Before
+ public void resetSlashPropertyNameWarnings() {
+ SQL2Parser.resetSlashPropertyNameWarnings();
+ }
+
@Test
public void testIgnoreSqlComment() throws ParseException {
p.parse("select * from [nt:unstructured] /* sql comment */");
@@ -250,6 +267,159 @@ public class SQL2ParserTest {
assertTrue("Converted xpath " + xpath + "doesn't end with 'order by [foo]'", c.convert(xpath).endsWith("order by [foo]"));
}
+ @Test
+ public void testWarnOnLeadingSlashInWhereProperty() throws ParseException {
+ List<String> logs = parseAndGetWarnings(
+ "SELECT * FROM [nt:base] AS s WHERE ISDESCENDANTNODE(s, '/content/dam') " +
+ "AND s.[/jcr:content/metadata/prism:expirationDate] > CAST('2025-08-12T08:00:00.000+08:00' AS DATE)");
+ assertEquals(logs.toString(), 1, logs.size());
+ assertTrue(logs.toString(), logs.get(0).contains("/jcr:content/metadata/prism:expirationDate"));
+ assertTrue(logs.toString(), logs.get(0).contains("leading '/'"));
+ }
+
+ @Test
+ public void testWarnOnLeadingSlashInWherePropertyWithoutSelector() throws ParseException {
+ List<String> logs = parseAndGetWarnings(
+ "SELECT * FROM [nt:base] WHERE [/jcr:title] = 'x'");
+ assertEquals(logs.toString(), 1, logs.size());
+ assertTrue(logs.toString(), logs.get(0).contains("/jcr:title"));
+ }
+
+ @Test
+ public void testWarnOnLeadingSlashInSelectColumn() throws ParseException {
+ List<String> logs = parseAndGetWarnings(
+ "SELECT s.[/jcr:content/foo] FROM [nt:base] AS s");
+ assertEquals(logs.toString(), 1, logs.size());
+ assertTrue(logs.toString(), logs.get(0).contains("/jcr:content/foo"));
+ }
+
+ @Test
+ public void testWarnOnLeadingSlashInOrderBy() throws ParseException {
+ List<String> logs = parseAndGetWarnings(
+ "SELECT * FROM [nt:base] AS s ORDER BY s.[/jcr:title]");
+ assertEquals(logs.toString(), 1, logs.size());
+ assertTrue(logs.toString(), logs.get(0).contains("/jcr:title"));
+ }
+
+ @Test
+ public void testWarnOnLeadingSlashInContains() throws ParseException {
+ List<String> logs = parseAndGetWarnings(
+ "SELECT * FROM [nt:base] AS s WHERE CONTAINS(s.[/jcr:content/foo], 'bar')");
+ assertEquals(logs.toString(), 1, logs.size());
+ assertTrue(logs.toString(), logs.get(0).contains("/jcr:content/foo"));
+ }
+
+ @Test
+ public void testWarnOnLeadingSlashInJoinCondition() throws ParseException {
+ List<String> logs = parseAndGetWarnings(
+ "SELECT * FROM [nt:base] AS a INNER JOIN [nt:base] AS b ON a.[/foo] = b.[bar]");
+ assertEquals(logs.toString(), 1, logs.size());
+ assertTrue(logs.toString(), logs.get(0).contains("/foo"));
+ }
+
+ @Test
+ public void testWarnOnLeadingSlashInFunctionOperand() throws ParseException {
+ List<String> lengthLogs = parseAndGetWarnings(
+ "SELECT * FROM [nt:base] AS s WHERE LENGTH(s.[/foo]) > 0");
+ assertEquals(lengthLogs.toString(), 1, lengthLogs.size());
+ assertTrue(lengthLogs.toString(), lengthLogs.get(0).contains("/foo"));
+
+ List<String> propertyLogs = parseAndGetWarnings(
+ "SELECT * FROM [nt:base] AS s WHERE PROPERTY(s.[/bar], 'string') = 'x'");
+ assertEquals(propertyLogs.toString(), 1, propertyLogs.size());
+ assertTrue(propertyLogs.toString(), propertyLogs.get(0).contains("/bar"));
+ }
+
+ @Test
+ public void testNoWarnOnAbsolutePathInFunctions() throws ParseException {
+ assertTrue(parseAndGetWarnings(
+ "SELECT * FROM [nt:base] AS s WHERE ISDESCENDANTNODE([/content/dam/approved])").isEmpty());
+ assertTrue(parseAndGetWarnings(
+ "SELECT * FROM [nt:base] AS s WHERE ISCHILDNODE([/content/dam])").isEmpty());
+ assertTrue(parseAndGetWarnings(
+ "SELECT * FROM [nt:base] AS s WHERE ISSAMENODE([/content/dam])").isEmpty());
+ }
+
+ @Test
+ public void testNoWarnOnValidPropertyNames() throws ParseException {
+ List<String> logs = parseAndGetWarnings(
+ "SELECT s.[jcr:content/foo] FROM [nt:base] AS s WHERE ISDESCENDANTNODE(s, '/content/dam') " +
+ "AND s.[jcr:content/metadata/prism:expirationDate] > CAST('2025-08-12T08:00:00.000+08:00' AS DATE) " +
+ "AND s.[j:c] = '/conf/wknd' ORDER BY s.[jcr:title]");
+ assertTrue(logs.toString(), logs.isEmpty());
+ }
+
+ @Test
+ public void testWarnOncePerQueryForRepeatedSlashProperty() throws ParseException {
+ List<String> logs = parseAndGetWarnings(
+ "SELECT s.[/jcr:content/foo] FROM [nt:base] AS s " +
+ "WHERE s.[/jcr:content/foo] = 'x' ORDER BY s.[/jcr:content/foo]");
+ assertEquals(logs.toString(), 1, logs.size());
+ }
+
+ @Test
+ public void testWarnsOncePerNameAcrossRepeatedQueryExecutions() throws ParseException {
+ // Each query execution builds a fresh parser; the warning must still be de-duplicated
+ // across executions so a hot offending query does not flood the logs.
+ String query = "SELECT s.[/jcr:content/foo] FROM [nt:base] AS s";
+ assertEquals(1, parseAndGetWarnings(query).size());
+ assertTrue(parseAndGetWarnings(query).isEmpty());
+ assertTrue(parseAndGetWarnings(query).isEmpty());
+ }
+
+ @Test
+ public void testWarnByDefaultWhenSuppressToggleRegisteredButDisabled() throws ParseException {
+ // Registered-but-not-enabled is the production default; the warning must still fire.
+ QueryEngineSettings settings = new QueryEngineSettings();
+ settings.setDisableWarnSlashPropertyNameFeature(
+ Feature.newFeature(QueryEngineSettings.FT_DISABLE_WARN_SLASH_PROPERTY_NAME, new DefaultWhiteboard()));
+ List<String> logs = parseAndGetWarnings(settings, "SELECT s.[/jcr:content/foo] FROM [nt:base] AS s");
+ assertEquals(logs.toString(), 1, logs.size());
+ }
+
+ @Test
+ public void testNoWarnWhenSuppressToggleEnabled() throws ParseException {
+ DefaultWhiteboard whiteboard = new DefaultWhiteboard();
+ Feature feature = Feature.newFeature(QueryEngineSettings.FT_DISABLE_WARN_SLASH_PROPERTY_NAME, whiteboard);
+ whiteboard.track(FeatureToggle.class).getServices().forEach(t -> t.setEnabled(true));
+ QueryEngineSettings settings = new QueryEngineSettings();
+ settings.setDisableWarnSlashPropertyNameFeature(feature);
+ List<String> logs = parseAndGetWarnings(settings, "SELECT s.[/jcr:content/foo] FROM [nt:base] AS s");
+ assertTrue(logs.toString(), logs.isEmpty());
+ }
+
+ @Test
+ public void testWarningSetIsCappedToBoundMemory() throws ParseException {
+ LogCustomizer customLogs = LogCustomizer.forLogger(SQL2Parser.class.getName()).enable(Level.WARN).create();
+ try {
+ customLogs.starting();
+ int distinct = SQL2Parser.MAX_LOGGED_SLASH_PROPERTY_NAMES + 50;
+ for (int i = 0; i < distinct; i++) {
+ p.parse("SELECT * FROM [nt:base] WHERE [/prop" + i + "] = 'x'");
+ }
+ // distinct offending names beyond the cap must not keep growing the set / logging
+ assertEquals(SQL2Parser.MAX_LOGGED_SLASH_PROPERTY_NAMES, customLogs.getLogs().size());
+ } finally {
+ customLogs.finished();
+ }
+ }
+
+ private static List<String> parseAndGetWarnings(String query) throws ParseException {
+ return parseAndGetWarnings(new QueryEngineSettings(), query);
+ }
+
+ private static List<String> parseAndGetWarnings(QueryEngineSettings settings, String query) throws ParseException {
+ SQL2Parser parser = createTestSQL2Parser(NamePathMapper.DEFAULT, nodeTypes, settings);
+ LogCustomizer customLogs = LogCustomizer.forLogger(SQL2Parser.class.getName()).enable(Level.WARN).create();
+ try {
+ customLogs.starting();
+ parser.parse(query);
+ return new ArrayList<>(customLogs.getLogs());
+ } finally {
+ customLogs.finished();
+ }
+ }
+
interface DummyQueryIndex extends QueryIndex, QueryIndex.AdvancedQueryIndex {
/*
* This is needed as AdvancedQueryIndex is what we what to mock but the QueryIndexProvider