Ticket #18731: 18138.1.patch

File 18138.1.patch, 17.9 KB (added by taylor.smock, 6 years ago)

Add tests and fix an issue with a possible NPE (if bbox is null, there is an error in searchNodes)

  • data/defaultpresets.xml

     
    294294        </chunk>
    295295        <chunk id="other_religions"> <!-- religions which don't have an own preset -->
    296296            <combo key="religion" text="Religion" values="bahai,caodaism,confucian,jain,sikh,spiritualist,taoist,tenrikyo,unitarian_universalist,zoroastrian" match="keyvalue!" values_searchable="true" />
    297         </chunk>
     297        </chunk>
    298298        <chunk id="christian_denominations"> <!-- christian denominations -->
    299299            <combo key="denomination" text="Denomination" values="anglican,baptist,catholic,church_of_scotland,evangelical,greek_catholic,greek_orthodox,iglesia_ni_cristo,jehovahs_witness,lutheran,methodist,mormon,new_apostolic,nondenominational,old_catholic,orthodox,pentecostal,presbyterian,protestant,quaker,reformed,roman_catholic,romanian_orthodox,russian_orthodox,serbian_orthodox,seventh_day_adventist,spiritist,united,united_methodist" values_searchable="true" />
    300300        </chunk>
    … …  
    311311            <combo key="denomination" text="Denomination" values="vaishnavism,shaivism,shaktism,smartism" values_searchable="true" />
    312312        </chunk>
    313313        <!-- shinto and the religions which don't have an own preset don't have denominations (yet) -->
    314     <!-- end of religions and denominations -->
     314    <!-- end of religions and denominations -->
    315315    <chunk id="voltage">
    316316        <combo key="voltage" text="Voltage in Volts (V)" values="1150000,765000,750000,735000,500000,450000,420000,400000,380000,350000,345000,330000,315000,300000,275000,238000,230000,225000,220000,200000,161000,154000,150000,138000,132000,120000,115000,110000,100000,90000,69000,66000,65000,63000,60000,55000,49000,45000,35000,33000,30000,22000,20000,15000,110000;20000" />
    317317    </chunk>
    … …  
    502502      <combo key="climbing:rock" text="Rock type" rows="3" values="limestone,sandstone,granite,basalt,slate" />
    503503      <check key="climbing:summit_log" text="Summit/route log/register" />
    504504      <check key="indoor" text="Climbing routes indoor" />
    505       <check key="outdoor" text="Climbing routes outdoor" />
     505      <check key="outdoor" text="Climbing routes outdoor" />
    506506      <text key="website" text="Official web site (e.g. operator)" />
    507507      <text key="url" text="Unofficial web site" />
    508508      <combo key="opening_hours" text="Opening Hours" delimiter="|" values="24/7|sunset-sunrise open; sunrise-sunset closed|Mar-Jun closed; Jul-Feb Mo-Su,PH sunrise-sunset|Mo-Fr 15:00-22:00; Sa-Su 11:00-22:00" values_no_i18n="true"/>
    … …  
    11521152            <check key="button_operated" text="Button operated" />
    11531153            <check key="traffic_signals:sound" text="Sound signals" />
    11541154        </item> <!-- Pedestrian Crossing -->
    1155         <group name="Traffic Calming" icon="presets/vehicle/choker.svg">
     1155        <group name="Traffic Calming" icon="presets/vehicle/choker.svg">
    11561156            <item name="Bump" icon="presets/vehicle/bump.svg" type="node,way" preset_name_label="true">
    11571157                <link wiki="Key:traffic_calming" />
    11581158                <space />
    … …  
    74947494                <role key="to" text="to way" requisite="required" count="1" type="way" />
    74957495            </roles>
    74967496        </item> <!-- Turn Restriction -->
     7497        <item name="Lane Connectivity" type="relation" preset_name_label="true">
     7498            <link wiki="Relation:connectivity" />
     7499            <space />
     7500            <key key="type" value="connectivity" />
     7501            <optional>
     7502                <text key="connectivity" text="Lane Connectivity" />
     7503            </optional>
     7504            <roles>
     7505                <role key="from" text="from way" requisite="required" count="1" type="way" />
     7506                <role key="via" text="via node or ways" requisite="required" type="way,node" />
     7507                <role key="to" text="to way" requisite="required" count="1" type="way" />
     7508            </roles>
     7509        </item> <!-- Lane Connectivity -->
    74977510        <item name="Enforcement" icon="presets/vehicle/restriction/speed_camera.svg" type="relation" preset_name_label="true">
    74987511            <link wiki="Relation:enforcement" />
    74997512            <space />

  • src/org/openstreetmap/josm/data/validation/OsmValidator.java

     
    4242import org.openstreetmap.josm.data.validation.tests.BarriersEntrances;
    4343import org.openstreetmap.josm.data.validation.tests.Coastlines;
    4444import org.openstreetmap.josm.data.validation.tests.ConditionalKeys;
     45import org.openstreetmap.josm.data.validation.tests.ConnectivityRelationCheck;
    4546import org.openstreetmap.josm.data.validation.tests.CrossingWays;
    4647import org.openstreetmap.josm.data.validation.tests.DuplicateNode;
    4748import org.openstreetmap.josm.data.validation.tests.DuplicateRelation;
    … …  
    6162import org.openstreetmap.josm.data.validation.tests.RelationChecker;
    6263import org.openstreetmap.josm.data.validation.tests.RightAngleBuildingTest;
    6364import org.openstreetmap.josm.data.validation.tests.SelfIntersectingWay;
     65import org.openstreetmap.josm.data.validation.tests.SharpAngles;
    6466import org.openstreetmap.josm.data.validation.tests.SimilarNamedWays;
    6567import org.openstreetmap.josm.data.validation.tests.TagChecker;
    6668import org.openstreetmap.josm.data.validation.tests.TurnrestrictionTest;
    … …  
    148149        LongSegment.class, // 3500 .. 3599
    149150        PublicTransportRouteTest.class, // 3600 .. 3699
    150151        RightAngleBuildingTest.class, // 3700 .. 3799
     152        ConnectivityRelationCheck.class, // 3800 .. 3899
    151153    };
    152154
    153155    /**

  • src/org/openstreetmap/josm/data/validation/tests/ConnectivityRelationCheck.java

     
     1// License: GPL. For details, see LICENSE file.
     2package org.openstreetmap.josm.data.validation.tests;
     3
     4import static org.openstreetmap.josm.tools.I18n.tr;
     5
     6import java.util.Collections;
     7import java.util.Comparator;
     8import java.util.HashMap;
     9import java.util.Map;
     10import java.util.Map.Entry;
     11import java.util.TreeMap;
     12import java.util.regex.Pattern;
     13
     14import org.openstreetmap.josm.data.osm.Node;
     15import org.openstreetmap.josm.data.osm.OsmPrimitive;
     16import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
     17import org.openstreetmap.josm.data.osm.Relation;
     18import org.openstreetmap.josm.data.osm.RelationMember;
     19import org.openstreetmap.josm.data.osm.Way;
     20import org.openstreetmap.josm.data.validation.Severity;
     21import org.openstreetmap.josm.data.validation.Test;
     22import org.openstreetmap.josm.data.validation.TestError;
     23
     24/**
     25 * Check for inconsistencies in lane information between relation and members.
     26 */
     27public class ConnectivityRelationCheck extends Test {
     28
     29    protected static final int INCONSISTENT_LANE_COUNT = 3800;
     30
     31    protected static final int UNKNOWN_CONNECTIVITY_ROLE = INCONSISTENT_LANE_COUNT + 1;
     32
     33    protected static final int NO_CONNECTIVITY_TAG = INCONSISTENT_LANE_COUNT + 2;
     34
     35    protected static final int TOO_MANY_ROLES = INCONSISTENT_LANE_COUNT + 3;
     36
     37    private static final String CONNECTIVITY_TAG = "connectivity";
     38    private static final String VIA = "via";
     39    private static final String TO = "to";
     40    private static final String FROM = "from";
     41
     42    /**
     43    * Constructor
     44    */
     45    public ConnectivityRelationCheck() {
     46        super(tr("Connectivity Relation Check"), tr("Checks that lane count of relation matches with lanes of members"));
     47    }
     48
     49    /**
     50     * Convert the connectivity tag into a map of values
     51     *
     52     * @param relation A relation with a {@code connectivity} tag.
     53     * @return A Map in the form of {@code Map<Lane From, Map<Lane To, Optional>>}
     54     * @since xxx
     55     */
     56    public static Map<Integer, Map<Integer, Boolean>> parseConnectivityTag(Relation relation) {
     57        final String joined = relation.get(CONNECTIVITY_TAG);
     58
     59        if (joined == null) {
     60            return new TreeMap<>();
     61        }
     62
     63        final Map<Integer, Map<Integer, Boolean>> result = new HashMap<>();
     64        String[] lanes = joined.split("\\|", -1);
     65        for (int i = 0; i < lanes.length; i++) {
     66            String[] lane = lanes[i].split(":", -1);
     67            int laneNumber = Integer.parseInt(lane[0].trim());
     68            Map<Integer, Boolean> connections = new HashMap<>();
     69            String[] toLanes = Pattern.compile("\\p{Zs}*[,:;]\\p{Zs}*").split(lane[1]);
     70            for (int j = 0; j < toLanes.length; j++) {
     71                String toLane = toLanes[j].trim();
     72                if (Pattern.compile("\\([0-9]+\\)").matcher(toLane).matches()) {
     73                    toLane = toLane.replace("(", "").replace(")", "").trim();
     74                    connections.put(Integer.parseInt(toLane), true);
     75                } else {
     76                    connections.put(Integer.parseInt(toLane), false);
     77                }
     78            }
     79            result.put(laneNumber, connections);
     80        }
     81        return result;
     82    }
     83
     84    @Override
     85    public void visit(Relation r) {
     86        if (r.hasTag("type", CONNECTIVITY_TAG)) {
     87            if (!r.hasKey(CONNECTIVITY_TAG)) {
     88                errors.add(TestError.builder(this, Severity.WARNING, NO_CONNECTIVITY_TAG)
     89                        .message(tr("No connectivity tag in connectivity relation")).primitives(r).build());
     90            } else if (!r.hasIncompleteMembers()) {
     91                boolean badRole = checkForBadRole(r);
     92                if (!badRole)
     93                    checkForInconsistentLanes(r);
     94            }
     95        }
     96    }
     97
     98    private void checkForInconsistentLanes(Relation relation) {
     99        // Lane count from connectivity tag
     100        Map<Integer, Map<Integer, Boolean>> connTagLanes = parseConnectivityTag(relation);
     101        // Lane count from member tags
     102        Map<String, Integer> roleLanes = new HashMap<>();
     103
     104        for (RelationMember rM : relation.getMembers()) {
     105            // Check lanes
     106            if (rM.getType() == OsmPrimitiveType.WAY) {
     107                OsmPrimitive prim = rM.getMember();
     108                if (prim.hasKey("lanes") && !rM.getRole().equals(VIA)) {
     109                    roleLanes.put(rM.getRole(), Integer.parseInt(prim.get("lanes")));
     110                }
     111            }
     112        }
     113        boolean fromCheck = roleLanes.get(FROM) < Collections
     114                .max(connTagLanes.entrySet(), Comparator.comparingInt(Map.Entry::getKey)).getKey();
     115        boolean toCheck = false;
     116        for (Entry<Integer, Map<Integer, Boolean>> to : connTagLanes.entrySet()) {
     117            toCheck = roleLanes.get(TO) < Collections
     118                    .max(to.getValue().entrySet(), Comparator.comparingInt(Map.Entry::getKey)).getKey();
     119        }
     120        if (fromCheck || toCheck) {
     121            errors.add(TestError.builder(this, Severity.WARNING, INCONSISTENT_LANE_COUNT)
     122                    .message(tr("Inconsistent lane numbering between relation and members")).primitives(relation)
     123                    .build());
     124        }
     125    }
     126
     127    private boolean checkForBadRole(Relation relation) {
     128        // Check role names
     129        int viaWays = 0;
     130        int viaNodes = 0;
     131        int toWays = 0;
     132        int fromWays = 0;
     133        for (RelationMember relationMember : relation.getMembers()) {
     134            if (relationMember.getMember() instanceof Way) {
     135                if (relationMember.hasRole(FROM))
     136                    fromWays++;
     137                else if (relationMember.hasRole(TO))
     138                    toWays++;
     139                else if (relationMember.hasRole(VIA))
     140                    viaWays++;
     141                else {
     142                    createUnknownRole(relation, relationMember.getMember());
     143                    return true;
     144                }
     145            } else if (relationMember.getMember() instanceof Node) {
     146                if (!relationMember.hasRole(VIA)) {
     147                    createUnknownRole(relation, relationMember.getMember());
     148                    return true;
     149                }
     150                viaNodes++;
     151            }
     152        }
     153        return mixedViaNodeAndWay(relation, viaWays, viaNodes, toWays, fromWays);
     154    }
     155
     156    private boolean mixedViaNodeAndWay(Relation relation, int viaWays, int viaNodes, int toWays, int fromWays) {
     157        String message = "";
     158        if ((viaWays != 0 && viaNodes != 0) || viaNodes > 1) {
     159            message = tr("Relation contains {1} {0} roles.", VIA, viaWays + viaNodes);
     160        } else if (toWays != 1) {
     161            message = tr("Relation contains too many {0} roles", TO);
     162        } else if (fromWays != 1) {
     163            message = tr("Relation contains too many {0} roles", FROM);
     164        }
     165        if (message.isEmpty()) {
     166            return false;
     167        } else {
     168            errors.add(TestError.builder(this, Severity.WARNING, TOO_MANY_ROLES)
     169                    .message(message).primitives(relation).build());
     170            return true;
     171        }
     172    }
     173
     174    private void createUnknownRole(Relation relation, OsmPrimitive primitive) {
     175        errors.add(TestError.builder(this, Severity.WARNING, UNKNOWN_CONNECTIVITY_ROLE)
     176                .message(tr("Unkown role in connectivity relation")).primitives(relation).highlight(primitive).build());
     177    }
     178}

  • test/unit/org/openstreetmap/josm/data/validation/tests/ConnectivityRelationCheckTest.java

     
     1// License: GPL. For details, see LICENSE file.
     2package org.openstreetmap.josm.data.validation.tests;
     3
     4import org.junit.Assert;
     5import org.junit.Before;
     6import org.junit.Test;
     7import org.openstreetmap.josm.JOSMFixture;
     8import org.openstreetmap.josm.TestUtils;
     9import org.openstreetmap.josm.data.coor.LatLon;
     10import org.openstreetmap.josm.data.osm.Node;
     11import org.openstreetmap.josm.data.osm.Relation;
     12import org.openstreetmap.josm.data.osm.RelationMember;
     13
     14/**
     15 * Test the ConnectivityRelationCheck validation test
     16 *
     17 * @author Taylor Smock
     18 */
     19public class ConnectivityRelationCheckTest {
     20    private ConnectivityRelationCheck check;
     21    private static final String CONNECTIVITY = "connectivity";
     22    /**
     23     * Setup test.
     24     *
     25     * @throws Exception if an error occurs
     26     */
     27    @Before
     28    public void setUp() throws Exception {
     29        JOSMFixture.createUnitTestFixture().init();
     30        check = new ConnectivityRelationCheck();
     31    }
     32
     33    private Relation createDefaultTestRelation() {
     34        Node connection = new Node(new LatLon(0, 0));
     35        return TestUtils.newRelation("type=connectivity connectivity=1:1",
     36                new RelationMember("from", TestUtils.newWay("lanes=4", new Node(new LatLon(-0.1, -0.1)), connection)),
     37                new RelationMember("via", connection),
     38                new RelationMember("to", TestUtils.newWay("lanes=4", connection, new Node(new LatLon(0.1, 0.1)))));
     39    }
     40
     41    /**
     42     * Test for connectivity relations without a connectivity tag
     43     */
     44    @Test
     45    public void testNoConnectivityTag() {
     46        Relation relation = createDefaultTestRelation();
     47        check.visit(relation);
     48
     49        Assert.assertEquals(0, check.getErrors().size());
     50
     51        relation.remove(CONNECTIVITY);
     52        check.visit(relation);
     53        Assert.assertEquals(1, check.getErrors().size());
     54    }
     55
     56    /**
     57     * Check for lanes that don't make sense
     58     */
     59    @Test
     60    public void testMisMatchedLanes() {
     61        Relation relation = createDefaultTestRelation();
     62        check.visit(relation);
     63        int expectedFailures = 0;
     64
     65        Assert.assertEquals(expectedFailures, check.getErrors().size());
     66
     67        relation.put(CONNECTIVITY, "45000:1");
     68        check.visit(relation);
     69        Assert.assertEquals(++expectedFailures, check.getErrors().size());
     70
     71        relation.put(CONNECTIVITY, "1:45000");
     72        check.visit(relation);
     73        Assert.assertEquals(++expectedFailures, check.getErrors().size());
     74
     75        relation.put(CONNECTIVITY, "1:1,2");
     76        check.visit(relation);
     77        Assert.assertEquals(expectedFailures, check.getErrors().size());
     78
     79        relation.put(CONNECTIVITY, "1:1,(2)");
     80        check.visit(relation);
     81        Assert.assertEquals(expectedFailures, check.getErrors().size());
     82
     83        relation.put(CONNECTIVITY, "1:1,(20000)");
     84        check.visit(relation);
     85        Assert.assertEquals(++expectedFailures, check.getErrors().size());
     86    }
     87
     88    /**
     89     * Check for bad roles (not from/via/to)
     90     */
     91    @Test
     92    public void testForBadRole() {
     93        Relation relation = createDefaultTestRelation();
     94        check.visit(relation);
     95        int expectedFailures = 0;
     96
     97        Assert.assertEquals(expectedFailures, check.getErrors().size());
     98
     99        for (int i = 0; i < relation.getMembers().size(); i++) {
     100            String tRole = replaceMember(relation, i, "badRole");
     101            check.visit(relation);
     102            Assert.assertEquals(++expectedFailures, check.getErrors().size());
     103            replaceMember(relation, i, tRole);
     104            check.visit(relation);
     105            Assert.assertEquals(expectedFailures, check.getErrors().size());
     106        }
     107    }
     108
     109    private String replaceMember(Relation relation, int index, String replacementRole) {
     110        RelationMember relationMember = relation.getMember(index);
     111        String currentRole = relationMember.getRole();
     112        relation.removeMember(index);
     113        relation.addMember(index, new RelationMember(replacementRole, relationMember.getMember()));
     114        return currentRole;
     115    }
     116}