Changeset 17341 in josm for trunk/src

Timestamp:

2020-11-23T20:45:59+01:00 (3 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/src/org/openstreetmap/josm

Files:

• actions/relation/DownloadMembersAction.java (modified) (2 diffs)
• data/osm/DataSetMerger.java (modified) (5 diffs)

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

@@ -7 +7 @@
 import java.awt.event.ActionEvent;
 import java.util.Collection;
+import java.util.List;
+import java.util.stream.Collectors;
 
 import org.openstreetmap.josm.data.osm.IPrimitive;
-import org.openstreetmap.josm.data.osm.Relation;
+import org.openstreetmap.josm.data.osm.PrimitiveId;
 import org.openstreetmap.josm.gui.MainApplication;
-import org.openstreetmap.josm.gui.dialogs.relation.DownloadRelationTask;
+import org.openstreetmap.josm.gui.io.DownloadPrimitivesTask;
 import org.openstreetmap.josm.tools.ImageProvider;
-import org.openstreetmap.josm.tools.SubclassFilteredCollection;
-import org.openstreetmap.josm.tools.Utils;
 
 /**
@@ -35 +35 @@
     public void actionPerformed(ActionEvent e) {
         if (!isEnabled() || relations.isEmpty() || !MainApplication.isDisplayingMapView()) return;
-        MainApplication.worker.submit(new DownloadRelationTask(
-                Utils.filteredCollection(relations, Relation.class), MainApplication.getLayerManager().getEditLayer()));
+        List<PrimitiveId> members = relations.stream()
+                .flatMap(r -> r.getMemberPrimitivesList().stream().filter(osm -> !osm.isNew()).map(IPrimitive::getOsmPrimitiveId))
+                .distinct()
+                .collect(Collectors.toList());
+
+        MainApplication.worker.submit(new DownloadPrimitivesTask(MainApplication.getLayerManager().getEditLayer(), members, false));
     }
 
     @Override
     public void setPrimitives(Collection<? extends IPrimitive> primitives) {
-        // selected non-new relations
-        this.relations = SubclassFilteredCollection.filter(getRelations(primitives), r -> !r.isNew());
+        this.relations = getRelations(primitives);
         updateEnabledState();
     }

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

@@ -302 +302 @@
             return true;
 
-        if (target.isIncomplete() && !source.isIncomplete()) {
-            // target is incomplete, source completes it
-            // => merge source into target
-            //
-            target.mergeFrom(source);
-            objectsWithChildrenToMerge.add(source.getPrimitiveId());
-        } else if (!target.isIncomplete() && source.isIncomplete()) {
-            // target is complete and source is incomplete
-            // => keep target, it has more information already
-            //
-        } else if (target.isIncomplete() && source.isIncomplete()) {
-            // target and source are incomplete. Doesn't matter which one to
-            // take. We take target.
-            //
-        } else if (!target.isModified() && !source.isModified() && target.isVisible() != source.isVisible()
-                && target.getVersion() == source.getVersion()) {
+        boolean mergeFromSource = false;
+        boolean haveSameVersion = target.getVersion() == source.getVersion();
+
+        if (haveSameVersion && !target.isModified() && !source.isModified()
+                && target.isVisible() != source.isVisible()) {
             // Same version, but different "visible" attribute and neither of them are modified.
             // It indicates a serious problem in datasets.
@@ -324 +313 @@
             throw new DataIntegrityProblemException(tr("Conflict in ''visible'' attribute for object of type {0} with id {1}",
                     target.getType(), target.getId()));
-        } else if (target.isDeleted() && !source.isDeleted() && target.getVersion() == source.getVersion()) {
+        }
+
+        if (!target.isModified() && source.isDeleted()) {
+            // target not modified and source is deleted
+            // So mark it to be deleted. See #20091
+            //
+            objectsToDelete.add(target);
+        } else if (source.isIncomplete()) {
+            // source is incomplete. Nothing to do.
+            //
+        } else if (target.isIncomplete()) {
+            // target is incomplete, source completes it
+            // => merge source into target
+            //
+            mergeFromSource = true;
+        } else if (target.isDeleted() && source.isDeleted() && !haveSameVersion) {
+            // both deleted. Source is newer. Take source. See #19783
+            mergeFromSource = true;
+        } else if (target.isDeleted() && !source.isDeleted() && haveSameVersion) {
             // same version, but target is deleted. Assume target takes precedence
             // otherwise too many conflicts when refreshing from the server
@@ -340 +347 @@
                 }
             }
-        } else if (!target.isModified() && source.isDeleted()) {
-            // target not modified. We can assume that source is the most recent version,
-            // so mark it to be deleted.
-            //
-            objectsToDelete.add(target);
         } else if (!target.isModified() && source.isModified()) {
             // target not modified. We can assume that source is the most recent version.
             // clone it into target.
-            target.mergeFrom(source);
-            objectsWithChildrenToMerge.add(source.getPrimitiveId());
-        } else if (!target.isModified() && !source.isModified() && target.getVersion() == source.getVersion()) {
-            // both not modified. Merge nevertheless.
+            mergeFromSource = true;
+        } else if (!target.isModified() && !source.isModified()) {
+            // both not modified. Merge nevertheless, even if versions are the same
             // This helps when updating "empty" relations, see #4295
-            target.mergeFrom(source);
-            objectsWithChildrenToMerge.add(source.getPrimitiveId());
-        } else if (!target.isModified() && !source.isModified() && target.getVersion() < source.getVersion()) {
-            // my not modified but other is newer. clone other onto mine.
-            //
-            target.mergeFrom(source);
-            objectsWithChildrenToMerge.add(source.getPrimitiveId());
-        } else if (target.isModified() && !source.isModified() && target.getVersion() == source.getVersion()) {
+            mergeFromSource = true;
+        } else if (target.isModified() && !source.isModified() && haveSameVersion) {
             // target is same as source but target is modified
             // => keep target and reset modified flag if target and source are semantically equal
@@ -368 +363 @@
         } else if (source.isDeleted() != target.isDeleted()) {
             // target is modified and deleted state differs.
-            // this have to be resolved manually.
+            // this has to be resolved manually.
             //
             addConflict(target, source);
@@ -381 +376 @@
             // attributes should already be equal if we get here.
             //
+            mergeFromSource = true;
+        }
+        if (mergeFromSource) {
             target.mergeFrom(source);
             objectsWithChildrenToMerge.add(source.getPrimitiveId());