Changeset 17384 in josm for trunk


Ignore:
Timestamp:
2020-12-02T08:30:13+01:00 (3 months 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:
2 edited

Legend:

Unmodified
Added
Removed
  • trunk/src/org/openstreetmap/josm/data/validation/tests/ConnectivityRelations.java

    r17374 r17384  
    7171     *
    7272     * @param relation A relation with a {@code connectivity} tag.
    73      * @return A Map in the form of {@code Map<Lane From, Map<Lane To, Optional>>} May contain nulls when errors are encountered
     73     * @return A Map in the form of {@code Map<Lane From, Map<Lane To, Optional>>} May contain nulls when errors are encountered,
     74     * empty collection if {@code connectivity} tag contains unusual values
    7475     */
    7576    public static Map<Integer, Map<Integer, Boolean>> parseConnectivityTag(Relation relation) {
    76         String cnTag = relation.get(CONNECTIVITY_TAG);
    77         if (cnTag == null) {
     77        final String cnTag = relation.get(CONNECTIVITY_TAG);
     78        if (cnTag == null || cnTag.isEmpty()) {
    7879            return Collections.emptyMap();
    7980        }
     
    8182
    8283        final Map<Integer, Map<Integer, Boolean>> result = new HashMap<>();
    83         String[] lanes = joined.split("\\|", -1);
    84         for (int i = 0; i < lanes.length; i++) {
    85             final String[] lane = lanes[i].split(":", -1);
     84        String[] lanePairs = joined.split("\\|", -1);
     85        for (final String lanePair : lanePairs) {
     86            final String[] lane = lanePair.split(":", -1);
     87            if (lane.length < 2)
     88                return Collections.emptyMap();
    8689            int laneNumber;
    87             //Ignore connections from bw, since we cannot derive a lane number from bw
    88             if (!"bw".equals(lane[0])) {
     90            try {
    8991                laneNumber = Integer.parseInt(lane[0].trim());
    90             } else {
    91                 laneNumber = BW;
    92             }
     92            } catch (NumberFormatException e) {
     93                return Collections.emptyMap();
     94            }
     95
    9396            Map<Integer, Boolean> connections = new HashMap<>();
    9497            String[] toLanes = TO_LANE_PATTERN.split(lane[1], -1);
    95             for (int j = 0; j < toLanes.length; j++) {
    96                 String toLane = toLanes[j].trim();
     98            for (String toLane : toLanes) {
    9799                try {
    98100                    if (OPTIONAL_LANE_PATTERN.matcher(toLane).matches()) {
    99101                        toLane = toLane.replace("(", "").replace(")", "").trim();
    100                         if (!"bw".equals(toLane)) {
    101                             connections.put(Integer.parseInt(toLane), Boolean.TRUE);
    102                         } else
    103                             connections.put(BW, Boolean.TRUE);
     102                        connections.put(Integer.parseInt(toLane), Boolean.TRUE);
    104103                    } else {
    105                         if (!toLane.contains("bw")) {
    106                             connections.put(Integer.parseInt(toLane), Boolean.FALSE);
    107                         } else {
    108                             connections.put(BW, Boolean.FALSE);
    109                         }
     104                        connections.put(Integer.parseInt(toLane), Boolean.FALSE);
    110105                    }
    111106                } catch (NumberFormatException e) {
    112107                    if (MISSING_COMMA_PATTERN.matcher(toLane).matches()) {
    113                         connections.put(null, true);
     108                        connections.put(null, Boolean.TRUE);
    114109                    } else {
    115110                        connections.put(null, null);
     
    129124                        .message(tr("Connectivity relation without connectivity tag")).primitives(r).build());
    130125            } else if (!r.hasIncompleteMembers()) {
    131                 boolean badRole = checkForBadRole(r);
    132                 boolean missingRole = checkForMissingRole(r);
    133                 if (!badRole && !missingRole) {
    134                     Map<String, Integer> roleLanes = checkForInconsistentLanes(r);
    135                     checkForImpliedConnectivity(r, roleLanes);
     126                Map<Integer, Map<Integer, Boolean>> connTagLanes = parseConnectivityTag(r);
     127                if (connTagLanes.isEmpty()) {
     128                    errors.add(TestError.builder(this, Severity.ERROR, MALFORMED_CONNECTIVITY_TAG)
     129                            .message(tr("Connectivity tag contains unusual data")).primitives(r).build());
     130                } else {
     131                    boolean badRole = checkForBadRole(r);
     132                    boolean missingRole = checkForMissingRole(r);
     133                    if (!badRole && !missingRole) {
     134                        Map<String, Integer> roleLanes = checkForInconsistentLanes(r, connTagLanes);
     135                        checkForImpliedConnectivity(r, roleLanes, connTagLanes);
     136                    }
    136137                }
    137138            }
     
    143144     *
    144145     * @param relation A relation with a {@code connectivity} tag.
     146     * @param connTagLanes result of {@link ConnectivityRelations#parseConnectivityTag(Relation)}
    145147     * @return A Map in the form of {@code Map<Role, Lane Count>}
    146148     */
    147     private Map<String, Integer> checkForInconsistentLanes(Relation relation) {
     149    private Map<String, Integer> checkForInconsistentLanes(Relation relation, Map<Integer, Map<Integer, Boolean>> connTagLanes) {
    148150        StringBuilder lanelessRoles = new StringBuilder();
    149151        int lanelessRolesCount = 0;
    150152        // Lane count from connectivity tag
    151         Map<Integer, Map<Integer, Boolean>> connTagLanes = parseConnectivityTag(relation);
     153        Map<String, Integer> roleLanes = new HashMap<>();
     154        if (connTagLanes.isEmpty())
     155            return roleLanes;
     156
    152157        // If the ways involved in the connectivity tag are assuming a standard 2-way bi-directional highway
    153158        boolean defaultLanes = true;
     
    165170        }
    166171        // Lane count from member tags
    167         Map<String, Integer> roleLanes = new HashMap<>();
    168172        for (RelationMember rM : relation.getMembers()) {
    169173            // Check lanes
     
    187191                    }
    188192
    189                     if (!laneCounts.equals(Collections.emptyList())) {
     193                    if (!laneCounts.isEmpty()) {
    190194                        maxLaneCount = Collections.max(laneCounts);
    191195                        roleLanes.put(rM.getRole(), (int) maxLaneCount);
    192196                    } else {
    193                         String addString = "'" + rM.getRole() + "'";
    194                         StringBuilder sb = new StringBuilder(addString);
    195197                        if (lanelessRoles.length() > 0) {
    196                             sb.insert(0, " and ");
     198                            lanelessRoles.append(" and ");
    197199                        }
    198                         lanelessRoles.append(sb.toString());
     200                        lanelessRoles.append('\'').append(rM.getRole()).append('\'');
    199201                        lanelessRolesCount++;
    200202                    }
     
    203205        }
    204206
    205         if (lanelessRoles.toString().isEmpty()) {
     207        if (lanelessRoles.length() == 0) {
    206208            boolean fromCheck = roleLanes.get(FROM) < Collections
    207209                    .max(connTagLanes.entrySet(), Comparator.comparingInt(Map.Entry::getKey)).getKey();
     
    243245     * @param relation A relation with a {@code connectivity} tag.
    244246     * @param roleLanes The lane counts for each relation role
    245      */
    246     private void checkForImpliedConnectivity(Relation relation, Map<String, Integer> roleLanes) {
    247         Map<Integer, Map<Integer, Boolean>> connTagLanes = parseConnectivityTag(relation);
     247     * @param connTagLanes result of {@link ConnectivityRelations#parseConnectivityTag(Relation)}
     248     */
     249    private void checkForImpliedConnectivity(Relation relation, Map<String, Integer> roleLanes,
     250            Map<Integer, Map<Integer, Boolean>> connTagLanes) {
    248251        // Don't flag connectivity as already implied when:
    249252        // - Lane counts are different on the roads
  • trunk/test/unit/org/openstreetmap/josm/data/validation/tests/ConnectivityRelationsTest.java

    r17275 r17384  
    55import org.junit.jupiter.api.BeforeEach;
    66import org.junit.jupiter.api.Test;
    7 import org.openstreetmap.josm.JOSMFixture;
     7import org.junit.jupiter.api.extension.RegisterExtension;
    88import org.openstreetmap.josm.TestUtils;
    99import org.openstreetmap.josm.data.coor.LatLon;
     
    1111import org.openstreetmap.josm.data.osm.Relation;
    1212import org.openstreetmap.josm.data.osm.RelationMember;
     13import org.openstreetmap.josm.testutils.JOSMTestRules;
     14
     15import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
    1316
    1417/**
     
    2023    private ConnectivityRelations check;
    2124    private static final String CONNECTIVITY = "connectivity";
     25
    2226    /**
    2327     * Setup test.
    24      *
    25      * @throws Exception if an error occurs
    2628     */
     29    @RegisterExtension
     30    @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
     31    public JOSMTestRules rule = new JOSMTestRules();
     32
    2733    @BeforeEach
    28     public void setUp() throws Exception {
    29         JOSMFixture.createUnitTestFixture().init();
     34    public void setUpCheck() throws Exception {
    3035        check = new ConnectivityRelations();
    3136    }
     
    8691    }
    8792
     93    /**
     94     * Non-regression test for <a href="https://josm.openstreetmap.de/ticket/201821">Bug #20182</a>.
     95     * @throws Exception if an error occurs
     96     */
     97    @Test
     98    void testTicket20182() throws Exception {
     99        Relation relation = createDefaultTestRelation();
     100        check.visit(relation);
     101        int expectedFailures = 0;
     102
     103        Assert.assertEquals(expectedFailures, check.getErrors().size());
     104
     105        relation.put(CONNECTIVITY, "left_turn");
     106        check.visit(relation);
     107        Assert.assertEquals(++expectedFailures, check.getErrors().size());
     108
     109        relation.put(CONNECTIVITY, "1");
     110        check.visit(relation);
     111        Assert.assertEquals(++expectedFailures, check.getErrors().size());
     112    }
    88113}
Note: See TracChangeset for help on using the changeset viewer.