Skip to content

[JXPATH-207] DOMNodePointer.asPath() can point to the wrong sibling for a namespaced element - #294

Open
sharanggupta wants to merge 1 commit into
apache:masterfrom
sharanggupta:JXPATH-207-dom-aspath-sibling-position
Open

sharanggupta wants to merge 1 commit into
apache:masterfrom
sharanggupta:JXPATH-207-dom-aspath-sibling-position

Conversation

@sharanggupta

Copy link
Copy Markdown

Fixes JXPATH-207.

What

DOMNodePointer.asPath() can return a path that resolves to a different node than the pointer it was called on. For a namespaced element with no usable prefix (for example a default-namespace element) it falls back to node()[N]. The node() test (DOMNodePointer.testNode, NODE_TYPE_NODE) matches every kind of child, but N came from getRelativePositionOfElement(), which counted only element siblings. With <root>before<item xmlns="urn:test"/></root>, asPath() returned /root[1]/node()[1], which selects the text node before.

How

The position is now counted over all preceding siblings, which is what node()[N] selects and what the method's Javadoc already said ("among all siblings"). The private method is renamed getRelativePositionAmongSiblings() because it no longer counts elements only. It is only used by this fallback in asPath(), so no other path changes.

Tests (JXPath207Test)

  • the reproducer from the ticket: asPath() is /root[1]/node()[2] and resolves back to the same element;
  • a text node, a comment, a processing instruction and a CDATA section before two namespaced elements: both paths (node()[4], node()[6]) resolve back to their own element;
  • control case with no preceding non-element sibling (unchanged: /root[1]/node()[1]).

Without the fix, 2 of the 3 tests fail (/root[1]/node()[1] resolves to the text node a); with it all 418 tests pass. Verified locally with the default mvn goal on Java 17 (RAT, japicmp, Checkstyle 0 violations, PMD, Javadoc) and the test suite on Java 8. The single skipped test is the existing one in JDOMModelTest.

Before you push a pull request, review this list:

  • Read the contribution guidelines for this project.
  • Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
  • I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute? Claude (Anthropic) helped draft the fix and the tests; I reviewed all of it, ran the builds and checked that the new tests fail without the change.
  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

…or a namespaced element

For a namespaced element without a usable prefix, asPath() falls back to
node()[N]. The node() test matches every kind of child (text, comment,
processing instruction, CDATA and element), but N was computed by
getRelativePositionOfElement(), which only counted element siblings. So
for <root>before<item xmlns="urn:test"/></root> asPath() returned
/root[1]/node()[1], which resolves to the text node, not the element.

The position is now counted over all preceding siblings, which is also
what the method's Javadoc already said. The method is renamed to
getRelativePositionAmongSiblings() since it no longer counts elements only.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant