diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexUpdate.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexUpdate.java index 0ab5829..770f6da 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexUpdate.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexUpdate.java @@ -276,9 +276,19 @@ public class IndexUpdate implements Editor, PathSource { rootState.corruptIndexHandler.skippingCorruptIndex(rootState.async, indexPath, ISO8601.parse(corruptSince)); continue; } - - Editor editor = rootState.provider.getIndexEditor(type, definition, rootState.root, + 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) diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexUpdateTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexUpdateTest.java index 90be3b8..135624c 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexUpdateTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexUpdateTest.java @@ -17,6 +17,7 @@ package org.apache.jackrabbit.oak.plugins.index; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.collect.ImmutableSet.of; import static java.util.Arrays.asList; import static org.apache.jackrabbit.JcrConstants.NT_BASE; import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.ASYNC_PROPERTY_NAME; @@ -32,6 +33,9 @@ import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.TYPE_PROPER import static org.apache.jackrabbit.oak.plugins.index.IndexUpdateCallback.NOOP; import static org.apache.jackrabbit.oak.plugins.index.IndexUtils.createIndexDefinition; import static org.apache.jackrabbit.oak.InitialContentHelper.INITIAL_CONTENT; +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.instanceOf; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -78,7 +82,9 @@ import org.apache.jackrabbit.oak.spi.state.NodeState; 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 +1014,68 @@ public class IndexUpdateTest { 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(createProperty(PROP_INCLUDED_PATHS, of("/test/a/b"), Type.STRINGS)); + index1.setProperty(createProperty(PROP_EXCLUDED_PATHS, 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) diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java index e1cb839..5c4621c 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java @@ -45,7 +45,9 @@ import org.apache.jackrabbit.oak.api.PropertyState; 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.mount.Mounts; 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 @@ public class PropertyIndexTest { @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,19 @@ public class PropertyIndexTest { 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); + Assert.assertThat(customLogs.getLogs(), IsCollectionContaining.hasItems(expectedLogMessage)); + } catch (IllegalStateException unexpected) { + // IllegalStateException not expected here now assertTrue(false); - } catch (IllegalStateException expected) {} + } finally { + customLogs.finished(); + } } @Test diff --git a/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/IndexDefinition.java b/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/IndexDefinition.java index b89d4ae..c0ce6bb 100644 --- a/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/IndexDefinition.java +++ b/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/IndexDefinition.java @@ -342,6 +342,8 @@ public class IndexDefinition implements Aggregate.AggregateMapper { } protected IndexDefinition(NodeState root, NodeState defn, IndexFormatVersion version, String uid, String indexPath) { + + try { this.root = root; this.version = checkNotNull(version); this.uid = uid; @@ -376,7 +378,7 @@ public class IndexDefinition implements Aggregate.AggregateMapper { if (defn.hasProperty(ENTRY_COUNT_PROPERTY_NAME)) { this.entryCountDefined = true; - this.entryCount = defn.getProperty(ENTRY_COUNT_PROPERTY_NAME).getValue(Type.LONG); + this.entryCount = getOptionalValue(defn, ENTRY_COUNT_PROPERTY_NAME, DEFAULT_ENTRY_COUNT); } else { this.entryCountDefined = false; this.entryCount = DEFAULT_ENTRY_COUNT; @@ -421,6 +423,12 @@ public class IndexDefinition implements Aggregate.AggregateMapper { 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); + } } public NodeState getDefinitionNodeState() { diff --git a/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/util/ConfigUtil.java b/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/util/ConfigUtil.java index 23af20e..20eaebb 100644 --- a/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/util/ConfigUtil.java +++ b/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/util/ConfigUtil.java @@ -36,34 +36,71 @@ import static com.google.common.base.Preconditions.checkArgument; */ public class ConfigUtil { + 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) { + 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) { + 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) { + 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) { + 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) { + 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) {