Add code to avoid accidentally uploading bad data in older plugin ver…

…sions. This and related commits will be backported. Signed-off-by: Taylor Smock <[email protected]>
JOSM · Jan 13, 2021 · 520a50f · 520a50f
1 parent 20f1388
commit 520a50f
Show file tree

Hide file tree

Showing 8 changed files with 105 additions and 22 deletions.
diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAILayer.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAILayer.java
@@ -11,6 +11,7 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
 import java.util.concurrent.locks.Lock;
@@ -22,6 +23,7 @@
 import javax.swing.Icon;
 import javax.swing.JCheckBoxMenuItem;
 import javax.swing.JLabel;
+import javax.swing.JOptionPane;
 import javax.swing.JPanel;
 import javax.swing.SwingConstants;
 
@@ -34,15 +36,18 @@
 import org.openstreetmap.josm.data.osm.UploadPolicy;
 import org.openstreetmap.josm.data.osm.Way;
 import org.openstreetmap.josm.gui.MainApplication;
+import org.openstreetmap.josm.gui.Notification;
 import org.openstreetmap.josm.gui.layer.Layer;
 import org.openstreetmap.josm.gui.layer.MainLayerManager.ActiveLayerChangeEvent;
 import org.openstreetmap.josm.gui.layer.MainLayerManager.ActiveLayerChangeListener;
 import org.openstreetmap.josm.gui.layer.OsmDataLayer;
 import org.openstreetmap.josm.gui.mappaint.MapPaintStyles;
 import org.openstreetmap.josm.gui.mappaint.StyleSource;
 import org.openstreetmap.josm.gui.util.GuiHelper;
+import org.openstreetmap.josm.gui.widgets.HtmlPanel;
 import org.openstreetmap.josm.plugins.mapwithai.MapWithAIPlugin;
 import org.openstreetmap.josm.plugins.mapwithai.data.mapwithai.MapWithAIInfo;
+import org.openstreetmap.josm.plugins.mapwithai.tools.BlacklistUtils;
 import org.openstreetmap.josm.spi.preferences.Config;
 import org.openstreetmap.josm.tools.GBC;
 import org.openstreetmap.josm.tools.ImageProvider;
@@ -213,6 +218,13 @@ public void onPostDownloadFromServer(Bounds bounds) {
 
     @Override
     public void selectionChanged(SelectionChangeEvent event) {
+        if (BlacklistUtils.isBlacklisted()) {
+            if (!event.getSelection().isEmpty()) {
+                GuiHelper.runInEDT(() -> getDataSet().setSelected(Collections.emptySet()));
+                createBadDataNotification();
+            }
+            return;
+        }
         super.selectionChanged(event);
         final int maximumAdditionSelection = MapWithAIPreferenceHelper.getMaximumAddition();
         if ((event.getSelection().size() - event.getOldSelection().size() > 1
@@ -234,6 +246,29 @@ public void selectionChanged(SelectionChangeEvent event) {
         }
     }
 
+    /**
+     * Create a notification for plugin versions that create bad data.
+     */
+    public static void createBadDataNotification() {
+        Notification badData = new Notification();
+        badData.setIcon(JOptionPane.ERROR_MESSAGE);
+        badData.setDuration(Notification.TIME_LONG);
+        HtmlPanel panel = new HtmlPanel();
+        StringBuilder message = new StringBuilder()
+                .append(tr("This version of the MapWithAI plugin is known to create bad data.")).append("<br />")
+                .append(tr("Please update plugins and/or JOSM."));
+        if (BlacklistUtils.isOffline()) {
+            message.append("<br />").append(tr("This message may occur when JOSM is offline."));
+        }
+        panel.setText(message.toString());
+        badData.setContent(panel);
+        MapWithAILayer layer = MapWithAIDataUtils.getLayer(false);
+        if (layer != null) {
+            layer.setMaximumAddition(0);
+        }
+        GuiHelper.runInEDT(badData::show);
+    }
+
     private static class OsmComparator implements Comparator<OsmPrimitive> {
         final Collection<OsmPrimitive> previousSelection;
 

diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIMoveAction.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIMoveAction.java
@@ -26,6 +26,7 @@
 import org.openstreetmap.josm.gui.layer.OsmDataLayer;
 import org.openstreetmap.josm.plugins.mapwithai.MapWithAIPlugin;
 import org.openstreetmap.josm.plugins.mapwithai.commands.MapWithAIAddCommand;
+import org.openstreetmap.josm.plugins.mapwithai.tools.BlacklistUtils;
 import org.openstreetmap.josm.tools.Shortcut;
 
 public class MapWithAIMoveAction extends JosmAction {
@@ -62,6 +63,10 @@ private static Shortcut obtainShortcut() {
 
     @Override
     public void actionPerformed(ActionEvent event) {
+        if (BlacklistUtils.isBlacklisted()) {
+            MapWithAILayer.createBadDataNotification();
+            return;
+        }
         for (final MapWithAILayer mapWithAI : MainApplication.getLayerManager().getLayersOfType(MapWithAILayer.class)) {
             final DataSet ds = mapWithAI.getDataSet();
             final int maxAddition = MapWithAIPreferenceHelper.getMaximumAddition();

diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/tools/BlacklistUtils.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/tools/BlacklistUtils.java
@@ -14,6 +14,7 @@
 import javax.json.JsonValue;
 
 import org.openstreetmap.josm.io.CachedFile;
+import org.openstreetmap.josm.io.NetworkManager;
 import org.openstreetmap.josm.plugins.mapwithai.MapWithAIPlugin;
 import org.openstreetmap.josm.tools.Logging;
 
@@ -39,8 +40,8 @@ private BlacklistUtils() {
      */
     public static boolean isBlacklisted() {
         String version = MapWithAIPlugin.getVersionInfo();
-        try (CachedFile blacklist = new CachedFile(blacklistUrl);
-                BufferedReader bufferedReader = blacklist.getContentReader();
+        CachedFile blacklist = new CachedFile(blacklistUrl);
+        try (BufferedReader bufferedReader = blacklist.getContentReader();
                 JsonReader reader = Json.createReader(bufferedReader)) {
             JsonStructure structure = reader.read();
             if (structure.getValueType() == JsonValue.ValueType.ARRAY) {
@@ -52,7 +53,14 @@ public static boolean isBlacklisted() {
                 return object.keySet().contains(version);
             }
         } catch (IOException | JsonException e) {
+            try {
+                blacklist.clear();
+            } catch (IOException e1) {
+                Logging.error(e1);
+            }
             Logging.error(e);
+        } finally {
+            blacklist.close();
         }
         return true;
     }
@@ -65,4 +73,13 @@ public static boolean isBlacklisted() {
     static void setBlacklistUrl(String url) {
         blacklistUrl = url;
     }
+
+    /**
+     * Check if we can reach the URL for the known-bad version list.
+     *
+     * @return {@code true} if the configured url is offline
+     */
+    public static boolean isOffline() {
+        return NetworkManager.isOffline(blacklistUrl);
+    }
 }
diff --git a/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAILayerTest.java b/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAILayerTest.java
@@ -24,6 +24,7 @@
 import org.awaitility.Awaitility;
 import org.awaitility.Durations;
 import org.junit.Before;
+import org.junit.BeforeClass;
 import org.junit.Rule;
 import org.junit.Test;
 import org.openstreetmap.josm.TestUtils;
@@ -44,6 +45,7 @@
 import org.openstreetmap.josm.plugins.mapwithai.backend.MapWithAILayer.ContinuousDownloadAction;
 import org.openstreetmap.josm.plugins.mapwithai.commands.MapWithAIAddCommand;
 import org.openstreetmap.josm.plugins.mapwithai.data.mapwithai.MapWithAIInfo;
+import org.openstreetmap.josm.plugins.mapwithai.testutils.MapWithAIPluginMock;
 import org.openstreetmap.josm.plugins.mapwithai.testutils.MapWithAITestRules;
 import org.openstreetmap.josm.spi.preferences.Config;
 import org.openstreetmap.josm.testutils.JOSMTestRules;
@@ -58,15 +60,21 @@
 public class MapWithAILayerTest {
     @Rule
     @SuppressFBWarnings("URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
-    public JOSMTestRules test = new MapWithAITestRules().wiremock().preferences().main().projection().fakeAPI()
-            .territories();
+    public JOSMTestRules test = new MapWithAITestRules().sources().wiremock().preferences().main().projection()
+    .fakeAPI().territories();
 
     MapWithAILayer layer;
 
+    @BeforeClass
+    public static void beforeAll() {
+        TestUtils.assumeWorkingJMockit();
+        new MapWithAIPluginMock();
+        Territories.initialize(); // Required to avoid an NPE (see JOSM-19132)
+    }
+
     @Before
-    public void setUp() {
+    public void beforeEach() {
         layer = new MapWithAILayer(new DataSet(), "test", null);
-        Territories.initialize(); // Required to avoid an NPE (see JOSM-19132)
     }
 
     @Test

diff --git a/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIMoveActionTest.java b/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIMoveActionTest.java
@@ -10,6 +10,7 @@
 import org.awaitility.Awaitility;
 import org.awaitility.Durations;
 import org.junit.Before;
+import org.junit.BeforeClass;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.jupiter.api.Assertions;
@@ -25,6 +26,8 @@
 import org.openstreetmap.josm.gui.util.GuiHelper;
 import org.openstreetmap.josm.plugins.mapwithai.backend.commands.conflation.ConnectedCommand;
 import org.openstreetmap.josm.plugins.mapwithai.backend.commands.conflation.DuplicateCommand;
+import org.openstreetmap.josm.plugins.mapwithai.testutils.MapWithAIPluginMock;
+import org.openstreetmap.josm.plugins.mapwithai.testutils.MapWithAITestRules;
 import org.openstreetmap.josm.plugins.mapwithai.testutils.MissingConnectionTagsMocker;
 import org.openstreetmap.josm.testutils.JOSMTestRules;
 import org.openstreetmap.josm.testutils.mockers.WindowMocker;
@@ -42,7 +45,13 @@ public class MapWithAIMoveActionTest {
 
     @Rule
     @SuppressFBWarnings("URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
-    public JOSMTestRules test = new JOSMTestRules().preferences().main().projection();
+    public JOSMTestRules test = new MapWithAITestRules().wiremock().preferences().main().projection().territories()
+    .assertionsInEDT();
+
+    @BeforeClass
+    public static void beforeAll() {
+        new MapWithAIPluginMock();
+    }
 
     @Before
     public void setUp() {
@@ -107,7 +116,7 @@ public void testConflationDupeKeyRemoval() {
 
         UndoRedoHandler.getInstance().undo();
         Awaitility.await().atMost(Durations.ONE_SECOND)
-                .until(() -> !((Way) ds.getPrimitiveById(way2)).lastNode().hasKey(DuplicateCommand.KEY));
+        .until(() -> !((Way) ds.getPrimitiveById(way2)).lastNode().hasKey(DuplicateCommand.KEY));
         assertFalse(way2.lastNode().hasKey(DuplicateCommand.KEY), "The dupe key should no longer exist");
         assertTrue(way1.lastNode().hasKey(DuplicateCommand.KEY), "The dupe key should no longer exist");
     }
@@ -159,7 +168,7 @@ public void testMaxAddNotification() {
         }
         for (int i = 0; i < 11; i++) {
             GuiHelper
-                    .runInEDTAndWaitWithException(() -> ds.setSelected(ds.allNonDeletedPrimitives().iterator().next()));
+            .runInEDTAndWaitWithException(() -> ds.setSelected(ds.allNonDeletedPrimitives().iterator().next()));
             moveAction.actionPerformed(null);
         }
         assertTrue(notification.shown);

diff --git a/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIUploadHookTest.java b/test/unit/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIUploadHookTest.java
@@ -26,6 +26,7 @@
 import org.openstreetmap.josm.plugins.PluginException;
 import org.openstreetmap.josm.plugins.PluginInformation;
 import org.openstreetmap.josm.plugins.mapwithai.data.mapwithai.MapWithAIInfo;
+import org.openstreetmap.josm.plugins.mapwithai.testutils.MapWithAITestRules;
 import org.openstreetmap.josm.testutils.JOSMTestRules;
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
@@ -37,7 +38,8 @@
 public class MapWithAIUploadHookTest {
     @Rule
     @SuppressFBWarnings("URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
-    public JOSMTestRules test = new JOSMTestRules().main().projection().preferences().territories();
+    public JOSMTestRules test = new MapWithAITestRules().sources().wiremock().main().projection().preferences()
+    .territories();
 
     /**
      * Test method for {@link MapWithAIUploadHook#modifyChangesetTags(Map)}.
@@ -106,8 +108,8 @@ public void testModifyChangesetTags() throws PluginException {
 
         BBox tBBox = new BBox(1, 0, 0, 1);
         MainApplication.getLayerManager()
-                .addLayer(new GpxLayer(DetectTaskingManagerUtils.createTaskingManagerGpxData(tBBox),
-                        DetectTaskingManagerUtils.MAPWITHAI_CROP_AREA));
+        .addLayer(new GpxLayer(DetectTaskingManagerUtils.createTaskingManagerGpxData(tBBox),
+                DetectTaskingManagerUtils.MAPWITHAI_CROP_AREA));
 
         tags.clear();
         hook.modifyChangesetTags(tags);

diff --git a/test/unit/org/openstreetmap/josm/plugins/mapwithai/testutils/MapWithAIPluginMock.java b/test/unit/org/openstreetmap/josm/plugins/mapwithai/testutils/MapWithAIPluginMock.java
@@ -0,0 +1,14 @@
+// License: GPL. For details, see LICENSE file.
+package org.openstreetmap.josm.plugins.mapwithai.testutils;
+
+import org.openstreetmap.josm.plugins.mapwithai.MapWithAIPlugin;
+
+import mockit.Mock;
+import mockit.MockUp;
+
+public class MapWithAIPluginMock extends MockUp<MapWithAIPlugin> {
+    @Mock
+    public static String getVersionInfo() {
+        return "1.0";
+    }
+}
diff --git a/test/unit/org/openstreetmap/josm/plugins/mapwithai/tools/BlacklistUtilsTest.java b/test/unit/org/openstreetmap/josm/plugins/mapwithai/tools/BlacklistUtilsTest.java
@@ -15,14 +15,13 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
+import org.openstreetmap.josm.TestUtils;
 import org.openstreetmap.josm.plugins.mapwithai.MapWithAIPlugin;
+import org.openstreetmap.josm.plugins.mapwithai.testutils.MapWithAIPluginMock;
 import org.openstreetmap.josm.testutils.JOSMTestRules;
 
 import com.github.tomakehurst.wiremock.WireMockServer;
 
-import mockit.Mock;
-import mockit.MockUp;
-
 /**
  * Tests for {@link BlacklistUtils}
  *
@@ -31,20 +30,14 @@
  */
 class BlacklistUtilsTest {
 
-    private static class MapWithAIPluginMock extends MockUp<MapWithAIPlugin> {
-        @Mock
-        public static String getVersionInfo() {
-            return "1.0";
-        }
-    }
-
     @RegisterExtension
     static JOSMTestRules rules = new JOSMTestRules();
 
     private static WireMockServer wireMock;
 
     @BeforeAll
     static void setup() {
+        TestUtils.assumeWorkingJMockit();
         wireMock = new WireMockServer(options().dynamicPort());
         wireMock.start();
         BlacklistUtils.setBlacklistUrl(wireMock.baseUrl() + "/JOSM_MapWithAI/json/blacklisted_versions.json");