When reviewing the fix applied for
JCR-4339 before going through an upgrade to 2.18.2, I noticed that, while it fixes the reported problem, it does so only in simple ('happy path') scenarios. Now it is broken when trying to use multiple index-rule definitions for one node type.
The logic for finding the applicable indexing rule for a specific property no longer considers (checks) if the there is a condition for that property. Instead, that check is postponed/moved to the method calling #getApplicableIndexingRule.
But this is incorrect because now the #getApplicableIndexingRule method returns the first type matching rule, regardless of the property it should be applicable for.
For example, it is perfectly feasible, and sometimes even needed, to have multiple index-rules for the same node type, like the following enhanced version of test resource indexing_config6.xml, which can be used to verify the logic now is broken:
The important points to expose the new bug is:
- the index-rule for property other is defined before the index-rule for property foo
- (for this example) the index-rule for property other doesn't have a condition
With the above, the property foo will not be indexed, regardless its value, because the first 'matching' rule returned from #getApplicableIndexingRule for a node of type nt:unstructured will be the rule for property other. But will always return false on the (now postponed/delegated) call to rule.isIndexed(propertyName: foo), because that rule doesn't has a propertyConfig for foo (only for other).
I'll attach a patch (based on trunk) to demonstrate the above failing using the new IndexingConfigurationImplTest#testMatchCondition test.
Note that the current #testMatchCondition() test itself also is broken: it actually does not test the intended condition, but tests for it to not match using assertFalse instead of assertTrue.
Which indeed is needed to pass the test because the indexing_config6.xml configuration file itself contains an invalid (incomplete) index-rule.
Instead of the current content:
it actually should be:
to pass the test with assertTrue.
I'll also fix that test method in my patch, which then however will fail, because of the above reported problem.