Ensure that dupe keys aren't kept when nodes aren't merged

Signed-off-by: Taylor Smock <taylor.smock@kaart.com>

src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/AbstractConflationCommand.java

@@ -8,6 +8,7 @@ import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.TreeMap;
 import java.util.stream.Collectors;
@@ -52,7 +53,7 @@ public abstract class AbstractConflationCommand extends Command {
      * @param primitives The primitives to run the command on
      * @return The command that will be run (may be {@code null})
      */
-    public Command getCommand(List<OsmPrimitive> primitives) {
+    public Command getCommand(Collection<OsmPrimitive> primitives) {
         possiblyAffectedPrimitives = primitives.stream().distinct().collect(Collectors.toList());
         return getRealCommand();
     }
@@ -71,6 +72,8 @@ public abstract class AbstractConflationCommand extends Command {
      * @return The primitives that the ids point to, if in the dataset.
      */
     public static OsmPrimitive[] getPrimitives(DataSet dataSet, String ids) {
+        Objects.requireNonNull(dataSet, tr("DataSet cannot be null"));
+        Objects.requireNonNull(ids, tr("The ids string cannot be null"));
         final Map<Integer, Pair<Long, OsmPrimitiveType>> missingPrimitives = new TreeMap<>();
         final String[] connections = ids.split(",", -1);
         final OsmPrimitive[] primitiveConnections = new OsmPrimitive[connections.length];

src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/DuplicateCommand.java

@@ -92,6 +92,9 @@ public class DuplicateCommand extends AbstractConflationCommand {
             List<Command> tCommands = duplicateNode(getAffectedDataSet(), tNode);
             // We have to execute the command to avoid duplicating the command later. Undo
             // occurs later, so that the state doesn't actually change.
+            if (tCommands.isEmpty()) {
+                tCommands.add(new ChangePropertyCommand(tNode, KEY, null));
+            }
             tCommands.forEach(Command::executeCommand);
             commands.addAll(tCommands);
         }
@@ -99,7 +102,9 @@ public class DuplicateCommand extends AbstractConflationCommand {
         commands.forEach(Command::undoCommand);
         Collections.reverse(commands);
         Command returnCommand = null;
-        if (!commands.isEmpty()) {
+        if (commands.size() == 1) {
+            returnCommand = commands.get(0);
+        } else if (!commands.isEmpty()) {
             returnCommand = new SequenceCommand(getDescriptionText(), commands);
         }
         return returnCommand;

test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/ConnectedCommandTest.java

@@ -0,0 +1,61 @@
+// License: GPL. For details, see LICENSE file.
+package org.openstreetmap.josm.plugins.mapwithai.backend.commands.conflation;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.Collections;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.openstreetmap.josm.TestUtils;
+import org.openstreetmap.josm.command.Command;
+import org.openstreetmap.josm.data.coor.LatLon;
+import org.openstreetmap.josm.data.osm.DataSet;
+import org.openstreetmap.josm.data.osm.Node;
+import org.openstreetmap.josm.data.osm.Way;
+import org.openstreetmap.josm.testutils.JOSMTestRules;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+public class ConnectedCommandTest {
+    /**
+     * Setup test.
+     */
+    @Rule
+    @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
+    public JOSMTestRules rule = new JOSMTestRules().projection();
+
+    @Test
+    public void testVariousConditions() {
+        DataSet ds = new DataSet();
+        ConnectedCommand command = new ConnectedCommand(ds);
+        Way way = TestUtils.newWay("", new Node(new LatLon(0, 0)), new Node(new LatLon(1, 0)));
+        way.getNodes().forEach(ds::addPrimitive);
+        ds.addPrimitive(way);
+
+        Node toAdd = new Node(new LatLon(0.5, 0));
+        ds.addPrimitive(toAdd);
+
+        assertThrows(NullPointerException.class, () -> command.getCommand(Collections.singletonList(toAdd)));
+
+        toAdd.put(command.getKey(),
+                "w" + way.getUniqueId() + ",n" + way.firstNode().getUniqueId() + ",n" + way.lastNode().getUniqueId());
+
+        Command realCommand = command.getCommand(Collections.singletonList(toAdd));
+        realCommand.executeCommand();
+        assertFalse(toAdd.hasKey(command.getKey()));
+        realCommand.undoCommand();
+        assertTrue(toAdd.hasKey(command.getKey()));
+
+        toAdd.setCoor(new LatLon(2, 0));
+
+        realCommand = command.getCommand(Collections.singletonList(toAdd));
+        realCommand.executeCommand();
+        assertFalse(toAdd.hasKey(command.getKey()));
+        realCommand.undoCommand();
+        assertTrue(toAdd.hasKey(command.getKey()));
+    }
+
+}

test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/DuplicateCommandTest.java

@@ -0,0 +1,66 @@
+// License: GPL. For details, see LICENSE file.
+package org.openstreetmap.josm.plugins.mapwithai.backend.commands.conflation;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.Collections;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.openstreetmap.josm.command.Command;
+import org.openstreetmap.josm.data.coor.LatLon;
+import org.openstreetmap.josm.data.osm.DataSet;
+import org.openstreetmap.josm.data.osm.Node;
+import org.openstreetmap.josm.testutils.JOSMTestRules;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+public class DuplicateCommandTest {
+    /**
+     * Setup test.
+     */
+    @Rule
+    @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
+    public JOSMTestRules rule = new JOSMTestRules().projection();
+
+    @Test
+    public void testVariousConditions() {
+        DataSet ds = new DataSet();
+        DuplicateCommand dupe = new DuplicateCommand(ds);
+
+        Node dupe1 = new Node(new LatLon(0, 0));
+        Node dupe2 = new Node(new LatLon(0, 0));
+        ds.addPrimitive(dupe2);
+        ds.addPrimitive(dupe1);
+
+        assertNull(dupe.getCommand(Collections.singleton(dupe1)));
+
+        dupe1.put(dupe.getKey(), "n" + dupe2.getUniqueId());
+
+        Command command = dupe.getCommand(Collections.singleton(dupe1));
+        command.executeCommand();
+        assertTrue(dupe1.isDeleted());
+        assertFalse(dupe2.isDeleted());
+        assertFalse(dupe2.hasKey(dupe.getKey()));
+        command.undoCommand();
+        assertFalse(dupe1.isDeleted());
+        assertTrue(dupe1.hasKey(dupe.getKey()));
+        assertFalse(dupe2.hasKey(dupe.getKey()));
+
+        dupe1.setCoor(new LatLon(1, 1));
+
+        command = dupe.getCommand(Collections.singleton(dupe1));
+        command.executeCommand();
+        assertFalse(dupe1.isDeleted());
+        assertFalse(dupe2.isDeleted());
+        assertFalse(dupe1.hasKey(dupe.getKey()));
+        assertFalse(dupe2.hasKey(dupe.getKey()));
+        command.undoCommand();
+        assertFalse(dupe1.isDeleted());
+        assertTrue(dupe1.hasKey(dupe.getKey()));
+        assertFalse(dupe2.hasKey(dupe.getKey()));
+    }
+
+}

test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/commands/conflation/MergeBuildingAddressTest.java

@@ -1,3 +1,4 @@
+// License: GPL. For details, see LICENSE file.
 package org.openstreetmap.josm.plugins.mapwithai.backend.commands.conflation;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;