[#263] Clarify error message on graph cycle issues

CHANGELOG.md

@@ -1,7 +1,22 @@
 # Change Log
 
-## 8.0.0
+## 8.0.1
+
+### Functionality Improvements
+
+#161: Hash mismatch error message should show the version name of the object
 
+### Technical Improvements
+
+### Bug Fixes
+
+#263: Correct and clarify the graph cycle error message
+
+
+#264: NPE caused when package-info.txt file w/ METADATA section exists in staticdata (data) folder
+Correcting error messages on graph cycles for complex databases
+
+## 8.0.0
 ### Functionality Improvements
 #111: Preventing concurrent deploys against a given schema
 

obevo-core/src/main/java/com/gs/obevo/impl/graph/GraphEnricherImpl.kt

@@ -38,7 +38,7 @@ class GraphEnricherImpl(private val convertDbObjectName: Function1<String, Strin
 
         changeIndexes.forEach { changeIndex -> inputs.forEach(changeIndex::add) }
 
-        val graph = DefaultDirectedGraph<T, DefaultEdge>(DefaultEdge::class.java)
+        val graph = DefaultDirectedGraph<T, DependencyEdge>(DependencyEdge::class.java)
 
         // First - add the core objects to the graph
         inputs.forEach { graph.addVertex(it) }
@@ -97,10 +97,20 @@ class GraphEnricherImpl(private val convertDbObjectName: Function1<String, Strin
 
         // validate
         GraphUtil.validateNoCycles(graph,
-                { t -> t.components.collect { sortableDependency -> "[" + sortableDependency.changeKey.objectKey.objectName + "." + sortableDependency.changeKey.changeName + "]" }.makeString(", ") },
-                { dependencyEdge -> (dependencyEdge as DependencyEdge).edgeType.name })
+                { vertex, target, edge ->
+                    vertex.components
+                            .map { sortableDependency ->
+                                val name = if (target && edge.edgeType == CodeDependencyType.DISCOVERED)
+                                    sortableDependency.changeKey.objectKey.objectName
+                                else sortableDependency.changeKey.objectKey.objectName + "." + sortableDependency.changeKey.changeName
+                                "[$name]"
+                            }
+                            .joinToString(", ")
+                },
+                { dependencyEdge -> dependencyEdge.edgeType.name }
+        )
 
-        return graph
+        return graph as Graph<T, DefaultEdge>
     }
 
     override fun <T> createSimpleDependencyGraph(inputs: Iterable<T>, edgesFunction: Function1<T, Iterable<T>>): Graph<T, DefaultEdge> {

obevo-core/src/main/java/com/gs/obevo/impl/graph/GraphUtil.kt

@@ -25,6 +25,7 @@ import org.jgrapht.Graph
 import org.jgrapht.alg.cycle.HawickJamesSimpleCycles
 import org.jgrapht.graph.AsSubgraph
 import org.jgrapht.graph.DefaultEdge
+import org.jgrapht.graph.EdgeReversedGraph
 
 /**
  * Utility class to work w/ graphs in the JGraphT library. There are a couple usages that need syntax sugar...
@@ -42,18 +43,19 @@ object GraphUtil {
 
     @JvmStatic
     fun <T> validateNoCycles(graph: Graph<T, DefaultEdge>) {
-        validateNoCycles(graph, { it.toString() }, DefaultEdge::toString)
+        validateNoCycles(graph, { v, _, _ -> v.toString() }, DefaultEdge::toString)
     }
 
+
     @JvmStatic
-    fun <T, E> validateNoCycles(graph: Graph<T, E>, vertexToString: Function1<in T, String>, edgeToString: Function1<in E, String>) {
+    fun <T, E> validateNoCycles(graph: Graph<T, E>, vertexToString: VertexToString<in T, in E>, edgeToString: Function1<in E, String>) {
         val simpleCycles = HawickJamesSimpleCycles(graph)
         val cycleComponents = simpleCycles.findSimpleCycles()
         cycleComponents.zipWithNext()
 
         if (cycleComponents.isNotEmpty()) {
             val cycleMessages = cycleComponents.mapIndexed { cycleCounter, cycleComponent ->
-                val subgraph = AsSubgraph(graph, cycleComponent.toSet())
+                val subgraph = EdgeReversedGraph(AsSubgraph(graph, cycleComponent.toSet()))
                 val visitedVertices = mutableSetOf<T>()
 
                 val sb = StringBuilder()
@@ -65,7 +67,7 @@ object GraphUtil {
                 while (walkIterator.hasNext()) {
                     val nextEdge = walkIterator.nextEdge()
                     val nextVertex = nextEdge.first
-                    sb.append("\n    " + vertexToString(prevVertex) + " ===> " + vertexToString(nextVertex) + "   (" + edgeToString(nextEdge.second) + " dependency)")
+                    sb.append("\n    " + vertexToString(prevVertex, false, nextEdge.second) + " == depends on ==> " + vertexToString(nextVertex, true, nextEdge.second) + "   (" + edgeToString(nextEdge.second) + " dependency)")
                     if (!visitedVertices.contains(nextVertex)) {
                         visitedVertices.add(nextVertex)
                     } else {
@@ -87,11 +89,15 @@ object GraphUtil {
                             "Changes are marked as [objectName.changeName]\n" +
                             "\n" +
                             "Overview of dependency types:\n" +
-                            " * DISCOVERED: dependencies found through the text code analysis. These are the likeliest candidates for causing cycles, and you should use excludeDependencies if appropriate on this\n" +
+                            " * DISCOVERED: dependencies found through the text code analysis.\n" +
+                            "       These are the likeliest candidates for causing cycles.\n" +
+                            "       Use excludeDependencies on the object name (not the change name) if needed on this\n" +
                             " * EXPLICIT: user-defined dependencies set via the includeDependencies or dependencies attributes\n" +
                             " * IMPLICIT: implied change dependencies determined by the order within incremental table changes\n" +
                             "\n" +
                             cycleMessages.joinToString("\n"), cycleComponents)
         }
     }
 }
+
+typealias VertexToString<T, E> = (vertex: T, target: Boolean, edge: E) -> String

obevo-core/src/test/java/com/gs/obevo/impl/graph/GraphEnricherImplTest.java

@@ -21,10 +21,8 @@
 import com.gs.obevo.api.appdata.ObjectKey;
 import com.gs.obevo.api.platform.ChangeType;
 import org.eclipse.collections.api.RichIterable;
-import org.eclipse.collections.api.block.function.Function2;
 import org.eclipse.collections.api.set.ImmutableSet;
 import org.eclipse.collections.impl.block.factory.Functions;
-import org.eclipse.collections.impl.block.factory.StringFunctions;
 import org.eclipse.collections.impl.factory.Lists;
 import org.eclipse.collections.impl.factory.Sets;
 import org.jgrapht.Graph;
@@ -216,7 +214,7 @@ public void testCycleValidationWithIncrementalChanges() {
         this.enricher = new GraphEnricherImpl(Functions.getStringPassThru()::valueOf);
 
         SortableDependencyGroup sch1Obj1C1 = newChange(schema1, type1, "obj1", "c1", 0, null);
-        SortableDependencyGroup sch1Obj1C2 = newChange(schema1, type1, "obj1", "c2", 1, Sets.immutable.<String>with("obj2"));
+        SortableDependencyGroup sch1Obj1C2 = newChangeWithDependency(schema1, type1, "obj1", "c2", 1, Sets.immutable.of(new CodeDependency("obj2", CodeDependencyType.DISCOVERED)));
         SortableDependencyGroup sch1Obj1C3 = newChange(schema1, type1, "obj1", "c3", 2, null);
         SortableDependencyGroup sch1Obj2C1 = newChange(schema1, type1, "obj2", "c1", 0, null);
         SortableDependencyGroup sch1Obj2C2 = newChange(schema1, type1, "obj2", "c2", 1, null);
@@ -230,6 +228,8 @@ public void testCycleValidationWithIncrementalChanges() {
         } catch (IllegalArgumentException exc) {
             exc.printStackTrace();
             assertThat(exc.getMessage(), containsString("Found cycles"));
+            // verify that we print a legible error message for discovered dependencies
+            assertThat(exc.getMessage(), containsString("[obj1.c2] == depends on ==> [obj2]   (DISCOVERED dependency)"));
         }
     }
 
@@ -247,6 +247,11 @@ private SortableDependencyGroup newChange(String schema, String changeType, Stri
     }
 
     private SortableDependencyGroup newChange(String schema, String changeTypeName, String objectName, String changeName, int orderWithinObject, ImmutableSet<String> dependencies) {
+        ImmutableSet<CodeDependency> codeDependencies = dependencies == null ? null : dependencies.collectWith(CodeDependency::new, CodeDependencyType.EXPLICIT);
+        return newChangeWithDependency(schema, changeTypeName, objectName, changeName, orderWithinObject, codeDependencies);
+    }
+
+    private SortableDependencyGroup newChangeWithDependency(String schema, String changeTypeName, String objectName, String changeName, int orderWithinObject, ImmutableSet<CodeDependency> dependencies) {
         ChangeType changeType = mock(ChangeType.class);
         when(changeType.getName()).thenReturn(changeTypeName);
         when(changeType.isRerunnable()).thenReturn(true);
@@ -255,12 +260,7 @@ private SortableDependencyGroup newChange(String schema, String changeTypeName,
         ObjectKey key = new ObjectKey(schema, objectName, changeType);
         when(sort.getChangeKey()).thenReturn(new ChangeKey(key, changeName));
         if (dependencies != null) {
-            when(sort.getCodeDependencies()).thenReturn(dependencies.collectWith(new Function2<String, CodeDependencyType, CodeDependency>() {
-                @Override
-                public CodeDependency value(String target, CodeDependencyType codeDependencyType) {
-                    return new CodeDependency(target, codeDependencyType);
-                }
-            }, CodeDependencyType.EXPLICIT));
+            when(sort.getCodeDependencies()).thenReturn(dependencies);
         }
         when(sort.getOrderWithinObject()).thenReturn(orderWithinObject);