diff --git a/src/org/openstreetmap/josm/data/osm/DataSetMerger.java b/src/org/openstreetmap/josm/data/osm/DataSetMerger.java index 26d631a3331..9bb0009deac 100644 --- a/src/org/openstreetmap/josm/data/osm/DataSetMerger.java +++ b/src/org/openstreetmap/josm/data/osm/DataSetMerger.java @@ -142,6 +142,12 @@ 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); + } + protected void fixIncomplete(Way other) { Way myWay = (Way) getMergeTarget(other); if (myWay == null) @@ -325,14 +331,21 @@ private boolean mergeById(OsmPrimitive source) { // same version, but target is deleted. Assume target takes precedence // 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 // If target dataset refers to the deleted primitive, conflict will be added in fixReferences method 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; } diff --git a/test/data/regress/23930/JOSM_conflict.joz b/test/data/regress/23930/JOSM_conflict.joz new file mode 100644 index 00000000000..d3cdf2b3773 Binary files /dev/null and b/test/data/regress/23930/JOSM_conflict.joz differ diff --git a/test/unit/org/openstreetmap/josm/data/osm/DataSetMergerTest.java b/test/unit/org/openstreetmap/josm/data/osm/DataSetMergerTest.java index fbd93e6c892..1c50e89a4b7 100644 --- a/test/unit/org/openstreetmap/josm/data/osm/DataSetMergerTest.java +++ b/test/unit/org/openstreetmap/josm/data/osm/DataSetMergerTest.java @@ -1,8 +1,10 @@ // License: GPL. For details, see LICENSE file. 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; import static org.junit.jupiter.api.Assertions.assertSame; @@ -10,10 +12,14 @@ import static org.junit.jupiter.api.Assertions.assertTrue; 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; import org.junit.jupiter.api.AfterEach; @@ -21,10 +27,17 @@ import org.junit.jupiter.api.Test; 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; /** * Unit tests for class {@link DataSetMerger}. @@ -1360,7 +1373,7 @@ void testTicket20091() { assertEquals(w1b, visitor.getConflicts().iterator().next().getMy()); } - static Stream> testNonRegression23846() { + static Stream> testTicket23846() { return Stream.of( (firstNode, secondNode) -> firstNode.setModified(true), (firstNode, secondNode) -> { /* No modifications */ } @@ -1369,7 +1382,7 @@ static Stream> testNonRegression23846() { @ParameterizedTest @MethodSource - void testNonRegression23846(BiConsumer nodeSetup) { + void testTicket23846(BiConsumer nodeSetup) { final Node firstNode = new Node(1234, 1); final Node secondNode = new Node(1234, 1); final DataSetMerger merge = new DataSetMerger(my, their); @@ -1385,4 +1398,38 @@ void testNonRegression23846(BiConsumer nodeSetup) { assertTrue(firstNode.isReferrersDownloaded()); assertTrue(secondNode.isReferrersDownloaded()); } + + /** + * Non-regression test for #23930 + */ + @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 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()); + } }