[JXPATH-204] Eagerly evaluate UnionContext before returning its value - #289
Open
akashchamp wants to merge 1 commit into
Open
akashchamp wants to merge 1 commit into
akashchamp wants to merge 1 commit into
Conversation
UnionContext only populates its NodeSet lazily, inside setPosition(). A caller that reads getValue() without first iterating the context (for example, an extension function invoked with a union expression, e.g. test:fn(a | b), as one of its arguments) received an empty NodeSet instead of the union of the operand node-sets. Override getValue() to force the node set to be built via getContextNodeList() before delegating to the parent implementation, per the fix suggested in the JIRA report. Adds a regression test in ExtensionFunctionTest that invokes a custom NodeSet-consuming function with a union of two location paths and asserts the full pointer count is seen. Generated-by: Claude Sonnet 5 (Claude Code)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes JXPATH-204.
Problem
UnionContext(theEvalContextproduced for a union expression likea | b)only populates its
NodeSetlazily, insidesetPosition(). Normal XPathevaluation always calls
setPosition()/nextNode()before reading values, sothis is invisible in most usages.
However,
ExtensionFunction.computeValue()resolves each argument by callinggetValue()on the argument'sEvalContextdirectly, without iterating itfirst:
EvalContext.getValue()delegates togetNodeSet(), andNodeSetContext.getNodeSet()just returns the backing field as-is (it doesnot iterate). Since
UnionContextnever hadsetPosition()invoked yet atthat point, the backing
NodeSetis still empty. The net effect: a customextension function invoked with a union expression as an argument, e.g.
my:fn(a | b), receives an emptyNodeSetinstead of the union ofaand
b.Fix
Override
getValue()inUnionContextto force the node list to be computed(via
getContextNodeList(), which is whatsetPosition()-driven iterationwould have done anyway) before delegating to
super.getValue(). This matchesthe fix proposed by the reporter in the JIRA issue.
Testing
ExtensionFunctionTest.testUnionOperatorArgument(), which calls aNodeSet-consuming extension function (test:countPointers) with a unionof two location paths (
/beans[1] | /beans[2]) and asserts both nodes areseen (
2), where previously it saw0.UnionContextfirst to confirmit reproduces the reported bug (
expected: <2> but was: <0>), then appliedthe fix and confirmed it passes.
mvn test→ 416 tests run, 0 failures, 0 errors (1 pre-existingskip, unrelated to this change).
mvn checkstyle:check pmd:check→ clean.mvn verify→ success.registering a custom
int countPointers(NodeSet)extension function andevaluating
my:countPointers(/beans[1] | /beans[2])viaJXPathContextprinted
0(bug) against the unfixed jar and2(correct) against thefixed jar, confirming the fix holds at the public API level, not just
inside the unit test.
Checklist (from PR template)
mvn).