Index: src/org/openstreetmap/josm/data/validation/tests/DuplicateRelation.java =================================================================== --- src/org/openstreetmap/josm/data/validation/tests/DuplicateRelation.java (revision 18972) +++ src/org/openstreetmap/josm/data/validation/tests/DuplicateRelation.java (working copy) @@ -1,11 +1,11 @@ // License: GPL. For details, see LICENSE file. 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; import java.util.Map; @@ -19,6 +19,7 @@ import org.openstreetmap.josm.command.SequenceCommand; 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; import org.openstreetmap.josm.data.osm.OsmPrimitiveType; @@ -36,6 +37,8 @@ */ public class DuplicateRelation extends Test { + private static final String DUPLICATED_RELATIONS = marktr("Duplicated relations"); + /** * Class to store one relation members and information about it */ @@ -110,13 +113,13 @@ */ private static class RelationMembers { /** Set of member objects of the relation */ - private final Set members; + private final List members; /** Store relation information * @param members The list of relation members */ RelationMembers(List members) { - this.members = new HashSet<>(members.size()); + this.members = new ArrayList<>(members.size()); for (RelationMember member : members) { this.members.add(new RelMember(member)); } @@ -175,20 +178,18 @@ /** Code number of relation with same members error */ protected static final int SAME_RELATION = 1902; - /** MultiMap of all relations */ - private MultiMap relations; + /** Code number of relation with same members error */ + protected static final int IDENTICAL_MEMBERLIST = 1903; - /** MultiMap of all relations, regardless of keys */ - private MultiMap, OsmPrimitive> relationsNoKeys; - /** List of keys without useful information */ - private final Set ignoreKeys = new HashSet<>(AbstractPrimitive.getUninterestingKeys()); + /** List of all initially visited testable relations*/ + List visited; /** * Default constructor */ 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.")); } @@ -195,51 +196,199 @@ @Override 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 duplicated : relations.values()) { + if (visited.isEmpty()) + return; + + MultiMap relations = new MultiMap<>(1000); + /** MultiMap of all relations, regardless of keys */ + MultiMap, Relation> sameMembers = new MultiMap<>(1000); + + for (Relation r : visited) { + final List 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 rMembers = getSortedMembers(r); + if (sameMembers.containsKey(rMembers)) + sameMembers.put(rMembers, r); + + addToRelations(relations, r, false); + } + } + + for (Set duplicated : sameMembers.values()) { if (duplicated.size() > 1) { - TestError testError = TestError.builder(this, Severity.ERROR, DUPLICATE_RELATION) - .message(tr("Duplicated relations")) - .primitives(duplicated) - .build(); - errors.add(testError); + checkOrderAndTags(duplicated); } } - relations = null; - for (Set duplicated : relationsNoKeys.values()) { + + performGeometryTest(relations); + visited = null; + super.endTest(); + } + + private void performGeometryTest(MultiMap 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 duplicated : relations.values()) { if (duplicated.size() > 1) { - TestError testError = TestError.builder(this, Severity.OTHER, SAME_RELATION) - .message(tr("Relations with same members")) + TestError testError = TestError.builder(this, Severity.ERROR, DUPLICATE_RELATION) + .message(tr(DUPLICATED_RELATIONS)) .primitives(duplicated) .build(); - errors.add(testError); + if (errors.stream().noneMatch(e -> e.isSimilar(testError))) { + errors.add(testError); + } } } - relationsNoKeys = null; - super.endTest(); } + private static void addToRelations(MultiMap 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 rMembers = r.getMembers(); - Map rkeys = r.getKeys(); - for (String key : ignoreKeys) { - rkeys.remove(key); + 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 sameMembers) { + MultiMap, Relation> sameOrder = new MultiMap<>(); + MultiMap, Relation> sameKeys = new MultiMap<>(); + for (Relation r : sameMembers) { + sameOrder.put(r.getMembers(), r); + sameKeys.put(cleanedKeys(r), r); } - RelationPair rKey = new RelationPair(rMembers, rkeys); - relations.put(rKey, r); - relationsNoKeys.put(rMembers, r); + for (Set duplicated : sameKeys.values()) { + if (duplicated.size() > 1) { + reportDuplicateKeys(duplicated); + } + } + for (Set duplicated : sameOrder.values()) { + if (duplicated.size() > 1) { + reportDuplicateMembers(duplicated); + } + } + List 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 duplicated) { + Relation first = duplicated.iterator().next(); + if (memberOrderMatters(first)) { + List toCheck = new ArrayList<>(duplicated); + while (toCheck.size() > 1) { + Relation ref = toCheck.iterator().next(); + List 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 duplicated) { + List 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 cleanedKeys(OsmPrimitive p) { + Map 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 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()); + } + + /** * Fix the error by removing all but one instance of duplicate relations * @param testError The error to fix, must be of type {@link #DUPLICATE_RELATION} */ @@ -300,7 +449,7 @@ @Override 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. Set rels = testError.primitives(Relation.class) Index: test/unit/org/openstreetmap/josm/data/validation/tests/DuplicateRelationTest.java =================================================================== --- test/unit/org/openstreetmap/josm/data/validation/tests/DuplicateRelationTest.java (revision 18972) +++ test/unit/org/openstreetmap/josm/data/validation/tests/DuplicateRelationTest.java (working copy) @@ -3,6 +3,7 @@ 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; import org.openstreetmap.josm.data.osm.DataSet; @@ -12,8 +13,6 @@ import org.openstreetmap.josm.gui.progress.NullProgressMonitor; import org.openstreetmap.josm.testutils.annotations.BasicPreferences; -import org.junit.jupiter.api.Test; - /** * JUnit Test of "Duplicate relation" validation test. */ @@ -44,6 +43,8 @@ ExpectedResult expected = expectations[i++]; assertEquals(expected.code, error.getCode()); assertEquals(expected.fixable, error.isFixable()); + if (error.isFixable()) + error.getFix(); } } @@ -63,8 +64,8 @@ @Test void testDuplicateRelationNoTags() { doTest("", "", - new ExpectedResult(DuplicateRelation.DUPLICATE_RELATION, true), - new ExpectedResult(DuplicateRelation.SAME_RELATION, false)); + new ExpectedResult(DuplicateRelation.DUPLICATE_RELATION, true) + ); } /** @@ -73,8 +74,8 @@ @Test 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) + ); } /** @@ -83,6 +84,122 @@ @Test 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)); + } }