Changeset 17376 in josm


Ignore:
Timestamp:
2020-11-30T10:57:50+01:00 (4 years ago)
Author:
GerdP
Message:

fix issues reported by sonar / coverity, simplify code, add unit tests to improve coverage

Location:
trunk
Files:
2 edited

Legend:

Unmodified
Added
Removed

  • trunk/src/org/openstreetmap/josm/data/validation/tests/DuplicateNode.java

    r17374 r17376  
    88
    99import java.util.ArrayList;
    10 import java.util.Collections;
    1110import java.util.HashMap;
    1211import java.util.Iterator;
    … …  
    1817import java.util.Set;
    1918import java.util.stream.Collectors;
     19import java.util.stream.Stream;
    2020
    2121import org.openstreetmap.josm.actions.MergeNodesAction;
    … …  
    2525import org.openstreetmap.josm.data.osm.Node;
    2626import org.openstreetmap.josm.data.osm.OsmPrimitive;
    27 import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
    2827import org.openstreetmap.josm.data.osm.Storage;
    2928import org.openstreetmap.josm.data.osm.Way;
    … …  
    162161        List<TestError> errors = new ArrayList<>();
    163162
    164         MultiMap<Map<String, String>, OsmPrimitive> mm = new MultiMap<>();
     163        MultiMap<Map<String, String>, Node> mm = new MultiMap<>();
    165164        for (Node n: nodes) {
    166165            mm.put(n.getKeys(), n);
    … …  
    171170        // check whether we have multiple nodes at the same position with the same tag set
    172171        for (Iterator<Map<String, String>> it = mm.keySet().iterator(); it.hasNext();) {
    173             Set<OsmPrimitive> primitives = mm.get(it.next());
     172            Set<Node> primitives = mm.get(it.next());
    174173            if (primitives.size() > 1) {
    175174
    … …  
    178177                }
    179178
    180                 for (OsmPrimitive p : primitives) {
    181                     if (p.getType() == OsmPrimitiveType.NODE) {
    182                         Node n = (Node) p;
    183                         List<OsmPrimitive> lp = n.getReferrers();
    184                         for (OsmPrimitive sp: lp) {
    185                             if (sp.getType() == OsmPrimitiveType.WAY) {
    186                                 boolean typed = false;
    187                                 Way w = (Way) sp;
    188                                 Map<String, String> keys = w.getKeys();
    189                                 for (Entry<String, Boolean> e : typeMap.entrySet()) {
    190                                     if (keys.containsKey(e.getKey())) {
    191                                         e.setValue(Boolean.TRUE);
    192                                         typed = true;
    193                                     }
    194                                 }
    195                                 if (!typed) {
    196                                     typeMap.put("none", Boolean.TRUE);
    197                                 }
     179                for (Node n : primitives) {
     180                    for (Way w: n.getParentWays()) {
     181                        boolean typed = false;
     182                        Map<String, String> keys = w.getKeys();
     183                        for (Entry<String, Boolean> e : typeMap.entrySet()) {
     184                            if (keys.containsKey(e.getKey())) {
     185                                e.setValue(Boolean.TRUE);
     186                                typed = true;
    198187                            }
     188                        }
     189                        if (!typed) {
     190                            typeMap.put("none", Boolean.TRUE);
    199191                        }
    200192                    }
    … …  
    202194
    203195                long nbType = typeMap.entrySet().stream().filter(Entry::getValue).count();
    204 
     196                final TestError.Builder builder;
    205197                if (nbType > 1) {
    206                     errors.add(TestError.builder(parentTest, Severity.WARNING, DUPLICATE_NODE_MIXED)
    207                             .message(tr("Mixed type duplicated nodes"))
    208                             .primitives(primitives)
    209                             .build());
    210                 } else if (typeMap.get(HIGHWAY)) {
    211                     errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_HIGHWAY)
    212                             .message(tr("Highway duplicated nodes"))
    213                             .primitives(primitives)
    214                             .build());
    215                 } else if (typeMap.get(RAILWAY)) {
    216                     errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_RAILWAY)
    217                             .message(tr("Railway duplicated nodes"))
    218                             .primitives(primitives)
    219                             .build());
    220                 } else if (typeMap.get(WATERWAY)) {
    221                     errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_WATERWAY)
    222                             .message(tr("Waterway duplicated nodes"))
    223                             .primitives(primitives)
    224                             .build());
    225                 } else if (typeMap.get("boundary")) {
    226                     errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_BOUNDARY)
    227                             .message(tr("Boundary duplicated nodes"))
    228                             .primitives(primitives)
    229                             .build());
    230                 } else if (typeMap.get("power")) {
    231                     errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_POWER)
    232                             .message(tr("Power duplicated nodes"))
    233                             .primitives(primitives)
    234                             .build());
    235                 } else if (typeMap.get("natural")) {
    236                     errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_NATURAL)
    237                             .message(tr("Natural duplicated nodes"))
    238                             .primitives(primitives)
    239                             .build());
    240                 } else if (typeMap.get("building")) {
    241                     errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_BUILDING)
    242                             .message(tr("Building duplicated nodes"))
    243                             .primitives(primitives)
    244                             .build());
    245                 } else if (typeMap.get("landuse")) {
    246                     errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_LANDUSE)
    247                             .message(tr("Landuse duplicated nodes"))
    248                             .primitives(primitives)
    249                             .build());
     198                    builder = TestError.builder(parentTest, Severity.WARNING, DUPLICATE_NODE_MIXED)
     199                            .message(tr("Mixed type duplicated nodes"));
     200                } else if (Boolean.TRUE.equals(typeMap.get(HIGHWAY))) {
     201                    builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_HIGHWAY)
     202                            .message(tr("Highway duplicated nodes"));
     203                } else if (Boolean.TRUE.equals(typeMap.get(RAILWAY))) {
     204                    builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_RAILWAY)
     205                            .message(tr("Railway duplicated nodes"));
     206                } else if (Boolean.TRUE.equals(typeMap.get(WATERWAY))) {
     207                    builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_WATERWAY)
     208                            .message(tr("Waterway duplicated nodes"));
     209                } else if (Boolean.TRUE.equals(typeMap.get("boundary"))) {
     210                    builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_BOUNDARY)
     211                            .message(tr("Boundary duplicated nodes"));
     212                } else if (Boolean.TRUE.equals(typeMap.get("power"))) {
     213                    builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_POWER)
     214                            .message(tr("Power duplicated nodes"));
     215                } else if (Boolean.TRUE.equals(typeMap.get("natural"))) {
     216                    builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_NATURAL)
     217                            .message(tr("Natural duplicated nodes"));
     218                } else if (Boolean.TRUE.equals(typeMap.get("building"))) {
     219                    builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_BUILDING)
     220                            .message(tr("Building duplicated nodes"));
     221                } else if (Boolean.TRUE.equals(typeMap.get("landuse"))) {
     222                    builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_LANDUSE)
     223                            .message(tr("Landuse duplicated nodes"));
    250224                } else {
    251                     errors.add(TestError.builder(parentTest, Severity.WARNING, DUPLICATE_NODE_OTHER)
    252                             .message(tr("Other duplicated nodes"))
    253                             .primitives(primitives)
    254                             .build());
     225                    builder = TestError.builder(parentTest, Severity.WARNING, DUPLICATE_NODE_OTHER)
     226                            .message(tr("Other duplicated nodes"));
    255227                }
     228                errors.add(builder.primitives(primitives).build());
    256229                it.remove();
    257230            }
    … …  
    261234        if (!mm.isEmpty()) {
    262235            List<OsmPrimitive> duplicates = new ArrayList<>();
    263             for (Set<OsmPrimitive> l: mm.values()) {
     236            for (Set<Node> l: mm.values()) {
    264237                duplicates.addAll(l);
    265238            }
    … …  
    278251    public void visit(Node n) {
    279252        if (n.isUsable()) {
    280             if (potentialDuplicates.get(n) == null) {
     253            Object old = potentialDuplicates.get(n);
     254            if (old == null) {
    281255                // in most cases there is just one node at a given position. We
    282256                // avoid to create an extra object and add remember the node
    283257                // itself at this position
    284258                potentialDuplicates.put(n);
    285             } else if (potentialDuplicates.get(n) instanceof Node) {
     259            } else if (old instanceof Node) {
    286260                // we have an additional node at the same position. Create an extra
    287261                // object to keep track of the nodes at this position.
    288262                //
    289                 Node n1 = (Node) potentialDuplicates.get(n);
    290                 List<Node> nodes = new ArrayList<>(2);
    291                 nodes.add(n1);
    292                 nodes.add(n);
    293                 potentialDuplicates.put(nodes);
    294             } else if (potentialDuplicates.get(n) instanceof List<?>) {
    295                 // we have multiple nodes at the same position.
     263                potentialDuplicates.put(Stream.of((Node) old, n).collect(Collectors.toList()));
     264            } else {
     265                // we have more  than two nodes at the same position.
    296266                //
    297                 List<Node> nodes = (List<Node>) potentialDuplicates.get(n);
    298                 nodes.add(n);
     267                ((List<Node>) old).add(n);
    299268            }
    300269        }
    … …  
    307276    @Override
    308277    public Command fixError(TestError testError) {
     278        if (!isFixable(testError))
     279            return null;
    309280        final Set<Node> nodes = testError.primitives(Node.class)
    310281                // Filter nodes that have already been deleted (see #5764 and #5773)
    … …  
    312283                .collect(Collectors.toCollection(LinkedHashSet::new));
    313284
    314         // Merge only if at least 2 nodes remain
    315         if (nodes.size() >= 2) {
    316             // Use first existing node or first node if all nodes are new
    317             Node target = nodes.stream()
    318                     .filter(n -> !n.isNew())
    319                     .findFirst()
    320                     .orElseGet(() -> nodes.iterator().next());
    321 
    322             if (Command.checkOutlyingOrIncompleteOperation(nodes, Collections.singleton(target)) == Command.IS_OK)
    323                 return MergeNodesAction.mergeNodes(nodes, target);
    324         }
    325 
    326         return null; // undoRedo handling done in mergeNodes
     285        // Use first existing node or first node if all nodes are new
     286        Node target = nodes.stream()
     287                .filter(n -> !n.isNew())
     288                .findFirst()
     289                .orElseGet(() -> nodes.iterator().next());
     290
     291        return MergeNodesAction.mergeNodes(nodes, target);
    327292    }
    328293

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

    r17275 r17376  
    33
    44import static org.junit.jupiter.api.Assertions.assertEquals;
    5 
     5import static org.junit.jupiter.api.Assertions.assertFalse;
     6import static org.junit.jupiter.api.Assertions.assertNotNull;
     7import static org.junit.jupiter.api.Assertions.assertTrue;
     8
     9import org.junit.jupiter.api.Test;
    610import org.junit.jupiter.api.extension.RegisterExtension;
    7 import org.junit.jupiter.api.Test;
     11import org.openstreetmap.josm.command.Command;
    812import org.openstreetmap.josm.data.coor.LatLon;
    913import org.openstreetmap.josm.data.osm.DataSet;
    … …  
    4448        assertEquals(code, error.getCode());
    4549        assertEquals(fixable, error.isFixable());
     50        if (fixable) {
     51            Command c = error.getFix();
     52            assertNotNull(c);
     53            c.executeCommand();
     54            assertFalse(error.isFixable());
     55            c.undoCommand();
     56            assertTrue(error.isFixable());
     57            error.getPrimitives().iterator().next().setDeleted(true);
     58            if (error.getPrimitives().size() == 2) {
     59                assertFalse(error.isFixable());
     60            } else {
     61                assertTrue(error.isFixable());
     62            }
     63            error.getPrimitives().iterator().next().setDeleted(false);
     64        }
    4665    }
    4766
    … …  
    6786
    6887    /**
     88     * Test of "Duplicate node" validation test - no duplicate
     89     */
     90    @Test
     91    void testNoDuplicateNode() {
     92        DataSet ds = new DataSet();
     93
     94        Node a = new Node(new LatLon(10.0, 5.0));
     95        Node b = new Node(new LatLon(10.0, 6.0));
     96        ds.addPrimitive(a);
     97        ds.addPrimitive(b);
     98
     99        a.put("foo", "bar");
     100        b.put("bar", "foo");
     101
     102        TEST.startTest(NullProgressMonitor.INSTANCE);
     103        TEST.visit(ds.allPrimitives());
     104        TEST.endTest();
     105
     106        assertEquals(0, TEST.getErrors().size());
     107    }
     108
     109    /**
     110     * Test of "Duplicate node" validation test - same position, with ele value
     111     */
     112    @Test
     113    void testDuplicateNodeWithEle() {
     114        DataSet ds = new DataSet();
     115
     116        Node a = new Node(new LatLon(10.0, 5.0));
     117        Node b = new Node(new LatLon(10.0, 5.0));
     118        ds.addPrimitive(a);
     119        ds.addPrimitive(b);
     120
     121        a.put("foo", "bar");
     122        b.put("bar", "foo");
     123        a.put("ele", "100");
     124        b.put("ele", "100");
     125
     126        TEST.startTest(NullProgressMonitor.INSTANCE);
     127        TEST.visit(ds.allPrimitives());
     128        TEST.endTest();
     129
     130        assertEquals(1, TEST.getErrors().size());
     131
     132        b.put("ele", "110");
     133
     134        TEST.startTest(NullProgressMonitor.INSTANCE);
     135        TEST.visit(ds.allPrimitives());
     136        TEST.endTest();
     137
     138        assertEquals(0, TEST.getErrors().size());
     139    }
     140
     141    /**
     142     * Test of "Duplicate node" validation test - three nodes
     143     */
     144    @Test
     145    void testDuplicateNodeTriple() {
     146        DataSet ds = new DataSet();
     147   
     148        Node a = new Node(new LatLon(10.0, 5.0));
     149        Node b = new Node(new LatLon(10.0, 5.0));
     150        Node c = new Node(new LatLon(10.0, 5.0));
     151        ds.addPrimitive(a);
     152        ds.addPrimitive(b);
     153        ds.addPrimitive(c);
     154   
     155        performTest(DuplicateNode.DUPLICATE_NODE_OTHER, ds, true);
     156        a.put("foo", "bar");
     157        b.put("foo", "bar");
     158        performTest(DuplicateNode.DUPLICATE_NODE_OTHER, ds, true);
     159    }
     160
     161    /**
    69162     * Test of "Duplicate node" validation test - different tag sets.
    70163     */
Note: See TracChangeset for help on using the changeset viewer.