Ticket #21881: josm21881_cycle_dector_v2.patch

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.List;
+import java.util.Map;
+import java.util.Queue;
+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.NodePair;
+import org.openstreetmap.josm.data.osm.OsmPrimitive;
+import org.openstreetmap.josm.data.osm.Way;
+import org.openstreetmap.josm.data.osm.WaySegment;
+import org.openstreetmap.josm.data.preferences.sources.ValidatorPrefHelper;
+import org.openstreetmap.josm.data.validation.Severity;
+import org.openstreetmap.josm.data.validation.Test;
+import org.openstreetmap.josm.data.validation.TestError;
+import org.openstreetmap.josm.gui.progress.ProgressMonitor;
+import org.openstreetmap.josm.spi.preferences.Config;
+import org.openstreetmap.josm.tools.Pair;
+
+/**
+ * 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 processed graph consists of ways labeled as waterway.
+ *
+ * @author gaben
+ * @since xxx
+ */
+public class CycleDetector extends Test {
+    public static final int CYCLE_DETECTED = 4200;
+
+    /** All waterways for cycle detection */
+    private final Set<Way> usableWaterways = new HashSet<>();
+
+    /** Already visited primitives */
+    private final Set<Way> visitedWays = new HashSet<>();
+
+    /** Currently used directional waterways from the OSM wiki */
+    private static List<String> directionalWaterways;
+
+    protected static final String PREFIX = ValidatorPrefHelper.PREFIX + "." + CycleDetector.class.getSimpleName();
+
+    public CycleDetector() {
+        super(tr("Cycle detector"), tr("Detects cycles in drainage systems."));
+    }
+
+    @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 startTest(ProgressMonitor progressMonitor) {
+        super.startTest(progressMonitor);
+        directionalWaterways = Config.getPref().getList(PREFIX + ".directionalWaterways",
+            Arrays.asList("river", "stream", "tidal_channel", "drain", "ditch", "fish_pass", "fairway"));
+    }
+
+    @Override
+    public void endTest() {
+        for (Collection<Way> graph : getGraphs()) {
+            NodeGraph nodeGraph = NodeGraph.createDirectedGraphFromWays(graph);
+            Tarjan tarjan = new Tarjan(nodeGraph);
+            Collection<List<Node>> scc = tarjan.getSCC();
+            Map<Node, List<Node>> graphMap = tarjan.getGraphMap();
+
+            for (Collection<Node> possibleCycle : scc) {
+                // there is a cycle in the graph if a strongly connected component has more than one node
+                if (possibleCycle.size() > 1) {
+                    errors.add(
+                        TestError.builder(this, Severity.ERROR, CYCLE_DETECTED)
+                            .message(tr("Cycle in directional waterway network"))
+                            .primitives(possibleCycle)
+                            .highlightWaySegments(createSegments(graphMap, possibleCycle))
+                            .build()
+                    );
+                }
+            }
+        }
+
+        super.endTest();
+    }
+
+    @Override
+    public void clear() {
+        super.clear();
+        usableWaterways.clear();
+        visitedWays.clear();
+    }
+
+    /**
+     * Creates WaySegments from Nodes for the error highlight function.
+     * @param graphMap the complete graph data
+     * @param nodes nodes to build the way segments from
+     * @return WaySegments from the Nodes
+     */
+    private static Collection<WaySegment> createSegments(Map<Node, List<Node>> graphMap, Collection<Node> nodes) {
+        List<NodePair> pairs = new ArrayList<>();
+
+        // build new graph exclusively from SCC nodes
+        for (Node node : nodes) {
+            for (Node successor : graphMap.get(node)) {
+                // check for outbound nodes
+                if (nodes.contains(successor)) {
+                    pairs.add(new NodePair(node, successor));
+                }
+            }
+        }
+
+        Collection<WaySegment> segments = new ArrayList<>();
+
+        for (NodePair pair : pairs) {
+            final Node n = pair.getA();
+            final Node m = pair.getB();
+
+            if (n != null && m != null && !n.equals(m)) {
+                List<Way> intersect = new ArrayList<>(n.getParentWays());
+                List<Way> mWays = m.getParentWays();
+                intersect.retainAll(mWays);
+
+                for (Way w : intersect) {
+                    if (w.getNeighbours(n).contains(m) && getNodeIndex(w, n) + 1 == getNodeIndex(w, m)) {
+                        segments.add(WaySegment.forNodePair(w, n, m));
+                    }
+                }
+            }
+        }
+
+        return segments;
+    }
+
+    /**
+     * Returns the way index of a node. Only the first occurrence is considered in case it's a closed way.
+     * @param w parent way
+     * @param n the node to look up
+     * @return >=0 if the node is found or<br>-1 if node not part of the way
+     */
+    private static int getNodeIndex(Way w, Node n) {
+        for (int i = 0; i < w.getNodesCount(); i++) {
+            if (w.getNode(i).equals(n)) {
+                return i;
+            }
+        }
+
+        return -1;
+    }
+
+    /**
+     * 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<>();
+        Queue<Way> queue = new ArrayDeque<>();
+        queue.offer(way);
+
+        while (!queue.isEmpty()) {
+            Way currentWay = queue.poll();
+            visitedWays.add(currentWay);
+
+            for (Node node : currentWay.getNodes()) {
+                Collection<Way> referrers = node.referrers(Way.class)
+                    .filter(this::isPrimitiveUsable)
+                    .filter(candidate -> candidate != currentWay)
+                    .collect(Collectors.toSet());
+
+                if (!referrers.isEmpty()) {
+                    for (Way referrer : referrers) {
+                        if (!visitedWays.contains(referrer)) {
+                            queue.offer(referrer);
+                            visitedWays.add(referrer);
+                        }
+                    }
+                    graph.addAll(referrers);
+                }
+            }
+        }
+        return graph;
+    }
+
+
+
+    /**
+     * Tarjan's strongly connected components algorithm 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 {
+
+        /**
+         * Used to remember visited nodes and its metadata. Key is used for storing
+         * the unique ID of the nodes instead of the full data to save space.
+         */
+        private final Map<Long, TarjanHelper> registry;
+
+        /** Used to store the graph data as a map. */
+        private final Map<Node, List<Node>> graphMap;
+
+        /** Used to store strongly connected components. */
+        private final Collection<List<Node>> scc = new HashSet<>();
+
+        /** Used on algorithm runtime to keep track discovery progress. */
+        private final Deque<Node> stack = new ArrayDeque<>();
+
+        /** Used on algorithm runtime to keep track discovery progress. */
+        private int index = 0;
+
+        public Tarjan(NodeGraph graph) {
+            graphMap = graph.createMap();
+
+            this.registry = new HashMap<>((int) (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<List<Node>> getSCC() {
+            for (Node node : graphMap.keySet()) {
+                if (!registry.containsKey(node.getUniqueId())) {
+                    strongConnect(node);
+                }
+            }
+            return scc;
+        }
+
+        /**
+         * Returns the graph data as a map.
+         * @see NodeGraph#createMap()
+         * @return the graph data as a map
+         */
+        public Map<Node, List<Node>> getGraphMap() {
+            return graphMap;
+        }
+
+        /**
+         * Calculates strongly connected components available from the given node, in an iterative fashion.
+         *
+         * @param u0 the node to generate strongly connected components from
+         */
+        private void strongConnect(final Node u0) {
+            final Deque<Pair<Node, Integer>> work = new ArrayDeque<>();
+            work.push(new Pair<>(u0, 0));
+            boolean recurse;
+
+            while (!work.isEmpty()) {
+                Pair<Node, Integer> popped = work.remove();
+                Node u = popped.a;
+                int j = popped.b;
+
+                if (j == 0) {
+                    index++;
+                    registry.put(u.getUniqueId(), new TarjanHelper(index));
+                    stack.push(u);
+                }
+
+                recurse = false;
+                List<Node> successors = getSuccessors(u);
+
+                for (int i = j; i < successors.size(); i++) {
+                    Node v = successors.get(i);
+                    if (!registry.containsKey(v.getUniqueId())) {
+                        work.push(new Pair<>(u, i + 1));
+                        work.push(new Pair<>(v, 0));
+                        recurse = true;
+                        break;
+                    } else if (stack.contains(v)) {
+                        TarjanHelper uHelper = registry.get(u.getUniqueId());
+                        TarjanHelper vHelper = registry.get(v.getUniqueId());
+                        uHelper.lowlink = Math.min(uHelper.lowlink, vHelper.index);
+                    }
+                }
+
+                if (!recurse) {
+                    TarjanHelper uHelper = registry.get(u.getUniqueId());
+                    if (uHelper.lowlink == uHelper.index) {
+                        List<Node> currentSCC = new ArrayList<>();
+                        Node v;
+                        do {
+                            v = stack.remove();
+                            currentSCC.add(v);
+                        } while (!v.equals(u));
+                        scc.add(currentSCC);
+                    }
+                    if (!work.isEmpty()) {
+                        Node v = u;
+                        Pair<Node, Integer> peeked = work.peek();
+                        u = peeked.a;
+                        TarjanHelper vHelper = registry.get(v.getUniqueId());
+                        uHelper = registry.get(u.getUniqueId());
+                        uHelper.lowlink = Math.min(uHelper.lowlink, vHelper.lowlink);
+                    }
+                }
+            }
+        }
+
+        /**
+         * Returns the next direct successors from the graph of the given node.
+         *
+         * @param node a node to start search from
+         * @return direct successors of the node or an empty list, if it's a terminal node
+         */
+        private List<Node> getSuccessors(Node node) {
+            return graphMap.getOrDefault(node, Collections.emptyList());
+        }
+
+        /**
+         * Helper class for storing the Tarjan algorithm runtime metadata.
+         */
+        private static class TarjanHelper {
+            private final int index;
+            private int lowlink;
+
+            private TarjanHelper(int index) {
+                this.index = index;
+                this.lowlink = index;
+            }
+        }
+    }
+}

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()
+    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

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;
+
+    /**
+     * 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 node
+     * @since xxx
+     */
+    public Map<Node, List<Node>> createMap() {
+        final Map<Node, List<Node>> result = new HashMap<>((int) (edges.size() / 0.75) + 1);
 
+        for (NodePair edge : edges) {
+            result.computeIfAbsent(edge.getA(), k -> new ArrayList<>()).add(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 +235 @@
         }
     }
 
+    /**
+     * See {@link #prepare()}
+     */
     protected void rememberPredecessors(NodePair pair) {
         List<NodePair> l = predecessors.computeIfAbsent(pair.getB(), k -> new ArrayList<>());
         if (!l.contains(pair)) {
@@ -157 +245 @@
         }
     }
 
+    /**
+     * 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 +268 @@
         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 +301 @@
         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 +311 @@
     }
 
     /**
-     * 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
      */
@@ -259 +321 @@
         if (startNode != null) {
             Deque<NodePair> path = new ArrayDeque<>();
             Set<NodePair> dupCheck = new HashSet<>();
-            Deque<NodePair> nextPairs = new ArrayDeque<>();
-            nextPairs.addAll(getOutboundPairs(startNode));
+            Deque<NodePair> nextPairs = new ArrayDeque<>(getOutboundPairs(startNode));
             while (!nextPairs.isEmpty()) {
                 NodePair cur = nextPairs.removeLast();
                 if (!dupCheck.contains(cur) && !dupCheck.contains(cur.swap())) {
@@ -280 +341 @@
 
     /**
      * Tries to find a path through the graph which visits each edge (i.e.
-     * the segment of a way) exactly once.
-     * <p><b>Note that duplicated edges are removed first!</b>
+     * 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()) {
-            // 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
+        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 edge (or
+            // two directed edges in the opposite direction) to the graph. A
             // graph built up from way segments is likely to include such
             // nodes, unless the edges build one or more closed rings.
             // We order the nodes to start with the best candidates, but
@@ -324 +385 @@
 
     /**
      * 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 +411 @@
 
     /**
      * 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())
             return Collections.emptySet();
-        // count appearance of nodes in edges
+        // count the appearance of nodes in edges
         Map<Node, Integer> counters = new HashMap<>();
         for (NodePair pair : edges) {
             Integer c = counters.get(pair.getA());

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

@@ -44 +44 @@
 import org.openstreetmap.josm.data.validation.tests.ConditionalKeys;
 import org.openstreetmap.josm.data.validation.tests.ConnectivityRelations;
 import org.openstreetmap.josm.data.validation.tests.CrossingWays;
+import org.openstreetmap.josm.data.validation.tests.CycleDetector;
 import org.openstreetmap.josm.data.validation.tests.DirectionNodes;
 import org.openstreetmap.josm.data.validation.tests.DuplicateNode;
 import org.openstreetmap.josm.data.validation.tests.DuplicateRelation;
@@ -154 +155 @@
         // 3700 .. 3799 is automatically removed since it clashed with pt_assistant.
         SharpAngles.class, // 3800 .. 3899
         ConnectivityRelations.class, // 3900 .. 3999
-        DirectionNodes.class, // 4000-4099
+        DirectionNodes.class, // 4000 .. 4099
         RightAngleBuildingTest.class, // 4100 .. 4199
+        CycleDetector.class, // 4200 .. 4299
     };
 
     /**