Changeset 17341 in josm for trunk


Ignore:
Timestamp:
2020-11-23T20:45:59+01:00 (4 years ago)
Author:
GerdP
Message:

fix #20091: Downloading incomplete, deleted members leads to data inconsitency

  • changes the logic of "download members" so that all non-new members of all selected relations are downloaded. It doesn't recurse down on child relations. The old code did a download of members using the relation id, so the server returned the members of the version as known by the server. Disadvantage: The new code is slower.
  • changes and simplifies DataSetMerger to fix the wrong handling of deleted objects in the source
  • additional unit tests to cover more branches in mergeById()
  • also fixes #19783
Location:
trunk
Files:
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/src/org/openstreetmap/josm/actions/relation/DownloadMembersAction.java

    r14397 r17341  
    77import java.awt.event.ActionEvent;
    88import java.util.Collection;
     9import java.util.List;
     10import java.util.stream.Collectors;
    911
    1012import org.openstreetmap.josm.data.osm.IPrimitive;
    11 import org.openstreetmap.josm.data.osm.Relation;
     13import org.openstreetmap.josm.data.osm.PrimitiveId;
    1214import org.openstreetmap.josm.gui.MainApplication;
    13 import org.openstreetmap.josm.gui.dialogs.relation.DownloadRelationTask;
     15import org.openstreetmap.josm.gui.io.DownloadPrimitivesTask;
    1416import org.openstreetmap.josm.tools.ImageProvider;
    15 import org.openstreetmap.josm.tools.SubclassFilteredCollection;
    16 import org.openstreetmap.josm.tools.Utils;
    1717
    1818/**
    … …  
    3535    public void actionPerformed(ActionEvent e) {
    3636        if (!isEnabled() || relations.isEmpty() || !MainApplication.isDisplayingMapView()) return;
    37         MainApplication.worker.submit(new DownloadRelationTask(
    38                 Utils.filteredCollection(relations, Relation.class), MainApplication.getLayerManager().getEditLayer()));
     37        List<PrimitiveId> members = relations.stream()
     38                .flatMap(r -> r.getMemberPrimitivesList().stream().filter(osm -> !osm.isNew()).map(IPrimitive::getOsmPrimitiveId))
     39                .distinct()
     40                .collect(Collectors.toList());
     41
     42        MainApplication.worker.submit(new DownloadPrimitivesTask(MainApplication.getLayerManager().getEditLayer(), members, false));
    3943    }
    4044
    4145    @Override
    4246    public void setPrimitives(Collection<? extends IPrimitive> primitives) {
    43         // selected non-new relations
    44         this.relations = SubclassFilteredCollection.filter(getRelations(primitives), r -> !r.isNew());
     47        this.relations = getRelations(primitives);
    4548        updateEnabledState();
    4649    }

  • trunk/src/org/openstreetmap/josm/data/osm/DataSetMerger.java

    r17136 r17341  
    302302            return true;
    303303
    304         if (target.isIncomplete() && !source.isIncomplete()) {
    305             // target is incomplete, source completes it
    306             // => merge source into target
    307             //
    308             target.mergeFrom(source);
    309             objectsWithChildrenToMerge.add(source.getPrimitiveId());
    310         } else if (!target.isIncomplete() && source.isIncomplete()) {
    311             // target is complete and source is incomplete
    312             // => keep target, it has more information already
    313             //
    314         } else if (target.isIncomplete() && source.isIncomplete()) {
    315             // target and source are incomplete. Doesn't matter which one to
    316             // take. We take target.
    317             //
    318         } else if (!target.isModified() && !source.isModified() && target.isVisible() != source.isVisible()
    319                 && target.getVersion() == source.getVersion()) {
     304        boolean mergeFromSource = false;
     305        boolean haveSameVersion = target.getVersion() == source.getVersion();
     306
     307        if (haveSameVersion && !target.isModified() && !source.isModified()
     308                && target.isVisible() != source.isVisible()) {
    320309            // Same version, but different "visible" attribute and neither of them are modified.
    321310            // It indicates a serious problem in datasets.
    … …  
    324313            throw new DataIntegrityProblemException(tr("Conflict in ''visible'' attribute for object of type {0} with id {1}",
    325314                    target.getType(), target.getId()));
    326         } else if (target.isDeleted() && !source.isDeleted() && target.getVersion() == source.getVersion()) {
     315        }
     316
     317        if (!target.isModified() && source.isDeleted()) {
     318            // target not modified and source is deleted
     319            // So mark it to be deleted. See #20091
     320            //
     321            objectsToDelete.add(target);
     322        } else if (source.isIncomplete()) {
     323            // source is incomplete. Nothing to do.
     324            //
     325        } else if (target.isIncomplete()) {
     326            // target is incomplete, source completes it
     327            // => merge source into target
     328            //
     329            mergeFromSource = true;
     330        } else if (target.isDeleted() && source.isDeleted() && !haveSameVersion) {
     331            // both deleted. Source is newer. Take source. See #19783
     332            mergeFromSource = true;
     333        } else if (target.isDeleted() && !source.isDeleted() && haveSameVersion) {
    327334            // same version, but target is deleted. Assume target takes precedence
    328335            // otherwise too many conflicts when refreshing from the server
    … …  
    340347                }
    341348            }
    342         } else if (!target.isModified() && source.isDeleted()) {
    343             // target not modified. We can assume that source is the most recent version,
    344             // so mark it to be deleted.
    345             //
    346             objectsToDelete.add(target);
    347349        } else if (!target.isModified() && source.isModified()) {
    348350            // target not modified. We can assume that source is the most recent version.
    349351            // clone it into target.
    350             target.mergeFrom(source);
    351             objectsWithChildrenToMerge.add(source.getPrimitiveId());
    352         } else if (!target.isModified() && !source.isModified() && target.getVersion() == source.getVersion()) {
    353             // both not modified. Merge nevertheless.
     352            mergeFromSource = true;
     353        } else if (!target.isModified() && !source.isModified()) {
     354            // both not modified. Merge nevertheless, even if versions are the same
    354355            // This helps when updating "empty" relations, see #4295
    355             target.mergeFrom(source);
    356             objectsWithChildrenToMerge.add(source.getPrimitiveId());
    357         } else if (!target.isModified() && !source.isModified() && target.getVersion() < source.getVersion()) {
    358             // my not modified but other is newer. clone other onto mine.
    359             //
    360             target.mergeFrom(source);
    361             objectsWithChildrenToMerge.add(source.getPrimitiveId());
    362         } else if (target.isModified() && !source.isModified() && target.getVersion() == source.getVersion()) {
     356            mergeFromSource = true;
     357        } else if (target.isModified() && !source.isModified() && haveSameVersion) {
    363358            // target is same as source but target is modified
    364359            // => keep target and reset modified flag if target and source are semantically equal
    … …  
    368363        } else if (source.isDeleted() != target.isDeleted()) {
    369364            // target is modified and deleted state differs.
    370             // this have to be resolved manually.
     365            // this has to be resolved manually.
    371366            //
    372367            addConflict(target, source);
    … …  
    381376            // attributes should already be equal if we get here.
    382377            //
     378            mergeFromSource = true;
     379        }
     380        if (mergeFromSource) {
    383381            target.mergeFrom(source);
    384382            objectsWithChildrenToMerge.add(source.getPrimitiveId());

  • trunk/test/unit/org/openstreetmap/josm/data/osm/DataSetMergerTest.java

    r17275 r17341  
    77import static org.junit.jupiter.api.Assertions.assertNotSame;
    88import static org.junit.jupiter.api.Assertions.assertSame;
     9import static org.junit.jupiter.api.Assertions.assertThrows;
    910import static org.junit.jupiter.api.Assertions.assertTrue;
    1011import static org.junit.jupiter.api.Assertions.fail;
    … …  
    2223import org.openstreetmap.josm.data.projection.ProjectionRegistry;
    2324import org.openstreetmap.josm.data.projection.Projections;
     25import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
    2426import org.openstreetmap.josm.testutils.JOSMTestRules;
    2527import org.openstreetmap.josm.tools.date.DateUtils;
    2628
    2729import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
     30
    2831/**
    2932 * Unit tests for class {@link DataSetMerger}.
    … …  
    974977    }
    975978
     979    /**
     980     * merge two equal objects with different visibility,
     981     * => make sure that DataIntegrityProblemException is thrown.
     982     */
     983    @Test
     984    void testSameVersionAndDifferentVisibility() {
     985
     986        // -- source dataset
     987
     988        // a complete node
     989        Node n1 = new Node(1, 2);
     990        n1.setCoor(new LatLon(1, 1));
     991        n1.setVisible(true);
     992        assertFalse(n1.isModified());
     993        their.addPrimitive(n1);
     994
     995        // --- target dataset
     996        // an complete node
     997        Node n1b = new Node(1, 2);
     998        n1b.setCoor(new LatLon(1, 1));
     999        n1b.setVisible(false);
     1000        assertFalse(n1b.isModified());
     1001        my.addPrimitive(n1b);
     1002
     1003        //-- merge it
     1004        DataSetMerger visitor = new DataSetMerger(my, their);
     1005        assertThrows(DataIntegrityProblemException.class, () -> visitor.merge());
     1006    }
     1007
     1008    /**
     1009     * node without referrers deleted in source
     1010     */
     1011    @Test
     1012    void testDeletedSingleNode() {
     1013
     1014        // -- source dataset
     1015
     1016        // a complete node
     1017        Node n1 = new Node(1, 1);
     1018        n1.setCoor(new LatLon(1, 1));
     1019        n1.setVisible(true);
     1020        assertFalse(n1.isModified());
     1021        their.addPrimitive(n1);
     1022
     1023        //-- merge it to create identical copies
     1024        DataSetMerger visitor = new DataSetMerger(my, their);
     1025        visitor.merge();
     1026
     1027        n1.setDeleted(true);
     1028
     1029        visitor = new DataSetMerger(my, their);
     1030        visitor.merge();
     1031        OsmPrimitive n1b = my.getPrimitiveById(n1);
     1032        assertTrue(n1b.isDeleted());
     1033    }
     1034
     1035    /**
     1036     * node without referrers deleted in source
     1037     */
     1038    @Test
     1039    void testDeletedSingleNodeWithMonitor() {
     1040
     1041        // -- source dataset
     1042
     1043        // a complete node
     1044        Node n1 = new Node(1, 1);
     1045        n1.setCoor(new LatLon(1, 1));
     1046        n1.setVisible(true);
     1047        assertFalse(n1.isModified());
     1048        their.addPrimitive(n1);
     1049
     1050        //-- merge it to create identical copies
     1051        DataSetMerger visitor = new DataSetMerger(my, their);
     1052        visitor.merge(NullProgressMonitor.INSTANCE);
     1053
     1054        n1.setDeleted(true);
     1055
     1056        visitor = new DataSetMerger(my, their);
     1057        visitor.merge(NullProgressMonitor.INSTANCE);
     1058        OsmPrimitive n1b = my.getPrimitiveById(n1);
     1059        assertTrue(n1b.isDeleted());
     1060    }
     1061
     1062    /**
     1063     * Way without referrers deleted in source
     1064     */
     1065    @Test
     1066    void testDeletedWayNoReferrers () {
     1067
     1068        // -- source dataset
     1069
     1070        // a complete way with two nodes
     1071        Node n1 = new Node(1, 1);
     1072        n1.setCoor(new LatLon(1, 1));
     1073        Node n2 = new Node(2, 1);
     1074        n2.setCoor(new LatLon(1, 1));
     1075        n1.setVisible(true);
     1076        n2.setVisible(true);
     1077        their.addPrimitive(n1);
     1078        their.addPrimitive(n2);
     1079        Way w1 = new Way(1,1);
     1080        their.addPrimitive(w1);
     1081        w1.addNode(n1);
     1082        w1.addNode(n2);
     1083        w1.setVisible(true);
     1084        w1.setModified(false);
     1085        assertFalse(n1.isModified());
     1086        assertFalse(n2.isModified());
     1087        assertFalse(w1.isModified());
     1088
     1089        //-- merge it to create identical copies
     1090        DataSetMerger visitor = new DataSetMerger(my, their);
     1091        visitor.merge();
     1092
     1093        w1.setDeleted(true);
     1094
     1095        visitor = new DataSetMerger(my, their);
     1096        visitor.merge();
     1097        OsmPrimitive w1b = my.getPrimitiveById(w1);
     1098        assertTrue(w1b.isDeleted());
     1099    }
     1100
     1101    /**
     1102     * Dependency loop in relations,, both deleted in source
     1103     * => make sure that DataIntegrityProblemException is thrown.
     1104     */
     1105    @Test
     1106    void testDeletedRelationLoop() {
     1107
     1108        // -- source dataset
     1109        Relation r1 = new Relation(1, 1);
     1110        Relation r2 = new Relation(2, 1);
     1111        their.addPrimitive(r1);
     1112        their.addPrimitive(r2);
     1113        // create dependency loop
     1114        r1.addMember(new RelationMember("", r2));
     1115        r2.addMember(new RelationMember("", r1));
     1116
     1117        //-- merge it to create identical copies
     1118        DataSetMerger visitor = new DataSetMerger(my, their);
     1119        visitor.merge();
     1120
     1121        r1.setMembers(null);
     1122        r2.setMembers(null);
     1123        r1.setDeleted(true);
     1124        r2.setDeleted(true);
     1125        visitor = new DataSetMerger(my, their);
     1126        visitor.merge();
     1127
     1128        OsmPrimitive r1b = my.getPrimitiveById(r1);
     1129        OsmPrimitive r2b = my.getPrimitiveById(r2);
     1130        assertTrue(r1b.isDeleted());
     1131        assertTrue(r2b.isDeleted());
     1132    }
     1133
     1134    /**
     1135     * Way is deleted in my but member of relation in their
     1136     */
     1137    @Test
     1138    void testDeletedWayStillMemberOfRelation() {
     1139
     1140        // -- source dataset
     1141        Node n1 = new Node(1, 1);
     1142        n1.setCoor(new LatLon(1, 1));
     1143        Node n2 = new Node(2, 1);
     1144        n2.setCoor(new LatLon(1, 1));
     1145        n1.setVisible(true);
     1146        n2.setVisible(true);
     1147        their.addPrimitive(n1);
     1148        their.addPrimitive(n2);
     1149        Way w1 = new Way(1,1);
     1150        their.addPrimitive(w1);
     1151        w1.addNode(n1);
     1152        w1.addNode(n2);
     1153        w1.setVisible(true);
     1154        w1.setModified(false);
     1155        assertFalse(n1.isModified());
     1156        assertFalse(n2.isModified());
     1157        assertFalse(w1.isModified());
     1158
     1159        //-- merge it to create identical copies
     1160        DataSetMerger visitor = new DataSetMerger(my, their);
     1161        visitor.merge();
     1162
     1163        // let relation use the way that is in my dataset
     1164        Relation r1 = new Relation(1, 1);
     1165        their.addPrimitive(r1);
     1166        r1.addMember(new RelationMember("", w1));
     1167
     1168        Way myWay = (Way) my.getPrimitiveById(w1);
     1169        myWay.setNodes(null);
     1170        myWay.setDeleted(true);
     1171
     1172        visitor = new DataSetMerger(my, their);
     1173        visitor.merge();
     1174        assertFalse(visitor.getConflicts().isEmpty());
     1175        assertFalse(myWay.isDeleted());
     1176        assertTrue(myWay.isEmpty());
     1177        my.clear(); // prevent error from consistency test
     1178    }
     1179
    9761180    private void doTestTicket7481(DataSet source, DataSet target) {
    9771181        // Local node A
    … …  
    11001304        assertTrue(n.isModified());
    11011305    }
     1306
     1307    /**
     1308     * Non-regression test for <a href="https://josm.openstreetmap.de/ticket/19783">Bug #19783</a>.
     1309     */
     1310    @Test
     1311    void testTicket19783() {
     1312        // Server node: deleted
     1313        Node n1 = new Node(1, 2);
     1314        n1.setDeleted(true);
     1315        n1.setVisible(false);
     1316        n1.setModified(false);
     1317        assertFalse(n1.isModified());
     1318        their.addPrimitive(n1);
     1319
     1320        // Local node: one modification: move
     1321        Node n1b = new Node(1, 1);
     1322        n1b.setCoor(new LatLon(1, 1));
     1323        n1.setIncomplete(false);
     1324        assertFalse(n1b.isModified());
     1325        my.addPrimitive(n1b);
     1326        n1b.setDeleted(true);
     1327        assertTrue(n1b.isModified());
     1328
     1329        // Merge
     1330        DataSetMerger visitor = new DataSetMerger(my, their);
     1331        visitor.merge();
     1332
     1333        // Check that modification is gone and version was merged
     1334        Node n = (Node) my.getPrimitiveById(1, OsmPrimitiveType.NODE);
     1335        assertNotNull(n);
     1336        assertFalse(n.isModified());
     1337        assertFalse(n.isVisible());
     1338        assertTrue(n.isDeleted());
     1339        assertEquals(2, n.getVersion());
     1340    }
     1341
     1342    /**
     1343     * Non-regression test for <a href="https://josm.openstreetmap.de/ticket/20091">Bug #20091</a>.
     1344     * Relation refers to incomplete way that ways deleted on server.
     1345     */
     1346    @Test
     1347    void testTicket20091() {
     1348        // Server way: deleted
     1349        Way w1 = new Way(1, 2);
     1350        w1.setDeleted(true);
     1351        w1.setVisible(false);
     1352        w1.setModified(false);
     1353        assertFalse(w1.isModified());
     1354        their.addPrimitive(w1);
     1355
     1356        Way w1b = new Way(1);
     1357        w1b.setIncomplete(true);
     1358        my.addPrimitive(w1b);
     1359        Relation r1 = new Relation(1, 1);
     1360        r1.addMember(new RelationMember("outer", w1b));
     1361        r1.setModified(true);
     1362        my.addPrimitive(r1);
     1363
     1364        // Merge
     1365        DataSetMerger visitor = new DataSetMerger(my, their);
     1366        visitor.merge();
     1367
     1368        assertEquals(1, visitor.getConflicts().size());
     1369        assertTrue(r1.isModified());
     1370        assertEquals(w1b, visitor.getConflicts().iterator().next().getMy());
     1371    }
     1372
    11021373}
Note: See TracChangeset for help on using the changeset viewer.