Release 1.8.0 - #60
Conversation
Merge main into develop (1.7.0 release)
…en-joining-paths TEDEFO-5148: Preserve predicates and the leading separator when joining paths
…adding an axis (#59) * TEDEFO-5150: Preserve predicates and always return a valid path when adding an axis * TEDEFO-5150: Classify steps from the parse tree instead of their text
There was a problem hiding this comment.
Here's the semantic (behavioral-level) review of PR #60 (by Kiro). It's organized by concern, not by file.
It could contain some interesting idea...
Predicate-preserving rewrite of XPath join and addAxis
Release 1.8.0 fixes two instances of the same fault: predicates were dropped whenever a path was recomposed from its steps. Both join and addAxis stitched output from each step's text alone (getStepText()), silently discarding […] predicates — and in join, the leading / or // of an absolute first operand. The fix routes both through XPathStep.toString() (text + predicates) and threads a new XPathAnchor through join so the separator survives. addAxis also gains input validation and a step-classification model. Public surface grows by one enum + three methods → japicmp minor. The core rewrite is sound and well-tested.
The three things to watch
join was not migrated to the new step-kind model (confirmed, the most interesting finding). addAxis now uses isNavigationStep() to recognize back-steps, but join's cancellation loop still keys off the literal string "..":
join("a/b", "parent::node()/c") → a/b/parent::node()/c — not cancelled, because parent::node() isn't literally "..". Harmless (valid output), but inconsistent with how addAxis treats the identical step.
More subtly: a spelled-out parent::node() at the end of the first operand isn't in dotSteps, so the loop is willing to cancel it against a .. as if it were an ordinary forward step. That's the exact latent error the kind model exists to prevent. Pre-existing, not a regression — but the PR built the tool to fix it and pointed it at only one of the two call sites.
addAxis silently discards an absolute anchor (confirmed, documented). /a/b and //a/b both become preceding::a/b. The Javadoc states this is intended (an axis can't be followed by a separator), and for the EFX-1 field-reference use case it's the only sensible behavior. The reviewer's point is about consistency of contract: this same commit added IllegalArgumentException for null/blank paths, yet silently reinterprets an absolute one — two different treatments of "input that doesn't fit the contract." Worth an explicit decision rather than silence.
equals/hashCode predicate-order wrinkle (confirmed, pre-existing, out of scope). hashCode hashes the predicate list in order while equals sorts, so equal steps with reordered predicates can hash differently. Not touched by this PR; flagged only because the PR leans on step equality as an advertised invariant.
Correctness of the parts that changed
Predicate preservation: uniform and correct — both methods compose from toString() = getStepText() + getPredicateText(). contextualize was already emitting both; it's merely tidied to "/" + step, no behavior change.
Anchor/separator handling: the empty-join resolution is the right split — join("/a","..") → /, join("a","..") → ., join("/","b") → /b. Reading the anchor from the parse tree (not the input string) correctly handles a leading comment hiding the separator.
The // special case: minimumStepsToKeep = 1 for DESCENDANT_FROM_ROOT correctly refuses to cancel the first step, because //a/.. selects the parents of every a, not the root. Genuinely subtle, and the tests capture it.
Step classification (readAxisStep/readFilterStep/namesAnyNode): reads correctly against the grammar. self::node() → navigation, self::text()/self::b → retargetable, attribute/namespace/expression → opaque. Sound and complete for the cases addAxis needs.
Equality ignoring kind/nodeTest
This is necessary, not incidental — a text-built step is OPAQUE with null nodeTest, the same text parsed is RETARGETABLE, and the PR advertises they're equal. Safe as used: contextualize's comparisons never look at kind, and the only reader of kind (addAxis) always works on freshly parsed steps. It becomes a trap only if future code pulls a step from an equality-keyed collection and then branches on isNavigationStep().
Edge cases / NPEs / invalid output
The old NoSuchElementException (TEDEFO-5150) is properly closed: zero-step paths hit if (steps.isEmpty()) return anyNodeOn(axis)..., and the new !second.isEmpty() guard stops getFirst() throwing. No new NPE or empty-string paths found beyond the already-scoped ones (the null-step toString() and join("a","/") degenerate cases, which the earlier PR review already addressed).
Backward/binary compatibility
Additive only — new enum plus three methods, equality/ordering unchanged. Consistent with the japicmp minor rating.
Test coverage
Strong where it counts: predicate preservation, leading-separator preservation, // first-step protection, resolution-to-root/context, anchor-past-a-comment, spelling equivalence, expression/attribute/namespace parking, null/blank rejection, and the round-trip equality invariant.
Gaps:
No join test with a spelled-out parent::node() (would expose finding #1).
No join test with an absolute second operand (its anchor is silently dropped — a pre-existing minor asymmetry).
No multi-predicate reordering test (would surface the hashCode wrinkle in #3).
Release of the eForms Core Library 1.8.0.
What is in it
Two fixes, both in
XPathProcessor, both of the same kind: predicates were being dropped when a path was rebuilt from its steps.joincomposed its result from the text of each step alone, so the expression selected every instance of a field instead of the ones the predicate asked for, and said nothing. The leading separator of an absolute first operand was lost in the same place.addAxishad the same fault, and threw a bareNoSuchElementExceptionon a path made only of parent steps. It now returns a valid path for every valid path given, and reads a step the same way however it is spelled.Neither affected SDK content: no field path or view template reaches the paths where a predicate was lost. Both were fixed because predicates are becoming load-bearing — a withholding condition in the undisclosed-fields model is a predicate.
Why this is a minor and not a patch
Both tickets are bug fixes, but TEDEFO-5148 added public API while fixing its bug, so
japicmptreats the release as minor:Nothing was removed and nothing changed incompatibly.
XPathStepkeeps its constructor, its runtime type, its equality and its ordering; a step read from a path and one built from its text remain equal.Release contents
CHANGELOG.mdrewritten for 1.8.0pom.xml: version1.8.0-SNAPSHOT→1.8.0,project.build.outputTimestamprefreshedVerification
mvn clean installpasses with the binary compatibility gate active, 83 tests. All 1377 tests of the EFX Toolkit pass against this build. Every path produced by the changed code was compiled with Saxon; a sweep of 30 inputs reports no valid input producing invalid output.After merging
1.8.0onmainand publish to Maven Central.mainintodevelop.developto1.9.0-SNAPSHOT.ECL 1.8.0released in Jira.