Index: trunk/src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java =================================================================== --- trunk/src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java (revision 19151) +++ trunk/src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java (revision 19152) @@ -6,5 +6,4 @@ import java.awt.geom.Area; import java.awt.geom.Rectangle2D; -import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -18,4 +17,5 @@ import java.util.concurrent.TimeoutException; import java.util.stream.Collectors; +import java.util.stream.Stream; import javax.swing.JOptionPane; @@ -65,4 +65,11 @@ 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,5 +88,5 @@ String msg = tr("Remote Control has been asked to load data from the API.") + "
" + tr("Bounding box: ") + new BBox(minlon, minlat, maxlon, maxlat).toStringCSV(", "); - if (args.containsKey("select") && !toSelect.isEmpty()) { + if (args.containsKey(SELECT) && !toSelect.isEmpty()) { msg += "
" + tr("Selection: {0}", toSelect.size()); } @@ -94,7 +101,7 @@ @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,4 +134,30 @@ @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 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,54 +170,10 @@ 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 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,92 +184,150 @@ 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 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 performSearchZoom() throws RequestHandlerErrorException { final Collection 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 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 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 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.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 forTagAdd, Bounds bbox) { + Set 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 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 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 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 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 forTagAdd) { + if (args.containsKey(ADDTAGS)) { // needs to run in EDT since forTagAdd is updated in EDT as well GuiHelper.executeByMainWorkerInEDT(() -> { @@ -290,11 +337,11 @@ 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,9 +350,9 @@ static void parseChangesetTags(Map 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,7 +413,11 @@ // 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)) { Index: trunk/test/unit/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandlerTest.java =================================================================== --- trunk/test/unit/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandlerTest.java (revision 19151) +++ trunk/test/unit/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandlerTest.java (revision 19152) @@ -2,19 +2,46 @@ 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,11 +49,29 @@ } + 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("\n" + + "\n" + + " \n" + + " \n" + + " \n" + + " \n" + + ""))); + } + /** * 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,9 +79,9 @@ /** * 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,20 +89,141 @@ /** * 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("\n" + + "\n" + + " \n" + + " \n" + + " \n" + + " \n" + + ""))); + 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 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 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")); } } Index: trunk/test/unit/org/openstreetmap/josm/testutils/annotations/BasicWiremock.java =================================================================== --- trunk/test/unit/org/openstreetmap/josm/testutils/annotations/BasicWiremock.java (revision 19151) +++ trunk/test/unit/org/openstreetmap/josm/testutils/annotations/BasicWiremock.java (revision 19152) @@ -3,5 +3,5 @@ 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,8 +19,8 @@ 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,9 +29,7 @@ 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,5 +41,4 @@ import com.github.tomakehurst.wiremock.client.WireMock; import com.github.tomakehurst.wiremock.extension.ResponseTransformerV2; -import com.github.tomakehurst.wiremock.verification.LoggedRequest; /** @@ -81,6 +78,19 @@ * */ - 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,12 +108,9 @@ new Pair<>(new Class[] {ExtensionContext.class }, new Object[] {context }), new Pair<>(new Class[0], new Object[0]))) { - try { - Constructor constructor = responseTransformer - .getConstructor(parameterMapping.a); - transformers.add(constructor.newInstance(parameterMapping.b)); - break; - } catch (ReflectiveOperationException e) { - fail(e); - } + Constructor constructor = assertDoesNotThrow(() -> + responseTransformer.getConstructor(parameterMapping.a)); + ResponseTransformerV2 transformerV2 = assertDoesNotThrow(() -> constructor.newInstance(parameterMapping.b)); + transformers.add(transformerV2); + break; } } @@ -114,4 +121,11 @@ } + static Builder defaultOptions() { + WireMockConfiguration options = WireMockConfiguration.options() + .usingFilesUnderDirectory(TestUtils.getTestDataRoot()) + .dynamicPort(); + return extensionOptions().options(options); + } + /** * Replace URL servers with wiremock @@ -120,5 +134,7 @@ * @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,36 +148,18 @@ @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 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 wireMockFields = AnnotationSupport.findAnnotatedFields(context.getRequiredTestClass(), BasicWiremock.class); @@ -172,4 +170,6 @@ try { field.set(context.getTestInstance().orElse(null), getWiremock(context)); + } catch (IllegalAccessException e) { + fail(e); } finally { field.setAccessible(isAccessible); @@ -185,4 +185,10 @@ 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,4 +198,10 @@ 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,12 +211,8 @@ * 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,11 +222,19 @@ 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)); } }