Cycles throw a VtlScriptException

Author: mstieland

vtl-engine/src/main/java/fr/insee/vtl/engine/VtlScriptEngine.java

@@ -4,6 +4,7 @@
 
 import fr.insee.vtl.engine.exceptions.VtlRuntimeException;
 import fr.insee.vtl.engine.exceptions.VtlSyntaxException;
+import fr.insee.vtl.engine.utils.dag.DAGBuilder;
 import fr.insee.vtl.engine.utils.dag.DAGStatement;
 import fr.insee.vtl.engine.visitors.AssignmentVisitor;
 import fr.insee.vtl.engine.visitors.DAGBuildingVisitor;
@@ -287,28 +288,32 @@ public void syntaxError(
    * @param start Root of parsetree
    * @return reordered VTL
    */
-  private VtlParser.StartContext reorderScript(VtlParser.StartContext start) {
-      DAGBuildingVisitor visitor = new DAGBuildingVisitor();
-      List<DAGStatement> sortedStatements = visitor.visit(start);
-
-      VtlParser.StartContext startReordered =
-              new VtlParser.StartContext((ParserRuleContext) start.getRuleContext(), start.invokingState);
-
-      // Build a set of unsorted indices that need reordering
-      Set<Integer> sortedIndices = sortedStatements.stream()
-              .map(DAGStatement::unsortedIndex)
-              .collect(Collectors.toSet());
-
-      int sortedIndex = 0;
-      for (int i = 0; i < start.getChildCount(); i++) {
-          if (sortedIndices.contains(i)) {
-              DAGStatement stmt = sortedStatements.get(sortedIndex++);
-              startReordered.addAnyChild(start.getChild(stmt.unsortedIndex()));
-          } else {
-              startReordered.addAnyChild(start.getChild(i));
-          }
+  private VtlParser.StartContext reorderScript(VtlParser.StartContext start)
+      throws VtlScriptException {
+    DAGBuildingVisitor visitor = new DAGBuildingVisitor();
+    List<DAGStatement> unsortedStatements = visitor.visit(start);
+
+    // Create DAG & topological sort
+    DAGBuilder dagBuilder = new DAGBuilder(unsortedStatements, start);
+    List<DAGStatement> sortedStatements = dagBuilder.topologicalSortedStatements();
+
+    VtlParser.StartContext startReordered =
+        new VtlParser.StartContext((ParserRuleContext) start.getRuleContext(), start.invokingState);
+
+    // Build a set of unsorted indices that need reordering
+    Set<Integer> sortedIndices =
+        sortedStatements.stream().map(DAGStatement::unsortedIndex).collect(Collectors.toSet());
+
+    int sortedIndex = 0;
+    for (int i = 0; i < start.getChildCount(); i++) {
+      if (sortedIndices.contains(i)) {
+        DAGStatement stmt = sortedStatements.get(sortedIndex++);
+        startReordered.addAnyChild(start.getChild(stmt.unsortedIndex()));
+      } else {
+        startReordered.addAnyChild(start.getChild(i));
       }
-      return startReordered;
+    }
+    return startReordered;
   }
 
   /**

vtl-engine/src/main/java/fr/insee/vtl/engine/utils/dag/DAGBuilder.java

@@ -1,19 +1,29 @@
 package fr.insee.vtl.engine.utils.dag;
 
+import fr.insee.vtl.engine.VtlScriptEngine;
+import fr.insee.vtl.model.exceptions.VtlScriptException;
+import fr.insee.vtl.parser.VtlParser;
 import java.util.*;
+import java.util.stream.Collectors;
+import org.antlr.v4.runtime.tree.ParseTree;
+import org.antlr.v4.runtime.tree.RuleNode;
+import org.antlr.v4.runtime.tree.TerminalNode;
 import org.jgrapht.Graph;
+import org.jgrapht.alg.cycle.CycleDetector;
 import org.jgrapht.graph.DefaultDirectedGraph;
 import org.jgrapht.graph.DefaultEdge;
 import org.jgrapht.traverse.TopologicalOrderIterator;
 
 /** Helper Class to build a DAG from VTL */
 public class DAGBuilder {
 
-    private final Graph<DAGStatement, DefaultEdge> graph;
+  private final Graph<DAGStatement, DefaultEdge> graph;
+  private final VtlParser.StartContext startContext;
 
-    public DAGBuilder(List<DAGStatement> statements) {
-        graph = buildDAG(statements);
-    }
+  public DAGBuilder(List<DAGStatement> statements, VtlParser.StartContext startContext) {
+    graph = buildDAG(statements);
+    this.startContext = startContext;
+  }
 
   /**
    * Builds a DAG from a list of VTL statements
@@ -44,9 +54,43 @@ private static boolean dependsOn(DAGStatement stmt2, DAGStatement stmt1) {
     return stmt2.consumes().contains(produced);
   }
 
-    public List<DAGStatement> topologicalSortedStatements() {
-        final List<DAGStatement> topologicalSorted = new ArrayList<>();
-        new TopologicalOrderIterator<>(graph).forEachRemaining(topologicalSorted::add);
-        return topologicalSorted;
+  public List<DAGStatement> topologicalSortedStatements() throws VtlScriptException {
+    Optional<VtlScriptException> cycleError = checkForCycles();
+    if (cycleError.isPresent()) {
+      throw cycleError.get();
+    }
+    final List<DAGStatement> topologicalSorted = new ArrayList<>();
+    new TopologicalOrderIterator<>(graph).forEachRemaining(topologicalSorted::add);
+    return topologicalSorted;
+  }
+
+  private Optional<VtlScriptException> checkForCycles() {
+    CycleDetector<DAGStatement, DefaultEdge> cycleDetector = new CycleDetector<>(graph);
+    Set<DAGStatement> statementsInCycle = cycleDetector.findCycles();
+    if (statementsInCycle.isEmpty()) {
+      return Optional.empty();
     }
+    String dagStatementsInCycle =
+        statementsInCycle.stream()
+            .map(d -> parseTreeToText(startContext.getChild(d.unsortedIndex())))
+            .collect(Collectors.joining(";", "", ""));
+    return Optional.of(
+        new VtlScriptException(
+            "Cycle detected in Script. The following statements form at least one cycle: "
+                + dagStatementsInCycle,
+            VtlScriptEngine.fromContext(startContext)));
+  }
+
+  private String parseTreeToText(ParseTree child) {
+    StringBuilder result = new StringBuilder();
+    if (child instanceof TerminalNode) {
+      result.append(child.getText());
+      result.append(" ");
+    } else if (child instanceof RuleNode && child.getChildCount() != 0) {
+      for (int i = 0; i < child.getChildCount(); ++i) {
+        result.append(parseTreeToText(child.getChild(i)));
+      }
+    }
+    return result.toString();
+  }
 }

vtl-engine/src/main/java/fr/insee/vtl/engine/visitors/DAGBuildingVisitor.java

@@ -1,6 +1,5 @@
 package fr.insee.vtl.engine.visitors;
 
-import fr.insee.vtl.engine.utils.dag.DAGBuilder;
 import fr.insee.vtl.engine.utils.dag.DAGStatement;
 import fr.insee.vtl.parser.VtlBaseVisitor;
 import fr.insee.vtl.parser.VtlParser;
@@ -21,10 +20,7 @@ public List<DAGStatement> visitChildren(RuleNode node) {
   @Override // explicit call to super, as visiting children is only supported for the start node
   // (only top level statements can be reordered)
   public List<DAGStatement> visitStart(VtlParser.StartContext node) {
-    final List<DAGStatement> unsortedStatements = super.visitChildren(node);
-    // Create DAG & topological sort
-    DAGBuilder dagBuilder = new DAGBuilder(unsortedStatements);
-    return dagBuilder.topologicalSortedStatements();
+    return super.visitChildren(node);
   }
 
   // Extract statements that can be reordered

vtl-engine/src/test/java/fr/insee/vtl/engine/utils/dag/DagTest.java

@@ -15,7 +15,6 @@
 import javax.script.ScriptEngine;
 import javax.script.ScriptEngineManager;
 import javax.script.ScriptException;
-import org.jgrapht.traverse.NotDirectedAcyclicGraphException;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
@@ -113,15 +112,16 @@ public void testDagSimpleExampleWithReordering() throws ScriptException {
   }
 
   @Test
-  public void testDagCircle() {
+  public void testDagCycle() {
     ScriptContext context = engine.getContext();
     context.setAttribute("a", 1L, ScriptContext.ENGINE_SCOPE);
     assertThatThrownBy(
             () -> {
-              engine.eval("b := a; c := b; a := c;");
+              engine.eval("e := a; b := a; c := b; a := c; f := a;");
             })
-        .isInstanceOf(NotDirectedAcyclicGraphException.class)
-        .hasMessage("Graph is not a DAG");
+        .isInstanceOf(VtlScriptException.class)
+        .hasMessage(
+            "Cycle detected in Script. The following statements form at least one cycle: b := a ;c := b ;a := c ");
   }
 
   @Test