Changeset 19619 in josm

Timestamp:

2026-08-29T10:42:43+02:00 (2 hours ago)

Author:

GerdP

Message:

Fix #24851: Power line checks won't report existing non-power nodes

• add the related parent objects to the error message so that they are not considered as unrelated when a partial validation is done on upload
• unit test by gaben (thanks!)

Location:

trunk

Files:

• src/org/openstreetmap/josm/data/validation/tests/PowerLines.java (modified) (7 diffs)
• test/unit/org/openstreetmap/josm/data/validation/tests/PowerLinesTest.java (modified) (3 diffs)

trunk/src/org/openstreetmap/josm/data/validation/tests/PowerLines.java

@@ -12 +12 @@
 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 +79 @@
     private double hillyCompensation;
     private double hillyThreshold;
-    private final Set<Node> badConnections = new HashSet<>();
-    private final Set<Node> missingTags = new HashSet<>();
+    private final Map<Node, Set<OsmPrimitive>> badConnections = new HashMap<>();
+    private final Map<Node, Set<OsmPrimitive>> missingTags = new HashMap<>();
     private final Set<Way> wrongLineType = new HashSet<>();
     private final Set<WaySegment> missingNodes = new HashSet<>();
@@ -99 +102 @@
     @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<Way> unrelatedParents = n.referrers(Way.class).filter(w -> !isPowerLineOrCable(w) && !isRelatedToPower(w))
+                .collect(Collectors.toList());
+        if (!unrelatedParents.isEmpty()) {
+            Set<OsmPrimitive> set = badConnections.computeIfAbsent(n, k -> new HashSet<>());
+            set.addAll(unrelatedParents);
+        }
     }
 
@@ -163 +165 @@
         }
         // Then return the errors
-        for (Node n : missingTags) {
+        for (Entry<Node, Set<OsmPrimitive>> 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<Node, Set<OsmPrimitive>> 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 +227 @@
 
         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<? extends OsmPrimitive> getAllPrimitives(Entry<Node, Set<OsmPrimitive>> entry) {
+        Set<OsmPrimitive> primitives = new LinkedHashSet<>();
+        primitives.add(entry.getKey());
+        primitives.addAll(entry.getValue());
+        return primitives;
     }
 
@@ -254 +271 @@
             /// 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<OsmPrimitive> set = missingTags.computeIfAbsent(n, k -> new HashSet<>());
+                set.add(w);
+            }
 
             /// handle missing nodes
@@ -670 +689 @@
 
     /**
+     * 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

trunk/test/unit/org/openstreetmap/josm/data/validation/tests/PowerLinesTest.java

@@ -1 +1 @@
 // 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 +12 @@
 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 +161 @@
         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");
+    }
 }