Changeset 18976 in josm

Timestamp:

2024-02-13T09:29:09+01:00 (3 months ago)

Author:

GerdP

Message:

fix #20424, fix #20425: Duplicate relations not detected with incomplete members, Duplicate Relation test is too lazy/too aggressive

• add code to compare just members so that it doesn't matter if they are complete
• change original test which compares geometry to use a list instead of a set to store members. That means that the order of members is taken into account as well as duplicated members
• before comparing tags only the "discardable" tag keys are removed, previously all "uninteresting" keys were removed, so fixme=* or note=* was ignored
• if two relations have the same members in the same order but different tags the new message informational message "Identical members" is produced.

Location:

trunk

Files:

• src/org/openstreetmap/josm/data/validation/tests/DuplicateRelation.java (modified) (9 diffs)
• test/unit/org/openstreetmap/josm/data/validation/tests/DuplicateRelationTest.java (modified) (6 diffs)

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

@@ -2 +2 @@
 package org.openstreetmap.josm.data.validation.tests;
 
+import static org.openstreetmap.josm.tools.I18n.marktr;
 import static org.openstreetmap.josm.tools.I18n.tr;
 
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
@@ -20 +20 @@
 import org.openstreetmap.josm.data.coor.LatLon;
 import org.openstreetmap.josm.data.osm.AbstractPrimitive;
+import org.openstreetmap.josm.data.osm.DataSet;
 import org.openstreetmap.josm.data.osm.Node;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
@@ -37 +38 @@
 public class DuplicateRelation extends Test {
 
+    private static final String DUPLICATED_RELATIONS = marktr("Duplicated relations");
+
     /**
      * Class to store one relation members and information about it
@@ -111 +114 @@
     private static class RelationMembers {
         /** Set of member objects of the relation */
-        private final Set<RelMember> members;
+        private final List<RelMember> members;
 
         /** Store relation information
@@ -117 +120 @@
          */
         RelationMembers(List<RelationMember> members) {
-            this.members = new HashSet<>(members.size());
+            this.members = new ArrayList<>(members.size());
             for (RelationMember member : members) {
                 this.members.add(new RelMember(member));
@@ -176 +179 @@
     protected static final int SAME_RELATION = 1902;
 
-    /** MultiMap of all relations */
-    private MultiMap<RelationPair, OsmPrimitive> relations;
-
-    /** MultiMap of all relations, regardless of keys */
-    private MultiMap<List<RelationMember>, OsmPrimitive> relationsNoKeys;
-
-    /** List of keys without useful information */
-    private final Set<String> ignoreKeys = new HashSet<>(AbstractPrimitive.getUninterestingKeys());
+    /** Code number of relation with same members error */
+    protected static final int IDENTICAL_MEMBERLIST = 1903;
+
+
+    /** List of all initially visited testable relations*/
+    List<Relation> visited;
 
     /**
@@ -189 +190 @@
      */
     public DuplicateRelation() {
-        super(tr("Duplicated relations"),
+        super(tr(DUPLICATED_RELATIONS),
                 tr("This test checks that there are no relations with same tags and same members with same roles."));
     }
@@ -196 +197 @@
     public void startTest(ProgressMonitor monitor) {
         super.startTest(monitor);
-        relations = new MultiMap<>(1000);
-        relationsNoKeys = new MultiMap<>(1000);
+        visited = new ArrayList<>();
+
     }
 
     @Override
     public void endTest() {
-        for (Set<OsmPrimitive> duplicated : relations.values()) {
+        if (!visited.isEmpty())
+            performChecks();
+        visited = null;
+        super.endTest();
+    }
+
+    private void performChecks() {
+        MultiMap<RelationPair, Relation> relations = new MultiMap<>(1000);
+        // MultiMap of all relations, regardless of keys
+        MultiMap<List<RelationMember>, Relation> sameMembers = new MultiMap<>(1000);
+
+        for (Relation r : visited) {
+            final List<RelationMember> rMembers = getSortedMembers(r);
+            sameMembers.put(rMembers, r);
+            addToRelations(relations, r, true);
+        }
+
+        if (partialSelection) {
+            // add data for relations which were not in the initial selection when
+            // they can be duplicates
+            DataSet ds = visited.iterator().next().getDataSet();
+            for (Relation r : ds.getRelations()) {
+                if (r.isDeleted() || r.getMembers().isEmpty() || visited.contains(r))
+                    continue;
+                final List<RelationMember> rMembers = getSortedMembers(r);
+                if (sameMembers.containsKey(rMembers))
+                    sameMembers.put(rMembers, r);
+
+                addToRelations(relations, r, false);
+            }
+        }
+
+        for (Set<Relation> duplicated : sameMembers.values()) {
+            if (duplicated.size() > 1) {
+                checkOrderAndTags(duplicated);
+            }
+        }
+
+        performGeometryTest(relations);
+    }
+
+    private void performGeometryTest(MultiMap<RelationPair, Relation> relations) {
+        // perform special test to find relations with different members but (possibly) same geometry
+        // this test is rather speculative and works only with complete members
+        for (Set<Relation> duplicated : relations.values()) {
             if (duplicated.size() > 1) {
                 TestError testError = TestError.builder(this, Severity.ERROR, DUPLICATE_RELATION)
-                        .message(tr("Duplicated relations"))
+                        .message(tr(DUPLICATED_RELATIONS))
                         .primitives(duplicated)
                         .build();
-                errors.add(testError);
-            }
-        }
-        relations = null;
-        for (Set<OsmPrimitive> duplicated : relationsNoKeys.values()) {
-            if (duplicated.size() > 1) {
-                TestError testError = TestError.builder(this, Severity.OTHER, SAME_RELATION)
-                        .message(tr("Relations with same members"))
-                        .primitives(duplicated)
-                        .build();
-                errors.add(testError);
-            }
-        }
-        relationsNoKeys = null;
-        super.endTest();
+                if (errors.stream().noneMatch(e -> e.isSimilar(testError))) {
+                    errors.add(testError);
+                }
+            }
+        }
+    }
+
+    private static void addToRelations(MultiMap<RelationPair, Relation> relations, Relation r, boolean forceAdd) {
+        if (r.isUsable() && !r.hasIncompleteMembers()) {
+            RelationPair rKey = new RelationPair(r.getMembers(), cleanedKeys(r));
+            if (forceAdd || (!relations.isEmpty() && !relations.containsKey(rKey)))
+                relations.put(rKey, r);
+        }
     }
 
     @Override
     public void visit(Relation r) {
-        if (!r.isUsable() || r.hasIncompleteMembers() || "tmc".equals(r.get("type")) || "TMC".equals(r.get("type"))
-               || "destination_sign".equals(r.get("type")) || r.getMembers().isEmpty())
-            return;
-        List<RelationMember> rMembers = r.getMembers();
-        Map<String, String> rkeys = r.getKeys();
-        for (String key : ignoreKeys) {
-            rkeys.remove(key);
-        }
-        RelationPair rKey = new RelationPair(rMembers, rkeys);
-        relations.put(rKey, r);
-        relationsNoKeys.put(rMembers, r);
+        if (!r.isDeleted() && r.getMembersCount() > 0)
+            visited.add(r);
+    }
+
+    /**
+     * Check a list of relations which are guaranteed to have the same members, possibly in different order
+     * and possibly different tags.
+     * @param sameMembers the list of relations
+     */
+    private void checkOrderAndTags(Set<Relation> sameMembers) {
+        MultiMap<List<RelationMember>, Relation> sameOrder = new MultiMap<>();
+        MultiMap<Map<String, String>, Relation> sameKeys = new MultiMap<>();
+        for (Relation r : sameMembers) {
+            sameOrder.put(r.getMembers(), r);
+            sameKeys.put(cleanedKeys(r), r);
+        }
+        for (Set<Relation> duplicated : sameKeys.values()) {
+            if (duplicated.size() > 1) {
+                reportDuplicateKeys(duplicated);
+            }
+        }
+        for (Set<Relation> duplicated : sameOrder.values()) {
+            if (duplicated.size() > 1) {
+                reportDuplicateMembers(duplicated);
+            }
+        }
+        List<Relation> primitives = sameMembers.stream().filter(r -> !ignoredType(r)).collect(Collectors.toList());
+        // report this collection if not already reported
+        if (primitives.size() > 1 && errors.stream().noneMatch(e -> e.getPrimitives().containsAll(primitives))) {
+            // same members, possibly different order
+            TestError testError = TestError.builder(this, Severity.OTHER, SAME_RELATION)
+                    .message(tr("Relations with same members"))
+                    .primitives(primitives)
+                    .build();
+            errors.add(testError);
+        }
+    }
+
+    /**
+     * Check collection of relations with the same keys and members, possibly in different order
+     * @param duplicated collection of relations, caller must make sure that they have the same keys and members
+     */
+    private void reportDuplicateKeys(Collection<Relation> duplicated) {
+        Relation first = duplicated.iterator().next();
+        if (memberOrderMatters(first)) {
+            List<Relation> toCheck = new ArrayList<>(duplicated);
+            while (toCheck.size() > 1) {
+                Relation ref = toCheck.iterator().next();
+                List<Relation> same = toCheck.stream()
+                        .filter(r -> r.getMembers().equals(ref.getMembers())).collect(Collectors.toList());
+                if (same.size() > 1) {
+                    // same members and keys, members in same order
+                    TestError testError = TestError.builder(this, Severity.ERROR, DUPLICATE_RELATION)
+                            .message(tr(DUPLICATED_RELATIONS))
+                            .primitives(same)
+                            .build();
+                    errors.add(testError);
+                }
+                toCheck.removeAll(same);
+            }
+        } else {
+            // same members and keys, possibly different order
+            TestError testError = TestError.builder(this, Severity.ERROR, DUPLICATE_RELATION)
+                    .message(tr(DUPLICATED_RELATIONS))
+                    .primitives(duplicated)
+                    .build();
+            errors.add(testError);
+        }
+    }
+
+    /**
+     * Report relations with the same member(s) in the same order, keys may be different
+     * @param duplicated collection of relations with identical member list
+     */
+    private void reportDuplicateMembers(Set<Relation> duplicated) {
+        List<Relation> primitives = duplicated.stream().filter(r -> !ignoredType(r)).collect(Collectors.toList());
+        if (primitives.size() > 1 && errors.stream().noneMatch(e -> e.getPrimitives().containsAll(primitives))) {
+            TestError testError = TestError.builder(this, Severity.OTHER, IDENTICAL_MEMBERLIST)
+                    .message(tr("Identical members"))
+                    .primitives(primitives)
+                    .build();
+            errors.add(testError);
+        }
+
+    }
+
+    private static boolean memberOrderMatters(Relation r) {
+        return r.hasTag("type", "route", "waterway"); // add more types?
+    }
+
+    private static boolean ignoredType(Relation r) {
+        return r.hasTag("type", "tmc", "TMC", "destination_sign"); // see r11783
+    }
+
+    /** return tags of a primitive after removing all discardable tags
+     *
+     * @param p the primitive
+     * @return map with cleaned tags
+     */
+    private static Map<String, String> cleanedKeys(OsmPrimitive p) {
+        Map<String, String> cleaned = p.getKeys();
+        for (String key : AbstractPrimitive.getDiscardableKeys()) {
+            cleaned.remove(key);
+        }
+        return cleaned;
+    }
+
+    /**
+     *  Order members of given relation by type, unique id, and role.
+     *  @param r the relation
+     * @return sorted list of members
+     */
+    private static List<RelationMember> getSortedMembers(Relation r) {
+        return r.getMembers().stream().sorted((m1, m2) -> {
+            int d = m1.getMember().compareTo(m2.getMember());
+            if (d != 0)
+                return d;
+            return m1.getRole().compareTo(m2.getRole());
+        }).collect(Collectors.toList());
     }
 
@@ -301 +452 @@
     public boolean isFixable(TestError testError) {
         if (!(testError.getTester() instanceof DuplicateRelation)
-            || testError.getCode() == SAME_RELATION) return false;
+            || testError.getCode() != DUPLICATE_RELATION) return false;
 
         // We fix it only if there is no more than one relation that is relation member.

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

@@ -4 +4 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
+import org.junit.jupiter.api.Test;
 import org.openstreetmap.josm.TestUtils;
 import org.openstreetmap.josm.data.coor.LatLon;
@@ -12 +13 @@
 import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
 import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
-
-import org.junit.jupiter.api.Test;
 
 /**
@@ -45 +44 @@
             assertEquals(expected.code, error.getCode());
             assertEquals(expected.fixable, error.isFixable());
+            if (error.isFixable())
+                error.getFix();
         }
     }
@@ -64 +65 @@
     void testDuplicateRelationNoTags() {
         doTest("", "",
-                new ExpectedResult(DuplicateRelation.DUPLICATE_RELATION, true),
-                new ExpectedResult(DuplicateRelation.SAME_RELATION, false));
+                new ExpectedResult(DuplicateRelation.DUPLICATE_RELATION, true)
+                );
     }
 
@@ -74 +75 @@
     void testDuplicateRelationSameTags() {
         doTest("type=boundary", "type=boundary",
-                new ExpectedResult(DuplicateRelation.DUPLICATE_RELATION, true),
-                new ExpectedResult(DuplicateRelation.SAME_RELATION, false));
+                new ExpectedResult(DuplicateRelation.DUPLICATE_RELATION, true)
+                );
     }
 
@@ -84 +85 @@
     void testDuplicateRelationDifferentTags() {
         doTest("type=boundary", "type=multipolygon",
-                new ExpectedResult(DuplicateRelation.SAME_RELATION, false));
+                new ExpectedResult(DuplicateRelation.IDENTICAL_MEMBERLIST, false));
+    }
+
+    /**
+     * Test of duplicate "tmc" relation, should not be ignored
+     */
+    @Test
+    void testTMCRelation1() {
+        doTest("type=tmc t1=v1", "type=tmc t1=v1",
+                new ExpectedResult(DuplicateRelation.DUPLICATE_RELATION, true));
+    }
+
+    /**
+     * Test of "tmc" relation with equal members but different tags, should be ignored
+     */
+    @Test
+    void testTMCRelation2() {
+        doTest("type=tmc t1=v1", "type=tmc t1=v2");
+    }
+
+    /**
+     * Test with incomplete members
+     */
+    @Test
+    void testIncomplete() {
+        DataSet ds = new DataSet();
+
+        Node a = new Node(1234);
+        ds.addPrimitive(a);
+        ds.addPrimitive(TestUtils.newRelation("type=multipolygon", new RelationMember(null, a)));
+        ds.addPrimitive(TestUtils.newRelation("type=multipolygon", new RelationMember(null, a)));
+        performTest(ds, new ExpectedResult(DuplicateRelation.DUPLICATE_RELATION, true));
+    }
+
+    /**
+     * Test with different order of members, order doesn't count
+     */
+    @Test
+    void testMemberOrder1() {
+        DataSet ds = new DataSet();
+
+        Node a = new Node(1);
+        Node b = new Node(2);
+        ds.addPrimitive(a);
+        ds.addPrimitive(b);
+        ds.addPrimitive(TestUtils.newRelation("type=multipolygon", new RelationMember(null, a), new RelationMember(null, b)));
+        ds.addPrimitive(TestUtils.newRelation("type=multipolygon", new RelationMember(null, b), new RelationMember(null, a)));
+        performTest(ds, new ExpectedResult(DuplicateRelation.DUPLICATE_RELATION, true));
+    }
+
+    /**
+     * Test with different order of members, order counts
+     */
+    @Test
+    void testMemberOrder2() {
+        DataSet ds = new DataSet();
+
+        Node a = new Node(1);
+        a.setCoor(new LatLon(10.0, 5.0));
+        Node b = new Node(2);
+        b.setCoor(new LatLon(10.0, 6.0));
+        ds.addPrimitive(a);
+        ds.addPrimitive(b);
+        ds.addPrimitive(TestUtils.newRelation("type=route", new RelationMember(null, a), new RelationMember(null, b)));
+        ds.addPrimitive(TestUtils.newRelation("type=route", new RelationMember(null, b), new RelationMember(null, a)));
+        performTest(ds, new ExpectedResult(DuplicateRelation.SAME_RELATION, false));
+    }
+
+    /**
+     * Test with different order of members, one is duplicated, order doesn't matter
+     */
+    @Test
+    void testMemberOrder3() {
+        DataSet ds = new DataSet();
+
+        Node a = new Node(1);
+        Node b = new Node(2);
+        ds.addPrimitive(a);
+        ds.addPrimitive(b);
+        ds.addPrimitive(TestUtils.newRelation("type=restriction", new RelationMember(null, a),
+                new RelationMember(null, b), new RelationMember(null, a)));
+        ds.addPrimitive(TestUtils.newRelation("type=restriction", new RelationMember(null, b),
+                new RelationMember(null, a), new RelationMember(null, a)));
+        performTest(ds, new ExpectedResult(DuplicateRelation.DUPLICATE_RELATION, true));
+    }
+
+    /**
+     * Test with different order of members, one is duplicated in one of the relations
+     */
+    @Test
+    void testMemberOrder4() {
+        DataSet ds = new DataSet();
+        Node a = new Node(new LatLon(10.0, 5.0));
+        Node b = new Node(new LatLon(10.0, 6.0));
+        ds.addPrimitive(a);
+        ds.addPrimitive(b);
+        ds.addPrimitive(TestUtils.newRelation("", new RelationMember(null, a), new RelationMember(null, b)));
+        ds.addPrimitive(TestUtils.newRelation("", new RelationMember(null, b), new RelationMember(null, a), new RelationMember(null, b)));
+        performTest(ds);
+    }
+
+    /**
+     * Test with two relations where members are different but geometry is equal.
+     */
+    @Test
+    void testImport() {
+        DataSet ds = new DataSet();
+        Node a = new Node(1234, 1);
+        a.setCoor(new LatLon(10.0, 5.0));
+
+        Node b = new Node(new LatLon(10.0, 5.0));
+
+        ds.addPrimitive(a);
+        ds.addPrimitive(b);
+        ds.addPrimitive(TestUtils.newRelation("type=multipolygon", new RelationMember(null, a)));
+        ds.addPrimitive(TestUtils.newRelation("type=multipolygon", new RelationMember(null, b)));
+        performTest(ds, new ExpectedResult(DuplicateRelation.DUPLICATE_RELATION, true));
     }
 }