Ticket #21881: josm21881_loop_detector_v1.patch

src/org/openstreetmap/josm/data/osm/NodeGraph.java

@@ -24 +24 @@
 
 /**
  * A directed or undirected graph of nodes.
+ *
  * @since 12463 (extracted from CombineWayAction)
  */
 public class NodeGraph {
+    private final Set<NodePair> edges;
+    private final Map<Node, List<NodePair>> successors = new LinkedHashMap<>();
+    private final Map<Node, List<NodePair>> predecessors = new LinkedHashMap<>();
+    private int numUndirectedEdges;
+
+    /**
+     * counts the number of edges that were added
+     */
+    private int addedEdges;
+
+    /**
+     * Constructs a new {@code NodeGraph}.
+     */
+    public NodeGraph() {
+        edges = new LinkedHashSet<>();
+    }
 
     /**
      * Builds a list of pair of nodes from the given way.
-     * @param way way
+     * @param way      way
      * @param directed if {@code true} each pair of nodes will occur once, in the way nodes order.
-     *                 if {@code false} each pair of nodes will occur twice (the pair and its inversed copy)
+     *                 if {@code false} each pair of nodes will occur twice (the pair and its inverse copy)
      * @return a list of pair of nodes from the given way
      */
     public static List<NodePair> buildNodePairs(Way way, boolean directed) {
         List<NodePair> pairs = new ArrayList<>();
-        for (Pair<Node, Node> pair: way.getNodePairs(false /* don't sort */)) {
+        for (Pair<Node, Node> pair : way.getNodePairs(false)) {
             pairs.add(new NodePair(pair));
             if (!directed) {
                 pairs.add(new NodePair(pair).swap());
@@ -48 +65 @@
 
     /**
      * Builds a list of pair of nodes from the given ways.
-     * @param ways ways
-     * @param directed if {@code true} each pair of nodes will occur once, in the way nodes order.
-     *                 if {@code false} each pair of nodes will occur twice (the pair and its inversed copy)
+     * @param ways     ways
+     * @param directed if {@code true} each pair of nodes will occur once, in the way nodes order.<p>
+     *                 if {@code false} each pair of nodes will occur twice (the pair and its inverse copy)
      * @return a list of pair of nodes from the given ways
      */
     public static List<NodePair> buildNodePairs(List<Way> ways, boolean directed) {
         List<NodePair> pairs = new ArrayList<>();
-        for (Way w: ways) {
+        for (Way w : ways) {
             pairs.addAll(buildNodePairs(w, directed));
         }
         return pairs;
     }
 
     /**
-     * Builds a new list of pair nodes without the duplicated pairs (including inversed copies).
+     * Builds a new list of pair nodes without the duplicated pairs (including inverse copies).
      * @param pairs existing list of pairs
      * @return a new list of pair nodes without the duplicated pairs
      */
     public static List<NodePair> eliminateDuplicateNodePairs(List<NodePair> pairs) {
         List<NodePair> cleaned = new ArrayList<>();
-        for (NodePair p: pairs) {
+        for (NodePair p : pairs) {
             if (!cleaned.contains(p) && !cleaned.contains(p.swap())) {
                 cleaned.add(p);
             }
@@ -76 +93 @@
         return cleaned;
     }
 
+    /**
+     * Create a directed graph from the given node pairs.
+     * @param pairs Node pairs to build the graph from
+     * @return node graph structure
+     */
     public static NodeGraph createDirectedGraphFromNodePairs(List<NodePair> pairs) {
         NodeGraph graph = new NodeGraph();
-        for (NodePair pair: pairs) {
+        for (NodePair pair : pairs) {
             graph.add(pair);
         }
         return graph;
     }
 
+    /**
+     * Create a directed graph from the given ways.
+     * @param ways ways to build the graph from
+     * @return node graph structure
+     */
     public static NodeGraph createDirectedGraphFromWays(Collection<Way> ways) {
         NodeGraph graph = new NodeGraph();
-        for (Way w: ways) {
-            graph.add(buildNodePairs(w, true /* directed */));
+        for (Way w : ways) {
+            graph.add(buildNodePairs(w, true));
         }
         return graph;
     }
@@ -99 +126 @@
      */
     public static NodeGraph createUndirectedGraphFromNodeList(List<NodePair> pairs) {
         NodeGraph graph = new NodeGraph();
-        for (NodePair pair: pairs) {
+        for (NodePair pair : pairs) {
             graph.add(pair);
             graph.add(pair.swap());
         }
@@ -108 +135 @@
 
     /**
      * Create an undirected graph from the given ways, but prevent reversing of all
-     * non-new ways by fix one direction.
+     * non-new ways by fixing one direction.
      * @param ways Ways to build the graph from
      * @return node graph structure
      * @since 8181
      */
     public static NodeGraph createUndirectedGraphFromNodeWays(Collection<Way> ways) {
         NodeGraph graph = new NodeGraph();
-        for (Way w: ways) {
-            graph.add(buildNodePairs(w, false /* undirected */));
+        for (Way w : ways) {
+            graph.add(buildNodePairs(w, false));
         }
         return graph;
     }
 
+    /**
+     * Create a nearly undirected graph from the given ways, but prevent reversing of all
+     * non-new ways by fixing one direction.
+     * The first new way gives the direction of the graph.
+     * @param ways Ways to build the graph from
+     * @return node graph structure
+     */
     public static NodeGraph createNearlyUndirectedGraphFromNodeWays(Collection<Way> ways) {
         boolean dir = true;
         NodeGraph graph = new NodeGraph();
-        for (Way w: ways) {
+        for (Way w : ways) {
             if (!w.isNew()) {
                 /* let the first non-new way give the direction (see #5880) */
                 graph.add(buildNodePairs(w, dir));
                 dir = false;
             } else {
-                graph.add(buildNodePairs(w, false /* undirected */));
+                graph.add(buildNodePairs(w, false));
             }
         }
         return graph;
     }
 
-    private final Set<NodePair> edges;
-    private int numUndirectedEges;
-    /** counts the number of edges that were added */
-    private int addedEdges;
-    private final Map<Node, List<NodePair>> successors = new LinkedHashMap<>();
-    private final Map<Node, List<NodePair>> predecessors = new LinkedHashMap<>();
+    /**
+     * Add a node pair.
+     * @param pair node pair
+     */
+    public void add(NodePair pair) {
+        addedEdges++;
+        edges.add(pair);
+    }
+
+    /**
+     * Add a list of node pairs.
+     * @param pairs collection of node pairs
+     */
+    public void add(Collection<NodePair> pairs) {
+        for (NodePair pair : pairs) {
+            add(pair);
+        }
+    }
 
+    /**
+     * Return the edges containing the node pairs of the graph.
+     * @return the edges containing the node pairs of the graph
+     */
+    public Set<NodePair> getEdges() {
+        return edges;
+    }
+
+    /**
+     * Return the graph's nodes.
+     * @return the graph's nodes
+     */
+    public Set<Node> getNodes() {
+        Set<Node> nodes = new LinkedHashSet<>(2 * edges.size());
+        for (NodePair pair : edges) {
+            nodes.add(pair.getA());
+            nodes.add(pair.getB());
+        }
+        return nodes;
+    }
+
+    /**
+     * Creates a lookup table from the existing edges to make querying possible.
+     * @return a map containing all the graph data, where the key is queryable, values are direct successors of the key
+     * @since xxx
+     */
+    public Map<Node, Set<Node>> createMap() {
+        Map<Node, Set<Node>> result = new HashMap<>((int) (edges.size() / 0.75) + 1);
+        for (NodePair edge : edges) {
+            if (result.containsKey(edge.getA())) {
+                result.get(edge.getA()).add(edge.getB());
+            } else {
+                result.put(edge.getA(), new HashSet<>(Collections.singletonList(edge.getB())));
+            }
+        }
+
+        return result;
+    }
+
+    /**
+     * See {@link #prepare()}
+     */
     protected void rememberSuccessor(NodePair pair) {
         List<NodePair> l = successors.computeIfAbsent(pair.getA(), k -> new ArrayList<>());
         if (!l.contains(pair)) {
@@ -150 +238 @@
         }
     }
 
+    /**
+     * See {@link #prepare()}
+     */
     protected void rememberPredecessors(NodePair pair) {
         List<NodePair> l = predecessors.computeIfAbsent(pair.getB(), k -> new ArrayList<>());
         if (!l.contains(pair)) {
@@ -157 +248 @@
         }
     }
 
+    /**
+     * Replies true if {@code n} is a terminal node of the graph. Internal variables should be initialized first.
+     * @param n Node to check
+     * @return {@code true} if it is a terminal node
+     * @see #prepare()
+     */
     protected boolean isTerminalNode(Node n) {
         if (successors.get(n) == null) return false;
         if (successors.get(n).size() != 1) return false;
@@ -174 +271 @@
         successors.clear();
         predecessors.clear();
 
-        for (NodePair pair: edges) {
+        for (NodePair pair : edges) {
             if (!undirectedEdges.contains(pair) && !undirectedEdges.contains(pair.swap())) {
                 undirectedEdges.add(pair);
             }
             rememberSuccessor(pair);
             rememberPredecessors(pair);
         }
-        numUndirectedEges = undirectedEdges.size();
-    }
-
-    /**
-     * Constructs a new {@code NodeGraph}.
-     */
-    public NodeGraph() {
-        edges = new LinkedHashSet<>();
+        numUndirectedEdges = undirectedEdges.size();
     }
 
     /**
-     * Add a node pair.
-     * @param pair node pair
+     * Return the terminal nodes of the graph.
+     * @return the terminal nodes of the graph
      */
-    public void add(NodePair pair) {
-        addedEdges++;
-        edges.add(pair);
-    }
-
-    /**
-     * Add a list of node pairs.
-     * @param pairs list of node pairs
-     */
-    public void add(Collection<NodePair> pairs) {
-        for (NodePair pair: pairs) {
-            add(pair);
-        }
-    }
-
     protected Set<Node> getTerminalNodes() {
         return getNodes().stream().filter(this::isTerminalNode).collect(Collectors.toCollection(LinkedHashSet::new));
     }
@@ -229 +304 @@
         return Optional.ofNullable(successors.get(node)).orElseGet(Collections::emptyList);
     }
 
-    protected Set<Node> getNodes() {
-        Set<Node> nodes = new LinkedHashSet<>(2 * edges.size());
-        for (NodePair pair: edges) {
-            nodes.add(pair.getA());
-            nodes.add(pair.getB());
-        }
-        return nodes;
-    }
-
     protected boolean isSpanningWay(Collection<NodePair> way) {
-        return numUndirectedEges == way.size();
+        return numUndirectedEdges == way.size();
     }
 
     protected List<Node> buildPathFromNodePairs(Deque<NodePair> path) {
@@ -248 +314 @@
     }
 
     /**
-     * Tries to find a spanning path starting from node <code>startNode</code>.
-     *
+     * Tries to find a spanning path starting from node {@code startNode}.
+     * <p>
      * Traverses the path in depth-first order.
-     *
      * @param startNode the start node
      * @return the spanning path; empty list if no path is found
      */
@@ -283 +348 @@
      * the segment of a way) exactly once.
      * <p><b>Note that duplicated edges are removed first!</b>
      *
-     * @return the path; null, if no path was found
+     * @return the path; {@code null}, if no path was found
      */
     public List<Node> buildSpanningPath() {
         prepare();
-        if (numUndirectedEges > 0 && isConnected()) {
+        if (numUndirectedEdges > 0 && isConnected()) {
             // try to find a path from each "terminal node", i.e. from a
             // node which is connected by exactly one undirected edges (or
             // two directed edges in opposite direction) to the graph. A
@@ -324 +389 @@
 
     /**
      * Find out if the graph is connected.
-     * @return true if it is connected.
+     * @return {@code true} if it is connected
      */
     private boolean isConnected() {
         Set<Node> nodes = getNodes();
@@ -350 +415 @@
 
     /**
      * Sort the nodes by number of appearances in the edges.
-     * @return set of nodes which can be start nodes in a spanning way.
+     * @return set of nodes which can be start nodes in a spanning way
      */
     private Set<Node> getMostFrequentVisitedNodesFirst() {
         if (edges.isEmpty())

new file src/org/openstreetmap/josm/data/validation/tests/CycleDetector.java

@@ +1 @@
+// License: GPL. For details, see LICENSE file.
+package org.openstreetmap.josm.data.validation.tests;
+
+import static org.openstreetmap.josm.tools.I18n.tr;
+
+import java.util.ArrayDeque;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Deque;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import org.openstreetmap.josm.data.osm.Node;
+import org.openstreetmap.josm.data.osm.NodeGraph;
+import org.openstreetmap.josm.data.osm.OsmPrimitive;
+import org.openstreetmap.josm.data.osm.Way;
+import org.openstreetmap.josm.data.validation.Severity;
+import org.openstreetmap.josm.data.validation.Test;
+import org.openstreetmap.josm.data.validation.TestError;
+
+/**
+ * Test for detecting <a href="https://en.wikipedia.org/wiki/Cycle_(graph_theory)">cycles</a> in a directed graph,
+ * currently used for waterways only. The graph consists of OSM dataset ways labeled as waterway.
+ *
+ * @author gaben
+ * @since xxx
+ */
+public class CycleDetector extends Test {
+    public static final int CYCLE_DETECTED = 4100;
+
+    /**
+     * All waterways for cycle detection.
+     */
+    private final Set<Way> usableWaterways = new HashSet<>();
+
+    private final Set<Way> visitedWays = new HashSet<>();
+
+    /**
+     * Currently used directional waterways from the OSM wiki
+     */
+    private static final Set<String> directionalWaterways = new HashSet<>(
+            Arrays.asList("river", "stream", "tidal_channel", "drain", "ditch", "fish_pass", "fairway"));
+
+    public CycleDetector() {
+        super(tr("Loop detector"), tr("Detects loops in connected, directional waterways."));
+    }
+
+    @Override
+    public boolean isPrimitiveUsable(OsmPrimitive p) {
+        return p.isUsable() && (p instanceof Way) && ((Way) p).getNodesCount() > 1 && p.hasTag("waterway", directionalWaterways);
+    }
+
+    @Override
+    public void visit(Way w) {
+        if (isPrimitiveUsable(w))
+            usableWaterways.add(w);
+    }
+
+    @Override
+    public void endTest() {
+        Collection<Collection<Way>> graphs = getGraphs();
+
+        for (Collection<Way> graph : graphs) {
+            NodeGraph nodeGraph = NodeGraph.createDirectedGraphFromWays(graph);
+            Tarjan tarjan = new Tarjan(nodeGraph);
+            Collection<Collection<Node>> scc = tarjan.getSCC();
+            for (Collection<Node> possibleCycle : scc) {
+                if (possibleCycle.size() > 1) {
+                    errors.add(
+                            TestError.builder(this, Severity.ERROR, CYCLE_DETECTED)
+                                    .message(tr("Cycle in directional waterway network"))
+                                    .primitives(possibleCycle)
+                                    .build()
+                    );
+                }
+            }
+        }
+
+        super.endTest();
+    }
+
+    @Override
+    public void clear() {
+        super.clear();
+        usableWaterways.clear();
+        visitedWays.clear();
+    }
+
+    /**
+     * Returns all directional waterways which connect to at least one other usable way.
+     *
+     * @return all directional waterways which connect to at least one other usable way
+     */
+    private Collection<Collection<Way>> getGraphs() {
+        // HashSet doesn't make a difference here
+        Collection<Collection<Way>> graphs = new ArrayList<>();
+
+        for (Way waterway : usableWaterways) {
+            Collection<Way> graph = buildGraph(waterway);
+
+            if (!graph.isEmpty())
+                graphs.add(graph);
+        }
+
+        return graphs;
+    }
+
+    /**
+     * Returns a collection of ways which belongs to the same graph.
+     *
+     * @param way starting way to extend the graph from
+     * @return a collection of ways which belongs to the same graph
+     */
+    private Collection<Way> buildGraph(Way way) {
+        if (visitedWays.contains(way))
+            return Collections.emptySet();
+
+        final Set<Way> graph = new HashSet<>();
+
+        for (Node node : way.getNodes()) {
+            Collection<Way> referrers = node.referrers(Way.class)
+                    .filter(this::isPrimitiveUsable)
+                    .filter(candidate -> candidate != way)
+                    .collect(Collectors.toSet());
+
+            if (!referrers.isEmpty()) {
+                visitedWays.add(way);
+                for (Way referrer : referrers) {
+                    graph.addAll(buildGraph(referrer));
+                }
+                graph.addAll(referrers);
+            }
+        }
+        return graph;
+    }
+
+    /**
+     * Tarjan's algorithm implementation for JOSM.
+     *
+     * @see <a href="https://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_algorithm">
+     * Tarjan's strongly connected components algorithm</a>
+     */
+    public static final class Tarjan {
+
+        /**
+         * A simple key-value registry to remember visited nodes and its metadata. Key is used for storing the unique
+         * ID of the primitives instead of the full primitives to save space.
+         */
+        private final Map<Long, TarjanHelper> registry;
+        private final Collection<Collection<Node>> scc = new HashSet<>();
+        private final Deque<Node> stack = new ArrayDeque<>();
+        private final NodeGraph graph;
+        private final Map<Node, Set<Node>> graphMap;
+        private int index = 0;
+
+        public Tarjan(NodeGraph graph) {
+            this.graph = graph;
+
+            graphMap = this.graph.createMap();
+
+            this.registry = new HashMap<>((int) (this.graph.getEdges().size() / 0.75) + 1);
+        }
+
+        /**
+         * Returns the strongly connected components in the current graph.
+         *
+         * @return the strongly connected components in the current graph
+         */
+        public Collection<Collection<Node>> getSCC() {
+            for (Node node : graph.getNodes()) {
+                if (!registry.containsKey(node.getUniqueId())) {
+                    strongConnect(node);
+                }
+            }
+            return scc;
+        }
+
+        /**
+         * Calculates strongly connected components available from the given node.
+         *
+         * @param v the node to generate strongly connected components from
+         */
+        private void strongConnect(Node v) {
+            registry.put(v.getUniqueId(), new TarjanHelper(index, index));
+            index++;
+            stack.push(v);
+
+            for (Node w : getSuccessors(v)) {
+                if (!registry.containsKey(w.getUniqueId())) {
+                    strongConnect(w);
+                    TarjanHelper vHelper = registry.get(v.getUniqueId());
+                    TarjanHelper wHelper = registry.get(w.getUniqueId());
+                    vHelper.lowlink = Math.min(vHelper.lowlink, wHelper.lowlink);
+                } else if (stack.contains(w)) {
+                    TarjanHelper vHelper = registry.get(v.getUniqueId());
+                    TarjanHelper wHelper = registry.get(w.getUniqueId());
+                    vHelper.lowlink = Math.min(vHelper.lowlink, wHelper.index);
+                }
+            }
+
+            final TarjanHelper vHelper = registry.get(v.getUniqueId());
+            if (vHelper.lowlink == vHelper.index) {
+                Collection<Node> currentSCC = new HashSet<>();
+                Node w;
+                do {
+                    w = stack.remove();
+                    currentSCC.add(w);
+                } while (!w.equals(v));
+                scc.add(currentSCC);
+            }
+        }
+
+        /**
+         * Returns the next direct successors from the graph of the given node.
+         *
+         * @param node a node to start search from
+         * @return a collection of nodes from the graph which are direct neighbors of the given node
+         */
+        private Set<Node> getSuccessors(Node node) {
+            return graphMap.getOrDefault(node, Collections.emptySet());
+        }
+
+        /**
+         * Helper class for storing the Tarjan algorithm runtime metadata.
+         */
+        private static final class TarjanHelper {
+            private final int index;
+            private int lowlink;
+
+            private TarjanHelper(int index, int lowlink) {
+                this.index = index;
+                this.lowlink = lowlink;
+            }
+        }
+    }
+}

src/org/openstreetmap/josm/data/validation/OsmValidator.java

@@ -53 +53 @@
 import org.openstreetmap.josm.data.validation.tests.InternetTags;
 import org.openstreetmap.josm.data.validation.tests.Lanes;
 import org.openstreetmap.josm.data.validation.tests.LongSegment;
+import org.openstreetmap.josm.data.validation.tests.CycleDetector;
 import org.openstreetmap.josm.data.validation.tests.MapCSSTagChecker;
 import org.openstreetmap.josm.data.validation.tests.MultipolygonTest;
 import org.openstreetmap.josm.data.validation.tests.NameMismatch;
@@ -154 +155 @@
         SharpAngles.class, // 3800 .. 3899
         ConnectivityRelations.class, // 3900 .. 3999
         DirectionNodes.class, // 4000-4099
+        CycleDetector.class, // 4100-4199
     };
 
     /**
@@ -607 +609 @@
     }
 
     /**
-     * Initialize grid details based on current projection system. Values based on
+     * Initialize grid details based on the current projection system. Values based on
      * the original value fixed for EPSG:4326 (10000) using heuristics (that is, test&error
      * until most bugs were discovered while keeping the processing time reasonable)
      */
@@ -636 +638 @@
     private static boolean testsInitialized;
 
     /**
-     * Initializes all tests if this operations hasn't been performed already.
+     * Initializes all tests if this operation hasn't been performed already.
      */
     public static synchronized void initializeTests() {
         if (!testsInitialized) {

new file test/data/CycleDetector_test_wikipedia.osm

@@ +1 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<osm version='0.6' upload='never' generator='JOSM'>
+  <node id='-137726' action='modify' visible='true' lat='47.74161657891' lon='17.37769604149' />
+  <node id='-137727' action='modify' visible='true' lat='47.74160961975' lon='17.37612305842' />
+  <node id='-137728' action='modify' visible='true' lat='47.74043350867' lon='17.37611270985' />
+  <node id='-137731' action='modify' visible='true' lat='47.74043350867' lon='17.37771673864' />
+  <node id='-137732' action='modify' visible='true' lat='47.74071883984' lon='17.37897926452' />
+  <node id='-137733' action='modify' visible='true' lat='47.74045438662' lon='17.38021074469' />
+  <node id='-137734' action='modify' visible='true' lat='47.74011337916' lon='17.37895856738' />
+  <node id='-137735' action='modify' visible='true' lat='47.74163049723' lon='17.38024179041' />
+  <node id='-137736' action='modify' visible='true' lat='47.74119902778' lon='17.38124560197' />
+  <node id='-137737' action='modify' visible='true' lat='47.74161657891' lon='17.38222871639' />
+  <node id='-137738' action='modify' visible='true' lat='47.7420091937' lon='17.38123761625' />
+  <node id='-137746' action='modify' visible='true' lat='47.74044046799' lon='17.38222871639' />
+  <node id='-137759' action='modify' visible='true' lat='47.73993243552' lon='17.38222871639' />
+  <node id='-137760' action='modify' visible='true' lat='47.73994635429' lon='17.38319113367' />
+  <node id='-137761' action='modify' visible='true' lat='47.74046134593' lon='17.38319113367' />
+  <way id='-103300' action='modify' visible='true'>
+    <nd ref='-137726' />
+    <nd ref='-137727' />
+    <tag k='waterway' v='ditch' />
+  </way>
+  <way id='-103301' action='modify' visible='true'>
+    <nd ref='-137727' />
+    <nd ref='-137728' />
+    <tag k='waterway' v='ditch' />
+  </way>
+  <way id='-103302' action='modify' visible='true'>
+    <nd ref='-137728' />
+    <nd ref='-137726' />
+    <tag k='waterway' v='ditch' />
+  </way>
+  <way id='-103305' action='modify' visible='true'>
+    <nd ref='-137731' />
+    <nd ref='-137728' />
+    <tag k='waterway' v='ditch' />
+  </way>
+  <way id='-103306' action='modify' visible='true'>
+    <nd ref='-137731' />
+    <nd ref='-137726' />
+    <tag k='waterway' v='ditch' />
+  </way>
+  <way id='-103307' action='modify' visible='true'>
+    <nd ref='-137733' />
+    <nd ref='-137732' />
+    <nd ref='-137731' />
+    <tag k='waterway' v='ditch' />
+  </way>
+  <way id='-103309' action='modify' visible='true'>
+    <nd ref='-137731' />
+    <nd ref='-137734' />
+    <nd ref='-137733' />
+    <tag k='waterway' v='ditch' />
+  </way>
+  <way id='-103311' action='modify' visible='true'>
+    <nd ref='-137733' />
+    <nd ref='-137735' />
+    <tag k='waterway' v='ditch' />
+  </way>
+  <way id='-103312' action='modify' visible='true'>
+    <nd ref='-137735' />
+    <nd ref='-137726' />
+    <tag k='waterway' v='ditch' />
+  </way>
+  <way id='-103313' action='modify' visible='true'>
+    <nd ref='-137735' />
+    <nd ref='-137736' />
+    <nd ref='-137737' />
+    <tag k='waterway' v='ditch' />
+  </way>
+  <way id='-103315' action='modify' visible='true'>
+    <nd ref='-137737' />
+    <nd ref='-137738' />
+    <nd ref='-137735' />
+    <tag k='waterway' v='ditch' />
+  </way>
+  <way id='-103324' action='modify' visible='true'>
+    <nd ref='-137746' />
+    <nd ref='-137733' />
+    <tag k='waterway' v='ditch' />
+  </way>
+  <way id='-103325' action='modify' visible='true'>
+    <nd ref='-137746' />
+    <nd ref='-137737' />
+    <tag k='waterway' v='ditch' />
+  </way>
+  <way id='-103359' action='modify' visible='true'>
+    <nd ref='-137746' />
+    <nd ref='-137759' />
+    <nd ref='-137760' />
+    <nd ref='-137761' />
+    <nd ref='-137746' />
+    <tag k='waterway' v='ditch' />
+  </way>
+</osm>

new file test/unit/org/openstreetmap/josm/data/validation/tests/CycleDetectorTest.java

@@ +1 @@
+// License: GPL. For details, see LICENSE file.
+package org.openstreetmap.josm.data.validation.tests;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.nio.file.Files;
+import java.nio.file.Paths;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.openstreetmap.josm.TestUtils;
+import org.openstreetmap.josm.data.osm.DataSet;
+import org.openstreetmap.josm.io.OsmReader;
+import org.openstreetmap.josm.testutils.JOSMTestRules;
+
+/**
+ * JUnit test for {@link CycleDetector} validation test.
+ */
+class CycleDetectorTest {
+    /**
+     * Setup test.
+     */
+    @RegisterExtension
+    public JOSMTestRules test = new JOSMTestRules();
+
+    /**
+     * Test all error cases manually created in direction-nodes.osm.
+     *
+     * @throws Exception in case of error
+     */
+    @Test
+    void testCoverage() throws Exception {
+        CycleDetector cycleDetector = new CycleDetector();
+        ValidatorTestUtils.testSampleFile("test/data/CycleDetector_test_wikipedia.osm", DataSet::getWays, null, cycleDetector);
+    }
+
+    @Test
+    void testCycleDetection() throws Exception {
+        CycleDetector cycleDetector = new CycleDetector();
+        DataSet ds = OsmReader.parseDataSet(
+                Files.newInputStream(Paths.get(TestUtils.getTestDataRoot(), "CycleDetector_test_wikipedia.osm")), null);
+        cycleDetector.startTest(null);
+        cycleDetector.visit(ds.allPrimitives());
+        cycleDetector.endTest();
+
+        // we have 4 cycles in the test file
+        assertEquals(4, cycleDetector.getErrors().size());
+    }
+}
+ No newline at end of file

test/unit/org/openstreetmap/josm/data/validation/tests/DirectionNodesTest.java

@@ -8 +8 @@
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 
 /**
- * JUnit Test of Multipolygon validation test.
+ * JUnit Test of {@link DirectionNodes} validation test.
  */
 class DirectionNodesTest {
 
-
     /**
      * Setup test.
      */
@@ -25 +24 @@
      * @throws Exception in case of error
      */
     @Test
-    void testDirectionsNodesTestFile() throws Exception {
+    void testDirectionNodesTestFile() throws Exception {
         final DirectionNodes test = new DirectionNodes();
         ValidatorTestUtils.testSampleFile("nodist/data/direction-nodes.osm", ds -> ds.getNodes(), null, test);
     }