From 4213a8d1e0a8832acb4c346fa0d16f5750b21986 Mon Sep 17 00:00:00 2001 From: "Martin W. Kirst" Date: Sun, 18 Oct 2020 18:47:52 +0200 Subject: [PATCH] Improve error handling on GROBID server connection issues (#7026) * add explicit configuration for connectTimeout in URLDownload add information on GROBID server connection issue for user relates to #6517 and #6891 * add CHANGELOG entry * fix style issues detected by checkstyle * use .toMilliseconds() is more convenient use assertThrow is more convenient * incorporate feedback from review reduce detail level from user message rework to use FetcherException instead of UncheckedIOException * switch to debug level in order to reduce verboseness in the log --- CHANGELOG.md | 1 + .../BibtexExtractorViewModel.java | 15 ++++++ .../fetcher/GrobidCitationFetcher.java | 46 +++++++++++++------ .../logic/importer/util/GrobidService.java | 2 + .../org/jabref/logic/net/URLDownload.java | 16 ++++++- src/main/resources/l10n/JabRef_de.properties | 1 + src/main/resources/l10n/JabRef_en.properties | 1 + .../fetcher/GrobidCitationFetcherTest.java | 25 ++++++++-- .../org/jabref/logic/net/URLDownloadTest.java | 8 ++++ 9 files changed, 97 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c26f8942fc..be408c6c365 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ inserting new citations in a OpenOffic/LibreOffice document. [#6957](https://git - We improved the duplicate detection when identifiers like DOI or arxiv are semantiaclly the same, but just syntactically differ (e.g. with or without http(s):// prefix). [#6707](https://github.com/JabRef/jabref/issues/6707) - We changed in the group interface "Generate groups from keywords in a BibTeX field" by "Generate groups from keywords in the following field". [#6983](https://github.com/JabRef/jabref/issues/6983) - We changed the name of a group type from "Searching for keywords" to "Searching for a keyword". [6995](https://github.com/JabRef/jabref/pull/6995) +- We changed connect timeouts for server requests to 30 seconds in general and 5 seconds for GROBID server (special) and improved user notifications on connection issues. [7026](https://github.com/JabRef/jabref/pull/7026) ### Fixed diff --git a/src/main/java/org/jabref/gui/bibtexextractor/BibtexExtractorViewModel.java b/src/main/java/org/jabref/gui/bibtexextractor/BibtexExtractorViewModel.java index a7181ba3ae1..f1d99d74331 100644 --- a/src/main/java/org/jabref/gui/bibtexextractor/BibtexExtractorViewModel.java +++ b/src/main/java/org/jabref/gui/bibtexextractor/BibtexExtractorViewModel.java @@ -15,6 +15,7 @@ import org.jabref.gui.externalfiletype.ExternalFileTypes; import org.jabref.gui.util.BackgroundTask; import org.jabref.gui.util.TaskExecutor; +import org.jabref.logic.importer.FetcherException; import org.jabref.logic.importer.fetcher.GrobidCitationFetcher; import org.jabref.logic.l10n.Localization; import org.jabref.model.database.BibDatabaseContext; @@ -22,8 +23,13 @@ import org.jabref.model.util.FileUpdateMonitor; import org.jabref.preferences.JabRefPreferences; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + public class BibtexExtractorViewModel { + private static final Logger LOGGER = LoggerFactory.getLogger(BibtexExtractorViewModel.class); + private final StringProperty inputTextProperty = new SimpleStringProperty(""); private DialogService dialogService; private GrobidCitationFetcher currentCitationfetcher; @@ -58,6 +64,15 @@ public StringProperty inputTextProperty() { public void startParsing() { BackgroundTask.wrap(() -> currentCitationfetcher.performSearch(inputTextProperty.getValue())) .onRunning(() -> dialogService.notify(Localization.lang("Your text is being parsed..."))) + .onFailure((e) -> { + if (e instanceof FetcherException) { + String msg = Localization.lang("There are connection issues with a JabRef server. Detailed information: %0.", + e.getMessage()); + dialogService.notify(msg); + } else { + LOGGER.warn("Missing exception handling.", e); + } + }) .onSuccess(parsedEntries -> { dialogService.notify(Localization.lang("%0 entries were parsed from your query.", String.valueOf(parsedEntries.size()))); importHandler.importEntries(parsedEntries); diff --git a/src/main/java/org/jabref/logic/importer/fetcher/GrobidCitationFetcher.java b/src/main/java/org/jabref/logic/importer/fetcher/GrobidCitationFetcher.java index 39d118c6df5..7a57535efe9 100644 --- a/src/main/java/org/jabref/logic/importer/fetcher/GrobidCitationFetcher.java +++ b/src/main/java/org/jabref/logic/importer/fetcher/GrobidCitationFetcher.java @@ -1,11 +1,13 @@ package org.jabref.logic.importer.fetcher; import java.io.IOException; +import java.net.SocketTimeoutException; import java.util.Arrays; import java.util.List; import java.util.Optional; import java.util.stream.Collectors; +import org.jabref.logic.importer.FetcherException; import org.jabref.logic.importer.ImportFormatPreferences; import org.jabref.logic.importer.ParseException; import org.jabref.logic.importer.SearchBasedFetcher; @@ -20,13 +22,18 @@ public class GrobidCitationFetcher implements SearchBasedFetcher { private static final Logger LOGGER = LoggerFactory.getLogger(GrobidCitationFetcher.class); + private static final String GROBID_URL = "http://grobid.jabref.org:8070"; private ImportFormatPreferences importFormatPreferences; private GrobidService grobidService; public GrobidCitationFetcher(ImportFormatPreferences importFormatPreferences) { + this(importFormatPreferences, new GrobidService(GROBID_URL)); + } + + GrobidCitationFetcher(ImportFormatPreferences importFormatPreferences, GrobidService grobidService) { this.importFormatPreferences = importFormatPreferences; - this.grobidService = new GrobidService(GROBID_URL); + this.grobidService = grobidService; } /** @@ -38,9 +45,14 @@ public GrobidCitationFetcher(ImportFormatPreferences importFormatPreferences) { private Optional parseUsingGrobid(String plainText) { try { return Optional.of(grobidService.processCitation(plainText, GrobidService.ConsolidateCitations.WITH_METADATA)); + } catch (SocketTimeoutException e) { + String msg = "Connection timed out."; + LOGGER.debug(msg, e); + throw new RuntimeException(msg, e); } catch (IOException e) { - LOGGER.debug("Could not process citation", e); - return Optional.empty(); + String msg = "Could not process citation. " + e.getMessage(); + LOGGER.debug(msg, e); + throw new RuntimeException(msg, e); } } @@ -54,20 +66,28 @@ private Optional parseBibToBibEntry(String bibtexString) { } @Override - public List performSearch(String query) { - return Arrays - .stream(query.split("\\r\\r+|\\n\\n+|\\r\\n(\\r\\n)+")) - .map(String::trim) - .filter(str -> !str.isBlank()) - .map(reference -> parseUsingGrobid(reference)) - .flatMap(Optional::stream) - .map(reference -> parseBibToBibEntry(reference)) - .flatMap(Optional::stream) - .collect(Collectors.toList()); + public List performSearch(String query) throws FetcherException { + List bibEntries = null; + try { + bibEntries = Arrays + .stream(query.split("\\r\\r+|\\n\\n+|\\r\\n(\\r\\n)+")) + .map(String::trim) + .filter(str -> !str.isBlank()) + .map(this::parseUsingGrobid) + .flatMap(Optional::stream) + .map(this::parseBibToBibEntry) + .flatMap(Optional::stream) + .collect(Collectors.toList()); + } catch (RuntimeException e) { + // un-wrap the wrapped exceptions + throw new FetcherException(e.getMessage(), e.getCause()); + } + return bibEntries; } @Override public String getName() { return "GROBID"; } + } diff --git a/src/main/java/org/jabref/logic/importer/util/GrobidService.java b/src/main/java/org/jabref/logic/importer/util/GrobidService.java index 4e3bc027c81..ec9b7567fc0 100644 --- a/src/main/java/org/jabref/logic/importer/util/GrobidService.java +++ b/src/main/java/org/jabref/logic/importer/util/GrobidService.java @@ -3,6 +3,7 @@ import java.io.IOException; import java.net.URLEncoder; import java.nio.charset.StandardCharsets; +import java.time.Duration; import org.jabref.logic.net.URLDownload; @@ -47,6 +48,7 @@ public String processCitation(String rawCitation, ConsolidateCitations consolida rawCitation = URLEncoder.encode(rawCitation, StandardCharsets.UTF_8); URLDownload urlDownload = new URLDownload(grobidServerURL + "/api/processCitation"); + urlDownload.setConnectTimeout(Duration.ofSeconds(5)); urlDownload.addHeader("Accept", MediaTypes.APPLICATION_BIBTEX); urlDownload.setPostData("citations=" + rawCitation + "&consolidateCitations=" + consolidateCitations); String httpResponse = urlDownload.asString(); diff --git a/src/main/java/org/jabref/logic/net/URLDownload.java b/src/main/java/org/jabref/logic/net/URLDownload.java index eec18e1756a..99887c7ac93 100644 --- a/src/main/java/org/jabref/logic/net/URLDownload.java +++ b/src/main/java/org/jabref/logic/net/URLDownload.java @@ -26,6 +26,7 @@ import java.nio.file.StandardCopyOption; import java.security.SecureRandom; import java.security.cert.X509Certificate; +import java.time.Duration; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -62,11 +63,13 @@ public class URLDownload { public static final String USER_AGENT = "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:79.0) Gecko/20100101 Firefox/79.0"; - private static final Logger LOGGER = LoggerFactory.getLogger(URLDownload.class); + private static final Duration DEFAULT_CONNECT_TIMEOUT = Duration.ofSeconds(30); + private final URL source; private final Map parameters = new HashMap<>(); private String postData = ""; + private Duration connectTimeout = DEFAULT_CONNECT_TIMEOUT; /** * @param source the URL to download from @@ -316,6 +319,7 @@ private void copy(InputStream in, Writer out, Charset encoding) throws IOExcepti private URLConnection openConnection() throws IOException { URLConnection connection = this.source.openConnection(); + connection.setConnectTimeout((int) connectTimeout.toMillis()); for (Entry entry : this.parameters.entrySet()) { connection.setRequestProperty(entry.getKey(), entry.getValue()); } @@ -346,4 +350,14 @@ private URLConnection openConnection() throws IOException { return connection; } + + public void setConnectTimeout(Duration connectTimeout) { + if (connectTimeout != null) { + this.connectTimeout = connectTimeout; + } + } + + public Duration getConnectTimeout() { + return connectTimeout; + } } diff --git a/src/main/resources/l10n/JabRef_de.properties b/src/main/resources/l10n/JabRef_de.properties index 1cb9fda8946..9f697461486 100644 --- a/src/main/resources/l10n/JabRef_de.properties +++ b/src/main/resources/l10n/JabRef_de.properties @@ -1637,6 +1637,7 @@ User=Benutzer Connect=Verbinden Connection\ error=Verbindungsfehler Connection\ to\ %0\ server\ established.=Verbindung zum %0 Server hergestellt. +There\ are\ connection\ issues\ with\ a\ JabRef\ server.\ Detailed\ information\:\ %0.=Es gibt Verbindungsprobleme mit einem JabRef Server. Detailinformation: %0. Required\ field\ "%0"\ is\ empty.=Erforederliches Feld "%0" ist leer. %0\ driver\ not\ available.=%0-Treiber nicht verfügbar. The\ connection\ to\ the\ server\ has\ been\ terminated.=Verbindung zum Server wurde abgebrochen. diff --git a/src/main/resources/l10n/JabRef_en.properties b/src/main/resources/l10n/JabRef_en.properties index 2d2bacc4113..47a7b78e9d8 100644 --- a/src/main/resources/l10n/JabRef_en.properties +++ b/src/main/resources/l10n/JabRef_en.properties @@ -1631,6 +1631,7 @@ User=User Connect=Connect Connection\ error=Connection error Connection\ to\ %0\ server\ established.=Connection to %0 server established. +There\ are\ connection\ issues\ with\ a\ JabRef\ server.\ Detailed\ information\:\ %0.=There are connection issues with a JabRef server. Detailed information: %0. Required\ field\ "%0"\ is\ empty.=Required field "%0" is empty. %0\ driver\ not\ available.=%0 driver not available. The\ connection\ to\ the\ server\ has\ been\ terminated.=The connection to the server has been terminated. diff --git a/src/test/java/org/jabref/logic/importer/fetcher/GrobidCitationFetcherTest.java b/src/test/java/org/jabref/logic/importer/fetcher/GrobidCitationFetcherTest.java index d223dca22a5..329748dcd56 100644 --- a/src/test/java/org/jabref/logic/importer/fetcher/GrobidCitationFetcherTest.java +++ b/src/test/java/org/jabref/logic/importer/fetcher/GrobidCitationFetcherTest.java @@ -1,10 +1,13 @@ package org.jabref.logic.importer.fetcher; +import java.io.IOException; import java.util.Collections; import java.util.List; import java.util.stream.Stream; +import org.jabref.logic.importer.FetcherException; import org.jabref.logic.importer.ImportFormatPreferences; +import org.jabref.logic.importer.util.GrobidService; import org.jabref.model.entry.BibEntry; import org.jabref.model.entry.field.StandardField; import org.jabref.model.entry.types.StandardEntryType; @@ -17,7 +20,11 @@ import org.mockito.Answers; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; @FetcherTest public class GrobidCitationFetcherTest { @@ -76,28 +83,38 @@ public static Stream provideInvalidInput() { @ParameterizedTest(name = "{0}") @MethodSource("provideExamplesForCorrectResultTest") - public void grobidPerformSearchCorrectResultTest(String testName, BibEntry expectedBibEntry, String searchQuery) { + public void grobidPerformSearchCorrectResultTest(String testName, BibEntry expectedBibEntry, String searchQuery) throws FetcherException { List entries = grobidCitationFetcher.performSearch(searchQuery); assertEquals(List.of(expectedBibEntry), entries); } @Test - public void grobidPerformSearchCorrectlySplitsStringTest() { + public void grobidPerformSearchCorrectlySplitsStringTest() throws FetcherException { List entries = grobidCitationFetcher.performSearch(example1 + "\n\n" + example2 + "\r\n\r\n" + example3 + "\r\r" + example4); assertEquals(List.of(example1AsBibEntry, example2AsBibEntry, example3AsBibEntry, example4AsBibEntry), entries); } @Test - public void grobidPerformSearchWithEmptyStringsTest() { + public void grobidPerformSearchWithEmptyStringsTest() throws FetcherException { List entries = grobidCitationFetcher.performSearch(" \n "); assertEquals(Collections.emptyList(), entries); } @ParameterizedTest @MethodSource("provideInvalidInput") - public void grobidPerformSearchWithInvalidDataTest(String invalidInput) { + public void grobidPerformSearchWithInvalidDataTest(String invalidInput) throws FetcherException { List entries = grobidCitationFetcher.performSearch(invalidInput); assertEquals(Collections.emptyList(), entries); } + @Test + public void performSearchThrowsExceptionInCaseOfConnectionIssues() throws IOException { + GrobidService grobidServiceMock = mock(GrobidService.class); + when(grobidServiceMock.processCitation(anyString(), any())).thenThrow(new IOException("Any IO Exception")); + grobidCitationFetcher = new GrobidCitationFetcher(importFormatPreferences, grobidServiceMock); + + assertThrows(FetcherException.class, () -> { + grobidCitationFetcher.performSearch("any text"); + }, "performSearch should throw an FetcherException, when there are underlying IOException."); + } } diff --git a/src/test/java/org/jabref/logic/net/URLDownloadTest.java b/src/test/java/org/jabref/logic/net/URLDownloadTest.java index f77f0fc2431..ea0f133c10f 100644 --- a/src/test/java/org/jabref/logic/net/URLDownloadTest.java +++ b/src/test/java/org/jabref/logic/net/URLDownloadTest.java @@ -109,4 +109,12 @@ public void testCheckConnectionFail() throws MalformedURLException { assertThrows(UnirestException.class, nonsense::canBeReached); } + @Test + public void connectTimeoutIsNeverNull() throws MalformedURLException { + URLDownload urlDownload = new URLDownload(new URL("http://www.example.com")); + assertNotNull(urlDownload.getConnectTimeout(), "there's a non-null default by the constructor"); + + urlDownload.setConnectTimeout(null); + assertNotNull(urlDownload.getConnectTimeout(), "no null value can be set"); + } }