From b3061cbdf49dd094dd471146dfcf08ed2dcd302c Mon Sep 17 00:00:00 2001 From: Mohit Kataria Date: Tue, 18 Jun 2019 14:20:55 +0530 Subject: [PATCH] OAK-7254: Indexes with excludedPaths, or includedPaths should not be picked for queries without path --- .../oak/api/StrictPathRestriction.java | 31 ++ .../oak/api/jmx/QueryEngineSettingsMBean.java | 26 ++ .../jackrabbit/oak/api/package-info.java | 2 +- .../java/org/apache/jackrabbit/oak/Oak.java | 9 + .../index/aggregate/AggregateIndexPlan.java | 6 + .../oak/query/QueryEngineSettings.java | 11 + .../oak/query/QueryEngineSettingsService.java | 13 + .../jackrabbit/oak/query/QueryImpl.java | 16 + .../oak/query/plan/SelectorExecutionPlan.java | 1 + .../index/lucene/LucenePropertyIndexTest.java | 30 +- ...LuceneStrictPathRestrictionEnableTest.java | 244 +++++++++++++++ .../LuceneStrictPathRestrictionWarnTest.java | 279 ++++++++++++++++++ .../jackrabbit/oak/spi/query/QueryIndex.java | 22 +- .../jackrabbit/oak/spi/query/QueryLimits.java | 6 + .../oak/spi/query/package-info.java | 2 +- .../spi/query/FulltextIndexPlanner.java | 16 + 16 files changed, 684 insertions(+), 30 deletions(-) create mode 100644 oak-api/src/main/java/org/apache/jackrabbit/oak/api/StrictPathRestriction.java create mode 100644 oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneStrictPathRestrictionEnableTest.java create mode 100644 oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneStrictPathRestrictionWarnTest.java diff --git a/oak-api/src/main/java/org/apache/jackrabbit/oak/api/StrictPathRestriction.java b/oak-api/src/main/java/org/apache/jackrabbit/oak/api/StrictPathRestriction.java new file mode 100644 index 0000000000..6c602b0632 --- /dev/null +++ b/oak-api/src/main/java/org/apache/jackrabbit/oak/api/StrictPathRestriction.java @@ -0,0 +1,31 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.oak.api; + +import java.util.Locale; + +public enum StrictPathRestriction { + ENABLE, + WARN, + DISABLE; + + public static StrictPathRestriction stringToEnum(String strictPathRestrictionInString) { + // OAK-260 : Locale english is being used explicitly. (DISABLE[in turkish] = DİSABLE[in English]--- Mind the dot above I) + return StrictPathRestriction.valueOf(strictPathRestrictionInString.toUpperCase(Locale.ENGLISH)); + } + +} diff --git a/oak-api/src/main/java/org/apache/jackrabbit/oak/api/jmx/QueryEngineSettingsMBean.java b/oak-api/src/main/java/org/apache/jackrabbit/oak/api/jmx/QueryEngineSettingsMBean.java index 14b3fb24a9..9177e46bf7 100644 --- a/oak-api/src/main/java/org/apache/jackrabbit/oak/api/jmx/QueryEngineSettingsMBean.java +++ b/oak-api/src/main/java/org/apache/jackrabbit/oak/api/jmx/QueryEngineSettingsMBean.java @@ -86,6 +86,32 @@ public interface QueryEngineSettingsMBean { void setFastQuerySize(boolean fastQuerySize); + /** + * Whether Path restrictions are enabled while figuring out index plan + * + * @return true if enabled + */ + String getStrictPathRestriction(); + + /** + * Whether path restrictions of indexes (excludedPaths / includedPaths) are taken into account during query execution, + * for Lucene indexes. When enabled, only indexes are considered if the index path restriction is compatible with the + * query path restrictions. When disabled, only the queryPaths of the index is taken into account. + * + * @param pathRestriction Set path restriction: Expected value is either of ENABLE/DISABLE/WARN + * ENABLE: enable path restriction- Index won't be used if index definition path restrictions are not compatible with query's path restriction + * DISABLE: path restrictions are not taken into account while querying + * WARN: path restrictions are not taken into account but a warning will be logged if query path restrictions are not compatible with index path restrictions + */ + @Description("Set path restriction: Expected value is either of ENABLE/DISABLE/WARN. " + + "ENABLE: enable path restriction- Index won't be used if index definition path restrictions are not compatible with query's path restriction. " + + "DISABLE: path restrictions are not taken into account while querying. " + + "WARN: path restrictions are not taken into account but a warning will be logged if query path restrictions are not compatible with index path restrictions." + ) + void setStrictPathRestriction( + @Name("pathRestriction") + String pathRestriction); + /** * Set or remove a query validator pattern. * diff --git a/oak-api/src/main/java/org/apache/jackrabbit/oak/api/package-info.java b/oak-api/src/main/java/org/apache/jackrabbit/oak/api/package-info.java index 5e9e1cf26c..ad18255218 100644 --- a/oak-api/src/main/java/org/apache/jackrabbit/oak/api/package-info.java +++ b/oak-api/src/main/java/org/apache/jackrabbit/oak/api/package-info.java @@ -18,7 +18,7 @@ /** * Oak repository API */ -@Version("3.1.2") +@Version("3.2.0") package org.apache.jackrabbit.oak.api; import org.osgi.annotation.versioning.Version; 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 68efb36671..1ce0dd3bb8 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 @@ -411,6 +411,7 @@ public class Oak { this.queryEngineSettings.setFullTextComparisonWithoutIndex(settings.getFullTextComparisonWithoutIndex()); this.queryEngineSettings.setLimitInMemory(settings.getLimitInMemory()); this.queryEngineSettings.setLimitReads(settings.getLimitReads()); + this.queryEngineSettings.setStrictPathRestriction(settings.getStrictPathRestriction()); return this; } @@ -933,6 +934,14 @@ public class Oak { settings.setFastQuerySize(fastQuerySize); } + public String getStrictPathRestriction() { + return settings.getStrictPathRestriction(); + } + + public void setStrictPathRestriction(String strictPathRestriction) { + settings.setStrictPathRestriction(strictPathRestriction); + } + @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/plugins/index/aggregate/AggregateIndexPlan.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/aggregate/AggregateIndexPlan.java index 0be7f5e7a8..1a2935fad0 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/aggregate/AggregateIndexPlan.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/aggregate/AggregateIndexPlan.java @@ -34,6 +34,7 @@ public class AggregateIndexPlan implements IndexPlan { private Filter filter; private boolean pathWithoutPlan; + private boolean logWarning; /** * The map of terms to plans. @@ -140,6 +141,11 @@ public class AggregateIndexPlan implements IndexPlan { return false; } + @Override + public boolean logWarning() { + return logWarning; + } + /** * Whether any base plan is a full text index. * 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 5baa135a6b..61a444716a 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 @@ -18,6 +18,7 @@ */ package org.apache.jackrabbit.oak.query; +import org.apache.jackrabbit.oak.api.StrictPathRestriction; import org.apache.jackrabbit.oak.api.jmx.QueryEngineSettingsMBean; import org.apache.jackrabbit.oak.query.stats.QueryStatsMBean; import org.apache.jackrabbit.oak.query.stats.QueryStatsMBeanImpl; @@ -76,6 +77,8 @@ public class QueryEngineSettings implements QueryEngineSettingsMBean, QueryLimit public static final boolean DEFAULT_FAST_QUERY_SIZE = Boolean.getBoolean(OAK_FAST_QUERY_SIZE); private boolean fastQuerySize = DEFAULT_FAST_QUERY_SIZE; + private StrictPathRestriction strictPathRestriction = StrictPathRestriction.DISABLE; + private final QueryStatsMBeanImpl queryStats = new QueryStatsMBeanImpl(this); /** @@ -134,6 +137,14 @@ public class QueryEngineSettings implements QueryEngineSettingsMBean, QueryLimit System.setProperty(OAK_FAST_QUERY_SIZE, String.valueOf(fastQuerySize)); } + public String getStrictPathRestriction() { + return strictPathRestriction.name(); + } + + public void setStrictPathRestriction(String strictPathRestriction) { + this.strictPathRestriction = StrictPathRestriction.stringToEnum(strictPathRestriction); + } + public void setFullTextComparisonWithoutIndex(boolean fullTextComparisonWithoutIndex) { this.fullTextComparisonWithoutIndex = fullTextComparisonWithoutIndex; } diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettingsService.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettingsService.java index a39269e852..7068491be8 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettingsService.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettingsService.java @@ -68,6 +68,16 @@ public class QueryEngineSettingsService { "Note: even if enabled, getSize may still return -1 if the index used does not support the feature." ) boolean fastQuerySize() default false; + + @AttributeDefinition( + name = "Enable Strict Path restrictions for indexes to be used", + description = "Whether path restrictions of indexes (excludedPaths / includedPaths) are taken into" + + "account during query execution, for Lucene indexes. When enabled, only indexes are considered if" + + "the index path restriction is compatible with the query path restrictions. When disabled, only" + + "the queryPaths of the index is taken into account." + ) + String getStrictPathRestrictionsForIndexes() default DISABLED_STRICT_PATH_RESTRICTION; + } // should be the same as QueryEngineSettings.DEFAULT_QUERY_LIMIT_IN_MEMORY @@ -82,6 +92,7 @@ public class QueryEngineSettingsService { static final String QUERY_FAIL_TRAVERSAL = "queryFailTraversal"; static final String QUERY_FAST_QUERY_SIZE = "fastQuerySize"; + static final String DISABLED_STRICT_PATH_RESTRICTION = "DISABLE"; private final Logger log = LoggerFactory.getLogger(getClass()); @@ -114,6 +125,8 @@ public class QueryEngineSettingsService { boolean fastQuerySizeSysProp = QueryEngineSettings.DEFAULT_FAST_QUERY_SIZE; boolean fastQuerySizeFromConfig = config.fastQuerySize(); queryEngineSettings.setFastQuerySize(fastQuerySizeFromConfig || fastQuerySizeSysProp); + + queryEngineSettings.setStrictPathRestriction(config.getStrictPathRestrictionsForIndexes()); log.info("Initialize QueryEngine settings {}", queryEngineSettings); } diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java index 3df64ed1df..554252709d 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java @@ -491,9 +491,25 @@ public class QueryImpl implements Query { return new ResultImpl(this); } + private boolean logWarning() { + if (selectors.size() > 0 && + selectors.get(0).getExecutionPlan() != null && + selectors.get(0).getExecutionPlan().getIndexPlan() != null && + selectors.get(0).getExecutionPlan().getIndexPlan().logWarning()) { + return true; + } + return false; + } + @Override public Iterator getRows() { prepare(); + + if (logWarning()){ + LOG.warn("Index definition of index used have path restrictions and query won't return nodes from" + + "those restricted paths"); + } + if (explain) { String plan = getPlan(); if (measure) { diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/plan/SelectorExecutionPlan.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/plan/SelectorExecutionPlan.java index eb3898a03c..34c0232f1a 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/plan/SelectorExecutionPlan.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/plan/SelectorExecutionPlan.java @@ -32,6 +32,7 @@ public class SelectorExecutionPlan implements ExecutionPlan { private final double estimatedCost; private final QueryIndex index; private final IndexPlan plan; + private boolean logWarning; public SelectorExecutionPlan(SelectorImpl selector, QueryIndex index, IndexPlan plan, double estimatedCost) { this.selector = selector; diff --git a/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LucenePropertyIndexTest.java b/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LucenePropertyIndexTest.java index a9edd08393..0a8c8bd364 100644 --- a/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LucenePropertyIndexTest.java +++ b/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LucenePropertyIndexTest.java @@ -86,6 +86,7 @@ import org.apache.jackrabbit.oak.plugins.nodetype.TypeEditorProvider; import org.apache.jackrabbit.oak.InitialContentHelper; import org.apache.jackrabbit.oak.plugins.nodetype.write.NodeTypeRegistry; import org.apache.jackrabbit.oak.query.AbstractQueryTest; +import org.apache.jackrabbit.oak.query.QueryEngineSettings; import org.apache.jackrabbit.oak.spi.commit.CommitInfo; import org.apache.jackrabbit.oak.spi.commit.EmptyHook; import org.apache.jackrabbit.oak.spi.commit.Observer; @@ -173,6 +174,8 @@ public class LucenePropertyIndexTest extends AbstractQueryTest { private ResultCountingIndexProvider queryIndexProvider; + private QueryEngineSettings queryEngineSettings = new QueryEngineSettings(); + @After public void after() { new ExecutorCloser(executorService).close(); @@ -199,6 +202,7 @@ public class LucenePropertyIndexTest extends AbstractQueryTest { .with(optionalEditorProvider) .with(new PropertyIndexEditorProvider()) .with(new NodeTypeIndexProvider()) + .with(queryEngineSettings) .createContentRepository(); } @@ -865,32 +869,6 @@ public class LucenePropertyIndexTest extends AbstractQueryTest { assertQuery("select [jcr:path] from [nt:base] where [propa] = 10", asList("/test/a", "/test/a/b")); } - @Test - public void pathExclude() throws Exception{ - Tree idx = createIndex("test1", of("propa", "propb")); - idx.setProperty(createProperty(PROP_EXCLUDED_PATHS, of("/test/a"), Type.STRINGS)); - //Do not provide type information - root.commit(); - - Tree test = root.getTree("/").addChild("test"); - test.addChild("a").setProperty("propa", 10); - test.addChild("a").addChild("b").setProperty("propa", 10); - test.addChild("c").setProperty("propa", 10); - root.commit(); - - assertThat(explain("select [jcr:path] from [nt:base] where [propa] = 10"), containsString("lucene:test1")); - - assertQuery("select [jcr:path] from [nt:base] where [propa] = 10", asList("/test/c")); - - //Make some change and then check - test = root.getTree("/").getChild("test"); - test.addChild("a").addChild("e").setProperty("propa", 10); - test.addChild("f").setProperty("propa", 10); - root.commit(); - - assertQuery("select [jcr:path] from [nt:base] where [propa] = 10", asList("/test/c", "/test/f")); - } - //OAK-4516 @Test public void wildcardQueryToLookupUnanalyzedText() throws Exception { diff --git a/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneStrictPathRestrictionEnableTest.java b/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneStrictPathRestrictionEnableTest.java new file mode 100644 index 0000000000..b0efa888db --- /dev/null +++ b/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneStrictPathRestrictionEnableTest.java @@ -0,0 +1,244 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.jackrabbit.oak.plugins.index.lucene; + +import org.apache.commons.io.FileUtils; +import org.apache.jackrabbit.JcrConstants; +import org.apache.jackrabbit.oak.InitialContentHelper; +import org.apache.jackrabbit.oak.Oak; +import org.apache.jackrabbit.oak.api.CommitFailedException; +import org.apache.jackrabbit.oak.api.ContentRepository; +import org.apache.jackrabbit.oak.api.StrictPathRestriction; +import org.apache.jackrabbit.oak.api.Tree; +import org.apache.jackrabbit.oak.api.Type; +import org.apache.jackrabbit.oak.commons.concurrent.ExecutorCloser; +import org.apache.jackrabbit.oak.plugins.index.lucene.directory.CopyOnReadDirectory; +import org.apache.jackrabbit.oak.plugins.index.nodetype.NodeTypeIndexProvider; +import org.apache.jackrabbit.oak.plugins.index.property.PropertyIndexEditorProvider; +import org.apache.jackrabbit.oak.plugins.index.search.ExtractedTextCache; +import org.apache.jackrabbit.oak.plugins.index.search.FulltextIndexConstants; +import org.apache.jackrabbit.oak.plugins.index.search.IndexDefinition; +import org.apache.jackrabbit.oak.plugins.memory.MemoryNodeStore; +import org.apache.jackrabbit.oak.plugins.memory.PropertyStates; +import org.apache.jackrabbit.oak.query.AbstractQueryTest; +import org.apache.jackrabbit.oak.query.QueryEngineSettings; +import org.apache.jackrabbit.oak.spi.commit.Observer; +import org.apache.jackrabbit.oak.spi.security.OpenSecurityProvider; +import org.apache.jackrabbit.oak.spi.state.NodeStore; +import org.apache.lucene.store.Directory; +import org.apache.lucene.store.FSDirectory; +import org.apache.lucene.store.FilterDirectory; +import org.junit.After; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.io.File; +import java.io.IOException; +import java.util.Set; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + +import static com.google.common.collect.ImmutableSet.of; +import static java.util.Arrays.asList; +import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.INDEX_DEFINITIONS_NAME; +import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.INDEX_DEFINITIONS_NODE_TYPE; +import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.REINDEX_PROPERTY_NAME; +import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.TYPE_PROPERTY_NAME; +import static org.apache.jackrabbit.oak.plugins.memory.PropertyStates.createProperty; +import static org.apache.jackrabbit.oak.spi.filter.PathFilter.PROP_EXCLUDED_PATHS; +import static org.apache.jackrabbit.oak.spi.filter.PathFilter.PROP_INCLUDED_PATHS; +import static org.hamcrest.CoreMatchers.containsString; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThat; + +@SuppressWarnings("ArraysAsListWithZeroOrOneArgument") +public class LuceneStrictPathRestrictionEnableTest extends AbstractQueryTest { + + private ExecutorService executorService = Executors.newFixedThreadPool(2); + @Rule + public TemporaryFolder temporaryFolder = new TemporaryFolder(new File("target")); + + private String corDir = null; + private String cowDir = null; + + private LuceneIndexEditorProvider editorProvider; + + private TestUtil.OptionalEditorProvider optionalEditorProvider = new TestUtil.OptionalEditorProvider(); + + private NodeStore nodeStore; + + private LuceneIndexProvider provider; + + private ResultCountingIndexProvider queryIndexProvider; + + private QueryEngineSettings queryEngineSettings = new QueryEngineSettings(); + + @After + public void after() { + new ExecutorCloser(executorService).close(); + IndexDefinition.setDisableStoredIndexDefinition(false); + } + + @Override + protected void createTestIndexNode() throws Exception { + setTraversalEnabled(false); + } + + @Override + protected ContentRepository createRepository() { + IndexCopier copier = createIndexCopier(); + editorProvider = new LuceneIndexEditorProvider(copier, new ExtractedTextCache(10 * FileUtils.ONE_MB, 100)); + provider = new LuceneIndexProvider(copier); + queryIndexProvider = new ResultCountingIndexProvider(provider); + nodeStore = new MemoryNodeStore(InitialContentHelper.INITIAL_CONTENT); + queryEngineSettings.setStrictPathRestriction(StrictPathRestriction.ENABLE.name()); + return new Oak(nodeStore) + .with(new OpenSecurityProvider()) + .with(queryIndexProvider) + .with((Observer) provider) + .with(editorProvider) + .with(optionalEditorProvider) + .with(new PropertyIndexEditorProvider()) + .with(new NodeTypeIndexProvider()) + .with(queryEngineSettings) + .createContentRepository(); + } + + private IndexCopier createIndexCopier() { + try { + return new IndexCopier(executorService, temporaryFolder.getRoot()) { + @Override + public Directory wrapForRead(String indexPath, LuceneIndexDefinition definition, + Directory remote, String dirName) throws IOException { + Directory ret = super.wrapForRead(indexPath, definition, remote, dirName); + corDir = getFSDirPath(ret); + return ret; + } + + @Override + public Directory wrapForWrite(LuceneIndexDefinition definition, + Directory remote, boolean reindexMode, String dirName, + COWDirectoryTracker cowDirectoryTracker) throws IOException { + Directory ret = super.wrapForWrite(definition, remote, reindexMode, dirName, cowDirectoryTracker); + cowDir = getFSDirPath(ret); + return ret; + } + + private String getFSDirPath(Directory dir) { + if (dir instanceof CopyOnReadDirectory) { + dir = ((CopyOnReadDirectory) dir).getLocal(); + } + + dir = unwrap(dir); + + if (dir instanceof FSDirectory) { + return ((FSDirectory) dir).getDirectory().getAbsolutePath(); + } + return null; + } + + private Directory unwrap(Directory dir) { + if (dir instanceof FilterDirectory) { + return unwrap(((FilterDirectory) dir).getDelegate()); + } + return dir; + } + + }; + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + @After + public void shutdownExecutor() { + executorService.shutdown(); + } + + @Test + public void pathIncludeWithPathRestrictionsEnabled() throws Exception { + + Tree idx = createIndex("test1", of("propa", "propb")); + idx.setProperty(createProperty(PROP_INCLUDED_PATHS, of("/test/a"), Type.STRINGS)); + root.commit(); + + Tree test = root.getTree("/").addChild("test"); + test.addChild("a").setProperty("propa", 10); + test.addChild("a").addChild("b").setProperty("propa", 10); + test.addChild("c").setProperty("propa", 10); + root.commit(); + + assertFalse(explain("select [jcr:path] from [nt:base] where [propa] = 10").contains("lucene:test1")); + assertThat(explain("select [jcr:path] from [nt:base] where [propa] = 10 and isDescendantNode('/test/a')"), containsString("lucene:test1")); + assertQuery("select [jcr:path] from [nt:base] where [propa] = 10 and isDescendantNode('/test/a')", asList("/test/a/b")); + + } + + @Test + public void pathExcludeWithPathRestrictionsEnabled() throws Exception { + Tree idx = createIndex("test1", of("propa", "propb")); + idx.setProperty(createProperty(PROP_EXCLUDED_PATHS, of("/test/a"), Type.STRINGS)); + root.commit(); + + Tree test = root.getTree("/").addChild("test"); + test.addChild("a").setProperty("propa", 10); + test.addChild("a").addChild("b").setProperty("propa", 10); + test.addChild("c").setProperty("propa", 10); + test.addChild("c").addChild("d").setProperty("propa", 10); + root.commit(); + + assertFalse(explain("select [jcr:path] from [nt:base] where [propa] = 10").contains("lucene:test1")); + assertThat(explain("select [jcr:path] from [nt:base] where [propa] = 10 and isDescendantNode('/test/c')"), containsString("lucene:test1")); + + assertQuery("select [jcr:path] from [nt:base] where [propa] = 10 and isDescendantNode('/test/c')", asList("/test/c/d")); + + //Make some change and then check + Tree testc = root.getTree("/").getChild("test").getChild("c"); + testc.addChild("e").addChild("f").setProperty("propa", 10); + root.commit(); + assertQuery("select [jcr:path] from [nt:base] where [propa] = 10 and isDescendantNode('/test/c')", asList("/test/c/d", "/test/c/e/f")); + assertThat(explain("select [jcr:path] from [nt:base] where [propa] = 10 and isDescendantNode('/test/c') and not(isDescendantNode('/test/c/e'))"), containsString("lucene:test1")); + assertQuery("select [jcr:path] from [nt:base] where [propa] = 10 and isDescendantNode('/test/c') and not(isDescendantNode('/test/c/e'))", asList("/test/c/d")); + } + + private String explain(String query) { + String explain = "explain " + query; + return executeQuery(explain, "JCR-SQL2").get(0); + } + + private Tree createIndex(String name, Set propNames) throws CommitFailedException { + Tree index = root.getTree("/"); + return createIndex(index, name, propNames); + } + + public static Tree createIndex(Tree index, String name, Set propNames) throws CommitFailedException { + Tree def = index.addChild(INDEX_DEFINITIONS_NAME).addChild(name); + def.setProperty(JcrConstants.JCR_PRIMARYTYPE, + INDEX_DEFINITIONS_NODE_TYPE, Type.NAME); + def.setProperty(TYPE_PROPERTY_NAME, LuceneIndexConstants.TYPE_LUCENE); + def.setProperty(REINDEX_PROPERTY_NAME, true); + def.setProperty(FulltextIndexConstants.FULL_TEXT_ENABLED, false); + def.setProperty(PropertyStates.createProperty(FulltextIndexConstants.INCLUDE_PROPERTY_NAMES, propNames, Type.STRINGS)); + def.setProperty(LuceneIndexConstants.SAVE_DIR_LISTING, true); + return index.getChild(INDEX_DEFINITIONS_NAME).getChild(name); + } + +} diff --git a/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneStrictPathRestrictionWarnTest.java b/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneStrictPathRestrictionWarnTest.java new file mode 100644 index 0000000000..0ff17e3b86 --- /dev/null +++ b/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneStrictPathRestrictionWarnTest.java @@ -0,0 +1,279 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.jackrabbit.oak.plugins.index.lucene; + +import ch.qos.logback.classic.LoggerContext; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; +import org.apache.commons.io.FileUtils; +import org.apache.jackrabbit.JcrConstants; +import org.apache.jackrabbit.oak.InitialContentHelper; +import org.apache.jackrabbit.oak.Oak; +import org.apache.jackrabbit.oak.api.CommitFailedException; +import org.apache.jackrabbit.oak.api.ContentRepository; +import org.apache.jackrabbit.oak.api.StrictPathRestriction; +import org.apache.jackrabbit.oak.api.Tree; +import org.apache.jackrabbit.oak.api.Type; +import org.apache.jackrabbit.oak.commons.concurrent.ExecutorCloser; +import org.apache.jackrabbit.oak.plugins.index.lucene.directory.CopyOnReadDirectory; +import org.apache.jackrabbit.oak.plugins.index.nodetype.NodeTypeIndexProvider; +import org.apache.jackrabbit.oak.plugins.index.property.PropertyIndexEditorProvider; +import org.apache.jackrabbit.oak.plugins.index.search.ExtractedTextCache; +import org.apache.jackrabbit.oak.plugins.index.search.FulltextIndexConstants; +import org.apache.jackrabbit.oak.plugins.index.search.IndexDefinition; +import org.apache.jackrabbit.oak.plugins.memory.MemoryNodeStore; +import org.apache.jackrabbit.oak.plugins.memory.PropertyStates; +import org.apache.jackrabbit.oak.query.AbstractQueryTest; +import org.apache.jackrabbit.oak.query.QueryEngineSettings; +import org.apache.jackrabbit.oak.spi.commit.Observer; +import org.apache.jackrabbit.oak.spi.security.OpenSecurityProvider; +import org.apache.jackrabbit.oak.spi.state.NodeStore; +import org.apache.lucene.store.Directory; +import org.apache.lucene.store.FSDirectory; +import org.apache.lucene.store.FilterDirectory; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.slf4j.LoggerFactory; + +import java.io.File; +import java.io.IOException; +import java.util.Set; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + +import static com.google.common.collect.ImmutableSet.of; +import static java.util.Arrays.asList; +import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.INDEX_DEFINITIONS_NAME; +import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.INDEX_DEFINITIONS_NODE_TYPE; +import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.REINDEX_PROPERTY_NAME; +import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.TYPE_PROPERTY_NAME; +import static org.apache.jackrabbit.oak.plugins.memory.PropertyStates.createProperty; +import static org.apache.jackrabbit.oak.spi.filter.PathFilter.PROP_EXCLUDED_PATHS; +import static org.apache.jackrabbit.oak.spi.filter.PathFilter.PROP_INCLUDED_PATHS; +import static org.hamcrest.CoreMatchers.containsString; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; + +@SuppressWarnings("ArraysAsListWithZeroOrOneArgument") +public class LuceneStrictPathRestrictionWarnTest extends AbstractQueryTest { + + private ExecutorService executorService = Executors.newFixedThreadPool(2); + @Rule + public TemporaryFolder temporaryFolder = new TemporaryFolder(new File("target")); + + ListAppender listAppender = null; + + private String corDir = null; + private String cowDir = null; + + private LuceneIndexEditorProvider editorProvider; + + private TestUtil.OptionalEditorProvider optionalEditorProvider = new TestUtil.OptionalEditorProvider(); + + private NodeStore nodeStore; + + private LuceneIndexProvider provider; + + private ResultCountingIndexProvider queryIndexProvider; + + private QueryEngineSettings queryEngineSettings = new QueryEngineSettings(); + + private final String warnMessage = "Index definition of index used have path restrictions and query won't return nodes from" + + "those restricted paths"; + + private final String queryImplLogger = "org.apache.jackrabbit.oak.query.QueryImpl"; + + @Before + public void loggingAppenderStart() { + LoggerContext context = (LoggerContext) LoggerFactory.getILoggerFactory(); + listAppender = new ListAppender<>(); + listAppender.start(); + context.getLogger(queryImplLogger).addAppender(listAppender); + } + + @After + public void loggingAppenderStop() { + listAppender.stop(); + } + + @After + public void after() { + new ExecutorCloser(executorService).close(); + IndexDefinition.setDisableStoredIndexDefinition(false); + } + + @Override + protected void createTestIndexNode() throws Exception { + setTraversalEnabled(false); + } + + @Override + protected ContentRepository createRepository() { + IndexCopier copier = createIndexCopier(); + editorProvider = new LuceneIndexEditorProvider(copier, new ExtractedTextCache(10 * FileUtils.ONE_MB, 100)); + provider = new LuceneIndexProvider(copier); + queryIndexProvider = new ResultCountingIndexProvider(provider); + nodeStore = new MemoryNodeStore(InitialContentHelper.INITIAL_CONTENT); + queryEngineSettings.setStrictPathRestriction(StrictPathRestriction.WARN.name()); + return new Oak(nodeStore) + .with(new OpenSecurityProvider()) + .with(queryIndexProvider) + .with((Observer) provider) + .with(editorProvider) + .with(optionalEditorProvider) + .with(new PropertyIndexEditorProvider()) + .with(new NodeTypeIndexProvider()) + .with(queryEngineSettings) + .createContentRepository(); + } + + private IndexCopier createIndexCopier() { + try { + return new IndexCopier(executorService, temporaryFolder.getRoot()) { + @Override + public Directory wrapForRead(String indexPath, LuceneIndexDefinition definition, + Directory remote, String dirName) throws IOException { + Directory ret = super.wrapForRead(indexPath, definition, remote, dirName); + corDir = getFSDirPath(ret); + return ret; + } + + @Override + public Directory wrapForWrite(LuceneIndexDefinition definition, + Directory remote, boolean reindexMode, String dirName, + COWDirectoryTracker cowDirectoryTracker) throws IOException { + Directory ret = super.wrapForWrite(definition, remote, reindexMode, dirName, cowDirectoryTracker); + cowDir = getFSDirPath(ret); + return ret; + } + + private String getFSDirPath(Directory dir) { + if (dir instanceof CopyOnReadDirectory) { + dir = ((CopyOnReadDirectory) dir).getLocal(); + } + + dir = unwrap(dir); + + if (dir instanceof FSDirectory) { + return ((FSDirectory) dir).getDirectory().getAbsolutePath(); + } + return null; + } + + private Directory unwrap(Directory dir) { + if (dir instanceof FilterDirectory) { + return unwrap(((FilterDirectory) dir).getDelegate()); + } + return dir; + } + + }; + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + @After + public void shutdownExecutor() { + executorService.shutdown(); + } + + @Test + public void pathIncludeWithPathRestrictionsWarn() throws Exception { + + Tree idx = createIndex("test1", of("propa", "propb")); + idx.setProperty(createProperty(PROP_INCLUDED_PATHS, of("/test/a"), Type.STRINGS)); + //Do not provide type information + root.commit(); + + Tree test = root.getTree("/").addChild("test"); + test.addChild("a").setProperty("propa", 10); + test.addChild("a").addChild("b").setProperty("propa", 10); + test.addChild("c").setProperty("propa", 10); + root.commit(); + + assertThat(explain("select [jcr:path] from [nt:base] where [propa] = 10 and isDescendantNode('/test/a')"), containsString("lucene:test1")); + assertQuery("select [jcr:path] from [nt:base] where [propa] = 10 and isDescendantNode('/test/a')", asList("/test/a/b")); + // List appender should not have any warn logs as we are searching under right descendant as per path restrictions + assertFalse(isWarnMessagePresent(listAppender)); + assertTrue(explain("select [jcr:path] from [nt:base] where [propa] = 10").contains("lucene:test1")); + // List appender now will have warn log as we are searching under root(/) but index definition have include path restriction. + assertTrue(isWarnMessagePresent(listAppender)); + } + + @Test + public void pathExcludeWithPathRestrictionsWarn() throws Exception { + Tree idx = createIndex("test1", of("propa", "propb")); + idx.setProperty(createProperty(PROP_EXCLUDED_PATHS, of("/test/a"), Type.STRINGS)); + //Do not provide type information + root.commit(); + + Tree test = root.getTree("/").addChild("test"); + test.addChild("a").setProperty("propa", 10); + test.addChild("a").addChild("b").setProperty("propa", 10); + test.addChild("c").setProperty("propa", 10); + test.addChild("c").addChild("d").setProperty("propa", 10); + root.commit(); + + assertThat(explain("select [jcr:path] from [nt:base] where [propa] = 10 and isDescendantNode('/test/c')"), containsString("lucene:test1")); + assertQuery("select [jcr:path] from [nt:base] where [propa] = 10 and isDescendantNode('/test/c')", asList("/test/c/d")); + // List appender should not have any warn logs as we are searching under right descendant as per path restrictions + assertFalse(isWarnMessagePresent(listAppender)); + assertTrue(explain("select [jcr:path] from [nt:base] where [propa] = 10").contains("lucene:test1")); + // List appender now will have warn log as we are searching under root(/) but index definition have exclude path restriction. + assertTrue(isWarnMessagePresent(listAppender)); + } + + private boolean isWarnMessagePresent(ListAppender listAppender) { + for (ILoggingEvent loggingEvent : listAppender.list) { + if (loggingEvent.getMessage().contains(warnMessage)) { + return true; + } + } + return false; + } + + private String explain(String query) { + String explain = "explain " + query; + return executeQuery(explain, "JCR-SQL2").get(0); + } + + private Tree createIndex(String name, Set propNames) throws CommitFailedException { + Tree index = root.getTree("/"); + return createIndex(index, name, propNames); + } + + public static Tree createIndex(Tree index, String name, Set propNames) throws CommitFailedException { + Tree def = index.addChild(INDEX_DEFINITIONS_NAME).addChild(name); + def.setProperty(JcrConstants.JCR_PRIMARYTYPE, + INDEX_DEFINITIONS_NODE_TYPE, Type.NAME); + def.setProperty(TYPE_PROPERTY_NAME, LuceneIndexConstants.TYPE_LUCENE); + def.setProperty(REINDEX_PROPERTY_NAME, true); + def.setProperty(FulltextIndexConstants.FULL_TEXT_ENABLED, false); + def.setProperty(PropertyStates.createProperty(FulltextIndexConstants.INCLUDE_PROPERTY_NAMES, propNames, Type.STRINGS)); + def.setProperty(LuceneIndexConstants.SAVE_DIR_LISTING, true); + return index.getChild(INDEX_DEFINITIONS_NAME).getChild(name); + } + +} diff --git a/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/QueryIndex.java b/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/QueryIndex.java index d7556b9651..05c50d3821 100644 --- a/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/QueryIndex.java +++ b/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/QueryIndex.java @@ -348,6 +348,10 @@ public interface QueryIndex { * @return if it is deprecated */ boolean isDeprecated(); + + default boolean logWarning() { + return false; + } /** * A builder for index plans. @@ -369,6 +373,7 @@ public interface QueryIndex { protected Map attributes = Maps.newHashMap(); protected String planName; protected boolean deprecated; + protected boolean logWarning; public Builder setCostPerExecution(double costPerExecution) { this.costPerExecution = costPerExecution; @@ -394,6 +399,11 @@ public interface QueryIndex { this.isDelayed = isDelayed; return this; } + + public Builder setLogWarning() { + this.logWarning = true; + return this; + } public Builder setFulltextIndex(boolean isFulltextIndex) { this.isFulltextIndex = isFulltextIndex; @@ -480,6 +490,7 @@ public interface QueryIndex { private final String planName = Builder.this.planName; private final boolean deprecated = Builder.this.deprecated; + private final boolean logWarning = Builder.this.logWarning; @Override public String toString() { @@ -496,7 +507,8 @@ public interface QueryIndex { + " propertyRestriction : %s," + " pathPrefix : %s," + " deprecated : %s," - + " supportsPathRestriction : %s }", + + " supportsPathRestriction : %s," + + " logWarning : %s}", costPerExecution, costPerEntry, estimatedEntryCount, @@ -509,7 +521,8 @@ public interface QueryIndex { propRestriction, pathPrefix, deprecated, - supportsPathRestriction + supportsPathRestriction, + logWarning ); } @@ -606,6 +619,11 @@ public interface QueryIndex { public boolean isDeprecated() { return deprecated; } + + @Override + public boolean logWarning() { + return logWarning; + } }; } diff --git a/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/QueryLimits.java b/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/QueryLimits.java index d6d2591a11..a1ee42de99 100644 --- a/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/QueryLimits.java +++ b/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/QueryLimits.java @@ -18,6 +18,8 @@ */ package org.apache.jackrabbit.oak.spi.query; +import org.apache.jackrabbit.oak.api.StrictPathRestriction; + public interface QueryLimits { long getLimitInMemory(); @@ -28,4 +30,8 @@ public interface QueryLimits { boolean getFailTraversal(); + default String getStrictPathRestriction() { + return StrictPathRestriction.DISABLE.name(); + } + } diff --git a/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/package-info.java b/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/package-info.java index 40d6b8104c..c64316e0b9 100644 --- a/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/package-info.java +++ b/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/package-info.java @@ -14,7 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -@Version("1.2.0") +@Version("1.3.0") package org.apache.jackrabbit.oak.spi.query; import org.osgi.annotation.versioning.Version; diff --git a/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/query/FulltextIndexPlanner.java b/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/query/FulltextIndexPlanner.java index 08196b428f..1c6d90508a 100644 --- a/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/query/FulltextIndexPlanner.java +++ b/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/query/FulltextIndexPlanner.java @@ -34,6 +34,7 @@ import com.google.common.collect.Iterables; import com.google.common.collect.Multimap; import org.apache.jackrabbit.JcrConstants; import org.apache.jackrabbit.oak.api.PropertyValue; +import org.apache.jackrabbit.oak.api.StrictPathRestriction; import org.apache.jackrabbit.oak.api.Type; import org.apache.jackrabbit.oak.commons.PathUtils; import org.apache.jackrabbit.oak.plugins.index.IndexConstants; @@ -44,6 +45,7 @@ import org.apache.jackrabbit.oak.plugins.index.search.IndexFormatVersion; import org.apache.jackrabbit.oak.plugins.index.search.IndexNode; import org.apache.jackrabbit.oak.plugins.index.search.IndexStatistics; import org.apache.jackrabbit.oak.plugins.index.search.PropertyDefinition; +import org.apache.jackrabbit.oak.spi.filter.PathFilter; import org.apache.jackrabbit.oak.spi.query.Filter; import org.apache.jackrabbit.oak.spi.query.Filter.PropertyRestriction; import org.apache.jackrabbit.oak.spi.query.QueryConstants; @@ -152,6 +154,9 @@ public class FulltextIndexPlanner { if (wrongIndex()) { return null; } + if (filter.getQueryLimits().getStrictPathRestriction().equals(StrictPathRestriction.ENABLE.name()) && !isPlanWithValidPathFilter()) { + return null; + } FullTextExpression ft = filter.getFullTextConstraint(); @@ -286,6 +291,11 @@ public class FulltextIndexPlanner { costPerEntryFactor += sortOrder.size(); IndexPlan.Builder plan = defaultPlan(); + + if (filter.getQueryLimits().getStrictPathRestriction().equals(StrictPathRestriction.WARN.name()) && !isPlanWithValidPathFilter()) { + plan.setLogWarning(); + } + if (plan == null) { return null; } @@ -333,6 +343,12 @@ public class FulltextIndexPlanner { return null; } + private boolean isPlanWithValidPathFilter() { + String pathFilter = filter.getPath(); + PathFilter definitionPathFilter = definition.getPathFilter(); + return definitionPathFilter.areAllDescendantsIncluded(pathFilter); + } + private boolean matchesValuePattern(PropertyRestriction pr, PropertyDefinition pd) { if (!pd.valuePattern.matchesAll()){ //So we have a valuePattern defined. So determine if -- 2.17.1