Index: /trunk/src/org/openstreetmap/josm/data/validation/tests/PowerLines.java =================================================================== --- /trunk/src/org/openstreetmap/josm/data/validation/tests/PowerLines.java (revision 19618) +++ /trunk/src/org/openstreetmap/josm/data/validation/tests/PowerLines.java (revision 19619) @@ -12,7 +12,10 @@ import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Map.Entry; import java.util.Set; +import java.util.stream.Collectors; import org.openstreetmap.josm.data.coor.ILatLon; @@ -76,6 +79,6 @@ private double hillyCompensation; private double hillyThreshold; - private final Set badConnections = new HashSet<>(); - private final Set missingTags = new HashSet<>(); + private final Map> badConnections = new HashMap<>(); + private final Map> missingTags = new HashMap<>(); private final Set wrongLineType = new HashSet<>(); private final Set missingNodes = new HashSet<>(); @@ -99,14 +102,13 @@ @Override public void visit(Node n) { - boolean nodeInLineOrCable = false; - boolean connectedToUnrelated = false; - for (Way parent : n.getParentWays()) { - if (parent.hasTag(POWER, "line", MINOR_LINE, "cable")) - nodeInLineOrCable = true; - else if (!isRelatedToPower(parent)) - connectedToUnrelated = true; - } - if (nodeInLineOrCable && connectedToUnrelated) - badConnections.add(n); + if (!n.isConnectionNode() || n.referrers(Way.class).noneMatch(w -> isPowerLineOrCable(w))) + return; + + List unrelatedParents = n.referrers(Way.class).filter(w -> !isPowerLineOrCable(w) && !isRelatedToPower(w)) + .collect(Collectors.toList()); + if (!unrelatedParents.isEmpty()) { + Set set = badConnections.computeIfAbsent(n, k -> new HashSet<>()); + set.addAll(unrelatedParents); + } } @@ -163,19 +165,22 @@ } // Then return the errors - for (Node n : missingTags) { + for (Entry> entry : missingTags.entrySet()) { + Node n = entry.getKey(); if (!isInPowerStation(n)) { errors.add(TestError.builder(this, Severity.WARNING, POWER_SUPPORT) // the "missing tag" grouping can become broken if the MapCSS message get reworded .message(tr("missing tag"), tr("node without power=*")) - .primitives(n) + .primitives(getAllPrimitives(entry)) + .highlight(n) .build()); } } - for (Node n : badConnections) { + for (Entry> entry : badConnections.entrySet()) { errors.add(TestError.builder(this, Severity.WARNING, POWER_CONNECTION) .message(tr("Node connects a power line or cable with an object " + "which is not related to the power infrastructure")) - .primitives(n) + .primitives(getAllPrimitives(entry)) + .highlight(entry.getKey()) .build()); } @@ -222,4 +227,16 @@ super.endTest(); + } + + /** + * Combine the node and the related objects. + * @param entry a map entry with a node and related objects + * @return set containing the node and the related objects + */ + private Collection getAllPrimitives(Entry> entry) { + Set primitives = new LinkedHashSet<>(); + primitives.add(entry.getKey()); + primitives.addAll(entry.getValue()); + return primitives; } @@ -254,6 +271,8 @@ /// handle missing power line support tags (e.g. tower) if (!isPowerTower(n) && !isPowerInfrastructure(n) && IN_DOWNLOADED_AREA.test(n) - && (!w.isFirstLastNode(n) || !isPowerStation(n))) - missingTags.add(n); + && (!w.isFirstLastNode(n) || !isPowerStation(n))) { + Set set = missingTags.computeIfAbsent(n, k -> new HashSet<>()); + set.add(w); + } /// handle missing nodes @@ -670,4 +689,13 @@ /** + * Determines if the specified way denotes a power line or cable. + * @param w The way to be tested + * @return {@code true} if power key is set and equal to line,minor_line or cable + */ + protected static boolean isPowerLineOrCable(Way w) { + return isPowerIn(w, Arrays.asList("line", MINOR_LINE, "cable")); + } + + /** * Determines if the specified primitive denotes a power station. * @param p The primitive to be tested Index: /trunk/test/unit/org/openstreetmap/josm/data/validation/tests/PowerLinesTest.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/data/validation/tests/PowerLinesTest.java (revision 19618) +++ /trunk/test/unit/org/openstreetmap/josm/data/validation/tests/PowerLinesTest.java (revision 19619) @@ -1,10 +1,4 @@ // License: GPL. For details, see LICENSE file. package org.openstreetmap.josm.data.validation.tests; - -import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; - -import java.util.ArrayList; import org.junit.jupiter.api.BeforeEach; @@ -18,7 +12,14 @@ import org.openstreetmap.josm.data.osm.TagMap; import org.openstreetmap.josm.data.osm.Way; +import org.openstreetmap.josm.data.validation.TestError; import org.openstreetmap.josm.gui.progress.NullProgressMonitor; import org.openstreetmap.josm.testutils.annotations.BasicPreferences; import org.openstreetmap.josm.testutils.annotations.Projection; + +import java.util.ArrayList; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; /** @@ -160,3 +161,62 @@ assertTrue(this.powerLines.getErrors().isEmpty()); } + + /** + * Test for ticket #24851. + * Simulates connecting a power line to an existing highway node without power tags. + * Validates that the resulting error contains both the Node AND the Way, so it + * doesn't get filtered out during partial validation on upload. + */ + @Test + void testTicket24851_ReportExistingNonPowerNodes() { + Node sharedNode = new Node(new LatLon(0, 0)); // no power tag attached + + // unrelated highway way + Way highway = TestUtils.newWay("highway=unclassified", + sharedNode, new Node(new LatLon(0.1, 0))); + + // power line way + Way powerline = TestUtils.newWay("power=line", + sharedNode, new Node(new LatLon(0, 0.1))); + + // second node has a valid tag + powerline.getNode(1).put("power", "tower"); + + ds.addPrimitiveRecursive(highway); + ds.addPrimitiveRecursive(powerline); + + powerLines.startTest(NullProgressMonitor.INSTANCE); + for (Way w : ds.getWays()) { + powerLines.visit(w); + } + for (Node n : ds.getNodes()) { + powerLines.visit(n); + } + powerLines.endTest(); + + assertFalse(powerLines.getErrors().isEmpty(), "Errors should be generated for the missing tag and bad connection"); + + boolean foundSupportError = false; + boolean foundConnectionError = false; + + for (TestError error : powerLines.getErrors()) { + // verify POWER_SUPPORT behavior (missing tag) + if (error.getCode() == PowerLines.POWER_SUPPORT && error.getPrimitives().contains(sharedNode)) { + foundSupportError = true; + assertTrue(error.getPrimitives().contains(powerline), + "MUST contain the parent powerline way. This prevents JOSM from discarding the error " + + "during partial validation if the node itself was unmodified."); + } + // verify POWER_CONNECTION behavior (bad connection) + if (error.getCode() == PowerLines.POWER_CONNECTION && error.getPrimitives().contains(sharedNode)) { + foundConnectionError = true; + assertTrue(error.getPrimitives().contains(highway), + "MUST contain the unrelated parent way. This prevents JOSM from discarding the error" + + "during partial validation if the node itself was unmodified."); + } + } + + assertTrue(foundSupportError, "Should report missing power tag on shared node"); + assertTrue(foundConnectionError, "Should report bad connection on shared node"); + } }