You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-dev@jackrabbit.apache.org by "thomasmueller (via GitHub)" <gi...@apache.org> on 2026/06/02 12:21:06 UTC
[PR] OAK-12125: gracefully handle property index missing propertyNames (#2924) [jackrabbit-oak]
thomasmueller opened a new pull request, #2926:
URL: https://github.com/apache/jackrabbit-oak/pull/2926
A missing 'propertyNames' caused an NPE that escaped IndexUpdate's IllegalStateException catch and broke the async indexing cycle. Add an opt-in fallback gated by FT_GRANITE-63829 (registered in @Activate, driven by LaunchDarkly in production): when on, the editor logs one WARN per misconfigured index path and uses a sentinel property name so the cycle continues. Default off preserves the pre-PR behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-12125: gracefully handle property index missing propertyNames (#2924) [jackrabbit-oak]
Posted by "tmvlad (via GitHub)" <gi...@apache.org>.
tmvlad commented on code in PR #2926:
URL: https://github.com/apache/jackrabbit-oak/pull/2926#discussion_r3346865136
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexEditorProvider.java:
##########
@@ -42,14 +51,33 @@ public class PropertyIndexEditorProvider implements IndexEditorProvider {
public static final String TYPE = "property";
+ public static final String FT_GRANITE_63829 = "FT_GRANITE-63829";
+
@Reference
private MountInfoProvider mountInfoProvider = Mounts.defaultMountInfoProvider();
+ @Nullable
+ private Feature feature;
+
+ @Activate
+ private void activate(BundleContext bundleContext) {
+ Whiteboard whiteboard = new OsgiWhiteboard(bundleContext);
+ this.feature = newFeature(FT_GRANITE_63829, whiteboard);
Review Comment:
I've seen a similar naming scheme used in other parts of the codebase and went along with it. Thanks for the catch, I'll revise it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-12125: gracefully handle property index missing propertyNames (#2924) [jackrabbit-oak]
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on code in PR #2926:
URL: https://github.com/apache/jackrabbit-oak/pull/2926#discussion_r3346660471
##########
oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java:
##########
@@ -1187,4 +1194,146 @@ private String getCommitPath(String changeNodeName) {
return PathUtils.concat(path, changeNodeName);
}
}
+
+ /**
+ * @see <a href="https://issues.apache.org/jira/browse/OAK-12125">OAK-12125</a>
+ *
+ * With no Feature wired into the provider (the default), a property index
+ * definition missing the required 'propertyNames' must NPE out of the editor
+ * constructor. IndexUpdate catches IllegalStateException but NOT NullPointerException,
+ * so the NPE propagates and breaks the indexing cycle. This pins the
+ * "default OFF = pre-PR behavior" contract.
+ */
+ @Test
+ public void missingPropertyNames_toggleOff_breaksIndexing() throws Exception {
+ NodeState root = INITIAL_CONTENT;
+ NodeBuilder builder = root.builder();
+
+ NodeBuilder invalidIndex = builder.child(INDEX_DEFINITIONS_NAME).child("invalidIndex_off");
+ invalidIndex.setProperty(JCR_PRIMARYTYPE,
+ IndexConstants.INDEX_DEFINITIONS_NODE_TYPE, Type.NAME);
+ invalidIndex.setProperty(IndexConstants.TYPE_PROPERTY_NAME,
+ PropertyIndexEditorProvider.TYPE);
+
+ NodeState before = builder.getNodeState();
+ builder.child("content").setProperty("foo", "bar");
+ NodeState after = builder.getNodeState();
+
+ try {
+ HOOK.processCommit(before, after, CommitInfo.EMPTY);
+ fail("Expected NullPointerException to propagate from the indexing cycle");
+ } catch (NullPointerException expected) {
+ // success path: NPE bubbled directly
+ } catch (Exception other) {
+ // Some Oak commit-hook layers wrap exceptions; accept NPE anywhere in cause chain
+ Throwable cause = other;
+ boolean foundNpe = false;
+ while (cause != null) {
+ if (cause instanceof NullPointerException) {
+ foundNpe = true;
+ break;
+ }
+ cause = cause.getCause();
+ }
+ assertTrue("Expected NullPointerException in the cause chain, got: " + other, foundNpe);
+ }
+ }
+
+ /**
+ * @see <a href="https://issues.apache.org/jira/browse/OAK-12125">OAK-12125</a>
+ *
+ * With an enabled Feature wired into the provider, a missing 'propertyNames'
+ * must NOT throw. Instead the editor falls back to a sentinel, logs one WARN
+ * naming the misconfigured index path, and lets the indexing cycle complete
+ * so the valid sibling index still works.
+ */
+ @Test
+ public void missingPropertyNames_toggleOn_logsWarnAndContinues() throws Exception {
+ EditorHook hook = hookWithFeatureEnabled(true);
+ LogCustomizer customLogs = LogCustomizer
+ .forLogger(PropertyIndexEditor.class.getName()).enable(Level.WARN).create();
+ try {
+ customLogs.starting();
+
+ NodeState root = INITIAL_CONTENT;
+ NodeBuilder builder = root.builder();
+
+ createIndexDefinition(builder.child(INDEX_DEFINITIONS_NAME), "foo",
+ true, false, Set.of("foo"), null);
+
+ NodeBuilder invalidIndex = builder.child(INDEX_DEFINITIONS_NAME).child("invalidIndex_on");
+ invalidIndex.setProperty(JCR_PRIMARYTYPE,
+ IndexConstants.INDEX_DEFINITIONS_NODE_TYPE, Type.NAME);
+ invalidIndex.setProperty(IndexConstants.TYPE_PROPERTY_NAME,
+ PropertyIndexEditorProvider.TYPE);
+
+ NodeState before = builder.getNodeState();
+ builder.child("content").setProperty("foo", "bar");
+ NodeState after = builder.getNodeState();
+
+ NodeState indexed = hook.processCommit(before, after, CommitInfo.EMPTY);
+
+ assertTrue("Expected a WARN message naming the invalid index path",
+ customLogs.getLogs().stream().anyMatch(
+ msg -> msg.contains("invalidIndex_on") && msg.contains(IndexConstants.PROPERTY_NAMES)));
+
+ FilterImpl f = createFilter(indexed, NT_BASE);
+ PropertyIndexLookup lookup = new PropertyIndexLookup(indexed);
+ assertEquals(Set.of("content"), find(lookup, "foo", "bar", f));
+ } finally {
+ customLogs.finished();
+ }
+ }
+
+ /**
+ * @see <a href="https://issues.apache.org/jira/browse/OAK-12125">OAK-12125</a>
+ *
+ * With the Feature enabled and the same invalid index hit twice, exactly one
+ * WARN must be emitted for that index path. Uses a unique index node name
+ * ("invalidIndex_dedup") so the JVM-wide dedup set doesn't get poisoned by
+ * earlier tests in the suite.
+ */
+ @Test
+ public void missingPropertyNames_toggleOn_warnLoggedOncePerIndexPath() throws Exception {
+ EditorHook hook = hookWithFeatureEnabled(true);
+ LogCustomizer customLogs = LogCustomizer
+ .forLogger(PropertyIndexEditor.class.getName()).enable(Level.WARN).create();
+ try {
+ customLogs.starting();
+
+ NodeState root = INITIAL_CONTENT;
+ NodeBuilder builder = root.builder();
+
+ NodeBuilder invalidIndex = builder.child(INDEX_DEFINITIONS_NAME).child("invalidIndex_dedup");
+ invalidIndex.setProperty(JCR_PRIMARYTYPE,
+ IndexConstants.INDEX_DEFINITIONS_NODE_TYPE, Type.NAME);
+ invalidIndex.setProperty(IndexConstants.TYPE_PROPERTY_NAME,
+ PropertyIndexEditorProvider.TYPE);
+
+ NodeState before = builder.getNodeState();
+ builder.child("content_a").setProperty("foo", "bar");
+ NodeState after1 = builder.getNodeState();
+ hook.processCommit(before, after1, CommitInfo.EMPTY);
+
+ builder.child("content_b").setProperty("foo", "baz");
+ NodeState after2 = builder.getNodeState();
+ hook.processCommit(after1, after2, CommitInfo.EMPTY);
+
+ long warnsForThisIndex = customLogs.getLogs().stream()
+ .filter(msg -> msg.contains("invalidIndex_dedup"))
+ .count();
+ assertEquals("Expected exactly one WARN for invalidIndex_dedup across two cycles, got: "
+ + customLogs.getLogs(), 1, warnsForThisIndex);
+ } finally {
+ customLogs.finished();
+ }
+ }
+
+ private EditorHook hookWithFeatureEnabled(boolean enabled) {
+ PropertyIndexEditorProvider provider = new PropertyIndexEditorProvider();
Review Comment:
Apparently this works but why is the name of the toggle not needed here? (just asking)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-12125: gracefully handle property index missing propertyNames (#2924) [jackrabbit-oak]
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on code in PR #2926:
URL: https://github.com/apache/jackrabbit-oak/pull/2926#discussion_r3346602220
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexEditorProvider.java:
##########
@@ -42,14 +51,33 @@ public class PropertyIndexEditorProvider implements IndexEditorProvider {
public static final String TYPE = "property";
+ public static final String FT_GRANITE_63829 = "FT_GRANITE-63829";
+
@Reference
private MountInfoProvider mountInfoProvider = Mounts.defaultMountInfoProvider();
+ @Nullable
+ private Feature feature;
+
+ @Activate
+ private void activate(BundleContext bundleContext) {
+ Whiteboard whiteboard = new OsgiWhiteboard(bundleContext);
+ this.feature = newFeature(FT_GRANITE_63829, whiteboard);
Review Comment:
Toggle name?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-12125: gracefully handle property index missing propertyNames (#2924) [jackrabbit-oak]
Posted by "tmvlad (via GitHub)" <gi...@apache.org>.
tmvlad commented on code in PR #2926:
URL: https://github.com/apache/jackrabbit-oak/pull/2926#discussion_r3346860365
##########
oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java:
##########
@@ -1187,4 +1194,146 @@ private String getCommitPath(String changeNodeName) {
return PathUtils.concat(path, changeNodeName);
}
}
+
+ /**
+ * @see <a href="https://issues.apache.org/jira/browse/OAK-12125">OAK-12125</a>
+ *
+ * With no Feature wired into the provider (the default), a property index
+ * definition missing the required 'propertyNames' must NPE out of the editor
+ * constructor. IndexUpdate catches IllegalStateException but NOT NullPointerException,
+ * so the NPE propagates and breaks the indexing cycle. This pins the
+ * "default OFF = pre-PR behavior" contract.
+ */
+ @Test
+ public void missingPropertyNames_toggleOff_breaksIndexing() throws Exception {
+ NodeState root = INITIAL_CONTENT;
+ NodeBuilder builder = root.builder();
+
+ NodeBuilder invalidIndex = builder.child(INDEX_DEFINITIONS_NAME).child("invalidIndex_off");
+ invalidIndex.setProperty(JCR_PRIMARYTYPE,
+ IndexConstants.INDEX_DEFINITIONS_NODE_TYPE, Type.NAME);
+ invalidIndex.setProperty(IndexConstants.TYPE_PROPERTY_NAME,
+ PropertyIndexEditorProvider.TYPE);
+
+ NodeState before = builder.getNodeState();
+ builder.child("content").setProperty("foo", "bar");
+ NodeState after = builder.getNodeState();
+
+ try {
+ HOOK.processCommit(before, after, CommitInfo.EMPTY);
+ fail("Expected NullPointerException to propagate from the indexing cycle");
+ } catch (NullPointerException expected) {
+ // success path: NPE bubbled directly
+ } catch (Exception other) {
+ // Some Oak commit-hook layers wrap exceptions; accept NPE anywhere in cause chain
+ Throwable cause = other;
+ boolean foundNpe = false;
+ while (cause != null) {
+ if (cause instanceof NullPointerException) {
+ foundNpe = true;
+ break;
+ }
+ cause = cause.getCause();
+ }
+ assertTrue("Expected NullPointerException in the cause chain, got: " + other, foundNpe);
+ }
+ }
+
+ /**
+ * @see <a href="https://issues.apache.org/jira/browse/OAK-12125">OAK-12125</a>
+ *
+ * With an enabled Feature wired into the provider, a missing 'propertyNames'
+ * must NOT throw. Instead the editor falls back to a sentinel, logs one WARN
+ * naming the misconfigured index path, and lets the indexing cycle complete
+ * so the valid sibling index still works.
+ */
+ @Test
+ public void missingPropertyNames_toggleOn_logsWarnAndContinues() throws Exception {
+ EditorHook hook = hookWithFeatureEnabled(true);
+ LogCustomizer customLogs = LogCustomizer
+ .forLogger(PropertyIndexEditor.class.getName()).enable(Level.WARN).create();
+ try {
+ customLogs.starting();
+
+ NodeState root = INITIAL_CONTENT;
+ NodeBuilder builder = root.builder();
+
+ createIndexDefinition(builder.child(INDEX_DEFINITIONS_NAME), "foo",
+ true, false, Set.of("foo"), null);
+
+ NodeBuilder invalidIndex = builder.child(INDEX_DEFINITIONS_NAME).child("invalidIndex_on");
+ invalidIndex.setProperty(JCR_PRIMARYTYPE,
+ IndexConstants.INDEX_DEFINITIONS_NODE_TYPE, Type.NAME);
+ invalidIndex.setProperty(IndexConstants.TYPE_PROPERTY_NAME,
+ PropertyIndexEditorProvider.TYPE);
+
+ NodeState before = builder.getNodeState();
+ builder.child("content").setProperty("foo", "bar");
+ NodeState after = builder.getNodeState();
+
+ NodeState indexed = hook.processCommit(before, after, CommitInfo.EMPTY);
+
+ assertTrue("Expected a WARN message naming the invalid index path",
+ customLogs.getLogs().stream().anyMatch(
+ msg -> msg.contains("invalidIndex_on") && msg.contains(IndexConstants.PROPERTY_NAMES)));
+
+ FilterImpl f = createFilter(indexed, NT_BASE);
+ PropertyIndexLookup lookup = new PropertyIndexLookup(indexed);
+ assertEquals(Set.of("content"), find(lookup, "foo", "bar", f));
+ } finally {
+ customLogs.finished();
+ }
+ }
+
+ /**
+ * @see <a href="https://issues.apache.org/jira/browse/OAK-12125">OAK-12125</a>
+ *
+ * With the Feature enabled and the same invalid index hit twice, exactly one
+ * WARN must be emitted for that index path. Uses a unique index node name
+ * ("invalidIndex_dedup") so the JVM-wide dedup set doesn't get poisoned by
+ * earlier tests in the suite.
+ */
+ @Test
+ public void missingPropertyNames_toggleOn_warnLoggedOncePerIndexPath() throws Exception {
+ EditorHook hook = hookWithFeatureEnabled(true);
+ LogCustomizer customLogs = LogCustomizer
+ .forLogger(PropertyIndexEditor.class.getName()).enable(Level.WARN).create();
+ try {
+ customLogs.starting();
+
+ NodeState root = INITIAL_CONTENT;
+ NodeBuilder builder = root.builder();
+
+ NodeBuilder invalidIndex = builder.child(INDEX_DEFINITIONS_NAME).child("invalidIndex_dedup");
+ invalidIndex.setProperty(JCR_PRIMARYTYPE,
+ IndexConstants.INDEX_DEFINITIONS_NODE_TYPE, Type.NAME);
+ invalidIndex.setProperty(IndexConstants.TYPE_PROPERTY_NAME,
+ PropertyIndexEditorProvider.TYPE);
+
+ NodeState before = builder.getNodeState();
+ builder.child("content_a").setProperty("foo", "bar");
+ NodeState after1 = builder.getNodeState();
+ hook.processCommit(before, after1, CommitInfo.EMPTY);
+
+ builder.child("content_b").setProperty("foo", "baz");
+ NodeState after2 = builder.getNodeState();
+ hook.processCommit(after1, after2, CommitInfo.EMPTY);
+
+ long warnsForThisIndex = customLogs.getLogs().stream()
+ .filter(msg -> msg.contains("invalidIndex_dedup"))
+ .count();
+ assertEquals("Expected exactly one WARN for invalidIndex_dedup across two cycles, got: "
+ + customLogs.getLogs(), 1, warnsForThisIndex);
+ } finally {
+ customLogs.finished();
+ }
+ }
+
+ private EditorHook hookWithFeatureEnabled(boolean enabled) {
+ PropertyIndexEditorProvider provider = new PropertyIndexEditorProvider();
Review Comment:
The provider class doesn't get the toggle from the constructor (only the editor does). It has the toggle name hardcoded in the `activate` method, where only the osgiContext is needed, for the creating the whiteboard with the shared state.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-12125: gracefully handle property index missing propertyNames (#2924) [jackrabbit-oak]
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller merged PR #2926:
URL: https://github.com/apache/jackrabbit-oak/pull/2926
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-12125: gracefully handle property index missing propertyNames (#2924) [jackrabbit-oak]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #2926:
URL: https://github.com/apache/jackrabbit-oak/pull/2926#issuecomment-4660408110
## [](https://sonarcloud.io/dashboard?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=2926) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=2926&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=2926&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=2926&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [91.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=2926&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=2926&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=2926)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org