Changeset 18805 in josm


Ignore:
Timestamp:
2023-08-14T17:34:52+02:00 (16 months ago)
Author:
taylor.smock
Message:

Fix #23117: NPE in PlaceSelection$NamedResultTableModel

This occurs when (a) search results are returned and then (b) the user searches
for more results, but nominatim returns garbage.

In addition, this also fixes some lint issues.

Location:
trunk
Files:
2 added
2 edited

Legend:

Unmodified
Added
Removed

  • trunk/src/org/openstreetmap/josm/gui/download/PlaceSelection.java

    r18173 r18805  
    8383
    8484    private static class Server {
    85         public final String name;
    86         public final BiFunction<String, Collection<SearchResult>, URL> urlFunction;
    87         public final String thirdcol;
    88         public final String fourthcol;
     85        final String name;
     86        final BiFunction<String, Collection<SearchResult>, URL> urlFunction;
     87        final String thirdcol;
     88        final String fourthcol;
    8989
    9090        Server(String n, BiFunction<String, Collection<SearchResult>, URL> u, String t, String f) {
    … …  
    133133    /**
    134134     * Adds a new tab to the download dialog in JOSM.
    135      *
     135     * <p>
    136136     * This method is, for all intents and purposes, the constructor for this class.
    137137     */
    … …  
    192192        public void actionPerformed(ActionEvent e) {
    193193            String searchExpression = cbSearchExpression.getText();
    194             if (!isEnabled() || searchExpression.trim().isEmpty())
     194            if (!isEnabled() || searchExpression.trim().isEmpty() || serverComboBox.getSelectedItem() == null)
    195195                return;
    196196            cbSearchExpression.addCurrentItemToHistory();
    … …  
    298298                            JOptionPane.WARNING_MESSAGE, null
    299299                    ));
     300                    generateLastException(e);
    300301                }
    301302            } catch (IOException | ParserConfigurationException e) {
    302                 if (!canceled) {
    303                     OsmTransferException ex = new OsmTransferException(e);
    304                     ex.setUrl(url.toString());
    305                     lastException = ex;
    306                 }
     303                generateLastException(e);
     304            }
     305        }
     306
     307        /**
     308         * Generate an {@link OsmTransferException} that will be stored in {@link #lastException} if the operation is
     309         * not cancelled.
     310         * @param throwable The throwable to store as an {@link OsmTransferException}
     311         */
     312        private void generateLastException(Throwable throwable) {
     313            if (!canceled) {
     314                OsmTransferException ex = new OsmTransferException(throwable);
     315                ex.setUrl(url.toString());
     316                lastException = ex;
    307317            }
    308318        }
    … …  
    337347        }
    338348
     349        /**
     350         * Add data to the table
     351         * @param data The data to add
     352         */
    339353        public void addData(List<SearchResult> data) {
    340354            this.data.addAll(data);
    … …  
    403417        }
    404418
     419        /**
     420         * Set the header column values for the third and fourth columns
     421         * @param third The new header for the third column
     422         * @param fourth The new header for the fourth column
     423         */
    405424        public void setHeadlines(String third, String fourth) {
    406425            col3.setHeaderValue(third);

  • trunk/test/unit/org/openstreetmap/josm/gui/download/PlaceSelectionTest.java

    r18037 r18805  
    22package org.openstreetmap.josm.gui.download;
    33
     4import static org.junit.jupiter.api.Assertions.assertEquals;
     5import static org.junit.jupiter.api.Assertions.assertInstanceOf;
     6import static org.junit.jupiter.api.Assertions.fail;
     7
     8import java.io.IOException;
     9import java.io.Reader;
     10import java.util.Collections;
     11import java.util.List;
     12import java.util.concurrent.atomic.AtomicBoolean;
     13import java.util.function.Supplier;
     14import java.util.stream.Stream;
     15
     16import javax.swing.JButton;
     17import javax.swing.JPanel;
     18import javax.xml.parsers.ParserConfigurationException;
     19
     20import org.awaitility.Awaitility;
     21import org.junit.jupiter.api.Test;
     22import org.junit.jupiter.params.ParameterizedTest;
     23import org.junit.jupiter.params.provider.Arguments;
     24import org.junit.jupiter.params.provider.MethodSource;
     25import org.junit.platform.commons.support.ReflectionSupport;
     26import org.openstreetmap.josm.TestUtils;
    427import org.openstreetmap.josm.data.Bounds;
     28import org.openstreetmap.josm.gui.MainApplication;
     29import org.openstreetmap.josm.gui.util.GuiHelper;
     30import org.openstreetmap.josm.gui.widgets.HistoryComboBox;
     31import org.openstreetmap.josm.io.NameFinder;
    532import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
     33import org.openstreetmap.josm.testutils.mockers.BugReportMock;
     34import org.openstreetmap.josm.testutils.mockers.ImageProviderMock;
     35import org.xml.sax.SAXException;
     36import org.xml.sax.SAXParseException;
    637
    7 import org.junit.jupiter.api.Test;
     38import com.github.tomakehurst.wiremock.client.WireMock;
     39import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
     40import com.github.tomakehurst.wiremock.junit5.WireMockTest;
     41import mockit.Mock;
     42import mockit.MockUp;
    843
    944/**
    … …  
    1146 */
    1247@BasicPreferences
     48@WireMockTest
    1349class PlaceSelectionTest {
    1450    /**
    … …  
    2258        sel.setDownloadArea(new Bounds(0, 0, 1, 1));
    2359    }
     60
     61    static Stream<Arguments> testTicket23117() {
     62        return Stream.of(Arguments.of(SAXParseException.class, (Supplier<Throwable>) () -> new SAXParseException("", "", "", 0, 0)),
     63                Arguments.of(IOException.class, (Supplier<Throwable>) IOException::new),
     64                Arguments.of(ParserConfigurationException.class, (Supplier<Throwable>) ParserConfigurationException::new));
     65    }
     66
     67    /**
     68     * This checks to make certain that an exception thrown in parsing does not cause a {@link RuntimeException}.
     69     * @param expectedThrowable The expected throwable class, mostly used to indicate which throwable was under test
     70     * @param exceptionSupplier The supplier for the throwable
     71     * @param wireMockRuntimeInfo The wiremock to avoid network requests
     72     */
     73    @ParameterizedTest
     74    @MethodSource
     75    void testTicket23117(Class<? extends Throwable> expectedThrowable, Supplier<Throwable> exceptionSupplier,
     76                         WireMockRuntimeInfo wireMockRuntimeInfo) throws Exception {
     77        TestUtils.assumeWorkingJMockit();
     78        NameFinder.NOMINATIM_URL_PROP.put(wireMockRuntimeInfo.getHttpBaseUrl() + '/');
     79        // We just want to return something quickly so we can get to the place where we throw an exception in a mock.
     80        wireMockRuntimeInfo.getWireMock().register(WireMock.get(WireMock.anyUrl()).willReturn(WireMock.aResponse()));
     81        new ImageProviderMock();
     82        final NameFinderMock nameFinderMock = new NameFinderMock(exceptionSupplier);
     83        final BugReportMock bugReportMock = new BugReportMock();
     84        final PlaceSelection placeSelection = new PlaceSelection();
     85        // We need to call addGui prior to buildSearchPanel -- we really want the panel, but it needs objects initialized
     86        // in addGui
     87        placeSelection.addGui(null);
     88        final JPanel searchPanel = placeSelection.buildSearchPanel();
     89        final HistoryComboBox cbSearchExpression = (HistoryComboBox) ((JPanel) searchPanel.getComponent(0)).getComponent(3);
     90        final PlaceSelection.SearchAction search = (PlaceSelection.SearchAction) ((JButton) searchPanel.getComponent(1)).getAction();
     91        final PlaceSelection.NamedResultTableModel model = (PlaceSelection.NamedResultTableModel)
     92                ReflectionSupport.tryToReadFieldValue(PlaceSelection.class.getDeclaredField("model"), placeSelection).get();
     93
     94        // This is needed in order to enable the action
     95        cbSearchExpression.setText("Somewhere, Podunk");
     96        search.updateState();
     97        search.actionPerformed(null);
     98        // Sync threads
     99        final AtomicBoolean threadSync = new AtomicBoolean();
     100        MainApplication.worker.execute(() -> GuiHelper.runInEDTAndWaitWithException(() -> threadSync.set(true)));
     101        Awaitility.await().untilTrue(threadSync);
     102        model.addData(Collections.singletonList(new NameFinder.SearchResult()));
     103        assertEquals(1, nameFinderMock.calledTimes);
     104        // Call the search again -- this is needed since we need to be in the "Search more..." code paths
     105        search.updateState();
     106        search.actionPerformed(null);
     107        // Sync threads again
     108        threadSync.set(false);
     109        MainApplication.worker.execute(() -> GuiHelper.runInEDTAndWaitWithException(() -> threadSync.set(true)));
     110        Awaitility.await().untilTrue(threadSync);
     111        assertEquals(2, nameFinderMock.calledTimes);
     112        if (bugReportMock.throwable() != null) {
     113            assertInstanceOf(expectedThrowable, bugReportMock.throwable().getCause(),
     114                    "Some explanations will cause a bug report window. " +
     115                            "In those cases, the expected throwable should be the same class as the cause.");
     116        }
     117    }
     118
     119    private static class NameFinderMock extends MockUp<NameFinder> {
     120        private final Supplier<Throwable> exceptionSupplier;
     121        int calledTimes;
     122        NameFinderMock(Supplier<Throwable> exceptionSupplier) {
     123            this.exceptionSupplier = exceptionSupplier;
     124        }
     125
     126        @Mock
     127        public List<NameFinder.SearchResult> parseSearchResults(Reader reader) throws IOException, ParserConfigurationException, SAXException {
     128            this.calledTimes++;
     129            final Throwable throwable = this.exceptionSupplier.get();
     130            if (throwable instanceof IOException) {
     131                throw (IOException) throwable;
     132            } else if (throwable instanceof ParserConfigurationException) {
     133                throw (ParserConfigurationException) throwable;
     134            } else if (throwable instanceof SAXException) {
     135                throw (SAXException) throwable;
     136            }
     137            fail("This method does not throw the specified throwable", throwable);
     138            return Collections.emptyList();
     139        }
     140    }
    24141}
Note: See TracChangeset for help on using the changeset viewer.