Changeset 17384 in josm

Timestamp:

2020-12-02T08:30:13+01:00 (4 years ago)

Author:

GerdP

Message:

fix #20182: NumberFormatException in ConnectivityRelations.parseConnectivityTag

• fix crash with invalid connectivity=right_turn or a single number like connectivity=1
• remove code which looks for "bw" after "bw" was replaced by -1000.
• some more cleanup and code simplifications
• use JOSMTestRules() instead of JOSMFixture

I don't like that method parseConnectivityTag() is public but didn't change it so far. It seems a bit strange to return an empty map for different kinds of problems found in the connectivity tag.

Location:

trunk

Files:

• src/org/openstreetmap/josm/data/validation/tests/ConnectivityRelations.java (modified) (8 diffs)
• test/unit/org/openstreetmap/josm/data/validation/tests/ConnectivityRelationsTest.java (modified) (4 diffs)

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

@@ -71 +71 @@
      *
      * @param relation A relation with a {@code connectivity} tag.
-     * @return A Map in the form of {@code Map<Lane From, Map<Lane To, Optional>>} May contain nulls when errors are encountered
+     * @return A Map in the form of {@code Map<Lane From, Map<Lane To, Optional>>} May contain nulls when errors are encountered,
+     * empty collection if {@code connectivity} tag contains unusual values
      */
     public static Map<Integer, Map<Integer, Boolean>> parseConnectivityTag(Relation relation) {
-        String cnTag = relation.get(CONNECTIVITY_TAG);
-        if (cnTag == null) {
+        final String cnTag = relation.get(CONNECTIVITY_TAG);
+        if (cnTag == null || cnTag.isEmpty()) {
             return Collections.emptyMap();
         }
@@ -81 +82 @@
 
         final Map<Integer, Map<Integer, Boolean>> result = new HashMap<>();
-        String[] lanes = joined.split("\\|", -1);
-        for (int i = 0; i < lanes.length; i++) {
-            final String[] lane = lanes[i].split(":", -1);
+        String[] lanePairs = joined.split("\\|", -1);
+        for (final String lanePair : lanePairs) {
+            final String[] lane = lanePair.split(":", -1);
+            if (lane.length < 2)
+                return Collections.emptyMap();
             int laneNumber;
-            //Ignore connections from bw, since we cannot derive a lane number from bw
-            if (!"bw".equals(lane[0])) {
+            try {
                 laneNumber = Integer.parseInt(lane[0].trim());
-            } else {
-                laneNumber = BW;
-            }
+            } catch (NumberFormatException e) {
+                return Collections.emptyMap();
+            }
+
             Map<Integer, Boolean> connections = new HashMap<>();
             String[] toLanes = TO_LANE_PATTERN.split(lane[1], -1);
-            for (int j = 0; j < toLanes.length; j++) {
-                String toLane = toLanes[j].trim();
+            for (String toLane : toLanes) {
                 try {
                     if (OPTIONAL_LANE_PATTERN.matcher(toLane).matches()) {
                         toLane = toLane.replace("(", "").replace(")", "").trim();
-                        if (!"bw".equals(toLane)) {
-                            connections.put(Integer.parseInt(toLane), Boolean.TRUE);
-                        } else
-                            connections.put(BW, Boolean.TRUE);
+                        connections.put(Integer.parseInt(toLane), Boolean.TRUE);
                     } else {
-                        if (!toLane.contains("bw")) {
-                            connections.put(Integer.parseInt(toLane), Boolean.FALSE);
-                        } else {
-                            connections.put(BW, Boolean.FALSE);
-                        }
+                        connections.put(Integer.parseInt(toLane), Boolean.FALSE);
                     }
                 } catch (NumberFormatException e) {
                     if (MISSING_COMMA_PATTERN.matcher(toLane).matches()) {
-                        connections.put(null, true);
+                        connections.put(null, Boolean.TRUE);
                     } else {
                         connections.put(null, null);
@@ -129 +124 @@
                         .message(tr("Connectivity relation without connectivity tag")).primitives(r).build());
             } else if (!r.hasIncompleteMembers()) {
-                boolean badRole = checkForBadRole(r);
-                boolean missingRole = checkForMissingRole(r);
-                if (!badRole && !missingRole) {
-                    Map<String, Integer> roleLanes = checkForInconsistentLanes(r);
-                    checkForImpliedConnectivity(r, roleLanes);
+                Map<Integer, Map<Integer, Boolean>> connTagLanes = parseConnectivityTag(r);
+                if (connTagLanes.isEmpty()) {
+                    errors.add(TestError.builder(this, Severity.ERROR, MALFORMED_CONNECTIVITY_TAG)
+                            .message(tr("Connectivity tag contains unusual data")).primitives(r).build());
+                } else {
+                    boolean badRole = checkForBadRole(r);
+                    boolean missingRole = checkForMissingRole(r);
+                    if (!badRole && !missingRole) {
+                        Map<String, Integer> roleLanes = checkForInconsistentLanes(r, connTagLanes);
+                        checkForImpliedConnectivity(r, roleLanes, connTagLanes);
+                    }
                 }
             }
@@ -143 +144 @@
      *
      * @param relation A relation with a {@code connectivity} tag.
+     * @param connTagLanes result of {@link ConnectivityRelations#parseConnectivityTag(Relation)}
      * @return A Map in the form of {@code Map<Role, Lane Count>}
      */
-    private Map<String, Integer> checkForInconsistentLanes(Relation relation) {
+    private Map<String, Integer> checkForInconsistentLanes(Relation relation, Map<Integer, Map<Integer, Boolean>> connTagLanes) {
         StringBuilder lanelessRoles = new StringBuilder();
         int lanelessRolesCount = 0;
         // Lane count from connectivity tag
-        Map<Integer, Map<Integer, Boolean>> connTagLanes = parseConnectivityTag(relation);
+        Map<String, Integer> roleLanes = new HashMap<>();
+        if (connTagLanes.isEmpty())
+            return roleLanes;
+
         // If the ways involved in the connectivity tag are assuming a standard 2-way bi-directional highway
         boolean defaultLanes = true;
@@ -165 +170 @@
         }
         // Lane count from member tags
-        Map<String, Integer> roleLanes = new HashMap<>();
         for (RelationMember rM : relation.getMembers()) {
             // Check lanes
@@ -187 +191 @@
                     }
 
-                    if (!laneCounts.equals(Collections.emptyList())) {
+                    if (!laneCounts.isEmpty()) {
                         maxLaneCount = Collections.max(laneCounts);
                         roleLanes.put(rM.getRole(), (int) maxLaneCount);
                     } else {
-                        String addString = "'" + rM.getRole() + "'";
-                        StringBuilder sb = new StringBuilder(addString);
                         if (lanelessRoles.length() > 0) {
-                            sb.insert(0, " and ");
+                            lanelessRoles.append(" and ");
                         }
-                        lanelessRoles.append(sb.toString());
+                        lanelessRoles.append('\'').append(rM.getRole()).append('\'');
                         lanelessRolesCount++;
                     }
@@ -203 +205 @@
         }
 
-        if (lanelessRoles.toString().isEmpty()) {
+        if (lanelessRoles.length() == 0) {
             boolean fromCheck = roleLanes.get(FROM) < Collections
                     .max(connTagLanes.entrySet(), Comparator.comparingInt(Map.Entry::getKey)).getKey();
@@ -243 +245 @@
      * @param relation A relation with a {@code connectivity} tag.
      * @param roleLanes The lane counts for each relation role
-     */
-    private void checkForImpliedConnectivity(Relation relation, Map<String, Integer> roleLanes) {
-        Map<Integer, Map<Integer, Boolean>> connTagLanes = parseConnectivityTag(relation);
+     * @param connTagLanes result of {@link ConnectivityRelations#parseConnectivityTag(Relation)}
+     */
+    private void checkForImpliedConnectivity(Relation relation, Map<String, Integer> roleLanes,
+            Map<Integer, Map<Integer, Boolean>> connTagLanes) {
         // Don't flag connectivity as already implied when:
         // - Lane counts are different on the roads

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

@@ -5 +5 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
-import org.openstreetmap.josm.JOSMFixture;
+import org.junit.jupiter.api.extension.RegisterExtension;
 import org.openstreetmap.josm.TestUtils;
 import org.openstreetmap.josm.data.coor.LatLon;
@@ -11 +11 @@
 import org.openstreetmap.josm.data.osm.Relation;
 import org.openstreetmap.josm.data.osm.RelationMember;
+import org.openstreetmap.josm.testutils.JOSMTestRules;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 
 /**
@@ -20 +23 @@
     private ConnectivityRelations check;
     private static final String CONNECTIVITY = "connectivity";
+
     /**
      * Setup test.
-     *
-     * @throws Exception if an error occurs
      */
+    @RegisterExtension
+    @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
+    public JOSMTestRules rule = new JOSMTestRules();
+
     @BeforeEach
-    public void setUp() throws Exception {
-        JOSMFixture.createUnitTestFixture().init();
+    public void setUpCheck() throws Exception {
         check = new ConnectivityRelations();
     }
@@ -86 +91 @@
     }
 
+    /**
+     * Non-regression test for <a href="https://josm.openstreetmap.de/ticket/201821">Bug #20182</a>.
+     * @throws Exception if an error occurs
+     */
+    @Test
+    void testTicket20182() throws Exception {
+        Relation relation = createDefaultTestRelation();
+        check.visit(relation);
+        int expectedFailures = 0;
+
+        Assert.assertEquals(expectedFailures, check.getErrors().size());
+
+        relation.put(CONNECTIVITY, "left_turn");
+        check.visit(relation);
+        Assert.assertEquals(++expectedFailures, check.getErrors().size());
+
+        relation.put(CONNECTIVITY, "1");
+        check.visit(relation);
+        Assert.assertEquals(++expectedFailures, check.getErrors().size());
+    }
 }