Changeset 19228 in josm

Timestamp:

2024-09-20T14:06:54+02:00 (2 months ago)

Author:

taylor.smock

Message:

Fix #23930: Merging duplicated layers with little differences stalls JOSM

This is fixed by keeping the "last" conflict in the problem if statement body.

Location:

trunk

Files:

• src/org/openstreetmap/josm/data/osm/DataSetMerger.java (modified) (3 diffs)
• test/data/regress/23930 (added)
• test/data/regress/23930/JOSM_conflict.joz (added)
• test/unit/org/openstreetmap/josm/data/osm/DataSetMergerTest.java (modified) (6 diffs)

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

@@ -141 +141 @@
     protected void addConflict(OsmPrimitive my, OsmPrimitive their) {
         addConflict(new Conflict<>(my, their));
+    }
+
+    private void replaceConflict(Conflict<?> oldConflict, Conflict<?> newConflict) {
+        newConflict.setMergedMap(mergedMap);
+        conflicts.remove(oldConflict);
+        conflicts.add(newConflict);
     }
 
@@ -326 +332 @@
             // otherwise too many conflicts when refreshing from the server
             // but, if source is modified, there is a conflict
+            Conflict<?> currentConflict = null;
             if (source.isModified()) {
-                addConflict(new Conflict<>(target, source, true));
+                currentConflict = new Conflict<>(target, source, true);
+                addConflict(currentConflict);
             }
             // or, if source has a referrer that is not in the target dataset there is a conflict
@@ -333 +341 @@
             for (OsmPrimitive referrer: source.getReferrers()) {
                 if (targetDataSet.getPrimitiveById(referrer.getPrimitiveId()) == null) {
-                    addConflict(new Conflict<>(target, source, true));
+                    final Conflict<?> newConflict = new Conflict<>(target, source, true);
+                    if (currentConflict != null) { // See #23930
+                        replaceConflict(currentConflict, newConflict);
+                    } else {
+                        addConflict(newConflict);
+                    }
                     target.setDeleted(false);
                     break;

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

@@ -2 +2 @@
 package org.openstreetmap.josm.data.osm;
 
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNotSame;
@@ -11 +13 @@
 import static org.junit.jupiter.api.Assertions.fail;
 
+import java.io.File;
+import java.io.IOException;
 import java.io.StringWriter;
 import java.time.Instant;
 import java.util.Arrays;
+import java.util.List;
 import java.util.function.BiConsumer;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -22 +28 @@
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.MethodSource;
+import org.openstreetmap.josm.TestUtils;
+import org.openstreetmap.josm.data.conflict.Conflict;
+import org.openstreetmap.josm.data.conflict.ConflictCollection;
 import org.openstreetmap.josm.data.coor.LatLon;
 import org.openstreetmap.josm.data.projection.ProjectionRegistry;
 import org.openstreetmap.josm.data.projection.Projections;
+import org.openstreetmap.josm.gui.layer.OsmDataLayer;
 import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
+import org.openstreetmap.josm.io.IllegalDataException;
+import org.openstreetmap.josm.io.session.SessionReader;
+import org.openstreetmap.josm.tools.Logging;
 
 /**
@@ -1361 +1374 @@
     }
 
-    static Stream<BiConsumer<Node, Node>> testNonRegression23846() {
+    static Stream<BiConsumer<Node, Node>> testTicket23846() {
         return Stream.of(
                 (firstNode, secondNode) -> firstNode.setModified(true),
@@ -1370 +1383 @@
     @ParameterizedTest
     @MethodSource
-    void testNonRegression23846(BiConsumer<Node, Node> nodeSetup) {
+    void testTicket23846(BiConsumer<Node, Node> nodeSetup) {
         final Node firstNode = new Node(1234, 1);
         final Node secondNode = new Node(1234, 1);
@@ -1386 +1399 @@
         assertTrue(secondNode.isReferrersDownloaded());
     }
+
+    /**
+     * Non-regression test for <a href="https://josm.openstreetmap.de/ticket/23930">#23930</a>
+     */
+    @Test
+    void testTicket23930() throws IOException, IllegalDataException {
+        final File file = new File(TestUtils.getRegressionDataFile(23930, "JOSM_conflict.joz"));
+        final SessionReader reader = new SessionReader();
+        reader.loadSession(file, true, NullProgressMonitor.INSTANCE);
+        final List<OsmDataLayer> layers = reader.getLayers().stream()
+                .filter(OsmDataLayer.class::isInstance).map(OsmDataLayer.class::cast).collect(Collectors.toList());
+        final DataSet newWay = layers.stream().filter(layer -> layer.getName().equals("new_way.osm"))
+                .map(OsmDataLayer::getDataSet).findFirst().orElseThrow();
+        final DataSet nodeDeleted = layers.stream().filter(layer -> layer.getName().equals("node_deleted.osm"))
+                .map(OsmDataLayer::getDataSet).findFirst().orElseThrow();
+        final DataSetMerger merge = new DataSetMerger(nodeDeleted, newWay);
+        Logging.clearLastErrorAndWarnings();
+        assertDoesNotThrow(() -> merge.merge(NullProgressMonitor.INSTANCE));
+        assertTrue(Logging.getLastErrorAndWarnings().isEmpty(), String.join("\n", Logging.getLastErrorAndWarnings()));
+        final ConflictCollection conflicts = merge.getConflicts();
+        // There are a few differences in the files
+        // 1. New node in layer 2: No need for conflict
+        // 2. node 2427358529: layer 1 deletes it, layer 2 modifies it (conflict required)
+        // 3. new way in layer 2 with new node and node 2427358529 (conflict required)
+        // 4. Modification of way 32277602 in layer 1 removing node 2427358529 (conflict required)
+        // Therefore, conflicts are as follows:
+        // 1. A deleted node (n2427358529) with referrers (w32277602 and new way) and new tags ("fix tag=recheck position")
+        assertEquals(1, conflicts.size());
+        final Conflict<?> conflict = conflicts.iterator().next();
+        final Node myNode = assertInstanceOf(Node.class, conflict.getMy());
+        final Node theirNode = assertInstanceOf(Node.class, conflict.getTheir());
+        assertFalse(theirNode.isDeleted());
+        assertFalse(myNode.isDeleted());
+    }
 }