Merge pull request #220 from refactorfirst/#215-handle-stack-overflow-sealed-depth

#215 Fix computeSealedDepth to handle cycle-safe ancestor traversal

Author: jimbethancourt

codebase-graph-builder/src/main/java/org/hjug/graphbuilder/metrics/ClassMetrics.java

@@ -149,6 +149,14 @@ void freeze() {
      * sealed class has depth=1; each indirect descendant nests deeper.
      * Computed post-walk in
      * {@link GraphMetricsCollector#finalizeMetrics()}.
+     *
+     * <p>Cyclic-depth semantics: the depth is the longest acyclic
+     * ancestor path terminating at a sealed root within the analyzed
+     * batch; a class with no such path (including a cycle member whose
+     * ancestor paths do not reach a sealed root, or a chain dead-ending at a
+     * non-sealed in-batch class) has depth 0. Ancestors
+     * absent from the batch preserve a minimum depth of 2 as a
+     * relationship signal. (Cycle safety: issue #215.)
      */
     @Getter
     private int sealedHierarchyDepth;

codebase-graph-builder/src/main/java/org/hjug/graphbuilder/metrics/GraphMetricsCollector.java

@@ -261,9 +261,12 @@ public void finalizeMetrics() {
      * <ul>
      *   <li>{@link ClassMetrics#setSealedHierarchyDepth(int)} — root sealed
      *       class has depth 1; each direct permittee inherits depth 2, and
-     *       so on. Computed by walking up the sealed-hierarchy ancestor
-     *       chain until reaching a class whose {@link ClassMetrics#isSealed()}
-     *       flag is {@code false}.</li>
+     *       so on. The depth is the longest acyclic ancestor path that
+     *       terminates at a sealed root; classes with no such path (including
+     *       cycle members without a path to a sealed root and non-sealed
+     *       dead-ends) get depth 0 and are not marked.
+     *       Third-party ancestors absent from the batch preserve a minimum
+     *       depth of 2 as a relationship signal. (Cycle safety: issue #215.)</li>
      *   <li>{@link ClassMetrics#setHasExplicitLogic(boolean)} — true when a
      *       Kotlin {@code data class} declares any non-accessor method,
      *       feeding the {@code (hasExplicitLogic || WMC > 14)} criterion of
@@ -286,30 +289,77 @@ private void computeKotlinDerivedMetrics() {
         }
     }
 
+    /**
+     * Computes a class's depth in the collected sealed hierarchy.
+     *
+     * @return {@code 0} for a non-sealed class with no recorded ancestors, {@code 1}
+     *         for a sealed class with no recorded ancestors, {@code 2} when all
+     *         recorded ancestors are outside this collector, or one more than
+     *         the greatest depth among ancestors present in this collector
+     */
     private int computeSealedDepth(ClassMetrics metrics) {
+        return computeSealedDepth(metrics, new HashSet<>());
+    }
+
+    /**
+     * Cycle-safe variant of {@link #computeSealedDepth(ClassMetrics)}. The
+     * {@code visiting} set tracks the FQNs on the current ancestor-traversal
+     * path; re-entering a class already on the path means the collected
+     * ancestor data contains a cycle (issue #215: Java {@code implements}
+     * chains recorded in a cycle, e.g. by type attribution without the full
+     * classpath), so the traversal stops instead of recursing forever.
+     *
+     * <p><b>Cyclic-depth semantics:</b> a class's depth is the length of the
+     * longest acyclic ancestor path that terminates at a sealed root
+     * ({@code isSealed() == true} with no ancestors, depth 1) within the
+     * analyzed batch. A class with no such path — including a cycle member
+     * whose ancestor paths do not reach a sealed root, a class whose only
+     * paths enter a cycle, or a class whose chain dead-ends at a non-sealed
+     * class in the batch — has depth 0: it is not an observable member of any
+     * sealed hierarchy. Ancestors absent from the batch
+     * (third-party) preserve the minimum depth of 2 as a relationship signal.
+     *
+     * @param visiting the class FQNs on the current ancestor-traversal path
+     * @return {@code 0} when this class is already on that path; otherwise its
+     *         depth from the collected ancestors
+     */
+    private int computeSealedDepth(ClassMetrics metrics, Set<String> visiting) {
         Set<String> ancestors = metrics.getSealedHierarchyAncestors();
         if (ancestors.isEmpty()) {
             // No sealed hierarchy ancestors: depth 1 if sealed, 0 otherwise
             return metrics.isSealed() ? 1 : 0;
         }
+        if (!visiting.add(metrics.getFullyQualifiedName())) {
+            // Cycle: this path has no valid depth contribution
+            return 0;
+        }
         // Has sealed hierarchy ancestors: traverse them first
         int maxAncestorDepth = 0;
-        boolean hasObservableAncestor = false;
+        boolean hasExternalAncestor = false;
         for (String ancestorFqn : ancestors) {
             ClassMetrics ancestor = classMetrics.get(ancestorFqn);
             if (ancestor == null) {
                 // Ancestor not in this codebase batch (third-party): preserve
                 // minimum depth of 2 to record the relationship
+                hasExternalAncestor = true;
                 continue;
             }
-            hasObservableAncestor = true;
-            maxAncestorDepth = Math.max(maxAncestorDepth, computeSealedDepth(ancestor));
+            // Dead-end paths (depth 0) contribute nothing; only paths that
+            // reach a sealed root propagate a positive depth upward
+            maxAncestorDepth = Math.max(maxAncestorDepth, computeSealedDepth(ancestor, visiting));
+        }
+        visiting.remove(metrics.getFullyQualifiedName());
+        if (maxAncestorDepth > 0) {
+            return maxAncestorDepth + 1;
         }
-        if (!hasObservableAncestor) {
-            // All ancestors are external; preserve depth 2 to record relationship
+        if (hasExternalAncestor) {
+            // No sealed root reachable in-batch, but external ancestors may
+            // hold the root; preserve depth 2 to record relationship
             return 2;
         }
-        return maxAncestorDepth + 1;
+        // No ancestor path reaches a sealed root (cycle or non-sealed
+        // dead-end): not a member of any observable sealed hierarchy
+        return 0;
     }
 
     /**

codebase-graph-builder/src/test/java/org/hjug/graphbuilder/metrics/GraphMetricsCollectorSealedDepthTest.java

@@ -0,0 +1,213 @@
+package org.hjug.graphbuilder.metrics;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+import org.jgrapht.graph.DefaultDirectedWeightedGraph;
+import org.jgrapht.graph.DefaultWeightedEdge;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Regression tests for
+ * <a href="https://github.com/refactorfirst/RefactorFirst/issues/215">issue #215</a>:
+ * {@code StackOverflowError} in
+ * {@link GraphMetricsCollector}'s {@code computeSealedDepth} when the collected
+ * sealed-hierarchy ancestor data contains a cycle (e.g. a Java {@code implements}
+ * chain recorded in a cycle by type attribution without the full classpath).
+ *
+ * <p>Also defines the <b>cyclic-depth semantics</b> of the public
+ * {@link ClassMetrics#getSealedHierarchyDepth()} metric:
+ *
+ * <p>A class's depth is the length of the longest acyclic ancestor path that
+ * terminates at a sealed root ({@code isSealed() == true} with no ancestors,
+ * depth 1) <em>within the analyzed batch</em>. A class with no such path —
+ * including a cycle member whose ancestor paths do not reach a sealed root,
+ * a class whose only ancestor paths enter a cycle, or a class whose chain
+ * dead-ends at a non-sealed class in the batch — has depth 0: it is not a
+ * member of any observable sealed hierarchy. External ancestors (not present
+ * in the batch) preserve the pre-existing minimum depth of 2 as a relationship
+ * signal. When both cyclic and valid paths exist, the valid path's depth wins.
+ */
+class GraphMetricsCollectorSealedDepthTest {
+
+    private GraphMetricsCollector newCollector() {
+        return new GraphMetricsCollector(
+                new DefaultDirectedWeightedGraph<>(DefaultWeightedEdge.class),
+                new DefaultDirectedWeightedGraph<>(DefaultWeightedEdge.class));
+    }
+
+    // --- Termination (issue #215 regression) -------------------------------
+
+    @DisplayName("a two-class ancestor cycle does not overflow the stack")
+    @Test
+    void twoClassAncestorCycle_terminates() {
+        GraphMetricsCollector collector = newCollector();
+        collector.getOrCreateClassMetrics("com.example.A").addSealedHierarchyAncestor("com.example.B");
+        collector.getOrCreateClassMetrics("com.example.B").addSealedHierarchyAncestor("com.example.A");
+
+        assertDoesNotThrow(collector::finalizeMetrics, "cyclic sealed-hierarchy ancestors must not StackOverflowError");
+    }
+
+    @DisplayName("a self-referencing ancestor does not overflow the stack")
+    @Test
+    void selfReferencingAncestor_terminates() {
+        GraphMetricsCollector collector = newCollector();
+        collector.getOrCreateClassMetrics("com.example.A").addSealedHierarchyAncestor("com.example.A");
+
+        assertDoesNotThrow(
+                collector::finalizeMetrics, "self-referencing sealed-hierarchy ancestor must not StackOverflowError");
+    }
+
+    // --- Cyclic-depth semantics ---------------------------------------------
+
+    @DisplayName("cycle members have depth 0: a cycle has no sealed root")
+    @Test
+    void twoClassAncestorCycle_membersHaveDepthZero() {
+        GraphMetricsCollector collector = newCollector();
+        ClassMetrics a = collector.getOrCreateClassMetrics("com.example.A");
+        ClassMetrics b = collector.getOrCreateClassMetrics("com.example.B");
+        a.addSealedHierarchyAncestor("com.example.B");
+        b.addSealedHierarchyAncestor("com.example.A");
+
+        collector.finalizeMetrics();
+
+        assertEquals(0, a.getSealedHierarchyDepth(), "cycle member A has no path to a sealed root");
+        assertEquals(0, b.getSealedHierarchyDepth(), "cycle member B has no path to a sealed root");
+    }
+
+    @DisplayName("a self-referencing class has depth 0")
+    @Test
+    void selfReferencingAncestor_hasDepthZero() {
+        GraphMetricsCollector collector = newCollector();
+        ClassMetrics a = collector.getOrCreateClassMetrics("com.example.A");
+        a.addSealedHierarchyAncestor("com.example.A");
+
+        collector.finalizeMetrics();
+
+        assertEquals(0, a.getSealedHierarchyDepth());
+    }
+
+    @DisplayName("members of a three-class cycle all have depth 0")
+    @Test
+    void threeClassAncestorCycle_membersHaveDepthZero() {
+        GraphMetricsCollector collector = newCollector();
+        ClassMetrics a = collector.getOrCreateClassMetrics("com.example.A");
+        ClassMetrics b = collector.getOrCreateClassMetrics("com.example.B");
+        ClassMetrics c = collector.getOrCreateClassMetrics("com.example.C");
+        a.addSealedHierarchyAncestor("com.example.B");
+        b.addSealedHierarchyAncestor("com.example.C");
+        c.addSealedHierarchyAncestor("com.example.A");
+
+        collector.finalizeMetrics();
+
+        assertEquals(0, a.getSealedHierarchyDepth());
+        assertEquals(0, b.getSealedHierarchyDepth());
+        assertEquals(0, c.getSealedHierarchyDepth());
+    }
+
+    @DisplayName("a class whose only ancestor path enters a cycle has depth 0")
+    @Test
+    void descendantOfCycle_hasDepthZero() {
+        GraphMetricsCollector collector = newCollector();
+        ClassMetrics a = collector.getOrCreateClassMetrics("com.example.A");
+        ClassMetrics b = collector.getOrCreateClassMetrics("com.example.B");
+        ClassMetrics e = collector.getOrCreateClassMetrics("com.example.E");
+        a.addSealedHierarchyAncestor("com.example.B");
+        b.addSealedHierarchyAncestor("com.example.A");
+        e.addSealedHierarchyAncestor("com.example.A");
+
+        collector.finalizeMetrics();
+
+        assertEquals(0, e.getSealedHierarchyDepth(), "E's only path dead-ends in the A<->B cycle");
+    }
+
+    @DisplayName("a valid sealed-root path wins over a cyclic path")
+    @Test
+    void mixedCyclicAndValidPaths_validPathWins() {
+        GraphMetricsCollector collector = newCollector();
+        ClassMetrics root = collector.getOrCreateClassMetrics("com.example.Shape");
+        root.setSealed(true);
+        ClassMetrics a = collector.getOrCreateClassMetrics("com.example.A");
+        ClassMetrics b = collector.getOrCreateClassMetrics("com.example.B");
+        ClassMetrics e = collector.getOrCreateClassMetrics("com.example.Circle");
+        a.addSealedHierarchyAncestor("com.example.B");
+        b.addSealedHierarchyAncestor("com.example.A");
+        // Circle lists both a cyclic ancestor and a genuine sealed root
+        e.addSealedHierarchyAncestor("com.example.A");
+        e.addSealedHierarchyAncestor("com.example.Shape");
+
+        collector.finalizeMetrics();
+
+        assertEquals(
+                2,
+                e.getSealedHierarchyDepth(),
+                "Circle's depth comes from the Shape root path; the cyclic path contributes nothing");
+        assertEquals(1, root.getSealedHierarchyDepth());
+    }
+
+    @DisplayName("a cycle member with a sealed-root path receives its valid depth")
+    @Test
+    void cycleMemberWithSealedRootPath_hasValidDepth() {
+        GraphMetricsCollector collector = newCollector();
+        ClassMetrics root = collector.getOrCreateClassMetrics("com.example.Shape");
+        root.setSealed(true);
+        ClassMetrics a = collector.getOrCreateClassMetrics("com.example.A");
+        ClassMetrics b = collector.getOrCreateClassMetrics("com.example.B");
+        a.addSealedHierarchyAncestor("com.example.B");
+        b.addSealedHierarchyAncestor("com.example.A");
+        b.addSealedHierarchyAncestor("com.example.Shape");
+
+        collector.finalizeMetrics();
+
+        assertEquals(3, a.getSealedHierarchyDepth(), "A reaches the Shape root through B");
+        assertEquals(2, b.getSealedHierarchyDepth(), "B reaches the Shape root directly");
+    }
+
+    @DisplayName("a chain ending at a non-sealed class in the batch has depth 0")
+    @Test
+    void chainEndingAtNonSealedClass_hasDepthZero() {
+        GraphMetricsCollector collector = newCollector();
+        collector.getOrCreateClassMetrics("com.example.PlainInterface");
+        ClassMetrics impl = collector.getOrCreateClassMetrics("com.example.Impl");
+        impl.addSealedHierarchyAncestor("com.example.PlainInterface");
+
+        collector.finalizeMetrics();
+
+        assertEquals(0, impl.getSealedHierarchyDepth(), "no sealed root is reachable, so no depth is recorded");
+    }
+
+    // --- Unchanged semantics -------------------------------------------------
+
+    @DisplayName("acyclic sealed hierarchy depths are unchanged by cycle protection")
+    @Test
+    void acyclicHierarchy_depthsUnchanged() {
+        GraphMetricsCollector collector = newCollector();
+        ClassMetrics root = collector.getOrCreateClassMetrics("com.example.Shape");
+        root.setSealed(true);
+        ClassMetrics child = collector.getOrCreateClassMetrics("com.example.Circle");
+        child.addSealedHierarchyAncestor("com.example.Shape");
+        ClassMetrics grandchild = collector.getOrCreateClassMetrics("com.example.UnitCircle");
+        grandchild.addSealedHierarchyAncestor("com.example.Circle");
+
+        collector.finalizeMetrics();
+
+        assertEquals(1, root.getSealedHierarchyDepth());
+        assertEquals(2, child.getSealedHierarchyDepth());
+        assertEquals(3, grandchild.getSealedHierarchyDepth());
+    }
+
+    @DisplayName("external-only ancestors keep minimum depth 2 (relationship signal)")
+    @Test
+    void externalOnlyAncestors_keepMinimumDepthTwo() {
+        GraphMetricsCollector collector = newCollector();
+        ClassMetrics impl = collector.getOrCreateClassMetrics("com.example.Circle");
+        impl.addSealedHierarchyAncestor("com.thirdparty.Shape"); // not in batch
+
+        collector.finalizeMetrics();
+
+        assertEquals(
+                2,
+                impl.getSealedHierarchyDepth(),
+                "external ancestors preserve minimum depth of 2 to record the relationship");
+    }
+}