From d236f691deb4c815da319bc1e900d4b9b63013a4 Mon Sep 17 00:00:00 2001 From: Vikas Saurabh Date: Tue, 5 Jan 2016 18:01:14 +0000 Subject: [PATCH] OAK-3838: IndexPlanner incorrectly lets all full text indices to participate for suggest/spellcheck queries Index planner, for spellcheck and suggest queries, would now validate selectNodeTypeName=indexRule.nodeTypeName and that at least one property in the index need to supporting the features. git-svn-id: https://svn.apache.org/repos/asf/jackrabbit/oak/trunk@1723142 13f79535-47bb-0310-9956-ffa450edef68 --- .../jackrabbit/oak/query/ast/SelectorImpl.java | 4 + .../jackrabbit/oak/query/index/FilterImpl.java | 8 ++ .../apache/jackrabbit/oak/spi/query/Filter.java | 8 ++ .../jackrabbit/oak/spi/query/package-info.java | 2 +- .../oak/plugins/index/lucene/IndexDefinition.java | 46 +++++++- .../oak/plugins/index/lucene/IndexPlanner.java | 30 ++++- .../oak/plugins/index/lucene/IndexPlannerTest.java | 122 +++++++++++++++++++++ .../index/lucene/LuceneIndexSuggestionTest.java | 2 +- 8 files changed, 211 insertions(+), 11 deletions(-) diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/ast/SelectorImpl.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/ast/SelectorImpl.java index 09ec9b7..e22f768 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/ast/SelectorImpl.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/ast/SelectorImpl.java @@ -193,6 +193,10 @@ public class SelectorImpl extends SourceImpl { return selectorName; } + public String getNodeTypeName() { + return nodeTypeName; + } + public boolean matchesAllTypes() { return matchesAllTypes; } diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/index/FilterImpl.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/index/FilterImpl.java index 8e05c58..b6f3081 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/index/FilterImpl.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/index/FilterImpl.java @@ -223,6 +223,14 @@ public class FilterImpl implements Filter { return selector; } + @Override @Nullable + public String getNodeTypeName() { + if (selector == null) { + return null; + } + return selector.getNodeTypeName(); + } + @Override public boolean matchesAllTypes() { return matchesAllTypes; diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/query/Filter.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/query/Filter.java index 9272346..3a806f0 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/query/Filter.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/query/Filter.java @@ -131,6 +131,14 @@ public interface Filter { String getPathPlan(); /** + * Returns the name of the filter node type. + * + * @return nodetype name + */ + @Nullable + String getNodeTypeName(); + + /** * Checks whether nodes of all types can match this filter. * * @return {@code true} iff there are no type restrictions diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/query/package-info.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/query/package-info.java index 86867cd..28c69ee 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/query/package-info.java +++ b/oak-core/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("4.0.0") +@Version("5.0.0") @Export(optional = "provide:=true") package org.apache.jackrabbit.oak.spi.query; diff --git a/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexDefinition.java b/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexDefinition.java index 830d57b..7ed13fe 100644 --- a/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexDefinition.java +++ b/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexDefinition.java @@ -207,6 +207,10 @@ class IndexDefinition implements Aggregate.AggregateMapper { private final boolean secureFacets; + private final boolean suggestEnabled; + + private final boolean spellcheckEnabled; + public IndexDefinition(NodeState root, NodeState defn) { this(root, defn, null); } @@ -274,6 +278,8 @@ class IndexDefinition implements Aggregate.AggregateMapper { this.saveDirListing = getOptionalValue(defn, LuceneIndexConstants.SAVE_DIR_LISTING, true); this.suggestAnalyzed = getOptionalValue(defn, LuceneIndexConstants.SUGGEST_ANALYZED, false); this.secureFacets = defn.hasChildNode(FACETS) && getOptionalValue(defn.getChildNode(FACETS), PROP_SECURE_FACETS, true); + this.suggestEnabled = evaluateSuggestionEnabled(); + this.spellcheckEnabled = evaluateSpellcheckEnabled(); } public NodeState getDefinitionNodeState() { @@ -595,25 +601,46 @@ class IndexDefinition implements Aggregate.AggregateMapper { return ntBaseRule != null; } - public boolean isSuggestEnabled() { - boolean suggestEnabled = false; + private boolean evaluateSuggestionEnabled() { for (IndexingRule indexingRule : definedRules) { for (PropertyDefinition propertyDefinition : indexingRule.propConfigs.values()) { if (propertyDefinition.useInSuggest) { - suggestEnabled = true; - break; + return true; } } for (NamePattern np : indexingRule.namePatterns) { if (np.getConfig().useInSuggest) { - suggestEnabled = true; - break; + return true; } } } + return false; + } + + public boolean isSuggestEnabled() { return suggestEnabled; } + private boolean evaluateSpellcheckEnabled() { + for (IndexingRule indexingRule : definedRules) { + for (PropertyDefinition propertyDefinition : indexingRule.propConfigs.values()) { + if (propertyDefinition.useInSpellcheck) { + return true; + } + } + for (NamePattern np : indexingRule.namePatterns) { + if (np.getConfig().useInSpellcheck) { + return true; + } + } + } + return false; + } + + public boolean isSpellcheckEnabled() { + return spellcheckEnabled; + } + @CheckForNull public String getIndexPathFromConfig() { return definition.getString(LuceneIndexConstants.INDEX_PATH); @@ -730,6 +757,13 @@ class IndexDefinition implements Aggregate.AggregateMapper { return nodeTypeName; } + /** + * @return name of the base node type. + */ + public String getBaseNodeType() { + return baseNodeType; + } + public List getNullCheckEnabledProperties() { return nullCheckEnabledProperties; } diff --git a/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexPlanner.java b/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexPlanner.java index 8066562..98ea42c 100644 --- a/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexPlanner.java +++ b/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexPlanner.java @@ -30,6 +30,7 @@ import javax.annotation.CheckForNull; import com.google.common.collect.Iterables; import org.apache.jackrabbit.JcrConstants; +import org.apache.jackrabbit.oak.api.PropertyValue; import org.apache.jackrabbit.oak.api.Type; import org.apache.jackrabbit.oak.commons.PathUtils; import org.apache.jackrabbit.oak.plugins.index.lucene.IndexDefinition.IndexingRule; @@ -139,9 +140,7 @@ class IndexPlanner { if (definition.hasFunctionDefined() && filter.getPropertyRestriction(definition.getFunctionName()) != null) { - //If native function is handled by this index then ensure - // that lowest cost if returned - return defaultPlan().setEstimatedEntryCount(1); + return getNativeFunctionPlanBuilder(indexingRule.getBaseNodeType()); } List indexedProps = newArrayListWithCapacity(filter.getPropertyRestrictions().size()); @@ -227,6 +226,31 @@ class IndexPlanner { return null; } + private IndexPlan.Builder getNativeFunctionPlanBuilder(String indexingRuleBaseNodeType) { + boolean canHandleNativeFunction = true; + + PropertyValue pv = filter.getPropertyRestriction(definition.getFunctionName()).first; + String query = pv.getValue(Type.STRING); + + if (query.startsWith("suggest?term=")) { + if (definition.isSuggestEnabled()) { + canHandleNativeFunction = indexingRuleBaseNodeType.equals(filter.getNodeTypeName()); + } else { + canHandleNativeFunction = false; + } + } else if (query.startsWith("spellcheck?term=")) { + if (definition.isSpellcheckEnabled()) { + canHandleNativeFunction = indexingRuleBaseNodeType.equals(filter.getNodeTypeName()); + } else { + canHandleNativeFunction = false; + } + } + + //If native function can be handled by this index then ensure + // that lowest cost if returned + return canHandleNativeFunction ? defaultPlan().setEstimatedEntryCount(1) : null; + } + /** * Check if there is a mismatch between QueryPaths associated with index * and path restriction specified in query diff --git a/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexPlannerTest.java b/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexPlannerTest.java index 446e750..d703736 100644 --- a/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexPlannerTest.java +++ b/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexPlannerTest.java @@ -54,6 +54,7 @@ import static javax.jcr.PropertyType.TYPENAME_STRING; import static org.apache.jackrabbit.JcrConstants.JCR_SYSTEM; import static org.apache.jackrabbit.oak.api.Type.NAMES; import static org.apache.jackrabbit.oak.api.Type.STRINGS; +import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.DECLARING_NODE_TYPES; import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.INDEX_DEFINITIONS_NAME; import static org.apache.jackrabbit.oak.plugins.index.PathFilter.PROP_INCLUDED_PATHS; import static org.apache.jackrabbit.oak.plugins.index.lucene.LuceneIndexConstants.INDEX_DATA_CHILD_NAME; @@ -459,6 +460,127 @@ public class IndexPlannerTest { assertNull(plan); } + //------ Suggestion/spellcheck plan tests + @Test + public void nonSuggestIndex() throws Exception { + //An index which doesn't define any property to support suggestions shouldn't turn up in plan. + String indexNodeType = "nt:base"; + String queryNodeType = "nt:base"; + boolean enableSuggestionIndex = false; + boolean enableSpellcheckIndex = false; + boolean queryForSugggestion = true; + + IndexNode node = createSuggestionOrSpellcheckIndex(indexNodeType, enableSuggestionIndex, enableSpellcheckIndex); + QueryIndex.IndexPlan plan = getSuggestOrSpellcheckIndexPlan(node, queryNodeType, queryForSugggestion); + + assertNull(plan); + } + + @Test + public void nonSpellcheckIndex() throws Exception { + //An index which doesn't define any property to support spell check shouldn't turn up in plan. + String indexNodeType = "nt:base"; + String queryNodeType = "nt:base"; + boolean enableSuggestionIndex = false; + boolean enableSpellcheckIndex = false; + boolean queryForSugggestion = false; + + IndexNode node = createSuggestionOrSpellcheckIndex(indexNodeType, enableSuggestionIndex, enableSpellcheckIndex); + QueryIndex.IndexPlan plan = getSuggestOrSpellcheckIndexPlan(node, queryNodeType, queryForSugggestion); + + assertNull(plan); + } + + @Test + public void simpleSuggestIndexPlan() throws Exception { + //An index defining a property for suggestions should turn up in plan. + String indexNodeType = "nt:base"; + String queryNodeType = "nt:base"; + boolean enableSuggestionIndex = true; + boolean enableSpellcheckIndex = false; + boolean queryForSugggestion = true; + + IndexNode node = createSuggestionOrSpellcheckIndex(indexNodeType, enableSuggestionIndex, enableSpellcheckIndex); + QueryIndex.IndexPlan plan = getSuggestOrSpellcheckIndexPlan(node, queryNodeType, queryForSugggestion); + + assertNotNull(plan); + } + + @Test + public void simpleSpellcheckIndexPlan() throws Exception { + //An index defining a property for spellcheck should turn up in plan. + String indexNodeType = "nt:base"; + String queryNodeType = "nt:base"; + boolean enableSuggestionIndex = false; + boolean enableSpellcheckIndex = true; + boolean queryForSugggestion = false; + + IndexNode node = createSuggestionOrSpellcheckIndex(indexNodeType, enableSuggestionIndex, enableSpellcheckIndex); + QueryIndex.IndexPlan plan = getSuggestOrSpellcheckIndexPlan(node, queryNodeType, queryForSugggestion); + + assertNotNull(plan); + } + + @Test + public void suggestionIndexingRuleHierarchy() throws Exception { + //An index defining a property for suggestion on a base type shouldn't turn up in plan. + String indexNodeType = "nt:base"; + String queryNodeType = "nt:unstructured"; + boolean enableSuggestionIndex = true; + boolean enableSpellcheckIndex = false; + boolean queryForSugggestion = true; + + IndexNode node = createSuggestionOrSpellcheckIndex(indexNodeType, enableSuggestionIndex, enableSpellcheckIndex); + QueryIndex.IndexPlan plan = getSuggestOrSpellcheckIndexPlan(node, queryNodeType, queryForSugggestion); + + assertNull(plan); + } + + @Test + public void spellcheckIndexingRuleHierarchy() throws Exception { + //An index defining a property for spellcheck on a base type shouldn't turn up in plan. + String indexNodeType = "nt:base"; + String queryNodeType = "nt:unstructured"; + boolean enableSuggestionIndex = false; + boolean enableSpellcheckIndex = true; + boolean queryForSugggestion = false; + + IndexNode node = createSuggestionOrSpellcheckIndex(indexNodeType, enableSuggestionIndex, enableSpellcheckIndex); + QueryIndex.IndexPlan plan = getSuggestOrSpellcheckIndexPlan(node, queryNodeType, queryForSugggestion); + + assertNull(plan); + } + + private IndexNode createSuggestionOrSpellcheckIndex(String nodeType, + boolean enableSuggestion, + boolean enableSpellcheck) throws Exception { + NodeBuilder defn = newLucenePropertyIndexDefinition(builder, "test", of("foo"), "async"); + defn.setProperty(DECLARING_NODE_TYPES, nodeType); + + defn = IndexDefinition.updateDefinition(defn.getNodeState().builder()); + NodeBuilder foob = getNode(defn, "indexRules/" + nodeType + "/properties/foo"); + foob.setProperty(LuceneIndexConstants.PROP_ANALYZED, true); + if (enableSuggestion) { + foob.setProperty(LuceneIndexConstants.PROP_USE_IN_SUGGEST, true); + } if (enableSpellcheck) { + foob.setProperty(LuceneIndexConstants.PROP_USE_IN_SPELLCHECK, true); + } + + IndexDefinition indexDefinition = new IndexDefinition(root, defn.getNodeState()); + return createIndexNode(indexDefinition); + } + + private QueryIndex.IndexPlan getSuggestOrSpellcheckIndexPlan(IndexNode indexNode, String nodeType, + boolean forSugggestion) throws Exception { + FilterImpl filter = createFilter(nodeType); + filter.restrictProperty(indexNode.getDefinition().getFunctionName(), Operator.EQUAL, + PropertyValues.newString((forSugggestion?"suggest":"spellcheck") + "?term=foo")); + IndexPlanner planner = new IndexPlanner(indexNode, "/foo", filter, Collections.emptyList()); + + return planner.getPlan(); + } + //------ END - Suggestion/spellcheck plan tests + private IndexNode createIndexNode(IndexDefinition defn, long numOfDocs) throws IOException { return new IndexNode("foo", defn, createSampleDirectory(numOfDocs), null); } diff --git a/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexSuggestionTest.java b/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexSuggestionTest.java index 087710c..6c64ab1 100644 --- a/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexSuggestionTest.java +++ b/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexSuggestionTest.java @@ -201,7 +201,7 @@ public class LuceneIndexSuggestionTest { final String indexPropName = "description"; final String indexPropValue = "this is just a sample text which should get some response in suggestions"; final String suggestQueryText = "th"; - final boolean shouldSuggest = true; + final boolean shouldSuggest = false; checkSuggestions(indexNodeType, queryNodeType, indexPropName, indexPropValue, -- 2.6.2