Changeset 19152 in josm for trunk

Timestamp:

2024-07-25T20:56:22+02:00 (4 months ago)

Author:

taylor.smock

Message:

See #23821: Refactor LoadAndZoomHandler and add tests

This fixes some lint issues, specifically in regard to method complexity.

Location:

trunk

Files:

• src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java (modified) (11 diffs)
• test/unit/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandlerTest.java (modified) (4 diffs)
• test/unit/org/openstreetmap/josm/testutils/annotations/BasicWiremock.java (modified) (14 diffs)

trunk/src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java

@@ -6 +6 @@
 import java.awt.geom.Area;
 import java.awt.geom.Rectangle2D;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@@ -18 +17 @@
 import java.util.concurrent.TimeoutException;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import javax.swing.JOptionPane;
@@ -65 +65 @@
     public static final String command2 = "zoom";
     private static final String CURRENT_SELECTION = "currentselection";
+    private static final String SELECT = "select";
+    private static final String ADDTAGS = "addtags";
+    private static final String CHANGESET_COMMENT = "changeset_comment";
+    private static final String CHANGESET_SOURCE = "changeset_source";
+    private static final String CHANGESET_HASHTAGS = "changeset_hashtags";
+    private static final String CHANGESET_TAGS = "changeset_tags";
+    private static final String SEARCH = "search";
 
     // Mandatory arguments
@@ -81 +88 @@
         String msg = tr("Remote Control has been asked to load data from the API.") +
                 "<br>" + tr("Bounding box: ") + new BBox(minlon, minlat, maxlon, maxlat).toStringCSV(", ");
-        if (args.containsKey("select") && !toSelect.isEmpty()) {
+        if (args.containsKey(SELECT) && !toSelect.isEmpty()) {
             msg += "<br>" + tr("Selection: {0}", toSelect.size());
         }
@@ -94 +101 @@
     @Override
     public String[] getOptionalParams() {
-        return new String[] {"new_layer", "layer_name", "addtags", "select", "zoom_mode",
-                "changeset_comment", "changeset_source", "changeset_hashtags", "changeset_tags",
-                "search", "layer_locked", "download_policy", "upload_policy"};
+        return new String[] {"new_layer", "layer_name", ADDTAGS, SELECT, "zoom_mode",
+                CHANGESET_COMMENT, CHANGESET_SOURCE, CHANGESET_HASHTAGS, CHANGESET_TAGS,
+                SEARCH, "layer_locked", "download_policy", "upload_policy"};
     }
 
@@ -127 +134 @@
     @Override
     protected void handleRequest() throws RequestHandlerErrorException {
+        download();
+        /*
+         * deselect objects if parameter addtags given
+         */
+        if (args.containsKey(ADDTAGS) && !isKeepingCurrentSelection) {
+            GuiHelper.executeByMainWorkerInEDT(() -> {
+                DataSet ds = MainApplication.getLayerManager().getEditDataSet();
+                if (ds == null) // e.g. download failed
+                    return;
+                ds.clearSelection();
+            });
+        }
+
+        Collection<OsmPrimitive> forTagAdd = performSearchZoom();
+
+        // This comes before the other changeset tags, so that they can be overridden
+        parseChangesetTags(args);
+
+        // add changeset tags after download if necessary
+        addChangesetTags();
+
+        // add tags to objects
+        addTags(forTagAdd);
+    }
+
+    private void download() throws RequestHandlerErrorException {
         DownloadOsmTask osmTask = new DownloadOsmTask();
         try {
@@ -137 +170 @@
                     Area toDownload = null;
                     if (!settings.isNewLayer()) {
-                        // find out whether some data has already been downloaded
-                        Area present = null;
-                        DataSet ds = MainApplication.getLayerManager().getEditDataSet();
-                        if (ds != null) {
-                            present = ds.getDataSourceArea();
-                        }
-                        if (present != null && !present.isEmpty()) {
-                            toDownload = new Area(new Rectangle2D.Double(minlon, minlat, maxlon-minlon, maxlat-minlat));
-                            toDownload.subtract(present);
-                            if (!toDownload.isEmpty()) {
-                                // the result might not be a rectangle (L shaped etc)
-                                Rectangle2D downloadBounds = toDownload.getBounds2D();
-                                minlat = downloadBounds.getMinY();
-                                minlon = downloadBounds.getMinX();
-                                maxlat = downloadBounds.getMaxY();
-                                maxlon = downloadBounds.getMaxX();
-                            }
-                        }
+                        toDownload = removeAlreadyDownloadedArea();
                     }
                     if (toDownload != null && toDownload.isEmpty()) {
                         Logging.info("RemoteControl: no download necessary");
                     } else {
-                        Future<?> future = MainApplication.worker.submit(
-                                new PostDownloadHandler(osmTask, osmTask.download(settings, new Bounds(minlat, minlon, maxlat, maxlon),
-                                        null /* let the task manage the progress monitor */)));
-                        GuiHelper.executeByMainWorkerInEDT(() -> {
-                            try {
-                                future.get(OSM_DOWNLOAD_TIMEOUT.get(), TimeUnit.SECONDS);
-                                if (osmTask.isFailed()) {
-                                    Object error = osmTask.getErrorObjects().get(0);
-                                    if (error instanceof OsmApiException) {
-                                        throw (OsmApiException) error;
-                                    }
-                                    List<Throwable> exceptions = osmTask.getErrorObjects().stream()
-                                                    .filter(Throwable.class::isInstance).map(Throwable.class::cast)
-                                                    .collect(Collectors.toList());
-                                    OsmTransferException osmTransferException =
-                                            new OsmTransferException(String.join(", ", osmTask.getErrorMessages()));
-                                    if (!exceptions.isEmpty()) {
-                                        osmTransferException.initCause(exceptions.get(0));
-                                        exceptions.remove(0);
-                                        exceptions.forEach(osmTransferException::addSuppressed);
-                                    }
-                                    throw osmTransferException;
-                                }
-                            } catch (InterruptedException | ExecutionException | TimeoutException |
-                                    OsmTransferException | RuntimeException ex) { // NOPMD
-                                ExceptionDialogUtil.explainException(ex);
-                            }
-                        });
+                        performDownload(osmTask, settings);
                     }
                 }
@@ -195 +184 @@
             throw new RequestHandlerErrorException(ex);
         }
-
-        /*
-         * deselect objects if parameter addtags given
-         */
-        if (args.containsKey("addtags") && !isKeepingCurrentSelection) {
-            GuiHelper.executeByMainWorkerInEDT(() -> {
-                DataSet ds = MainApplication.getLayerManager().getEditDataSet();
-                if (ds == null) // e.g. download failed
-                    return;
-                ds.clearSelection();
-            });
-        }
-
+    }
+
+    /**
+     * Remove areas that has already been downloaded
+     * @return The area to download
+     */
+    private Area removeAlreadyDownloadedArea() {
+        // find out whether some data has already been downloaded
+        Area toDownload = null;
+        Area present = null;
+        DataSet ds = MainApplication.getLayerManager().getEditDataSet();
+        if (ds != null) {
+            present = ds.getDataSourceArea();
+        }
+        if (present != null && !present.isEmpty()) {
+            toDownload = new Area(new Rectangle2D.Double(minlon, minlat, maxlon-minlon, maxlat-minlat));
+            toDownload.subtract(present);
+            if (!toDownload.isEmpty()) {
+                // the result might not be a rectangle (L shaped etc)
+                Rectangle2D downloadBounds = toDownload.getBounds2D();
+                minlat = downloadBounds.getMinY();
+                minlon = downloadBounds.getMinX();
+                maxlat = downloadBounds.getMaxY();
+                maxlon = downloadBounds.getMaxX();
+            }
+        }
+        return toDownload;
+    }
+
+    private void performDownload(DownloadOsmTask osmTask, DownloadParams settings) {
+        Future<?> future = MainApplication.worker.submit(
+                new PostDownloadHandler(osmTask, osmTask.download(settings, new Bounds(minlat, minlon, maxlat, maxlon),
+                        null /* let the task manage the progress monitor */)));
+        GuiHelper.executeByMainWorkerInEDT(() -> {
+            try {
+                future.get(OSM_DOWNLOAD_TIMEOUT.get(), TimeUnit.SECONDS);
+                if (osmTask.isFailed()) {
+                    Object error = osmTask.getErrorObjects().get(0);
+                    if (error instanceof OsmApiException) {
+                        throw (OsmApiException) error;
+                    }
+                    List<Throwable> exceptions = osmTask.getErrorObjects().stream()
+                            .filter(Throwable.class::isInstance).map(Throwable.class::cast)
+                            .collect(Collectors.toList());
+                    OsmTransferException osmTransferException =
+                            new OsmTransferException(String.join(", ", osmTask.getErrorMessages()));
+                    if (!exceptions.isEmpty()) {
+                        osmTransferException.initCause(exceptions.get(0));
+                        exceptions.remove(0);
+                        exceptions.forEach(osmTransferException::addSuppressed);
+                    }
+                    throw osmTransferException;
+                }
+            } catch (InterruptedException ex) {
+                Thread.currentThread().interrupt();
+                ExceptionDialogUtil.explainException(ex);
+            } catch (ExecutionException | TimeoutException |
+                     OsmTransferException | RuntimeException ex) { // NOPMD
+                ExceptionDialogUtil.explainException(ex);
+            }
+        });
+    }
+
+    private Collection<OsmPrimitive> performSearchZoom() throws RequestHandlerErrorException {
         final Collection<OsmPrimitive> forTagAdd = new LinkedHashSet<>();
         final Bounds bbox = new Bounds(minlat, minlon, maxlat, maxlon);
-        if (args.containsKey("select") && PermissionPrefWithDefault.CHANGE_SELECTION.isAllowed()) {
+        if (args.containsKey(SELECT) && PermissionPrefWithDefault.CHANGE_SELECTION.isAllowed()) {
             // select objects after downloading, zoom to selection.
-            GuiHelper.executeByMainWorkerInEDT(() -> {
-                Set<OsmPrimitive> newSel = new LinkedHashSet<>();
-                DataSet ds = MainApplication.getLayerManager().getEditDataSet();
-                if (ds == null) // e.g. download failed
-                    return;
-                for (SimplePrimitiveId id : toSelect) {
-                    final OsmPrimitive p = ds.getPrimitiveById(id);
-                    if (p != null) {
-                        newSel.add(p);
-                        forTagAdd.add(p);
-                    }
-                }
-                if (isKeepingCurrentSelection) {
-                    Collection<OsmPrimitive> sel = ds.getSelected();
-                    newSel.addAll(sel);
-                    forTagAdd.addAll(sel);
-                }
-                toSelect.clear();
-                ds.setSelected(newSel);
-                zoom(newSel, bbox);
-                MapFrame map = MainApplication.getMap();
-                if (MainApplication.isDisplayingMapView() && map.relationListDialog != null) {
-                    map.relationListDialog.selectRelations(null); // unselect all relations to fix #7342
-                    map.relationListDialog.dataChanged(null);
-                    map.relationListDialog.selectRelations(Utils.filteredCollection(newSel, Relation.class));
-                }
-            });
-        } else if (args.containsKey("search") && PermissionPrefWithDefault.CHANGE_SELECTION.isAllowed()) {
-            try {
-                final SearchCompiler.Match search = SearchCompiler.compile(args.get("search"));
-                MainApplication.worker.submit(() -> {
-                    final DataSet ds = MainApplication.getLayerManager().getEditDataSet();
-                    final Collection<OsmPrimitive> filteredPrimitives = SubclassFilteredCollection.filter(ds.allPrimitives(), search);
-                    ds.setSelected(filteredPrimitives);
-                    forTagAdd.addAll(filteredPrimitives);
-                    zoom(filteredPrimitives, bbox);
-                });
-            } catch (SearchParseError ex) {
-                Logging.error(ex);
-                throw new RequestHandlerErrorException(ex);
-            }
+            GuiHelper.executeByMainWorkerInEDT(() -> selectAndZoom(forTagAdd, bbox));
+        } else if (args.containsKey(SEARCH) && PermissionPrefWithDefault.CHANGE_SELECTION.isAllowed()) {
+            searchAndZoom(forTagAdd, bbox);
         } else {
             // after downloading, zoom to downloaded area.
-            zoom(Collections.<OsmPrimitive>emptySet(), bbox);
-        }
-
-        // This comes before the other changeset tags, so that they can be overridden
-        parseChangesetTags(args);
-
-        // add changeset tags after download if necessary
-        if (args.containsKey("changeset_comment") || args.containsKey("changeset_source") || args.containsKey("changeset_hashtags")) {
+            zoom(Collections.emptySet(), bbox);
+        }
+        return forTagAdd;
+    }
+
+    private void selectAndZoom(Collection<OsmPrimitive> forTagAdd, Bounds bbox) {
+        Set<OsmPrimitive> newSel = new LinkedHashSet<>();
+        DataSet ds = MainApplication.getLayerManager().getEditDataSet();
+        if (ds == null) // e.g. download failed
+            return;
+        for (SimplePrimitiveId id : toSelect) {
+            final OsmPrimitive p = ds.getPrimitiveById(id);
+            if (p != null) {
+                newSel.add(p);
+                forTagAdd.add(p);
+            }
+        }
+        if (isKeepingCurrentSelection) {
+            Collection<OsmPrimitive> sel = ds.getSelected();
+            newSel.addAll(sel);
+            forTagAdd.addAll(sel);
+        }
+        toSelect.clear();
+        ds.setSelected(newSel);
+        zoom(newSel, bbox);
+        MapFrame map = MainApplication.getMap();
+        if (MainApplication.isDisplayingMapView() && map.relationListDialog != null) {
+            map.relationListDialog.selectRelations(null); // unselect all relations to fix #7342
+            map.relationListDialog.dataChanged(null);
+            map.relationListDialog.selectRelations(Utils.filteredCollection(newSel, Relation.class));
+        }
+    }
+
+    private void searchAndZoom(Collection<OsmPrimitive> forTagAdd, Bounds bbox) throws RequestHandlerErrorException {
+        try {
+            final SearchCompiler.Match search = SearchCompiler.compile(args.get(SEARCH));
             MainApplication.worker.submit(() -> {
-                DataSet ds = MainApplication.getLayerManager().getEditDataSet();
-                if (ds != null) {
-                    for (String tag : Arrays.asList("changeset_comment", "changeset_source", "changeset_hashtags")) {
-                        if (args.containsKey(tag)) {
-                            final String tagKey = tag.substring("changeset_".length());
-                            final String value = args.get(tag);
-                            if (!Utils.isStripEmpty(value)) {
-                                ds.addChangeSetTag(tagKey, value);
-                            } else {
-                                ds.addChangeSetTag(tagKey, null);
-                            }
-                        }
-                    }
-                }
+                final DataSet ds = MainApplication.getLayerManager().getEditDataSet();
+                final Collection<OsmPrimitive> filteredPrimitives = SubclassFilteredCollection.filter(ds.allPrimitives(), search);
+                ds.setSelected(filteredPrimitives);
+                forTagAdd.addAll(filteredPrimitives);
+                zoom(filteredPrimitives, bbox);
             });
-        }
-
-        // add tags to objects
-        if (args.containsKey("addtags")) {
+        } catch (SearchParseError ex) {
+            Logging.error(ex);
+            throw new RequestHandlerErrorException(ex);
+        }
+    }
+
+    private void addChangesetTags() {
+        List<String> values = Stream.of(CHANGESET_COMMENT, CHANGESET_SOURCE, CHANGESET_HASHTAGS)
+                .filter(args::containsKey).collect(Collectors.toList());
+        if (values.isEmpty()) {
+            return;
+        }
+        MainApplication.worker.submit(() -> {
+            DataSet ds = MainApplication.getLayerManager().getEditDataSet();
+            if (ds != null) {
+                for (String tag : values) {
+                    final String tagKey = tag.substring("changeset_".length());
+                    final String value = args.get(tag);
+                    if (!Utils.isStripEmpty(value)) {
+                        ds.addChangeSetTag(tagKey, value);
+                    } else {
+                        ds.addChangeSetTag(tagKey, null);
+                    }
+                }
+            }
+        });
+    }
+
+    private void addTags(Collection<OsmPrimitive> forTagAdd) {
+        if (args.containsKey(ADDTAGS)) {
             // needs to run in EDT since forTagAdd is updated in EDT as well
             GuiHelper.executeByMainWorkerInEDT(() -> {
@@ -290 +337 @@
                     new Notification(isKeepingCurrentSelection
                             ? tr("You clicked on a JOSM remotecontrol link that would apply tags onto selected objects.\n"
-                                    + "Since no objects have been selected before this click, no tags were added.\n"
-                                    + "Select one or more objects and click the link again.")
+                            + "Since no objects have been selected before this click, no tags were added.\n"
+                            + "Select one or more objects and click the link again.")
                             : tr("You clicked on a JOSM remotecontrol link that would apply tags onto objects.\n"
-                                    + "Unfortunately that link seems to be broken.\n"
-                                    + "Technical explanation: the URL query parameter ''select='' or ''search='' has an invalid value.\n"
-                                    + "Ask someone at the origin of the clicked link to fix this.")
-                        ).setIcon(JOptionPane.WARNING_MESSAGE).setDuration(Notification.TIME_LONG).show();
+                            + "Unfortunately that link seems to be broken.\n"
+                            + "Technical explanation: the URL query parameter ''select='' or ''search='' has an invalid value.\n"
+                            + "Ask someone at the origin of the clicked link to fix this.")
+                    ).setIcon(JOptionPane.WARNING_MESSAGE).setDuration(Notification.TIME_LONG).show();
                 }
             });
@@ -303 +350 @@
 
     static void parseChangesetTags(Map<String, String> args) {
-        if (args.containsKey("changeset_tags")) {
+        if (args.containsKey(CHANGESET_TAGS)) {
             MainApplication.worker.submit(() -> {
                 DataSet ds = MainApplication.getLayerManager().getEditDataSet();
                 if (ds != null) {
-                    AddTagsDialog.parseUrlTagsToKeyValues(args.get("changeset_tags")).forEach(ds::addChangeSetTag);
+                    AddTagsDialog.parseUrlTagsToKeyValues(args.get(CHANGESET_TAGS)).forEach(ds::addChangeSetTag);
                 }
             });
@@ -366 +413 @@
 
         // Process optional argument 'select'
-        if (args != null && args.containsKey("select")) {
+        validateSelect();
+    }
+
+    private void validateSelect() {
+        if (args != null && args.containsKey(SELECT)) {
             toSelect.clear();
-            for (String item : args.get("select").split(",", -1)) {
+            for (String item : args.get(SELECT).split(",", -1)) {
                 if (!item.isEmpty()) {
                     if (CURRENT_SELECTION.equalsIgnoreCase(item)) {

trunk/test/unit/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandlerTest.java

@@ -2 +2 @@
 package org.openstreetmap.josm.io.remotecontrol.handler;
 
+import static org.junit.jupiter.api.Assertions.assertAll;
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
-
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Collection;
+
+import com.github.tomakehurst.wiremock.client.WireMock;
+import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
+import com.github.tomakehurst.wiremock.matching.RequestPatternBuilder;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.openstreetmap.josm.data.osm.DataSet;
+import org.openstreetmap.josm.data.osm.Node;
+import org.openstreetmap.josm.data.osm.OsmPrimitive;
+import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
+import org.openstreetmap.josm.gui.MainApplication;
 import org.openstreetmap.josm.io.remotecontrol.handler.RequestHandler.RequestHandlerBadRequestException;
 import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
 
 import org.junit.jupiter.api.Test;
+import org.openstreetmap.josm.testutils.annotations.BasicWiremock;
+import org.openstreetmap.josm.testutils.annotations.Main;
+import org.openstreetmap.josm.testutils.annotations.Projection;
+import org.openstreetmap.josm.testutils.annotations.ThreadSync;
 
 /**
  * Unit tests of {@link LoadAndZoomHandler} class.
  */
+@BasicPreferences
+@BasicWiremock
+@ExtendWith(BasicWiremock.OsmApiExtension.class)
 class LoadAndZoomHandlerTest {
+    private static final String DEFAULT_BBOX_URL = "https://localhost/load_and_zoom?left=0&bottom=0&right=0.001&top=0.001";
     private static LoadAndZoomHandler newHandler(String url) throws RequestHandlerBadRequestException {
         LoadAndZoomHandler req = new LoadAndZoomHandler();
+        req.myCommand = LoadAndZoomHandler.command;
         if (url != null)
             req.setUrl(url);
@@ -22 +49 @@
     }
 
+    private static void syncThreads() {
+        // There are calls to the worker thread and EDT
+        new ThreadSync.ThreadSyncExtension().threadSync();
+    }
+
+    @BeforeEach
+    void setup(WireMockRuntimeInfo wireMockRuntimeInfo) {
+        String common = "visible=\"true\" version=\"1\" changeset=\"1\" timestamp=\"2000-01-01T00:00:00Z\" user=\"tsmock\" uid=\"1\"";
+        wireMockRuntimeInfo.getWireMock().register(WireMock.get("/api/0.6/map?bbox=0.0,0.0,0.001,0.001")
+                .willReturn(WireMock.aResponse().withBody("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
+                        "<osm version=\"0.6\" generator=\"hand\" copyright=\"JOSM\" attribution=\"\" license=\"\">\n" +
+                        " <bounds minlat=\"0\" minlon=\"0\" maxlat=\"0.001\" maxlon=\"0.001\"/>\n" +
+                        " <node id=\"1\" " + common + " lat=\"0\" lon=\"0\"/>\n" +
+                        " <node id=\"2\" " + common + " lat=\"0.0001\" lon=\"0.0001\"/>\n" +
+                        " <node id=\"3\" " + common + " lat=\"0.0002\" lon=\"0.0002\"/>\n" +
+                        "</osm>")));
+    }
+
     /**
      * Unit test for bad request - no param.
-     * @throws Exception if any error occurs
-     */
-    @Test
-    void testBadRequestNoParam() throws Exception {
-        Exception e = assertThrows(RequestHandlerBadRequestException.class, () -> newHandler(null).handle());
+     */
+    @Test
+    void testBadRequestNoParam() {
+        final LoadAndZoomHandler handler = assertDoesNotThrow(() -> newHandler(null));
+        Exception e = assertThrows(RequestHandlerBadRequestException.class, handler::handle);
         assertEquals("NumberFormatException (empty String)", e.getMessage());
     }
@@ -34 +79 @@
     /**
      * Unit test for bad request - invalid URL.
-     * @throws Exception if any error occurs
-     */
-    @Test
-    void testBadRequestInvalidUrl() throws Exception {
-        Exception e = assertThrows(RequestHandlerBadRequestException.class, () -> newHandler("invalid_url").handle());
+     */
+    @Test
+    void testBadRequestInvalidUrl() {
+        final LoadAndZoomHandler handler = assertDoesNotThrow(() -> newHandler("invalid_url"));
+        Exception e = assertThrows(RequestHandlerBadRequestException.class, handler::handle);
         assertEquals("The following keys are mandatory, but have not been provided: bottom, top, left, right", e.getMessage());
     }
@@ -44 +89 @@
     /**
      * Unit test for bad request - incomplete URL.
-     * @throws Exception if any error occurs
-     */
-    @Test
-    void testBadRequestIncompleteUrl() throws Exception {
-        Exception e = assertThrows(RequestHandlerBadRequestException.class, () -> newHandler("https://localhost").handle());
+     */
+    @Test
+    void testBadRequestIncompleteUrl() {
+        final LoadAndZoomHandler handler = assertDoesNotThrow(() -> newHandler("https://localhost"));
+        Exception e = assertThrows(RequestHandlerBadRequestException.class, handler::handle);
         assertEquals("The following keys are mandatory, but have not been provided: bottom, top, left, right", e.getMessage());
     }
 
     /**
-     * Unit test for nominal request - local data file.
-     * @throws Exception if any error occurs
-     */
-    @Test
-    @BasicPreferences
-    void testNominalRequest() throws Exception {
-        assertDoesNotThrow(() -> newHandler("https://localhost?bottom=0&top=0&left=1&right=1").handle());
+     * Ensure that a download is called and completed
+     * @param wireMockRuntimeInfo The wiremock information
+     * @throws RequestHandlerBadRequestException If there is an issue with the handler
+     */
+    @Test
+    void testDownload(WireMockRuntimeInfo wireMockRuntimeInfo) throws RequestHandlerBadRequestException {
+        LoadAndZoomHandler handler = newHandler(DEFAULT_BBOX_URL);
+        assertDoesNotThrow(handler::handle);
+        syncThreads();
+        final DataSet ds = MainApplication.getLayerManager().getEditDataSet();
+        assertNotNull(ds);
+        assertAll(() -> assertNotNull(ds.getPrimitiveById(1, OsmPrimitiveType.NODE)),
+                () -> assertNotNull(ds.getPrimitiveById(2, OsmPrimitiveType.NODE)),
+                () -> assertNotNull(ds.getPrimitiveById(3, OsmPrimitiveType.NODE)),
+                () -> assertNull(ds.getPrimitiveById(4, OsmPrimitiveType.NODE)),
+                () -> assertTrue(ds.selectionEmpty()));
+        wireMockRuntimeInfo.getWireMock().verifyThat(1,
+                RequestPatternBuilder.newRequestPattern().withUrl("/api/0.6/map?bbox=0.0,0.0,0.001,0.001"));
+    }
+
+    /**
+     * Ensure that an area isn't downloaded twice
+     * @param wireMockRuntimeInfo The wiremock information
+     * @throws RequestHandlerBadRequestException If there is an issue with the handler
+     */
+    @Test
+    void testDoubleDownload(WireMockRuntimeInfo wireMockRuntimeInfo) throws RequestHandlerBadRequestException {
+        testDownload(wireMockRuntimeInfo);
+        testDownload(wireMockRuntimeInfo);
+        // testDownload checks that the URL has been called once. Since it doesn't reset anything, we don't need
+        // a specific test here.
+    }
+
+    /**
+     * Ensure that an overlapping area is trimmed before download
+     * @param wireMockRuntimeInfo The wiremock information
+     * @throws RequestHandlerBadRequestException If there is an issue with the handler
+     */
+    @Test
+    void testOverlappingArea(WireMockRuntimeInfo wireMockRuntimeInfo) throws RequestHandlerBadRequestException {
+        LoadAndZoomHandler handler = newHandler(DEFAULT_BBOX_URL);
+        assertDoesNotThrow(handler::handle);
+        syncThreads();
+        // The scientific notation is ok server-side.
+        final String mapCall = "/api/0.6/map?bbox=2.5E-4,0.001,7.5E-4,0.00125";
+        final String commonNode = "visible=\"true\" version=\"1\" changeset=\"1\" timestamp=\"2000-01-01T00:00:00Z\" user=\"tsmock\" uid=\"1\"";
+        wireMockRuntimeInfo.getWireMock().register(WireMock.get(mapCall)
+                .willReturn(WireMock.aResponse().withBody("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
+                        "<osm version=\"0.6\" generator=\"hand\" copyright=\"JOSM\" attribution=\"\" license=\"\">\n" +
+                        " <bounds minlat=\"0.001\" minlon=\"0.00025\" maxlat=\"0.00125\" maxlon=\"0.00075\"/>\n" +
+                        " <node id=\"4\" " + commonNode + " lat=\"0.00111\" lon=\"0.00026\"/>\n" +
+                        " <node id=\"5\" " + commonNode + " lat=\"0.0011\" lon=\"0.00025\"/>\n" +
+                        " <node id=\"6\" " + commonNode + " lat=\"0.0012\" lon=\"0.000251\"/>\n" +
+                        "</osm>")));
+        String request = "https://localhost/load_and_zoom?left=0.00025&bottom=0.00025&right=0.00075&top=0.00125";
+        handler = newHandler(request);
+        assertDoesNotThrow(handler::handle);
+        syncThreads();
+        wireMockRuntimeInfo.getWireMock().verifyThat(1, RequestPatternBuilder.newRequestPattern().withUrl(mapCall));
+        final DataSet ds = MainApplication.getLayerManager().getEditDataSet();
+        assertNotNull(ds);
+        assertAll(() -> assertNotNull(ds.getPrimitiveById(1, OsmPrimitiveType.NODE)),
+                () -> assertNotNull(ds.getPrimitiveById(2, OsmPrimitiveType.NODE)),
+                () -> assertNotNull(ds.getPrimitiveById(3, OsmPrimitiveType.NODE)),
+                () -> assertNotNull(ds.getPrimitiveById(4, OsmPrimitiveType.NODE)));
+    }
+
+    /**
+     * Check search and zoom functionality
+     * @throws RequestHandlerBadRequestException If there is an issue with the handler
+     */
+    @Main
+    @Projection
+    @Test
+    void testSearchAndZoom() throws RequestHandlerBadRequestException {
+        final LoadAndZoomHandler handler = newHandler(DEFAULT_BBOX_URL + "&search=id:1");
+        assertDoesNotThrow(handler::handle);
+        syncThreads();
+        final DataSet ds = MainApplication.getLayerManager().getEditDataSet();
+        Collection<OsmPrimitive> selected = ds.getSelected();
+        assertEquals(1, selected.size());
+        assertTrue(selected.contains(ds.getPrimitiveById(1, OsmPrimitiveType.NODE)));
+        assertTrue(ds.searchNodes(MainApplication.getMap().mapView.getRealBounds().toBBox())
+                .contains((Node) ds.getPrimitiveById(1, OsmPrimitiveType.NODE)));
+    }
+
+    /**
+     * Check select and zoom functionality
+     * @throws RequestHandlerBadRequestException If there is an issue with the handler
+     */
+    @Main
+    @Projection
+    @Test
+    void testSelectAndZoom() throws RequestHandlerBadRequestException {
+        final LoadAndZoomHandler handler = newHandler(DEFAULT_BBOX_URL + "&select=n1");
+        assertDoesNotThrow(handler::handle);
+        syncThreads();
+        final DataSet ds = MainApplication.getLayerManager().getEditDataSet();
+        Collection<OsmPrimitive> selected = ds.getSelected();
+        assertEquals(1, selected.size());
+        assertTrue(selected.contains(ds.getPrimitiveById(1, OsmPrimitiveType.NODE)));
+        assertTrue(ds.searchNodes(MainApplication.getMap().mapView.getRealBounds().toBBox())
+                .contains((Node) ds.getPrimitiveById(1, OsmPrimitiveType.NODE)));
+    }
+
+    /**
+     * Check changeset tag functionality
+     * @throws RequestHandlerBadRequestException If there is an issue with the handler
+     */
+    @Test
+    void testChangesetTags() throws RequestHandlerBadRequestException {
+        final String comment = "Add buildings, roads, and other random stuff";
+        final String source = "This isn't Bing";
+        final String hashtag = "#test-hashcodes";
+        final String customTags = "custom=tag|is=here";
+        final LoadAndZoomHandler handler = newHandler(DEFAULT_BBOX_URL
+                + "&changeset_comment=" + URLEncoder.encode(comment, StandardCharsets.UTF_8)
+                + "&changeset_source=" + URLEncoder.encode(source, StandardCharsets.UTF_8)
+                + "&changeset_hashtags=" + URLEncoder.encode(hashtag, StandardCharsets.UTF_8)
+                + "&changeset_tags=" + URLEncoder.encode(customTags, StandardCharsets.UTF_8));
+        assertDoesNotThrow(handler::handle);
+        syncThreads();
+        final DataSet ds = MainApplication.getLayerManager().getEditDataSet();
+        assertEquals(comment, ds.getChangeSetTags().get("comment"));
+        assertEquals(source, ds.getChangeSetTags().get("source"));
+        assertEquals(hashtag, ds.getChangeSetTags().get("hashtags"));
+        assertEquals("tag", ds.getChangeSetTags().get("custom"));
+        assertEquals("here", ds.getChangeSetTags().get("is"));
     }
 }

trunk/test/unit/org/openstreetmap/josm/testutils/annotations/BasicWiremock.java

@@ -3 +3 @@
 
 import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.options;
-import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.fail;
 
@@ -19 +19 @@
 import java.util.Arrays;
 import java.util.List;
-import java.util.stream.Collectors;
-
+
+import com.github.tomakehurst.wiremock.core.WireMockConfiguration;
+import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
 import org.junit.jupiter.api.extension.AfterAllCallback;
-import org.junit.jupiter.api.extension.AfterEachCallback;
 import org.junit.jupiter.api.extension.BeforeAllCallback;
 import org.junit.jupiter.api.extension.BeforeEachCallback;
@@ -29 +29 @@
 import org.junit.jupiter.api.extension.ParameterContext;
 import org.junit.jupiter.api.extension.ParameterResolutionException;
-import org.junit.jupiter.api.extension.ParameterResolver;
 import org.junit.platform.commons.support.AnnotationSupport;
 import org.openstreetmap.josm.TestUtils;
 import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
-import org.openstreetmap.josm.gui.util.GuiHelper;
 import org.openstreetmap.josm.io.OsmApi;
 import org.openstreetmap.josm.spi.preferences.Config;
@@ -43 +41 @@
 import com.github.tomakehurst.wiremock.client.WireMock;
 import com.github.tomakehurst.wiremock.extension.ResponseTransformerV2;
-import com.github.tomakehurst.wiremock.verification.LoggedRequest;
 
 /**
@@ -81 +78 @@
      *
      */
-    class WireMockExtension
-            implements AfterAllCallback, AfterEachCallback, BeforeAllCallback, BeforeEachCallback, ParameterResolver {
+    class WireMockExtension extends com.github.tomakehurst.wiremock.junit5.WireMockExtension {
+        protected WireMockExtension() {
+            this(defaultOptions());
+        }
+
+        /**
+         * Create a new extension with options
+         *
+         * @param builder a {@link Builder}
+         *                instance holding the initialisation parameters for the extension.
+         */
+        protected WireMockExtension(Builder builder) {
+            super(builder);
+        }
+
         /**
          * Get the default wiremock server
@@ -98 +108 @@
                             new Pair<>(new Class<?>[] {ExtensionContext.class }, new Object[] {context }),
                             new Pair<>(new Class<?>[0], new Object[0]))) {
-                        try {
-                            Constructor<? extends ResponseTransformerV2> constructor = responseTransformer
-                                    .getConstructor(parameterMapping.a);
-                            transformers.add(constructor.newInstance(parameterMapping.b));
-                            break;
-                        } catch (ReflectiveOperationException e) {
-                            fail(e);
-                        }
+                        Constructor<? extends ResponseTransformerV2> constructor = assertDoesNotThrow(() ->
+                                responseTransformer.getConstructor(parameterMapping.a));
+                        ResponseTransformerV2 transformerV2 = assertDoesNotThrow(() -> constructor.newInstance(parameterMapping.b));
+                        transformers.add(transformerV2);
+                        break;
                     }
                 }
@@ -114 +121 @@
         }
 
+        static Builder defaultOptions() {
+            WireMockConfiguration options = WireMockConfiguration.options()
+                    .usingFilesUnderDirectory(TestUtils.getTestDataRoot())
+                    .dynamicPort();
+            return extensionOptions().options(options);
+        }
+
         /**
          * Replace URL servers with wiremock
@@ -120 +134 @@
          * @param url            The URL to fix
          * @return A url that points at the wiremock server
+         * @deprecated since 19152 (not used in core; no known users)
          */
+        @Deprecated(forRemoval = true, since = "19152")
         public static String replaceUrl(WireMockServer wireMockServer, String url) {
             try {
@@ -132 +148 @@
 
         @Override
-        public void afterAll(ExtensionContext context) throws Exception {
-            // Run in EDT to avoid stopping wiremock server before wiremock requests finish.
-            GuiHelper.runInEDTAndWait(getWiremock(context)::stop);
-        }
-
-        @Override
-        public void afterEach(ExtensionContext context) throws Exception {
-            List<LoggedRequest> missed = getWiremock(context).findUnmatchedRequests().getRequests();
-            missed.forEach(r -> Logging.error(r.getAbsoluteUrl()));
-            try {
-                assertTrue(missed.isEmpty(), missed.stream().map(LoggedRequest::getUrl).collect(Collectors.joining("\n\n")));
-            } finally {
-                getWiremock(context).resetRequests();
-                getWiremock(context).resetToDefaultMappings();
-                getWiremock(context).resetScenarios();
-                if (AnnotationUtils.elementIsAnnotated(context.getElement(), BasicWiremock.class)
-                        || getWiremock(context) == null) {
-                    this.afterAll(context);
-                }
-            }
-        }
-
-        @Override
-        public void beforeAll(ExtensionContext context) throws Exception {
-            getWiremock(context).start();
-        }
-
-        @Override
-        public void beforeEach(ExtensionContext context) throws Exception {
-            if (AnnotationUtils.elementIsAnnotated(context.getElement(), BasicWiremock.class) || getWiremock(context) == null) {
-                this.beforeAll(context);
-            }
+        protected void onBeforeAll(ExtensionContext extensionContext, WireMockRuntimeInfo wireMockRuntimeInfo) {
+            extensionContext.getStore(ExtensionContext.Namespace.create(BasicWiremock.WireMockExtension.class))
+                    .put(BasicWiremock.WireMockExtension.class, this);
+        }
+
+        @Override
+        protected void onAfterAll(ExtensionContext extensionContext, WireMockRuntimeInfo wireMockRuntimeInfo) {
+            // Sync threads to ensure that no further wiremock requests will be made
+            final ThreadSync.ThreadSyncExtension threadSyncExtension = new ThreadSync.ThreadSyncExtension();
+            assertDoesNotThrow(() -> threadSyncExtension.afterEach(extensionContext));
+        }
+
+        @Override
+        protected void onBeforeEach(ExtensionContext context, WireMockRuntimeInfo wireMockRuntimeInfo) {
             if (context.getTestClass().isPresent()) {
                 List<Field> wireMockFields = AnnotationSupport.findAnnotatedFields(context.getRequiredTestClass(), BasicWiremock.class);
@@ -172 +170 @@
                         try {
                             field.set(context.getTestInstance().orElse(null), getWiremock(context));
+                        } catch (IllegalAccessException e) {
+                            fail(e);
                         } finally {
                             field.setAccessible(isAccessible);
@@ -185 +185 @@
         public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext)
                 throws ParameterResolutionException {
+            if (super.supportsParameter(parameterContext, extensionContext)) {
+                return true;
+            }
+            if (WireMockRuntimeInfo.class.isAssignableFrom(parameterContext.getParameter().getType())) {
+                return true;
+            }
             return parameterContext.getParameter().getAnnotation(BasicWiremock.class) != null
                     && parameterContext.getParameter().getType() == WireMockServer.class;
@@ -192 +198 @@
         public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext)
                 throws ParameterResolutionException {
+            if (super.supportsParameter(parameterContext, extensionContext)) {
+                return super.resolveParameter(parameterContext, extensionContext);
+            }
+            if (WireMockRuntimeInfo.class.isAssignableFrom(parameterContext.getParameter().getType())) {
+                return getRuntimeInfo();
+            }
             return getWiremock(extensionContext);
         }
@@ -199 +211 @@
      * A class specifically to mock OSM API calls
      */
-    class OsmApiExtension extends WireMockExtension {
-        @Override
-        public void afterAll(ExtensionContext context) throws Exception {
-            try {
-                super.afterAll(context);
-            } finally {
-                Config.getPref().put("osm-server.url", "https://invalid.url");
-            }
+    class OsmApiExtension implements BeforeAllCallback, BeforeEachCallback, AfterAllCallback {
+        @Override
+        public void afterAll(ExtensionContext context) {
+            Config.getPref().put("osm-server.url", "https://invalid.url");
         }
 
@@ -214 +222 @@
                 fail("OsmApiExtension requires @BasicPreferences");
             }
-            super.beforeAll(context);
-            Config.getPref().put("osm-server.url", getWiremock(context).baseUrl() + "/api");
-            getWiremock(context).stubFor(WireMock.get("/api/0.6/capabilities")
+            this.beforeEach(context);
+        }
+
+        @Override
+        public void beforeEach(ExtensionContext extensionContext) throws Exception {
+            BasicWiremock.WireMockExtension extension =
+                    extensionContext.getStore(ExtensionContext.Namespace.create(BasicWiremock.WireMockExtension.class))
+                    .get(BasicWiremock.WireMockExtension.class, BasicWiremock.WireMockExtension.class);
+            WireMockRuntimeInfo wireMockRuntimeInfo = extension.getRuntimeInfo();
+            Config.getPref().put("osm-server.url", wireMockRuntimeInfo.getHttpBaseUrl() + "/api");
+            wireMockRuntimeInfo.getWireMock().register(WireMock.get("/api/0.6/capabilities")
                     .willReturn(WireMock.aResponse().withBodyFile("api/0.6/capabilities")));
-            getWiremock(context).stubFor(WireMock.get("/api/capabilities")
+            wireMockRuntimeInfo.getWireMock().register(WireMock.get("/api/capabilities")
                     .willReturn(WireMock.aResponse().withBodyFile("api/capabilities")));
-            OsmApi.getOsmApi().initialize(NullProgressMonitor.INSTANCE);
+            assertDoesNotThrow(() -> OsmApi.getOsmApi().initialize(NullProgressMonitor.INSTANCE));
         }
     }