Index: /trunk/src/org/openstreetmap/josm/data/validation/tests/DuplicateNode.java =================================================================== --- /trunk/src/org/openstreetmap/josm/data/validation/tests/DuplicateNode.java (revision 17375) +++ /trunk/src/org/openstreetmap/josm/data/validation/tests/DuplicateNode.java (revision 17376) @@ -8,5 +8,4 @@ import java.util.ArrayList; -import java.util.Collections; import java.util.HashMap; import java.util.Iterator; @@ -18,4 +17,5 @@ import java.util.Set; import java.util.stream.Collectors; +import java.util.stream.Stream; import org.openstreetmap.josm.actions.MergeNodesAction; @@ -25,5 +25,4 @@ import org.openstreetmap.josm.data.osm.Node; import org.openstreetmap.josm.data.osm.OsmPrimitive; -import org.openstreetmap.josm.data.osm.OsmPrimitiveType; import org.openstreetmap.josm.data.osm.Storage; import org.openstreetmap.josm.data.osm.Way; @@ -162,5 +161,5 @@ List errors = new ArrayList<>(); - MultiMap, OsmPrimitive> mm = new MultiMap<>(); + MultiMap, Node> mm = new MultiMap<>(); for (Node n: nodes) { mm.put(n.getKeys(), n); @@ -171,5 +170,5 @@ // check whether we have multiple nodes at the same position with the same tag set for (Iterator> it = mm.keySet().iterator(); it.hasNext();) { - Set primitives = mm.get(it.next()); + Set primitives = mm.get(it.next()); if (primitives.size() > 1) { @@ -178,23 +177,16 @@ } - for (OsmPrimitive p : primitives) { - if (p.getType() == OsmPrimitiveType.NODE) { - Node n = (Node) p; - List lp = n.getReferrers(); - for (OsmPrimitive sp: lp) { - if (sp.getType() == OsmPrimitiveType.WAY) { - boolean typed = false; - Way w = (Way) sp; - Map keys = w.getKeys(); - for (Entry e : typeMap.entrySet()) { - if (keys.containsKey(e.getKey())) { - e.setValue(Boolean.TRUE); - typed = true; - } - } - if (!typed) { - typeMap.put("none", Boolean.TRUE); - } + for (Node n : primitives) { + for (Way w: n.getParentWays()) { + boolean typed = false; + Map keys = w.getKeys(); + for (Entry e : typeMap.entrySet()) { + if (keys.containsKey(e.getKey())) { + e.setValue(Boolean.TRUE); + typed = true; } + } + if (!typed) { + typeMap.put("none", Boolean.TRUE); } } @@ -202,56 +194,37 @@ long nbType = typeMap.entrySet().stream().filter(Entry::getValue).count(); - + final TestError.Builder builder; if (nbType > 1) { - errors.add(TestError.builder(parentTest, Severity.WARNING, DUPLICATE_NODE_MIXED) - .message(tr("Mixed type duplicated nodes")) - .primitives(primitives) - .build()); - } else if (typeMap.get(HIGHWAY)) { - errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_HIGHWAY) - .message(tr("Highway duplicated nodes")) - .primitives(primitives) - .build()); - } else if (typeMap.get(RAILWAY)) { - errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_RAILWAY) - .message(tr("Railway duplicated nodes")) - .primitives(primitives) - .build()); - } else if (typeMap.get(WATERWAY)) { - errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_WATERWAY) - .message(tr("Waterway duplicated nodes")) - .primitives(primitives) - .build()); - } else if (typeMap.get("boundary")) { - errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_BOUNDARY) - .message(tr("Boundary duplicated nodes")) - .primitives(primitives) - .build()); - } else if (typeMap.get("power")) { - errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_POWER) - .message(tr("Power duplicated nodes")) - .primitives(primitives) - .build()); - } else if (typeMap.get("natural")) { - errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_NATURAL) - .message(tr("Natural duplicated nodes")) - .primitives(primitives) - .build()); - } else if (typeMap.get("building")) { - errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_BUILDING) - .message(tr("Building duplicated nodes")) - .primitives(primitives) - .build()); - } else if (typeMap.get("landuse")) { - errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_LANDUSE) - .message(tr("Landuse duplicated nodes")) - .primitives(primitives) - .build()); + builder = TestError.builder(parentTest, Severity.WARNING, DUPLICATE_NODE_MIXED) + .message(tr("Mixed type duplicated nodes")); + } else if (Boolean.TRUE.equals(typeMap.get(HIGHWAY))) { + builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_HIGHWAY) + .message(tr("Highway duplicated nodes")); + } else if (Boolean.TRUE.equals(typeMap.get(RAILWAY))) { + builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_RAILWAY) + .message(tr("Railway duplicated nodes")); + } else if (Boolean.TRUE.equals(typeMap.get(WATERWAY))) { + builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_WATERWAY) + .message(tr("Waterway duplicated nodes")); + } else if (Boolean.TRUE.equals(typeMap.get("boundary"))) { + builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_BOUNDARY) + .message(tr("Boundary duplicated nodes")); + } else if (Boolean.TRUE.equals(typeMap.get("power"))) { + builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_POWER) + .message(tr("Power duplicated nodes")); + } else if (Boolean.TRUE.equals(typeMap.get("natural"))) { + builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_NATURAL) + .message(tr("Natural duplicated nodes")); + } else if (Boolean.TRUE.equals(typeMap.get("building"))) { + builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_BUILDING) + .message(tr("Building duplicated nodes")); + } else if (Boolean.TRUE.equals(typeMap.get("landuse"))) { + builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_LANDUSE) + .message(tr("Landuse duplicated nodes")); } else { - errors.add(TestError.builder(parentTest, Severity.WARNING, DUPLICATE_NODE_OTHER) - .message(tr("Other duplicated nodes")) - .primitives(primitives) - .build()); + builder = TestError.builder(parentTest, Severity.WARNING, DUPLICATE_NODE_OTHER) + .message(tr("Other duplicated nodes")); } + errors.add(builder.primitives(primitives).build()); it.remove(); } @@ -261,5 +234,5 @@ if (!mm.isEmpty()) { List duplicates = new ArrayList<>(); - for (Set l: mm.values()) { + for (Set l: mm.values()) { duplicates.addAll(l); } @@ -278,23 +251,19 @@ public void visit(Node n) { if (n.isUsable()) { - if (potentialDuplicates.get(n) == null) { + Object old = potentialDuplicates.get(n); + if (old == null) { // in most cases there is just one node at a given position. We // avoid to create an extra object and add remember the node // itself at this position potentialDuplicates.put(n); - } else if (potentialDuplicates.get(n) instanceof Node) { + } else if (old instanceof Node) { // we have an additional node at the same position. Create an extra // object to keep track of the nodes at this position. // - Node n1 = (Node) potentialDuplicates.get(n); - List nodes = new ArrayList<>(2); - nodes.add(n1); - nodes.add(n); - potentialDuplicates.put(nodes); - } else if (potentialDuplicates.get(n) instanceof List) { - // we have multiple nodes at the same position. + potentialDuplicates.put(Stream.of((Node) old, n).collect(Collectors.toList())); + } else { + // we have more than two nodes at the same position. // - List nodes = (List) potentialDuplicates.get(n); - nodes.add(n); + ((List) old).add(n); } } @@ -307,4 +276,6 @@ @Override public Command fixError(TestError testError) { + if (!isFixable(testError)) + return null; final Set nodes = testError.primitives(Node.class) // Filter nodes that have already been deleted (see #5764 and #5773) @@ -312,17 +283,11 @@ .collect(Collectors.toCollection(LinkedHashSet::new)); - // Merge only if at least 2 nodes remain - if (nodes.size() >= 2) { - // Use first existing node or first node if all nodes are new - Node target = nodes.stream() - .filter(n -> !n.isNew()) - .findFirst() - .orElseGet(() -> nodes.iterator().next()); - - if (Command.checkOutlyingOrIncompleteOperation(nodes, Collections.singleton(target)) == Command.IS_OK) - return MergeNodesAction.mergeNodes(nodes, target); - } - - return null; // undoRedo handling done in mergeNodes + // Use first existing node or first node if all nodes are new + Node target = nodes.stream() + .filter(n -> !n.isNew()) + .findFirst() + .orElseGet(() -> nodes.iterator().next()); + + return MergeNodesAction.mergeNodes(nodes, target); } Index: /trunk/test/unit/org/openstreetmap/josm/data/validation/tests/DuplicateNodeTest.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/data/validation/tests/DuplicateNodeTest.java (revision 17375) +++ /trunk/test/unit/org/openstreetmap/josm/data/validation/tests/DuplicateNodeTest.java (revision 17376) @@ -3,7 +3,11 @@ import static org.junit.jupiter.api.Assertions.assertEquals; - +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; -import org.junit.jupiter.api.Test; +import org.openstreetmap.josm.command.Command; import org.openstreetmap.josm.data.coor.LatLon; import org.openstreetmap.josm.data.osm.DataSet; @@ -44,4 +48,19 @@ assertEquals(code, error.getCode()); assertEquals(fixable, error.isFixable()); + if (fixable) { + Command c = error.getFix(); + assertNotNull(c); + c.executeCommand(); + assertFalse(error.isFixable()); + c.undoCommand(); + assertTrue(error.isFixable()); + error.getPrimitives().iterator().next().setDeleted(true); + if (error.getPrimitives().size() == 2) { + assertFalse(error.isFixable()); + } else { + assertTrue(error.isFixable()); + } + error.getPrimitives().iterator().next().setDeleted(false); + } } @@ -67,4 +86,78 @@ /** + * Test of "Duplicate node" validation test - no duplicate + */ + @Test + void testNoDuplicateNode() { + DataSet ds = new DataSet(); + + Node a = new Node(new LatLon(10.0, 5.0)); + Node b = new Node(new LatLon(10.0, 6.0)); + ds.addPrimitive(a); + ds.addPrimitive(b); + + a.put("foo", "bar"); + b.put("bar", "foo"); + + TEST.startTest(NullProgressMonitor.INSTANCE); + TEST.visit(ds.allPrimitives()); + TEST.endTest(); + + assertEquals(0, TEST.getErrors().size()); + } + + /** + * Test of "Duplicate node" validation test - same position, with ele value + */ + @Test + void testDuplicateNodeWithEle() { + DataSet ds = new DataSet(); + + Node a = new Node(new LatLon(10.0, 5.0)); + Node b = new Node(new LatLon(10.0, 5.0)); + ds.addPrimitive(a); + ds.addPrimitive(b); + + a.put("foo", "bar"); + b.put("bar", "foo"); + a.put("ele", "100"); + b.put("ele", "100"); + + TEST.startTest(NullProgressMonitor.INSTANCE); + TEST.visit(ds.allPrimitives()); + TEST.endTest(); + + assertEquals(1, TEST.getErrors().size()); + + b.put("ele", "110"); + + TEST.startTest(NullProgressMonitor.INSTANCE); + TEST.visit(ds.allPrimitives()); + TEST.endTest(); + + assertEquals(0, TEST.getErrors().size()); + } + + /** + * Test of "Duplicate node" validation test - three nodes + */ + @Test + void testDuplicateNodeTriple() { + DataSet ds = new DataSet(); + + Node a = new Node(new LatLon(10.0, 5.0)); + Node b = new Node(new LatLon(10.0, 5.0)); + Node c = new Node(new LatLon(10.0, 5.0)); + ds.addPrimitive(a); + ds.addPrimitive(b); + ds.addPrimitive(c); + + performTest(DuplicateNode.DUPLICATE_NODE_OTHER, ds, true); + a.put("foo", "bar"); + b.put("foo", "bar"); + performTest(DuplicateNode.DUPLICATE_NODE_OTHER, ds, true); + } + + /** * Test of "Duplicate node" validation test - different tag sets. */