Index: trunk/src/org/openstreetmap/josm/data/validation/tests/ConnectivityRelations.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/validation/tests/ConnectivityRelations.java (revision 17383) +++ trunk/src/org/openstreetmap/josm/data/validation/tests/ConnectivityRelations.java (revision 17384) @@ -71,9 +71,10 @@ * * @param relation A relation with a {@code connectivity} tag. - * @return A Map in the form of {@code Map>} May contain nulls when errors are encountered + * @return A Map in the form of {@code Map>} May contain nulls when errors are encountered, + * empty collection if {@code connectivity} tag contains unusual values */ public static Map> 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,35 +82,29 @@ final Map> 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 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,9 +124,15 @@ .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 roleLanes = checkForInconsistentLanes(r); - checkForImpliedConnectivity(r, roleLanes); + Map> 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 roleLanes = checkForInconsistentLanes(r, connTagLanes); + checkForImpliedConnectivity(r, roleLanes, connTagLanes); + } } } @@ -143,11 +144,15 @@ * * @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} */ - private Map checkForInconsistentLanes(Relation relation) { + private Map checkForInconsistentLanes(Relation relation, Map> connTagLanes) { StringBuilder lanelessRoles = new StringBuilder(); int lanelessRolesCount = 0; // Lane count from connectivity tag - Map> connTagLanes = parseConnectivityTag(relation); + Map 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,5 +170,4 @@ } // Lane count from member tags - Map roleLanes = new HashMap<>(); for (RelationMember rM : relation.getMembers()) { // Check lanes @@ -187,14 +191,12 @@ } - 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,5 +205,5 @@ } - if (lanelessRoles.toString().isEmpty()) { + if (lanelessRoles.length() == 0) { boolean fromCheck = roleLanes.get(FROM) < Collections .max(connTagLanes.entrySet(), Comparator.comparingInt(Map.Entry::getKey)).getKey(); @@ -243,7 +245,8 @@ * @param relation A relation with a {@code connectivity} tag. * @param roleLanes The lane counts for each relation role - */ - private void checkForImpliedConnectivity(Relation relation, Map roleLanes) { - Map> connTagLanes = parseConnectivityTag(relation); + * @param connTagLanes result of {@link ConnectivityRelations#parseConnectivityTag(Relation)} + */ + private void checkForImpliedConnectivity(Relation relation, Map roleLanes, + Map> connTagLanes) { // Don't flag connectivity as already implied when: // - Lane counts are different on the roads Index: trunk/test/unit/org/openstreetmap/josm/data/validation/tests/ConnectivityRelationsTest.java =================================================================== --- trunk/test/unit/org/openstreetmap/josm/data/validation/tests/ConnectivityRelationsTest.java (revision 17383) +++ trunk/test/unit/org/openstreetmap/josm/data/validation/tests/ConnectivityRelationsTest.java (revision 17384) @@ -5,5 +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,4 +11,7 @@ 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,12 +23,14 @@ 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,3 +91,23 @@ } + /** + * Non-regression test for Bug #20182. + * @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()); + } }