Index: oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexUpdate.java =================================================================== --- oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexUpdate.java (revision 1860715) +++ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexUpdate.java (working copy) @@ -277,8 +277,18 @@ continue; } - Editor editor = rootState.provider.getIndexEditor(type, definition, rootState.root, - rootState.newCallback(indexPath, shouldReindex, getEstimatedCount(definition))); + Editor editor = null; + try { + editor = rootState.provider.getIndexEditor(type, definition, rootState.root, + rootState.newCallback(indexPath, shouldReindex, getEstimatedCount(definition))); + } catch (IllegalStateException e) { + // This will be caught here in case there is any config related error in the index definition + // where multiple values are assigned to a property that is supposed to be single valued + // We log an error message here and continue - this way the bad index defintion is ignored and doesn't block the async index update + log.error("Unable to get Index Editor for index at {} . Please correct the index definition " + + "and reindex after correction. Additional Info : {}", indexPath, e.getMessage(), e); + continue; + } if (editor == null) { // if this isn't an async cycle AND definition has "async" property // (and implicitly isIncluded method allows async def in non-async cycle only for nrt/sync defs) Index: oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexUpdateTest.java =================================================================== --- oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexUpdateTest.java (revision 1860715) +++ oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexUpdateTest.java (working copy) @@ -61,6 +61,7 @@ import org.apache.jackrabbit.oak.plugins.index.reference.ReferenceEditorProvider; import org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState; import org.apache.jackrabbit.oak.plugins.memory.MemoryNodeStore; +import org.apache.jackrabbit.oak.plugins.memory.PropertyStates; import org.apache.jackrabbit.oak.plugins.memory.PropertyValues; import org.apache.jackrabbit.oak.query.NodeStateNodeTypeInfoProvider; import org.apache.jackrabbit.oak.query.QueryEngineSettings; @@ -72,6 +73,7 @@ import org.apache.jackrabbit.oak.spi.commit.Editor; import org.apache.jackrabbit.oak.spi.commit.EditorHook; import org.apache.jackrabbit.oak.spi.commit.EditorProvider; +import org.apache.jackrabbit.oak.spi.filter.PathFilter; import org.apache.jackrabbit.oak.spi.query.Filter; import org.apache.jackrabbit.oak.spi.state.NodeBuilder; import org.apache.jackrabbit.oak.spi.state.NodeState; @@ -78,7 +80,9 @@ import org.apache.jackrabbit.oak.spi.state.NodeStateUtils; import org.apache.jackrabbit.oak.spi.state.NodeStore; import org.apache.jackrabbit.util.ISO8601; +import org.hamcrest.core.IsCollectionContaining; import org.jetbrains.annotations.NotNull; +import org.junit.Assert; import org.junit.Test; import com.google.common.collect.ImmutableSet; @@ -1008,6 +1012,71 @@ customLogs.finished(); } + + /* + Given 2 index defintions - one with a config error and another ok , the content under second should get indexed + while the first with error gets ignored with an error message logged. + */ + @Test + public void testConfigErrorInIndexDefintion() throws Exception{ + LogCustomizer customLogs = LogCustomizer.forLogger(IndexUpdate.class.getName()).enable(Level.ERROR).create(); + builder.child("testRoot").setProperty("foo", "abc"); + //Create 2 index def - one with config related error and one without + + NodeBuilder index1 = createIndexDefinition(builder.child(INDEX_DEFINITIONS_NAME), + "rootIndex1", true, false, ImmutableSet.of("foo"), null); + + index1.setProperty(PropertyStates.createProperty( + PathFilter.PROP_INCLUDED_PATHS, ImmutableSet.of("/test/a/b"), Type.STRINGS)); + index1.setProperty(PropertyStates.createProperty( + PathFilter.PROP_EXCLUDED_PATHS, ImmutableSet.of("/test/a"), Type.STRINGS)); + + createIndexDefinition(builder.child(INDEX_DEFINITIONS_NAME), + "rootIndex2", true, false, ImmutableSet.of("foo2"), null); + + NodeState before = builder.getNodeState(); + + // Add some content and process it through the property index hook (for index1) + builder.child("test").child("a").setProperty("foo", "abc"); + builder.child("test").child("a").child("b").setProperty("foo", "abc"); + //Now for for index2 + builder.child("test").child("b").setProperty("foo2","abc"); + builder.child("test").child("a").child("b").setProperty("foo2", "abc"); + + NodeState after = builder.getNodeState(); + NodeState indexed; + try{ + customLogs.starting(); + String expectedLogMessage = "Unable to get Index Editor for index at /oak:index/rootIndex1 . " + + "Please correct the index definition and reindex after correction. " + + "Additional Info : No valid include provided. Includes [/test/a/b], Excludes [/test/a]"; + indexed = HOOK.processCommit(before, after, CommitInfo.EMPTY); + Assert.assertThat(customLogs.getLogs(), IsCollectionContaining.hasItems(expectedLogMessage)); + } finally { + customLogs.finished(); + } + + // Now check that the index content nodes doesn't exists and the reindex flag is still set(Since it got skipped) + NodeState ns = checkPathExists(indexed, INDEX_DEFINITIONS_NAME, + "rootIndex1"); + assertFalse(ns.getChildNode(INDEX_CONTENT_NODE_NAME).exists()); + PropertyState ps = ns.getProperty(REINDEX_PROPERTY_NAME); + assertNotNull(ps); + assertTrue(ps.getValue(Type.BOOLEAN)); + + //Now check everything is fine with index2 - indexed data node exists and reindex flag is false + NodeState ns2 = checkPathExists(indexed, INDEX_DEFINITIONS_NAME, + "rootIndex2"); + checkPathExists(ns2,INDEX_CONTENT_NODE_NAME); + PropertyState ps2 = ns2.getProperty(REINDEX_PROPERTY_NAME); + assertNotNull(ps2); + assertFalse(ps2.getValue(Type.BOOLEAN)); + + // next, lookup should work for the index def 2 which did not have any config errors + PropertyIndexLookup lookup = new PropertyIndexLookup(indexed); + assertEquals(ImmutableSet.of("test/b","test/a/b"), find(lookup, "foo2", "abc")); + + } private static void markCorrupt(NodeBuilder builder, String indexName) { builder.getChildNode(INDEX_DEFINITIONS_NAME).getChildNode(indexName) Index: oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java =================================================================== --- oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java (revision 1860715) +++ oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java (working copy) @@ -45,7 +45,9 @@ 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.commons.junit.LogCustomizer; import org.apache.jackrabbit.oak.plugins.index.IndexConstants; +import org.apache.jackrabbit.oak.plugins.index.IndexUpdate; import org.apache.jackrabbit.oak.plugins.index.IndexUpdateProvider; import org.apache.jackrabbit.oak.plugins.index.property.strategy.ContentMirrorStoreStrategy; import org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState; @@ -71,6 +73,8 @@ import org.apache.jackrabbit.oak.spi.query.Filter; import org.apache.jackrabbit.oak.spi.state.NodeBuilder; import org.apache.jackrabbit.oak.spi.state.NodeState; +import org.hamcrest.core.IsCollectionContaining; +import org.junit.Assert; import org.junit.Test; import org.slf4j.LoggerFactory; @@ -847,6 +851,7 @@ @Test public void testPathExcludeInclude() throws Exception{ + LogCustomizer customLogs = LogCustomizer.forLogger(IndexUpdate.class.getName()).enable(Level.ERROR).create(); NodeState root = INITIAL_CONTENT; // Add index definition @@ -863,9 +868,18 @@ NodeState after = builder.getNodeState(); try { + customLogs.starting(); + String expectedLogMessage = "Unable to get Index Editor for index at /oak:index/foo . " + + "Please correct the index definition and reindex after correction. " + + "Additional Info : No valid include provided. Includes [/test/a/b], Excludes [/test/a]"; HOOK.processCommit(before, after, CommitInfo.EMPTY); - assertTrue(false); - } catch (IllegalStateException expected) {} + Assert.assertThat(customLogs.getLogs(), IsCollectionContaining.hasItems(expectedLogMessage)); + } catch (IllegalStateException unexpected) { + // IllegalStateException not expected here now + assertTrue(false); + } finally { + customLogs.finished(); + } } @Test Index: oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/IndexDefinition.java =================================================================== --- oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/IndexDefinition.java (revision 1860715) +++ oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/IndexDefinition.java (working copy) @@ -342,85 +342,91 @@ } protected IndexDefinition(NodeState root, NodeState defn, IndexFormatVersion version, String uid, String indexPath) { - this.root = root; - this.version = checkNotNull(version); - this.uid = uid; - this.definition = defn; - this.indexPath = checkNotNull(indexPath); - this.indexName = indexPath; - this.indexTags = getOptionalStrings(defn, IndexConstants.INDEX_TAGS); - this.nodeTypeIndex = getOptionalValue(defn, FulltextIndexConstants.PROP_INDEX_NODE_TYPE, false); + try { + this.root = root; + this.version = checkNotNull(version); + this.uid = uid; + this.definition = defn; + this.indexPath = checkNotNull(indexPath); + this.indexName = indexPath; + this.indexTags = getOptionalStrings(defn, IndexConstants.INDEX_TAGS); + this.nodeTypeIndex = getOptionalValue(defn, FulltextIndexConstants.PROP_INDEX_NODE_TYPE, false); - this.blobSize = getOptionalValue(defn, BLOB_SIZE, DEFAULT_BLOB_SIZE); + this.blobSize = getOptionalValue(defn, BLOB_SIZE, DEFAULT_BLOB_SIZE); - this.aggregates = nodeTypeIndex ? Collections.emptyMap() : collectAggregates(defn); + this.aggregates = nodeTypeIndex ? Collections.emptyMap() : collectAggregates(defn); - NodeState rulesState = defn.getChildNode(FulltextIndexConstants.INDEX_RULES); - if (!rulesState.exists()){ - rulesState = createIndexRules(defn).getNodeState(); - } + NodeState rulesState = defn.getChildNode(FulltextIndexConstants.INDEX_RULES); + if (!rulesState.exists()){ + rulesState = createIndexRules(defn).getNodeState(); + } - this.testMode = getOptionalValue(defn, FulltextIndexConstants.TEST_MODE, false); - List definedIndexRules = newArrayList(); - this.indexRules = collectIndexRules(rulesState, definedIndexRules); - this.definedRules = ImmutableList.copyOf(definedIndexRules); + this.testMode = getOptionalValue(defn, FulltextIndexConstants.TEST_MODE, false); + List definedIndexRules = newArrayList(); + this.indexRules = collectIndexRules(rulesState, definedIndexRules); + this.definedRules = ImmutableList.copyOf(definedIndexRules); - this.fullTextEnabled = hasFulltextEnabledIndexRule(definedIndexRules); - this.evaluatePathRestrictions = getOptionalValue(defn, EVALUATE_PATH_RESTRICTION, false); + this.fullTextEnabled = hasFulltextEnabledIndexRule(definedIndexRules); + this.evaluatePathRestrictions = getOptionalValue(defn, EVALUATE_PATH_RESTRICTION, false); - String functionName = getOptionalValue(defn, FulltextIndexConstants.FUNC_NAME, null); - if (fullTextEnabled && functionName == null){ - functionName = getDefaultFunctionName(); - } - this.funcName = functionName != null ? "native*" + functionName : null; + String functionName = getOptionalValue(defn, FulltextIndexConstants.FUNC_NAME, null); + if (fullTextEnabled && functionName == null) { + functionName = getDefaultFunctionName(); + } + this.funcName = functionName != null ? "native*" + functionName : null; - if (defn.hasProperty(ENTRY_COUNT_PROPERTY_NAME)) { - this.entryCountDefined = true; - this.entryCount = defn.getProperty(ENTRY_COUNT_PROPERTY_NAME).getValue(Type.LONG); - } else { - this.entryCountDefined = false; - this.entryCount = DEFAULT_ENTRY_COUNT; - } + if (defn.hasProperty(ENTRY_COUNT_PROPERTY_NAME)) { + this.entryCountDefined = true; + this.entryCount = getOptionalValue(defn, ENTRY_COUNT_PROPERTY_NAME, DEFAULT_ENTRY_COUNT); + } else { + this.entryCountDefined = false; + this.entryCount = DEFAULT_ENTRY_COUNT; + } - this.maxFieldLength = getOptionalValue(defn, FulltextIndexConstants.MAX_FIELD_LENGTH, DEFAULT_MAX_FIELD_LENGTH); - this.costPerEntry = getOptionalValue(defn, FulltextIndexConstants.COST_PER_ENTRY, getDefaultCostPerEntry(version)); - this.costPerExecution = getOptionalValue(defn, FulltextIndexConstants.COST_PER_EXECUTION, 1.0); - this.hasCustomTikaConfig = getTikaConfigNode().exists(); - this.customTikaMimeTypeMappings = buildMimeTypeMap(definition.getChildNode(TIKA).getChildNode(TIKA_MIME_TYPES)); - this.maxExtractLength = determineMaxExtractLength(); - this.suggesterUpdateFrequencyMinutes = evaluateSuggesterUpdateFrequencyMinutes(defn, - DEFAULT_SUGGESTER_UPDATE_FREQUENCY_MINUTES); - this.scorerProviderName = getOptionalValue(defn, FulltextIndexConstants.PROP_SCORER_PROVIDER, null); - this.reindexCount = getOptionalValue(defn, REINDEX_COUNT, 0); - this.pathFilter = PathFilter.from(new ReadOnlyBuilder(defn)); - this.queryPaths = getOptionalStrings(defn, IndexConstants.QUERY_PATHS); - this.suggestAnalyzed = evaluateSuggestAnalyzed(defn, false); + this.maxFieldLength = getOptionalValue(defn, FulltextIndexConstants.MAX_FIELD_LENGTH, DEFAULT_MAX_FIELD_LENGTH); + this.costPerEntry = getOptionalValue(defn, FulltextIndexConstants.COST_PER_ENTRY, getDefaultCostPerEntry(version)); + this.costPerExecution = getOptionalValue(defn, FulltextIndexConstants.COST_PER_EXECUTION, 1.0); + this.hasCustomTikaConfig = getTikaConfigNode().exists(); + this.customTikaMimeTypeMappings = buildMimeTypeMap(definition.getChildNode(TIKA).getChildNode(TIKA_MIME_TYPES)); + this.maxExtractLength = determineMaxExtractLength(); + this.suggesterUpdateFrequencyMinutes = evaluateSuggesterUpdateFrequencyMinutes(defn, + DEFAULT_SUGGESTER_UPDATE_FREQUENCY_MINUTES); + this.scorerProviderName = getOptionalValue(defn, FulltextIndexConstants.PROP_SCORER_PROVIDER, null); + this.reindexCount = getOptionalValue(defn, REINDEX_COUNT, 0); + this.pathFilter = PathFilter.from(new ReadOnlyBuilder(defn)); + this.queryPaths = getOptionalStrings(defn, IndexConstants.QUERY_PATHS); + this.suggestAnalyzed = evaluateSuggestAnalyzed(defn, false); - { - PropertyState randomPS = defn.getProperty(PROP_RANDOM_SEED); - if (randomPS != null && randomPS.getType() == Type.LONG) { - randomSeed = randomPS.getValue(Type.LONG); + { + PropertyState randomPS = defn.getProperty(PROP_RANDOM_SEED); + if (randomPS != null && randomPS.getType() == Type.LONG) { + randomSeed = randomPS.getValue(Type.LONG); + } else { + // create a random number + randomSeed = UUID.randomUUID().getMostSignificantBits(); + } + } + if (defn.hasChildNode(FACETS)) { + NodeState facetsConfig = defn.getChildNode(FACETS); + this.secureFacets = SecureFacetConfiguration.getInstance(randomSeed, facetsConfig); + this.numberOfTopFacets = getOptionalValue(facetsConfig, PROP_FACETS_TOP_CHILDREN, DEFAULT_FACET_COUNT); } else { - // create a random number - randomSeed = UUID.randomUUID().getMostSignificantBits(); + this.secureFacets = SecureFacetConfiguration.getInstance(randomSeed, null); + this.numberOfTopFacets = DEFAULT_FACET_COUNT; } + + this.suggestEnabled = evaluateSuggestionEnabled(); + this.spellcheckEnabled = evaluateSpellcheckEnabled(); + this.nrtIndexMode = supportsNRTIndexing(defn); + this.syncIndexMode = supportsSyncIndexing(defn); + this.syncPropertyIndexes = definedRules.stream().anyMatch(ir -> !ir.syncProps.isEmpty()); + this.useIfExists = getOptionalValue(defn, IndexConstants.USE_IF_EXISTS, null); + this.deprecated = getOptionalValue(defn, IndexConstants.INDEX_DEPRECATED, false); + } catch (IllegalStateException e) { + log.error("Config error for index definition at {} . Please correct the index definition " + + "and reindex after correction. Additional Info : {}", indexPath, e.getMessage(), e); + throw new IllegalStateException(e); } - if (defn.hasChildNode(FACETS)) { - NodeState facetsConfig = defn.getChildNode(FACETS); - this.secureFacets = SecureFacetConfiguration.getInstance(randomSeed, facetsConfig); - this.numberOfTopFacets = getOptionalValue(facetsConfig, PROP_FACETS_TOP_CHILDREN, DEFAULT_FACET_COUNT); - } else { - this.secureFacets = SecureFacetConfiguration.getInstance(randomSeed, null); - this.numberOfTopFacets = DEFAULT_FACET_COUNT; - } - - this.suggestEnabled = evaluateSuggestionEnabled(); - this.spellcheckEnabled = evaluateSpellcheckEnabled(); - this.nrtIndexMode = supportsNRTIndexing(defn); - this.syncIndexMode = supportsSyncIndexing(defn); - this.syncPropertyIndexes = definedRules.stream().anyMatch(ir -> !ir.syncProps.isEmpty()); - this.useIfExists = getOptionalValue(defn, IndexConstants.USE_IF_EXISTS, null); - this.deprecated = getOptionalValue(defn, IndexConstants.INDEX_DEPRECATED, false); } public NodeState getDefinitionNodeState() { Index: oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/util/ConfigUtil.java =================================================================== --- oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/util/ConfigUtil.java (revision 1860715) +++ oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/util/ConfigUtil.java (working copy) @@ -36,34 +36,69 @@ */ public class ConfigUtil { - public static boolean getOptionalValue(NodeState definition, String propName, boolean defaultVal){ - PropertyState ps = definition.getProperty(propName); - return ps == null ? defaultVal : ps.getValue(Type.BOOLEAN); + private static final String ILLEGAL_STATE_EXCEPTION_ERROR_MESSAGE = "Multiple values provided for property %s in index definition . Single value was expected"; + + public static boolean getOptionalValue(NodeState definition, String propName, boolean defaultVal) { + try { + PropertyState ps = definition.getProperty(propName); + return ps == null ? defaultVal : ps.getValue(Type.BOOLEAN); + } catch (IllegalStateException e) { + throw new IllegalStateException(String.format(ILLEGAL_STATE_EXCEPTION_ERROR_MESSAGE, propName), e); + } } - public static int getOptionalValue(NodeState definition, String propName, int defaultVal){ - PropertyState ps = definition.getProperty(propName); - return ps == null ? defaultVal : Ints.checkedCast(ps.getValue(Type.LONG)); + public static int getOptionalValue(NodeState definition, String propName, int defaultVal) { + try { + PropertyState ps = definition.getProperty(propName); + return ps == null ? defaultVal : Ints.checkedCast(ps.getValue(Type.LONG)); + } catch (IllegalStateException e) { + throw new IllegalStateException(String.format(ILLEGAL_STATE_EXCEPTION_ERROR_MESSAGE, propName), e); + } } - public static String getOptionalValue(NodeState definition, String propName, String defaultVal){ - PropertyState ps = definition.getProperty(propName); - return ps == null ? defaultVal : ps.getValue(Type.STRING); + public static String getOptionalValue(NodeState definition, String propName, String defaultVal) { + try { + PropertyState ps = definition.getProperty(propName); + return ps == null ? defaultVal : ps.getValue(Type.STRING); + } catch (IllegalStateException e) { + throw new IllegalStateException(String.format(ILLEGAL_STATE_EXCEPTION_ERROR_MESSAGE, propName), e); + } } - public static float getOptionalValue(NodeState definition, String propName, float defaultVal){ - PropertyState ps = definition.getProperty(propName); - return ps == null ? defaultVal : ps.getValue(Type.DOUBLE).floatValue(); + public static float getOptionalValue(NodeState definition, String propName, float defaultVal) { + try { + PropertyState ps = definition.getProperty(propName); + return ps == null ? defaultVal : ps.getValue(Type.DOUBLE).floatValue(); + } catch (IllegalStateException e) { + throw new IllegalStateException(String.format(ILLEGAL_STATE_EXCEPTION_ERROR_MESSAGE, propName), e); + } } - public static double getOptionalValue(NodeState definition, String propName, double defaultVal){ - PropertyState ps = definition.getProperty(propName); - return ps == null ? defaultVal : ps.getValue(Type.DOUBLE); + public static double getOptionalValue(NodeState definition, String propName, double defaultVal) { + try { + PropertyState ps = definition.getProperty(propName); + return ps == null ? defaultVal : ps.getValue(Type.DOUBLE); + } catch (IllegalStateException e) { + throw new IllegalStateException(String.format(ILLEGAL_STATE_EXCEPTION_ERROR_MESSAGE, propName), e); + } } + + public static long getOptionalValue(NodeState definition, String propName, long defaultVal) { + try { + PropertyState ps = definition.getProperty(propName); + return ps == null ? defaultVal : ps.getValue(Type.LONG); + } catch (IllegalStateException e) { + throw new IllegalStateException(String.format(ILLEGAL_STATE_EXCEPTION_ERROR_MESSAGE, propName), e); + } + } public static String getPrimaryTypeName(NodeState nodeState) { - PropertyState ps = nodeState.getProperty(JcrConstants.JCR_PRIMARYTYPE); - return (ps == null) ? JcrConstants.NT_BASE : ps.getValue(Type.NAME); + try { + PropertyState ps = nodeState.getProperty(JcrConstants.JCR_PRIMARYTYPE); + return (ps == null) ? JcrConstants.NT_BASE : ps.getValue(Type.NAME); + } catch (IllegalStateException e) { + throw new IllegalStateException(String.format(ILLEGAL_STATE_EXCEPTION_ERROR_MESSAGE, JcrConstants.JCR_PRIMARYTYPE), e); + } } public static Iterable getMixinNames(NodeState nodeState) {