perf: multi-pass DFS in ComponentTreeRewriter; resource-type pre-filter in NodeBasedRewriteRule

[im-shiv]

ComponentTreeRewriter: replace full-tree-restart-per-match loop with path-based DFS (ArrayDeque) and outer repeat-until-stable loop. Previous O(M²) visits for M-field forms; now O(M·passes) — typically 2-4 passes.

NodeBasedRewriteRule: pre-check sling:resourceType before getPrimaryNodeType() to avoid segment-store reads on non-matching nodes (~95% of tree). Also fix sibling ordering: capture next-sibling name before rename and restore position after replacement.

[iamsudhanshu]

What is the impact on reaching MAX_PASSES. Also, in what case we can have so many passes?
Is this related to number of components? If so - what happens when customers have so many components - does it fail?

[im-shiv]

MAX_PASSES is not related to component count — it's a guard against pathological rule behaviour, not a scaling limit.

Each pass is a full DFS walk of the tree. A pass increments only when a rule fires and modifies the tree structure. For a well-behaved rule set, the number of passes equals the number of distinct structural "depths" that need resolving — for forms conversion this is typically 2–4 (ContainerRule → RootPanelRule → leaf components → cleanup). A 1000-component form still converges in the same 2–4 passes; component count drives visit count per pass, not pass count.

MAX_PASSES=50 would only be reached if a rule repeatedly mutates the tree in a way that keeps producing new matches every pass (e.g. a rule that re-adds a node it just matched). In that case the loop exits after 50 passes and returns whatever state the tree is in — it does not throw or fail. The form is left partially converted, which is the same behaviour as a rule-level exception. The processed map prevents any single rule from firing on the same node path twice, which already eliminates most infinite-loop risks. MAX_PASSES is a last-resort safety net for edge cases we haven't anticipated.

[iamsudhanshu]

Please clarify why this is not required now.

[im-shiv]

The old algorithm called session.save() internally after each rule match during the traversal — so by the time rewrite() returned, all changes were already flushed to disk. The assertions assertTrue(session.hasPendingChanges()) and session.save() were testing that mid-traversal save behaviour.

The new algorithm never calls session.save() — it leaves all changes as pending in the session and lets the caller commit once. So after rewrite() returns, session.hasPendingChanges() is still true but session.save() is the caller's responsibility (the job executor handles it). The assertions were removed because they were testing an implementation detail of the old algorithm, not correctness of the rewrite. The actual correctness checks (node order, invocation counts) remain intact.

[iamsudhanshu]

Review comments from Claude:
PR #243 — Code Review

perf: multi-pass DFS in ComponentTreeRewriter; resource-type pre-filter in NodeBasedRewriteRule

Overview

Two independent changes shipped together:

1. ComponentTreeRewriter: Replaces TreeTraverser-restart-per-match with an iterative path-based DFS (ArrayDeque) plus an outer repeat-until-stable loop capped at MAX_PASSES = 50. The goal is O(M·passes) instead of O(M²) for large
   forms.
2. NodeBasedRewriteRule: Adds a sling:resourceType pre-filter in matches() to skip getPrimaryNodeType() for non-matching nodes. Separately fixes sibling ordering by capturing the next-sibling name before the rename/move and restoring position
   after replacement.

---

Critical Issue

1. finalPaths.add(path) is now unconditional — changes the semantics of the finalPaths contract

In the original code, finalPaths was exclusively populated by rule.applyTo() — the rule itself decided whether the resulting node should be excluded from further processing. The new code unconditionally marks every rule-processed path as
final:

Node result = rule.applyTo(node, finalPaths);
ruleProcessed.add(path);
finalPaths.add(path); // ← new, always fires

This means once any rule fires on a path, no other rule can ever match the resulting node at that path — even across passes. In NodeBasedRewriteRule.applyTo, a replacement node is created at the original path (the old node is moved to a tmp-
name first). Under the old behavior, if the replacement node was itself matchable by a second rule, that second rule would fire on it in the next restart. Under the new behavior, finalPaths blocks it permanently, regardless of whether the
rule set cq:rewriteFinal.

The only place finalPaths was intentionally unconditional in the old code was inside NodeBasedRewriteRule.applyTo when treeIsFinal = true — a flag the rule author explicitly sets. Rules that don't set treeIsFinal expected subsequent rules to
be able to process their output. This PR silently removes that guarantee for all rules.

Concrete regression scenario: rule A converts a fd/af/ComponentX node into a fd/af2/ComponentY node at the same path. Rule B is supposed to then convert fd/af2/ComponentY. With this change, rule B can never fire at that path because it's in
finalPaths after rule A.

---

Correctness Issues

2. MAX_PASSES exhaustion is silent

If a tree legitimately requires more than 50 passes (unlikely but not impossible with deeply nested rules), the loop exits without any log warning. The caller receives a partial result with no indication that processing was truncated. A
logger.warn at exit when passCount >= MAX_PASSES is essential here.

3. Sibling ordering fix: nextSiblingName can be stale after replacement

if (nextSiblingName != null) {
parent.orderBefore(originalName, nextSiblingName);
}

nextSiblingName is captured before the root.getSession().move(...) call. If the replacement process (JcrUtil.copy, mappings.processOrder, etc.) renames or removes the node named nextSiblingName, the orderBefore call will throw
javax.jcr.ItemNotFoundException. This is an edge case for rules whose replacement trees interact with adjacent siblings, but mappings.processOrder does process ordering of the new tree's children — it's not obviously safe to assume
nextSiblingName survives intact.

4. Children are pushed to stack then visitedInPass-checked both before push and after pop

// Before push:
if (!visitedInPass.contains(childPath)) {
stack.push(childPath);
}
// After pop:
if (!visitedInPass.add(path)) {
continue;
}

The pre-push check avoids duplicates on the stack (good for memory). The post-pop check is the real guard. Together they're harmless, but the before-push check is only correct if visitedInPass always contains what's been popped, not just
pushed — which is true here, but subtly relies on the pop-check always running first. This is fine as written, but a comment clarifying the intent would help future readers.

---

Behavioral Changes to Existing Tests

5. preservesOrder no longer tests order preservation under rewriting

The removed assertions (assertTrue(session.hasPendingChanges()) + session.save()) existed because the old code pre-moved every node to the end of its parent during traversal, making changes even when no rules fired. The new code correctly
doesn't do this. The assertions were removed accordingly — that's correct. But the test now just loads JSON and reads it back without any rewrite operation producing meaningful changes. It no longer covers the scenario that motivated it: that
the rewriter doesn't disturb sibling order when firing rules. A test that fires a real rule and verifies the output order is preserved would be meaningful.

6. infiniteLoop test passes but for a different reason

In the old code, the NoOpRewriteRule (always matches, returns same node unchanged) could fire at each path only once because ruleProcessedPaths blocked re-processing — giving exactly 9 invocations for 9 nodes. In the new code, the same result
(9 invocations) occurs because after pass 1, all 9 paths are also in finalPaths (from issue #1 above), so pass 2 finds changedInPass = false immediately. The test passes, but it now relies on the finalPaths behavior added by this PR rather
than the ruleProcessed guard that was the original intent of the test.

---

Missing Tests

7. No test for the sibling ordering fix in NodeBasedRewriteRule

The PR description identifies a concrete bug (sibling order not restored after replacement) but adds no test exercising the pre-capture and orderBefore restore path. A test with an ordered parent, a middle sibling getting rewritten, and an
assertion on the final sibling order would provide the regression guard this fix deserves.

8. No test for the sling:resourceType pre-filter short-circuit

The optimization path (pattern has sling:resourceType, node doesn't → return false before getPrimaryNodeType()) has no dedicated test. While existing tests cover the general match behavior, a test explicitly for a node missing
sling:resourceType when the pattern requires it would pin the new early-exit behavior.

9. No test for MAX_PASSES enforcement

The infiniteLoop test name implies it guards against unbounded loops. Under the old code it was bounded by ruleProcessedPaths; under the new code it's bounded by both ruleProcessed/finalPaths (issue #1) and MAX_PASSES. A test using a rule
that always creates a new matchable node (true structural growth) would be the real infinite-loop guard.

---

Minor

• logger.debug → logger.info for the completion timing log: Called per conversion tree, potentially hundreds of times for a large form job. debug was appropriate here; info will create noise in production logs.
• Node[] startNodeRef = new Node[] { root }: A single-element array to simulate a mutable reference is an unusual Java idiom outside of lambda contexts. Here it's not in a lambda — a simple local variable updated in the rule-application block
  would be clearer.
• Javadoc removes @throws tags: The updated Javadoc drops @throws RewriteException and @throws RepositoryException from the method signature, but the method still declares both. The throws contract should be documented.

[bstopp]

See PR review from Claude, provided by @iamsudhanshu