Skip to content

Release 1.8.0 - #60

Open
rousso wants to merge 11 commits into
mainfrom
release/1.8.0
Open

Release 1.8.0#60
rousso wants to merge 11 commits into
mainfrom
release/1.8.0

Conversation

@rousso

@rousso rousso commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

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.

TEDEFO-5148 A predicate is discarded when a reference is written after a context override. join composed 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.
TEDEFO-5150 A predicate is discarded when an axis is added to a reference. addAxis had the same fault, and threw a bare NoSuchElementException on 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 japicmp treats the release as minor:

NEW ENUM:   XPathAnchor  (RELATIVE, ROOT, DESCENDANT_FROM_ROOT)
XPathInfo:  NEW METHOD getAnchor(), isAbsolute()
XPathStep:  NEW METHOD toString()

Nothing was removed and nothing changed incompatibly. XPathStep keeps 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.md rewritten for 1.8.0
  • pom.xml: version 1.8.0-SNAPSHOT1.8.0, project.build.outputTimestamp refreshed

Verification

mvn clean install passes 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. Tag 1.8.0 on main and publish to Maven Central.
  2. Back-merge main into develop.
  3. Set develop to 1.9.0-SNAPSHOT.
  4. Mark ECL 1.8.0 released in Jira.

rousso and others added 9 commits July 17, 2026 09:52
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
@rousso
rousso requested a review from rouschr September 2, 2026 10:12
@rousso rousso self-assigned this Sep 2, 2026
Comment thread src/main/java/eu/europa/ted/eforms/xpath/XPathProcessor.java
@rousso
rousso requested a review from rouschr September 2, 2026 10:55

@rouschr rouschr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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.

2 participants