Proposed fix for #3624 NPE at RangeQueryRewriter.rewriteLocationStep

[dizzzz]

Proposed fix for #3624 NPE at org.exist.xquery.modules.range.RangeQueryRewriter.rewriteLocationStep
Handle all operators

Help needed for tests

[joewiz]

I can confirm that this fixes the issue I reported in #3624. Thank you, @dizzzz!

[adamretter]

@dizzzz Interesting stuff... I have some questions ;-)

1. Previously this was limited to fn:matches but now it is only restricted to a Function that implements Lookup. Joe mentioned he did not see the problem in 5.2.0 - but this code would not have changed between 5.3.0 and 5.2.0, so I wonder where your insight for this fix came from?

2. What was the expression that was causing the issue?

3. This code can still cause an NPE:

Lookup func = Lookup.create(function.getContext(), operator, path);
func.setArguments(eqArgs);

The Lookup.Create function can return null. So the value of func should probably be checked/logged before it is used or not.

[dizzzz]

eq came in as option instead of matches, then I realized that only one item (eq) was handled. In other cases than matches NULL was returned, yielding in the NPE that @joewiz reported;

Instead of preventing the eq NPE, I thought it would make sense just to handle all enum options 👍 That should be OK as all options should accept 2 parameters,

Hmmm did I miss a potential NPE? I thought I covered it all....

[adamretter]

Hmmm, shouldn't eq be handled just above in the if (expression instanceof GeneralComparison) { branch?

[dizzzz]

Hmmmm you are right on this I guess. good spot. hmmmm

[dizzzz]

What shall I do? close PR?

[wolfgangmm]

I debugged the original issue myself and came to the conclusion that Dannes' proposed fix is basically correct. What we have here is an expression which has already been partially rewritten (the GeneralComparison was replaced by range:eq). I'm not sure where this partial optimization was applied - probably an earlier traversal of the expression (we would need a much more reduced test case to know), but it is certainly a possible case which should not lead to an exception.

As Dannes pointed out, RangeQueryRewriter.rewrite only handled matches (stupid me!). It should handle all functions implementing Lookup.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[dizzzz]

@wolfgangmm @adamretter ready to go

[joewiz]

@dizzzz I can confirm that the updated PR still fixes the issue I reported in #3624. Thank you!

[joewiz]

@dizzzz Thank you so much for this fix! And thanks to @adamretter and @wolfgangmm for your reviews.

[adamretter]

I just dug into this further to try and understand why this fix works. I reduced Dannes's changes to the absolute minimum to pass Joe's reported issue #3624, and was left with just this patch:

diff --git a/extensions/indexes/range/src/main/java/org/exist/xquery/modules/range/RangeQueryRewriter.java b/extensions/indexes/range/src/main/java/org/exist/xquery/modules/range/RangeQueryRewriter.java
index 04f3db1f89..f7d47facce 100644
--- a/extensions/indexes/range/src/main/java/org/exist/xquery/modules/range/RangeQueryRewriter.java
+++ b/extensions/indexes/range/src/main/java/org/exist/xquery/modules/range/RangeQueryRewriter.java
@@ -139,13 +139,12 @@ public class RangeQueryRewriter extends QueryRewriter {
             InternalFunctionCall fcall = (InternalFunctionCall) expression;
             Function function = fcall.getFunction();
             if (function instanceof Lookup) {
-                if (function.isCalledAs("matches")) {
                     eqArgs.add(function.getArgument(0));
                     eqArgs.add(function.getArgument(1));
-                    Lookup func = Lookup.create(function.getContext(), RangeIndex.Operator.MATCH, path);
+                    final RangeIndex.Operator operator = RangeIndex.Operator.getByName(function.getName().getLocalPart());
+                    Lookup func = Lookup.create(function.getContext(), operator, path);
                     func.setArguments(eqArgs);
                     return func;
-                }
             }
         }
         return null;

As @wolfgangmm hinted at previously, the statement if (function.isCalledAs("matches")) { meant that RangeQueryRewriter#rewrite(Expression, NodePath) would return null, if it was presented with an instance of the Lookup class (which is an expression that is optimized for the Range Index) that was not range:matches#2.

The more I think about this, the less sense the current code makes. If we already have an instance of Lookup then it is already optimized and so it doesn't need to be passed to RangeQueryRewriter#rewrite to be rewritten as a Lookup function, because it is already a Lookup function!

With that in mind I still think it likely that the current fix is addressing a symptom rather than the root cause of the problem, and that eXist-db is doing more work than necessary by replacing one optimized Lookup expression with exactly the same optimized Lookup expression. I will keep digging...

[adamretter]

If we already have an instance of Lookup then it is already optimized and so it doesn't need to be passed to RangeQueryRewriter#rewrite to be rewritten as a Lookup function, because it is already a Lookup function!

We (Evolved Binary) have provided an updated PR that takes a more efficient path in - #4882