diff --git a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java index cef3ae75..d5c6b32f 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java +++ b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java @@ -158,6 +158,7 @@ import org.labkey.panoramapublic.message.PrivateDataMessageScheduler; import org.labkey.panoramapublic.message.PrivateDataReminderSettings; import org.labkey.panoramapublic.ncbi.MockNcbiPublicationSearchService; +import org.labkey.panoramapublic.ncbi.NcbiApiKeyCheck; import org.labkey.panoramapublic.ncbi.NcbiPublicationSearchService; import org.labkey.panoramapublic.ncbi.NcbiPublicationSearchServiceImpl; import org.labkey.panoramapublic.ncbi.PublicationMatch; @@ -10084,6 +10085,55 @@ public static ActionURL getViewExperimentModificationsURL(int experimentAnnotati return result; } + @RequiresPermission(AdminOperationsPermission.class) + public static class ValidateNcbiApiKeyAction extends MutatingApiAction + { + @Override + public Object execute(PrivateDataReminderSettingsForm form, BindException errors) + { + ApiSimpleResponse response = new ApiSimpleResponse(); + response.put("success", true); + + // An empty field means check the key that is already saved, since the form never + // displays it. + boolean checkingSavedKey = StringUtils.isBlank(form.getNcbiApiKey()); + String apiKey = checkingSavedKey + ? PrivateDataReminderSettings.get().getNcbiApiKey() + : form.getNcbiApiKey().trim(); + + if (StringUtils.isBlank(apiKey)) + { + response.put("valid", false); + response.put("message", "Enter a key to validate, or save one first."); + return response; + } + + NcbiApiKeyCheck check = NcbiPublicationSearchService.get().checkApiKey(apiKey); + response.put("valid", check.isValid()); + if (check.isValid()) + { + // Validating does not store anything, so say so. Otherwise "accepted" reads as + // confirmation that the key is now in effect. + response.put("message", checkingSavedKey + ? "NCBI accepted the saved key." + : "NCBI accepted this key. Click Save to store it."); + LOG.info("NCBI accepted an API key entered on the Private Data Reminder Settings page."); + } + else + { + // The short message goes beside the field. NCBI's own words are offered separately, + // since the admin holding the key is the one who has to act on them. + response.put("message", check.isRejected() + ? "NCBI rejected this key." + : "Could not reach NCBI to check this key."); + response.put("detail", check.getMessage()); + LOG.warn("Could not confirm an API key entered on the Private Data Reminder Settings page. {}", + check.getMessage()); + } + return response; + } + } + @RequiresPermission(AdminOperationsPermission.class) public static class PrivateDataReminderSettingsAction extends FormViewAction { @@ -10146,6 +10196,8 @@ public ModelAndView getView(PrivateDataReminderSettingsForm form, boolean reshow form.setExtensionLength(settings.getExtensionLength()); form.setEnablePublicationSearch(settings.isEnablePublicationSearch()); form.setPublicationSearchFrequency(settings.getPublicationSearchFrequency()); + // Do not put the saved key in the form. The JSP shows only whether one is stored. + form.setNcbiApiKeySet(PrivateDataReminderSettings.hasNcbiApiKey()); } VBox view = new VBox(); @@ -10168,6 +10220,17 @@ public boolean handlePost(PrivateDataReminderSettingsForm form, BindException er settings.setPublicationSearchFrequency(form.getPublicationSearchFrequency()); PrivateDataReminderSettings.save(settings); + // A blank field leaves the saved key alone, so editing the reminder schedule cannot + // erase it. Removing a key takes the explicit checkbox. + if (form.isClearNcbiApiKey()) + { + PrivateDataReminderSettings.saveNcbiApiKey(null); + } + else if (!StringUtils.isBlank(form.getNcbiApiKey())) + { + PrivateDataReminderSettings.saveNcbiApiKey(form.getNcbiApiKey()); + } + PrivateDataMessageScheduler.getInstance().initialize(settings.isEnableReminders()); return true; } @@ -10205,6 +10268,9 @@ public static class PrivateDataReminderSettingsForm private Integer _delayUntilFirstReminder; private boolean _enablePublicationSearch; private Integer _publicationSearchFrequency; + private String _ncbiApiKey; + private boolean _clearNcbiApiKey; + private boolean _ncbiApiKeySet; public boolean isEnabled() { @@ -10275,6 +10341,36 @@ public void setPublicationSearchFrequency(Integer publicationSearchFrequency) { _publicationSearchFrequency = publicationSearchFrequency; } + + public String getNcbiApiKey() + { + return _ncbiApiKey; + } + + public void setNcbiApiKey(String ncbiApiKey) + { + _ncbiApiKey = ncbiApiKey; + } + + public boolean isClearNcbiApiKey() + { + return _clearNcbiApiKey; + } + + public void setClearNcbiApiKey(boolean clearNcbiApiKey) + { + _clearNcbiApiKey = clearNcbiApiKey; + } + + public boolean isNcbiApiKeySet() + { + return _ncbiApiKeySet; + } + + public void setNcbiApiKeySet(boolean ncbiApiKeySet) + { + _ncbiApiKeySet = ncbiApiKeySet; + } } @RequiresPermission(AdminOperationsPermission.class) diff --git a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicModule.java b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicModule.java index 1c9ea49b..6942ac20 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicModule.java +++ b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicModule.java @@ -16,6 +16,7 @@ package org.labkey.panoramapublic; +import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.labkey.api.admin.FolderSerializationRegistry; @@ -34,6 +35,7 @@ import org.labkey.api.security.roles.RoleManager; import org.labkey.api.settings.AdminConsole; import org.labkey.api.targetedms.TargetedMSService; +import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.ActionURL; import org.labkey.api.view.BaseWebPartFactory; import org.labkey.api.view.HtmlView; @@ -46,6 +48,7 @@ import org.labkey.panoramapublic.bluesky.BlueskyApiClient; import org.labkey.panoramapublic.bluesky.PanoramaPublicLogoResourceType; import org.labkey.panoramapublic.catalog.CatalogImageAttachmentType; +import org.labkey.panoramapublic.message.PrivateDataMessageScheduler; import org.labkey.panoramapublic.message.PrivateDataReminderSettings; import org.labkey.panoramapublic.ncbi.NcbiPublicationSearchServiceImpl; import org.labkey.panoramapublic.model.Journal; @@ -82,6 +85,8 @@ public class PanoramaPublicModule extends SpringModule { + private static final Logger LOG = LogHelper.getLogger(PanoramaPublicModule.class, "Panorama Public module"); + public static final String NAME = "PanoramaPublic"; public static final String DOWNLOAD_DATA_INFO_WP = "Download Data"; @@ -151,6 +156,22 @@ protected void startupAfterSpringConfig(ModuleContext moduleContext) { fileContentService.addFileListener(new PanoramaPublicFileListener()); } + + } + + @Override + public void startBackgroundThreads() + { + // Re-establish the reminder schedule on every startup. Reminder messages contain absolute + // URLs, which are only safe to build once this method is called. + try + { + PrivateDataMessageScheduler.getInstance().initialize(PrivateDataReminderSettings.get().isEnableReminders()); + } + catch (RuntimeException e) + { + LOG.error("Failed to schedule the Panorama Public private data reminder job", e); + } } @NotNull diff --git a/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java b/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java index a75b411c..376e25e5 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java +++ b/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java @@ -15,6 +15,7 @@ */ package org.labkey.panoramapublic.message; +import org.apache.commons.lang3.StringUtils; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.junit.Assert; @@ -30,6 +31,7 @@ import java.time.format.DateTimeFormatter; import java.time.format.DateTimeParseException; import java.util.Date; +import java.util.Map; public class PrivateDataReminderSettings { @@ -41,6 +43,8 @@ public class PrivateDataReminderSettings public static final String PROP_EXTENSION_LENGTH = "Extension duration (months)"; public static final String PROP_ENABLE_PUBLICATION_SEARCH = "Enable publication search"; public static final String PROP_PUBLICATION_SEARCH_FREQUENCY = "Publication search frequency (months)"; + public static final String PROP_NCBI_API_KEY = "NCBI API key"; + public static final String PROP_NCBI_CREDENTIALS = "Panorama Public NCBI credentials"; private static final boolean DEFAULT_ENABLE_REMINDERS = false; public static final String DEFAULT_REMINDER_TIME = "8:00 AM"; @@ -61,6 +65,7 @@ public class PrivateDataReminderSettings private int _extensionLength; private boolean _enablePublicationSearch; private int _publicationSearchFrequency; + private String _ncbiApiKey; public static PrivateDataReminderSettings get() { @@ -101,6 +106,8 @@ public static PrivateDataReminderSettings get() ? DEFAULT_PUBLICATION_SEARCH_FREQUENCY : Integer.valueOf(settingsMap.get(PROP_PUBLICATION_SEARCH_FREQUENCY)); settings.setPublicationSearchFrequency(publicationSearchFrequency); + + settings.setNcbiApiKey(getNcbiApiKeyValue()); } else { @@ -147,9 +154,43 @@ public static void save(PrivateDataReminderSettings settings) settingsMap.put(PROP_REMINDER_TIME, settings.getReminderTimeFormatted()); settingsMap.put(PROP_ENABLE_PUBLICATION_SEARCH, String.valueOf(settings.isEnablePublicationSearch())); settingsMap.put(PROP_PUBLICATION_SEARCH_FREQUENCY, String.valueOf(settings.getPublicationSearchFrequency())); + // The API key is a credential and is saved separately, in the encrypted store. Saving the + // rest of the settings must never change it. + settingsMap.remove(PROP_NCBI_API_KEY); settingsMap.save(); } + /** + * Save the NCBI API key, or remove it when the key is blank. The key lives in the encrypted + * store, like the other credentials this module holds. + */ + public static void saveNcbiApiKey(@Nullable String apiKey) + { + PropertyManager.WritablePropertyMap credentials = + PropertyManager.getEncryptedStore().getWritableProperties(PROP_NCBI_CREDENTIALS, true); + if (StringUtils.isBlank(apiKey)) + { + credentials.remove(PROP_NCBI_API_KEY); + } + else + { + credentials.put(PROP_NCBI_API_KEY, apiKey.trim()); + } + credentials.save(); + } + + public static boolean hasNcbiApiKey() + { + return !StringUtils.isBlank(getNcbiApiKeyValue()); + } + + private static @Nullable String getNcbiApiKeyValue() + { + Map credentials = + PropertyManager.getEncryptedStore().getProperties(PROP_NCBI_CREDENTIALS); + return credentials.get(PROP_NCBI_API_KEY); + } + public void setEnableReminders(boolean enableReminders) { _enableReminders = enableReminders; @@ -225,6 +266,16 @@ public void setPublicationSearchFrequency(int publicationSearchFrequency) _publicationSearchFrequency = publicationSearchFrequency; } + public @Nullable String getNcbiApiKey() + { + return _ncbiApiKey; + } + + public void setNcbiApiKey(@Nullable String ncbiApiKey) + { + _ncbiApiKey = ncbiApiKey; + } + public @Nullable Date getReminderValidUntilDate(@NotNull DatasetStatus status) { return status.getLastReminderDate() == null ? null : addMonths(status.getLastReminderDate(), getReminderFrequency()); @@ -358,92 +409,130 @@ public void testIsLastReminderRecentScenarios() private void testExtensionIsValid(PrivateDataReminderSettings settings, int monthsOffset) { - testExtensionIsValid(settings, monthsOffset, 0, null, true); + testExtensionIsValid(settings, monthsOffset, true); } private void testExtensionIsExpired(PrivateDataReminderSettings settings, int monthsOffset) { - testExtensionIsValid(settings, monthsOffset, 0, null, false); + testExtensionIsValid(settings, monthsOffset, false); } private void testExtensionIsValidAsOf(PrivateDataReminderSettings settings, int monthsOffset, int minutesOffset) { - testExtensionIsValid(settings, monthsOffset, minutesOffset, dateFromNow(), true); + testExtensionAtExpiry(settings, monthsOffset, minutesOffset, true); } - private void testExtensionIsExpiredAsOf(PrivateDataReminderSettings settings, int monthsOffset, int minutesOffset) + /** + * Checks the boundary at the moment an extension expires. The current date comes from the + * expiry the settings calculate, not from today. Subtracting months and adding them back does + * not always return to the same day, because a shorter target month clamps the day, so a date + * built from today can sit on the wrong side of the boundary. On 31 August, subtracting six + * months gives 28 February and adding six back gives 28 August. + * + * @param minutesBeforeExpiry how long before the expiry to check. Zero is the expiry itself, + * and a negative value is after it. + */ + private void testExtensionAtExpiry(PrivateDataReminderSettings settings, int monthsOffset, + int minutesBeforeExpiry, boolean expectedValid) { - testExtensionIsValid(settings, monthsOffset, minutesOffset, dateFromNow(), false); + DatasetStatus datasetStatus = new DatasetStatus(); + datasetStatus.setExtensionRequestedDate(dateFromNow(monthsOffset, 0, 0)); + + Date expiry = settings.getExtensionValidUntilDate(datasetStatus); + Date currentDate = Date.from(expiry.toInstant().minusSeconds(minutesBeforeExpiry * 60L)); + + String failureMessage = String.format( + "Extension is %s; Extension Length: %d; Extension Requested On: %s; Valid Until: %s; Current Date: %s", + expectedValid ? "valid" : "expired", settings.getExtensionLength(), + datasetStatus.getExtensionRequestedDate(), expiry, currentDate); + + boolean isValid = settings.isExtensionValidAsOf(datasetStatus, currentDate); + if (expectedValid) assertTrue(failureMessage, isValid); + else assertFalse(failureMessage, isValid); } - private void testExtensionIsValid(PrivateDataReminderSettings settings, int monthsOffset, int minutesOffset, - Date currentDate, boolean expectedValid) + private void testExtensionIsExpiredAsOf(PrivateDataReminderSettings settings, int monthsOffset, int minutesOffset) { - Date extensionDate = dateFromNow(monthsOffset, 0, minutesOffset); + testExtensionAtExpiry(settings, monthsOffset, minutesOffset, false); + } + private void testExtensionIsValid(PrivateDataReminderSettings settings, int monthsOffset, boolean expectedValid) + { DatasetStatus datasetStatus = new DatasetStatus(); - datasetStatus.setExtensionRequestedDate(extensionDate); + datasetStatus.setExtensionRequestedDate(dateFromNow(monthsOffset, 0, 0)); String failureMessage = String.format("Extension is %s; Extension Length: %d; Extension Requested On: %s; Valid Until: %s", expectedValid ? "valid" : "expired", settings.getExtensionLength(), datasetStatus.getExtensionRequestedDate(), settings.getExtensionValidUntilDate(datasetStatus)); - if (currentDate != null) - { - failureMessage += String.format("; Current Date: %s", currentDate); - } - - boolean isValid = currentDate == null - ? settings.isExtensionValid(datasetStatus) - : settings.isExtensionValidAsOf(datasetStatus, currentDate); + + boolean isValid = settings.isExtensionValid(datasetStatus); if (expectedValid) assertTrue(failureMessage, isValid); else assertFalse(failureMessage, isValid); } private void testReminderIsRecent(PrivateDataReminderSettings settings, int daysOffset) { - testReminderIsRecent(settings, 0, daysOffset, 0, null, true); + testReminderIsRecent(settings, daysOffset, true); } private void testReminderIsOld(PrivateDataReminderSettings settings, int daysOffset) { - testReminderIsRecent(settings, 0, daysOffset, 0, null, false); + testReminderIsRecent(settings, daysOffset, false); } private void testReminderIsRecentAsOf(PrivateDataReminderSettings settings, int monthsOffset, int minutesOffset) { - testReminderIsRecent(settings, monthsOffset, 0, minutesOffset, dateFromNow(), true); + testReminderAtExpiry(settings, monthsOffset, minutesOffset, true); } private void testReminderIsOldAsOf(PrivateDataReminderSettings settings, int monthsOffset, int minutesOffset) { - testReminderIsRecent(settings, monthsOffset, 0, minutesOffset, dateFromNow(), false); + testReminderAtExpiry(settings, monthsOffset, minutesOffset, false); } - private void testReminderIsRecent(PrivateDataReminderSettings settings, int monthsOffset, int daysOffset, int minutesOffset, - Date currentDate, boolean expectedRecent) + /** + * Checks the boundary at the moment a reminder stops counting as recent. The current date + * comes from the date the settings calculate, for the reason given on + * {@link #testExtensionAtExpiry}. + * + * @param minutesBeforeExpiry how long before that date to check. Zero is the date itself, and + * a negative value is after it. + */ + private void testReminderAtExpiry(PrivateDataReminderSettings settings, int monthsOffset, + int minutesBeforeExpiry, boolean expectedRecent) { - Date reminderDate = dateFromNow(monthsOffset, daysOffset, minutesOffset); + DatasetStatus datasetStatus = new DatasetStatus(); + datasetStatus.setLastReminderDate(dateFromNow(monthsOffset, 0, 0)); + + Date expiry = settings.getReminderValidUntilDate(datasetStatus); + Date currentDate = Date.from(expiry.toInstant().minusSeconds(minutesBeforeExpiry * 60L)); + + String failureMessage = String.format( + "Reminder is %s; Reminder Frequency: %d; Reminder Sent On: %s; Valid Until: %s; Current Date: %s", + expectedRecent ? "recent" : "old", settings.getReminderFrequency(), + datasetStatus.getLastReminderDate(), expiry, currentDate); + boolean isRecent = settings.isLastReminderRecentAsOf(datasetStatus, currentDate); + if (expectedRecent) assertTrue(failureMessage, isRecent); + else assertFalse(failureMessage, isRecent); + } + + private void testReminderIsRecent(PrivateDataReminderSettings settings, int daysOffset, boolean expectedRecent) + { DatasetStatus datasetStatus = new DatasetStatus(); - datasetStatus.setLastReminderDate(reminderDate); + datasetStatus.setLastReminderDate(dateFromNow(0, daysOffset, 0)); String failureMessage = String.format("Reminder is %s; Reminder Frequency: %d; Reminder Sent On: %s; Valid Until: %s", expectedRecent ? "recent" : "old", settings.getReminderFrequency(), datasetStatus.getLastReminderDate(), settings.getReminderValidUntilDate(datasetStatus)); - if (currentDate != null) - { - failureMessage += String.format("; Current Date: %s", currentDate); - } - - boolean isValid = currentDate == null - ? settings.isLastReminderRecent(datasetStatus) - : settings.isLastReminderRecentAsOf(datasetStatus, currentDate); - if (expectedRecent) assertTrue(failureMessage, isValid); - else assertFalse(failureMessage, isValid); + + boolean isRecent = settings.isLastReminderRecent(datasetStatus); + if (expectedRecent) assertTrue(failureMessage, isRecent); + else assertFalse(failureMessage, isRecent); } private PrivateDataReminderSettings createTestSettingsExtensionLength(int extensionLength) diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java index b32c5864..067dabed 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java @@ -15,12 +15,16 @@ */ package org.labkey.panoramapublic.ncbi; +import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.Nullable; import org.json.JSONArray; import org.json.JSONObject; import java.io.IOException; +import java.net.URLDecoder; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -28,8 +32,9 @@ /** * Mock implementation of {@link NcbiPublicationSearchService} that returns canned data registered by tests. * Used by Selenium tests when running on TeamCity. - * Extends {@link NcbiPublicationSearchServiceImpl} and only overrides {@link #getString(String)}, - * the single method that makes HTTP calls to NCBI. All search logic, filtering, author/title + * Extends {@link NcbiPublicationSearchServiceImpl} and only overrides {@link #getString(String, Logger)}, + * the method every NCBI request passes through. The override takes the place of the real HTTP request + * in {@code executeGet()} and the retry loop around it. All search logic, filtering, author/title * verification, citation parsing, and priority filtering run through the real implementation code. * Tests register mock articles via {@link #register}, providing the database, ID, search key, * metadata fields, and citation. The mock builds internal lookup maps from this data and returns @@ -50,7 +55,7 @@ public class MockNcbiPublicationSearchService extends NcbiPublicationSearchServi /** * Register a mock article. The mock stores the data in internal lookup maps used by - * {@link #getString(String)}. + * {@link #getString(String, Logger)}. * @param database "pmc" or "pubmed" — the NCBI database this article is in * @param id the article ID in the given database (numeric ID for pmc or pubmed) * @param searchKey what ESearch query term finds this article (e.g. PXD ID for PMC, author last name for PubMed) @@ -132,7 +137,7 @@ public void register(String database, String id, String searchKey, * Handles ESearch, ESummary, and Citation Exporter URLs. */ @Override - protected String getString(String url) throws IOException + protected String getString(String url, Logger log) throws IOException { if (url.contains("esearch.fcgi")) { @@ -151,15 +156,23 @@ else if (url.contains("lit/ctxp")) private JSONObject handleESearch(String url) { - boolean isPmc = url.contains("db=pmc"); + boolean isPmc = "pmc".equals(extractQueryParam(url, "db")); Map> searchMap = isPmc ? _pmcSearchResults : _pubmedSearchResults; + // Match registered search keys against the decoded ESearch query term, so a key cannot + // match part of another parameter such as tool or email. The real ESearch term wraps the + // key in quotes (e.g. "PXD056793"), so contains() on the term is the right granularity. + String term = extractQueryParam(url, "term"); + JSONArray idList = new JSONArray(); - for (Map.Entry> entry : searchMap.entrySet()) + if (term != null) { - if (url.contains(entry.getKey())) + for (Map.Entry> entry : searchMap.entrySet()) { - entry.getValue().forEach(idList::put); + if (term.contains(entry.getKey())) + { + entry.getValue().forEach(idList::put); + } } } @@ -170,13 +183,18 @@ private JSONObject handleESearch(String url) private JSONObject handleESummary(String url) { - boolean isPmc = url.contains("db=pmc"); + boolean isPmc = "pmc".equals(extractQueryParam(url, "db")); Map metadataMap = isPmc ? _pmcMetadata : _pubmedMetadata; + // ESummary requests a comma-separated list of IDs in the "id" parameter. Match registered + // IDs against that list (exactly, not by substring), rather than scanning the whole URL. + String idParam = extractQueryParam(url, "id"); + List requestedIds = idParam == null ? List.of() : Arrays.asList(idParam.split(",")); + JSONObject result = new JSONObject(); for (Map.Entry entry : metadataMap.entrySet()) { - if (url.contains(entry.getKey())) + if (requestedIds.contains(entry.getKey())) { result.put(entry.getKey(), entry.getValue()); } @@ -192,20 +210,7 @@ private JSONObject handleESummary(String url) */ private JSONObject handleCitation(String url) { - // Extract the publication ID from the URL (last segment after "id=") - String id = null; - int idIdx = url.indexOf("id="); - if (idIdx >= 0) - { - id = url.substring(idIdx + 3); - // Remove any trailing query parameters - int ampIdx = id.indexOf('&'); - if (ampIdx >= 0) - { - id = id.substring(0, ampIdx); - } - } - + String id = extractQueryParam(url, "id"); String citation = id != null ? _citations.get(id) : null; if (citation != null) { @@ -213,4 +218,22 @@ private JSONObject handleCitation(String url) } return new JSONObject(); } + + /** + * Returns the URL-decoded value of the given query parameter, or null if it is not present. + */ + private static @Nullable String extractQueryParam(String url, String name) + { + int queryStart = url.indexOf('?'); + String query = queryStart >= 0 ? url.substring(queryStart + 1) : url; + for (String pair : query.split("&")) + { + int eq = pair.indexOf('='); + if (eq > 0 && pair.substring(0, eq).equals(name)) + { + return URLDecoder.decode(pair.substring(eq + 1), StandardCharsets.UTF_8); + } + } + return null; + } } diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiApiKeyCheck.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiApiKeyCheck.java new file mode 100644 index 00000000..b7bcf95e --- /dev/null +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiApiKeyCheck.java @@ -0,0 +1,74 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.labkey.panoramapublic.ncbi; + +import org.jetbrains.annotations.Nullable; + +/** + * The outcome of checking an NCBI API key. A rejected key is a configuration problem an admin has to + * correct. A check that could not be completed is not, so callers need to tell the two apart. + */ +public class NcbiApiKeyCheck +{ + public enum Status { VALID, REJECTED, UNCONFIRMED } + + private final Status _status; + private final String _message; + + private NcbiApiKeyCheck(Status status, @Nullable String message) + { + _status = status; + _message = message; + } + + public static NcbiApiKeyCheck valid() + { + return new NcbiApiKeyCheck(Status.VALID, null); + } + + public static NcbiApiKeyCheck rejected(@Nullable String message) + { + return new NcbiApiKeyCheck(Status.REJECTED, message); + } + + public static NcbiApiKeyCheck unconfirmed(@Nullable String message) + { + return new NcbiApiKeyCheck(Status.UNCONFIRMED, message); + } + + public Status getStatus() + { + return _status; + } + + /** + * @return NCBI's reason, with the API key removed. Null when the key was accepted. + */ + public @Nullable String getMessage() + { + return _message; + } + + public boolean isValid() + { + return _status == Status.VALID; + } + + public boolean isRejected() + { + return _status == Status.REJECTED; + } +} diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchService.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchService.java index 0e3de30d..b18bd913 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchService.java +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchService.java @@ -38,13 +38,26 @@ static NcbiPublicationSearchService get() @Nullable String getCitation(String publicationId, DB database); + /** + * Send a minimal request to NCBI with the given key. + */ + @NotNull NcbiApiKeyCheck checkApiKey(@Nullable String apiKey); + @Nullable Pair getPubMedLinkAndCitation(String pubmedId); /** * Searches PMC and PubMed for a publication associated with the experiment. * Returns the top match (highest priority) if multiple matches are found, or null if none. */ + /** + * @throws NcbiSearchException if a request to NCBI fails. An empty result therefore means no + * publication was found, not that the search could not be run. + */ @Nullable PublicationMatch searchForPublication(@NotNull ExperimentAnnotations expAnnotations, @Nullable Logger logger); + /** + * @throws NcbiSearchException if a request to NCBI fails. An empty result therefore means no + * publication was found, not that the search could not be run. + */ List searchForPublication(@NotNull ExperimentAnnotations expAnnotations, int maxResults, @Nullable Logger logger, boolean getCitations); } diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java index e6b0bae3..fc0a14d3 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java @@ -24,6 +24,7 @@ import org.apache.hc.client5.http.impl.classic.HttpClientBuilder; import org.apache.hc.client5.http.impl.io.BasicHttpClientConnectionManager; import org.apache.hc.client5.http.HttpResponseException; +import org.apache.hc.core5.http.ClassicHttpResponse; import org.apache.hc.core5.http.io.entity.EntityUtils; import org.apache.hc.core5.util.Timeout; import org.apache.logging.log4j.Logger; @@ -38,10 +39,12 @@ import org.labkey.api.util.StringUtilsLabKey; import org.labkey.api.util.logging.LogHelper; import org.labkey.panoramapublic.datacite.DataCiteService; +import org.labkey.panoramapublic.message.PrivateDataReminderSettings; import org.labkey.panoramapublic.model.ExperimentAnnotations; import org.labkey.panoramapublic.ncbi.NcbiConstants.DB; import java.io.IOException; +import java.net.SocketTimeoutException; import java.net.URLEncoder; import java.nio.charset.StandardCharsets; import java.text.Normalizer; @@ -57,6 +60,8 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import java.util.stream.Collectors; import static org.labkey.panoramapublic.ncbi.PublicationMatch.MATCH_DOI; @@ -92,9 +97,20 @@ public static void setInstance(NcbiPublicationSearchService impl) private static final String PMC_CITATION_EXPORTER_URL = "https://api.ncbi.nlm.nih.gov/lit/ctxp/v1/pmc/?format=citation&id="; // API parameters - private static final int RATE_LIMIT_DELAY_MS = 400; // NCBI allows 3 requests/sec + private static final int RATE_LIMIT_DELAY_MS = 400; // NCBI allows 3 requests/sec without an API key private static final int TIMEOUT_MS = 10000; // 10 seconds + // NCBI eutils intermittently returns transient 5xx errors and read timeouts, even well under + // the rate limit. Retry those a few times with exponential backoff before giving up. + private static final int MAX_HTTP_ATTEMPTS = 3; // initial try + 2 retries + private static final int RETRY_BASE_DELAY_MS = 500; // exponential backoff base + + private static final int TOO_MANY_REQUESTS = 429; // NCBI's answer when the request rate is exceeded + private static final int MAX_ERROR_BODY_CHARS = 500; + + private static final String REDACTED = "REDACTED"; + private static final Pattern API_KEY_PARAM = Pattern.compile("api_key=([^&\\s]*)"); + // NCBI suggests using the 'tool' and 'email' parameters on E-utilities URLs // https://www.nlm.nih.gov/dataguide/eutilities/utilities.html private static final String TOOL = "PanoramaPublic"; @@ -156,7 +172,7 @@ private static Logger getLog(@Nullable Logger logger) try { - String response = getString(queryUrl); + String response = getString(queryUrl, log); return parseCitation(response, publicationId, database, log); } catch (IOException e) @@ -326,7 +342,7 @@ private List executeSearch(String query, String database, Logger log) try { - JSONObject json = getJson(url); + JSONObject json = getJson(url, log); JSONObject eSearchResult = json.getJSONObject("esearchresult"); JSONArray idList = eSearchResult.getJSONArray("idlist"); @@ -340,7 +356,33 @@ private List executeSearch(String query, String database, Logger log) catch (IOException | JSONException e) { log.error("Error searching {} with query: {}", database, query, e); - return Collections.emptyList(); + throw new NcbiSearchException("Error searching " + database + " with query: " + query, e); + } + } + + @Override + public @NotNull NcbiApiKeyCheck checkApiKey(@Nullable String apiKey) + { + // A minimal ESearch request. A rejected key comes back as a 400 on the first attempt, while + // a 5xx or 429 is retried like any other request before it is called unconfirmed. + String url = ESEARCH_URL + "?" + buildCommonParams("pubmed", apiKey) + "&term=labkey&retmax=1&retmode=json"; + try + { + getString(url, LOG); + return NcbiApiKeyCheck.valid(); + } + catch (HttpResponseException e) + { + // NCBI rejects a key it does not recognise with a 4xx. A 429 is the request rate and a + // 5xx is NCBI's own failure, so neither counts as a rejection. + String message = redactApiKey(e.getMessage(), apiKey); + boolean rejected = e.getStatusCode() >= 400 && e.getStatusCode() < 500 + && e.getStatusCode() != TOO_MANY_REQUESTS; + return rejected ? NcbiApiKeyCheck.rejected(message) : NcbiApiKeyCheck.unconfirmed(message); + } + catch (IOException e) + { + return NcbiApiKeyCheck.unconfirmed(redactApiKey(e.getMessage(), apiKey)); } } @@ -349,16 +391,46 @@ private List executeSearch(String query, String database, Logger log) * @throws IOException if the request fails or the server returns a non-2xx response * @throws JSONException if the response body is not valid JSON */ - protected JSONObject getJson(String url) throws IOException + protected JSONObject getJson(String url, Logger log) throws IOException { - return new JSONObject(getString(url)); + return new JSONObject(getString(url, log)); } /** - * Execute an HTTP GET request and return the response body as a string. + * Execute an HTTP GET request and return the response body as a string. Retry warnings are + * written to {@code log}. The reminder job passes its pipeline job logger, and UI-triggered + * searches pass the static server logger. * @throws IOException if the request fails or the server returns a non-2xx response */ - protected String getString(String url) throws IOException + protected String getString(String url, Logger log) throws IOException + { + for (int attempt = 1; ; attempt++) + { + try + { + return executeGet(url); + } + catch (IOException e) + { + if (attempt >= MAX_HTTP_ATTEMPTS || !isRetryable(e)) + { + throw e; + } + long delayMs = retryDelayMs(attempt); + getLog(log).warn("NCBI request failed (attempt {} of {}). Retrying in {} ms. URL: {}. Cause: {}", + attempt, MAX_HTTP_ATTEMPTS, delayMs, redactApiKey(url, apiKeyFrom(url)), e.toString()); + if (!sleepMs(delayMs)) + { + // The thread was interrupted. Retrying now would run the remaining attempts with + // no delay, because every later sleep throws at once. + throw e; + } + } + } + } + + // Overridden in unit tests to drive the retry loop in getString() without real HTTP calls. + protected String executeGet(String url) throws IOException { ConnectionConfig connectionConfig = ConnectionConfig.custom() .setConnectTimeout(Timeout.ofMilliseconds(TIMEOUT_MS)) @@ -375,6 +447,9 @@ protected String getString(String url) throws IOException try (CloseableHttpClient client = HttpClientBuilder.create() .setDefaultRequestConfig(requestConfig) .setConnectionManager(connectionManager) + // HttpClient retries 429 and 503 once on its own, which would make MAX_HTTP_ATTEMPTS + // cost twice the requests it names. getString is the only retry. + .disableAutomaticRetries() .build()) { HttpGet getRequest = new HttpGet(url); @@ -382,13 +457,125 @@ protected String getString(String url) throws IOException int status = response.getCode(); if (status < 200 || status >= 300) { - throw new HttpResponseException(status, response.getReasonPhrase()); + throw new HttpResponseException(status, + errorDetail(status, response.getReasonPhrase(), readErrorBody(status, response), + apiKeyFrom(url))); } return EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8); }); } } + /** + * Read the body of a client error response. 5xx bodies are large, uninformative HTML error + * pages, so only client error bodies are read, and only the first {@link #MAX_ERROR_BODY_CHARS} + * characters. A body that cannot be read or parsed returns null, because losing NCBI's text is + * better than losing the status code the caller decides on. + */ + private static @Nullable String readErrorBody(int status, ClassicHttpResponse response) + { + if (status < 400 || status >= 500 || response.getEntity() == null) + { + return null; + } + + try + { + return EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8, MAX_ERROR_BODY_CHARS); + } + catch (IOException | org.apache.hc.core5.http.ParseException e) + { + return null; + } + } + + /** + * Build the message for a non-2xx HttpResponseException. For client errors (4xx) the response + * body is appended, so NCBI's reason (e.g. "API key invalid") reaches the log. Other statuses + * use only the reason phrase. The body is third-party text on its way to a log, so any + * occurrence of the API key is removed from it. + */ + static String errorDetail(int status, String reasonPhrase, @Nullable String body, @Nullable String apiKey) + { + if (status >= 400 && status < 500 && !StringUtils.isBlank(body)) + { + return reasonPhrase + " - " + redactApiKey(StringUtils.abbreviate(body.strip(), 500), apiKey); + } + return reasonPhrase; + } + + /** + * Replace the NCBI API key wherever it appears in text that is about to be logged. The key is a + * query parameter on every eutils URL. When the reminder job runs, the log is the pipeline job + * log, which is readable by anyone with read access to the folder the job ran in. + */ + static String redactApiKey(@Nullable String text, @Nullable String apiKey) + { + if (text == null) + { + return null; + } + String redacted = text.replaceAll("(api_key=)[^&\\s]*", "$1" + REDACTED); + if (!StringUtils.isBlank(apiKey)) + { + redacted = redacted.replace(apiKey.trim(), REDACTED); + } + return redacted; + } + + /** + * Returns the value of the api_key query parameter in the given URL, or null if there is none. + */ + static @Nullable String apiKeyFrom(String url) + { + Matcher matcher = API_KEY_PARAM.matcher(url); + return matcher.find() ? matcher.group(1) : null; + } + + /** + * Read timeouts, 5xx responses and 429 are transient NCBI failures worth retrying. NCBI answers + * 429 when the request rate is exceeded, which is the failure backoff exists for. Other 4xx + * errors are permanent. + */ + private static boolean isRetryable(IOException e) + { + if (e instanceof SocketTimeoutException) + { + return true; + } + if (e instanceof HttpResponseException hre) + { + return hre.getStatusCode() >= 500 || hre.getStatusCode() == TOO_MANY_REQUESTS; + } + return false; + } + + // 500ms after the first failure, then doubling. Jitter is not needed. Both callers, the daily + // reminder job and the UI search, issue NCBI requests sequentially, so a retry never collides + // with a sibling request. + private static long retryDelayMs(int attempt) + { + return (long) RETRY_BASE_DELAY_MS << (attempt - 1); + } + + /** + * @return false if the thread was interrupted, in which case the caller should stop rather than + * carry on without the delay it asked for. + */ + protected boolean sleepMs(long ms) + { + try + { + Thread.sleep(ms); + return true; + } + catch (InterruptedException e) + { + Thread.currentThread().interrupt(); + return false; + } + } + /** * Fetch metadata for PMC articles using ESummary API */ @@ -420,7 +607,7 @@ private Map fetchMetadata(Collection ids, String dat try { - JSONObject json = getJson(url); + JSONObject json = getJson(url, log); JSONObject result = json.optJSONObject("result"); if (result == null) return Collections.emptyMap(); @@ -439,7 +626,7 @@ private Map fetchMetadata(Collection ids, String dat catch (IOException | JSONException e) { log.error("Error fetching {} metadata for IDs: {}", database, ids, e); - return Collections.emptyMap(); + throw new NcbiSearchException("Error fetching " + database + " metadata for IDs: " + ids, e); } } @@ -919,16 +1106,9 @@ static List extractTitleKeywords(String title) /** * Rate limiting: wait 400ms between API requests */ - private static void rateLimit() + private void rateLimit() { - try - { - Thread.sleep(RATE_LIMIT_DELAY_MS); - } - catch (InterruptedException e) - { - Thread.currentThread().interrupt(); - } + sleepMs(RATE_LIMIT_DELAY_MS); } /** @@ -943,13 +1123,28 @@ static String stripQuerySpecialChars(String value) } /** - * Build the common NCBI API parameters (db, tool, email) with URL encoding. + * Build the common NCBI API parameters (db, tool, email, and api_key when one is configured) + * with URL encoding. */ private static String buildCommonParams(String database) { - return "db=" + URLEncoder.encode(database, StandardCharsets.UTF_8) + + return buildCommonParams(database, PrivateDataReminderSettings.get().getNcbiApiKey()); + } + + // The API key is passed in rather than read from the settings, so this overload runs without a server. + private static String buildCommonParams(String database, @Nullable String apiKey) + { + String params = "db=" + URLEncoder.encode(database, StandardCharsets.UTF_8) + "&tool=" + URLEncoder.encode(TOOL, StandardCharsets.UTF_8) + "&email=" + URLEncoder.encode(EMAIL, StandardCharsets.UTF_8); + + // An NCBI API key (configured in the Private Data Reminder Settings) raises the eutils + // rate limit from 3 to 10 requests/sec. + if (!StringUtils.isBlank(apiKey)) + { + params += "&api_key=" + URLEncoder.encode(apiKey.trim(), StandardCharsets.UTF_8); + } + return params; } /** @@ -1422,6 +1617,272 @@ public void testPublicationMatchRoundTrip() assertFalse(restored.matchesProteomeXchangeId()); } + // -- HTTP retry tests -- + + @Test + public void testIsRetryable() + { + // Read timeouts and 5xx responses are transient NCBI failures -> retry + assertTrue(isRetryable(new SocketTimeoutException("Read timed out"))); + assertTrue(isRetryable(new HttpResponseException(500, "Internal Server Error"))); + assertTrue(isRetryable(new HttpResponseException(503, "Service Unavailable"))); + + // 429 is NCBI's answer when the request rate is exceeded, which backoff is for + assertTrue(isRetryable(new HttpResponseException(429, "Too Many Requests"))); + + // Other 4xx and generic IO errors are permanent -> fail fast + assertFalse(isRetryable(new HttpResponseException(400, "Bad Request"))); + assertFalse(isRetryable(new HttpResponseException(404, "Not Found"))); + assertFalse(isRetryable(new IOException("connection reset"))); + } + + @Test + public void testRetryDelayMs() + { + // Exponential backoff of 500ms then 1000ms. With MAX_HTTP_ATTEMPTS at 3 the loop sleeps + // after the first two failures and rethrows after the third, so those are the only + // delays a request can wait. + assertEquals(500, retryDelayMs(1)); + assertEquals(1000, retryDelayMs(2)); + assertEquals("A request sleeps once per failed attempt except the last, so only the" + + " delays asserted above are reachable", 2, MAX_HTTP_ATTEMPTS - 1); + } + + @Test + public void testErrorDetail() + { + // 4xx: the response body is appended so the cause (e.g. an invalid API key) is logged + String detail = errorDetail(400, "Bad Request", "{\"error\":\"API key invalid\"}", null); + assertTrue(detail.contains("Bad Request")); + assertTrue(detail.contains("API key invalid")); + + // 5xx: body omitted (uninformative) + assertEquals("Internal Server Error", errorDetail(500, "Internal Server Error", "oops", null)); + + // 4xx with blank or null body: just the reason phrase, no trailing separator + assertEquals("Bad Request", errorDetail(400, "Bad Request", "", null)); + assertEquals("Bad Request", errorDetail(400, "Bad Request", null, null)); + + // A body that quotes the request back must not carry the key into the log + String echoed = errorDetail(400, "Bad Request", "invalid key SECRET123 for api_key=SECRET123", "SECRET123"); + assertFalse("errorDetail must not put the API key in the message", echoed.contains("SECRET123")); + } + + @Test + public void testRedactApiKey() + { + // The key is stripped from an eutils URL, and the rest of the URL is left intact + String url = "https://eutils.ncbi.nlm.nih.gov/esearch.fcgi?db=pmc&api_key=SECRET123&term=PXD001"; + String redacted = redactApiKey(url, "SECRET123"); + assertFalse("The redacted URL must not contain the key", redacted.contains("SECRET123")); + assertTrue("The redacted URL must keep its other parameters", redacted.contains("term=PXD001")); + assertTrue(redacted.contains("db=pmc")); + + // The key is stripped even when it appears without the api_key= prefix + assertFalse(redactApiKey("rejected key SECRET123", "SECRET123").contains("SECRET123")); + + // A URL with no key is unchanged, and null text stays null + String noKey = "https://eutils.ncbi.nlm.nih.gov/esearch.fcgi?db=pmc&term=PXD001"; + assertEquals(noKey, redactApiKey(noKey, null)); + assertNull(redactApiKey(null, "SECRET123")); + + // The key is recovered from the URL so callers do not have to read the settings + assertEquals("SECRET123", apiKeyFrom(url)); + assertNull(apiKeyFrom(noKey)); + } + + @Test + public void testBuildCommonParams() + { + // Always includes db, tool, email + String params = buildCommonParams("pmc", null); + assertTrue(params.contains("db=pmc")); + assertTrue(params.contains("tool=" + TOOL)); + assertTrue(params.contains("email=")); + + // No api_key when the key is null, empty, or blank + assertFalse("api_key should be absent when no key is set", params.contains("api_key")); + assertFalse(buildCommonParams("pmc", "").contains("api_key")); + assertFalse(buildCommonParams("pmc", " ").contains("api_key")); + + // api_key appended (and trimmed) when a key is set + assertTrue(buildCommonParams("pubmed", "ABC123").contains("api_key=ABC123")); + assertTrue(buildCommonParams("pmc", " ABC123 ").contains("api_key=ABC123")); + } + + @Test + public void testGetStringRetriesTransientFailures() throws IOException + { + // executeGet returns a 5xx twice, then succeeds. getString should retry and return the body. + int[] attempts = {0}; + NoWaitService service = new NoWaitService() + { + @Override + protected String executeGet(String url) throws IOException + { + if (++attempts[0] < 3) + throw new HttpResponseException(503, "Service Unavailable"); + return "body"; + } + }; + assertEquals("body", service.getString("http://test", LOG)); + assertEquals("Should retry until the 3rd attempt succeeds", 3, attempts[0]); + assertEquals("The loop should wait 500ms then 1000ms", List.of(500L, 1000L), service.sleeps); + } + + /** + * Runs the retry loop without waiting, and records the delays it asked for. A test can then + * check the delays the loop used, which retryDelayMs on its own cannot show. + */ + private static class NoWaitService extends NcbiPublicationSearchServiceImpl + { + private final List sleeps = new ArrayList<>(); + + @Override + protected boolean sleepMs(long ms) + { + sleeps.add(ms); + return true; + } + } + + @Test + public void testCheckApiKey() + { + // NCBI rejects a key it does not recognise with a 4xx. The reminder job stops for that, + // so a 5xx has to be reported as a check that could not be completed. + assertEquals(NcbiApiKeyCheck.Status.REJECTED, checkApiKeyAgainst(new HttpResponseException(400, "Bad Request")).getStatus()); + assertEquals(NcbiApiKeyCheck.Status.UNCONFIRMED, checkApiKeyAgainst(new HttpResponseException(503, "Service Unavailable")).getStatus()); + + // A 429 is the request rate, not a bad key. Calling it a rejection would stop the + // reminder job and send an admin to correct a key that works. + assertEquals(NcbiApiKeyCheck.Status.UNCONFIRMED, checkApiKeyAgainst(new HttpResponseException(429, "Too Many Requests")).getStatus()); + assertEquals(NcbiApiKeyCheck.Status.UNCONFIRMED, checkApiKeyAgainst(new SocketTimeoutException("Read timed out")).getStatus()); + assertEquals(NcbiApiKeyCheck.Status.VALID, checkApiKeyAgainst(null).getStatus()); + + // The key must not travel back to the caller in NCBI's reason + NcbiApiKeyCheck rejected = checkApiKeyAgainst( + new HttpResponseException(400, "Bad Request - invalid key SECRET123"), "SECRET123"); + assertFalse("A rejection must not carry the key", rejected.getMessage().contains("SECRET123")); + } + + private NcbiApiKeyCheck checkApiKeyAgainst(IOException failure) + { + return checkApiKeyAgainst(failure, "test-key"); + } + + private NcbiApiKeyCheck checkApiKeyAgainst(IOException failure, String apiKey) + { + NcbiPublicationSearchServiceImpl service = new NoWaitService() + { + @Override + protected String executeGet(String url) throws IOException + { + if (failure != null) + { + throw failure; + } + return "{\"esearchresult\":{\"idlist\":[]}}"; + } + }; + return service.checkApiKey(apiKey); + } + + @Test + public void testGetStringStopsWhenInterrupted() + { + // An interrupted thread cannot wait, so retrying would send the remaining attempts back + // to back. getString should give up after the first failure instead. + int[] attempts = {0}; + NcbiPublicationSearchServiceImpl service = new NcbiPublicationSearchServiceImpl() + { + @Override + protected String executeGet(String url) throws IOException + { + attempts[0]++; + Thread.currentThread().interrupt(); + throw new HttpResponseException(503, "Service Unavailable"); + } + }; + + try + { + service.getString("http://test", LOG); + fail("Expected the interrupted request to be rethrown"); + } + catch (IOException expected) + { + assertEquals("An interrupted request should not be retried", 1, attempts[0]); + } + finally + { + // Clear the flag so it cannot reach whatever test runs next. + Thread.interrupted(); + } + } + + @Test + public void testGetStringGivesUpAfterMaxAttempts() + { + // executeGet always returns a 5xx. getString should try MAX_HTTP_ATTEMPTS times, then rethrow. + int[] attempts = {0}; + NoWaitService service = new NoWaitService() + { + @Override + protected String executeGet(String url) throws IOException + { + attempts[0]++; + throw new HttpResponseException(500, "Internal Server Error"); + } + }; + try + { + service.getString("http://test", LOG); + fail("Expected HttpResponseException after exhausting retries"); + } + catch (HttpResponseException e) + { + assertEquals(500, e.getStatusCode()); + } + catch (IOException e) + { + fail("Expected HttpResponseException, got " + e); + } + assertEquals("Should attempt exactly MAX_HTTP_ATTEMPTS times", MAX_HTTP_ATTEMPTS, attempts[0]); + assertEquals("One wait fewer than attempts, and no wait after the last", List.of(500L, 1000L), service.sleeps); + } + + @Test + public void testGetStringDoesNotRetryClientErrors() + { + // A 4xx is permanent. getString should fail immediately without retrying. + int[] attempts = {0}; + NoWaitService service = new NoWaitService() + { + @Override + protected String executeGet(String url) throws IOException + { + attempts[0]++; + throw new HttpResponseException(400, "Bad Request"); + } + }; + try + { + service.getString("http://test", LOG); + fail("Expected HttpResponseException for a 4xx"); + } + catch (HttpResponseException e) + { + assertEquals(400, e.getStatusCode()); + } + catch (IOException e) + { + fail("Expected HttpResponseException, got " + e); + } + assertEquals("4xx must not be retried", 1, attempts[0]); + assertTrue("A 4xx must not wait", service.sleeps.isEmpty()); + } + // -- Helper methods for building test JSON -- private static JSONObject articleMetadata(String source, String fullJournalName) diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiSearchException.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiSearchException.java new file mode 100644 index 00000000..0e4a8806 --- /dev/null +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiSearchException.java @@ -0,0 +1,34 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.labkey.panoramapublic.ncbi; + +/** + * Thrown when a request to NCBI fails, so that callers can tell a failed search from a search that + * found nothing. Without it a rejected API key and a dataset with no published paper both arrive as + * an empty result. + */ +public class NcbiSearchException extends RuntimeException +{ + public NcbiSearchException(String message, Throwable cause) + { + super(message, cause); + } + + public NcbiSearchException(String message) + { + super(message); + } +} diff --git a/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java b/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java index 4b9d8ed6..eaf3971f 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java +++ b/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java @@ -40,7 +40,9 @@ import org.labkey.panoramapublic.model.ExperimentAnnotations; import org.labkey.panoramapublic.model.Journal; import org.labkey.panoramapublic.model.JournalSubmission; +import org.labkey.panoramapublic.ncbi.NcbiApiKeyCheck; import org.labkey.panoramapublic.ncbi.NcbiPublicationSearchService; +import org.labkey.panoramapublic.ncbi.NcbiSearchException; import org.labkey.panoramapublic.ncbi.PublicationMatch; import org.labkey.panoramapublic.query.DatasetStatusManager; import org.labkey.panoramapublic.query.ExperimentAnnotationsManager; @@ -251,6 +253,12 @@ private PublicationMatch searchForPublication(@NotNull ExperimentAnnotations exp return null; } } + catch (NcbiSearchException e) + { + // A search that could not run must not reset the deferral, which would record it + // as having found no new publication. + throw e; + } catch (Exception e) { log.error("Error re-searching publication for experiment {}: {}", expAnnotations.getId(), e.getMessage(), e); @@ -272,6 +280,12 @@ private PublicationMatch searchForPublication(@NotNull ExperimentAnnotations exp { return NcbiPublicationSearchService.get().searchForPublication(expAnnotations, log); } + catch (NcbiSearchException e) + { + // The caller records this as a failed search. Returning null here would make it + // indistinguishable from a dataset with no published paper. + throw e; + } catch (Exception e) { log.error("Error searching for publication for experiment {}: {}", expAnnotations.getId(), e.getMessage(), e); @@ -290,11 +304,52 @@ public void run() return; } + if (!ncbiApiKeyAccepted()) + { + setStatus(TaskStatus.error); + return; + } + postMessage(_experimentAnnotationsIds, _panoramaPublic); setStatus(TaskStatus.complete); } + /** + * Check a configured NCBI API key before any dataset is touched. A key NCBI rejects would fail the + * publication search for every dataset, so the job stops with nothing posted. The job goes ahead + * when the check could not be completed, since that says nothing about the key. + */ + private boolean ncbiApiKeyAccepted() + { + PrivateDataReminderSettings settings = PrivateDataReminderSettings.get(); + if (!settings.isEnablePublicationSearch() && !_forcePublicationCheck) + { + return true; + } + + String apiKey = settings.getNcbiApiKey(); + if (StringUtils.isBlank(apiKey)) + { + // Searches run without a key, at NCBI's lower request rate. + return true; + } + + NcbiApiKeyCheck check = NcbiPublicationSearchService.get().checkApiKey(apiKey); + if (check.isRejected()) + { + getLogger().error("NCBI rejected the API key, so no reminders were posted. Correct the key on the Private Data Reminder Settings page and run the job again. {}", + check.getMessage()); + return false; + } + + if (!check.isValid()) + { + getLogger().warn("Could not reach NCBI to check the API key. Continuing. {}", check.getMessage()); + } + return true; + } + private void postMessage(List expAnnotationIds, Journal panoramaPublic) { int total = expAnnotationIds.size(); @@ -328,6 +383,14 @@ private void processExperiments(List expAnnotationIds, ProcessingContex } for (Integer experimentAnnotationsId : exptIds) { + if (Thread.currentThread().isInterrupted()) + { + // Cancelling the job clears the NCBI rate limiter, because every sleep from here on + // throws at once. Stop instead of running the rest at full speed. + log.warn("Job was interrupted. Stopping before experiment {}.", experimentAnnotationsId); + break; + } + try (DbScope.Transaction transaction = PanoramaPublicManager.getSchema().getScope().ensureTransaction()) { processExperiment(experimentAnnotationsId, context, processingResults); @@ -384,8 +447,17 @@ private void processExperiment(Integer experimentAnnotationsId, ProcessingContex return; } - // Check for publications if enabled - PublicationMatch publicationResult = searchForPublication(expAnnotations, context.getSettings(), _forcePublicationCheck, getUser(), context.isTestMode(), processingResults._log); + // Check for publications if enabled. A search that could not run still gets a reminder, since + // the reminder is about the data being private, not about the paper. + PublicationMatch publicationResult = null; + try + { + publicationResult = searchForPublication(expAnnotations, context.getSettings(), _forcePublicationCheck, getUser(), context.isTestMode(), processingResults._log); + } + catch (NcbiSearchException e) + { + processingResults.addPublicationSearchFailed(experimentAnnotationsId, e); + } if (!context.isTestMode()) { @@ -662,6 +734,7 @@ private static class ProcessingResults private final List _submissionNotFound = new ArrayList<>(); private final List _announcementNotFound = new ArrayList<>(); private final List _submitterNotFound = new ArrayList<>(); + private final List _publicationSearchFailed = new ArrayList<>(); private final List _skipped = new ArrayList<>(); private int _processed = 0; private final int _total; @@ -702,6 +775,12 @@ public void addSubmitterNotFound(Integer experimentId) _log.error("Could not find a submitter user for experiment Id: {}.", experimentId); } + public void addPublicationSearchFailed(Integer experimentId, Exception e) + { + _publicationSearchFailed.add(experimentId); + _log.error("Publication search failed for experiment Id: {}. A reminder was still posted. {}", experimentId, e.getMessage(), e); + } + public void addSkipped(Integer experimentId, ReminderDecision decision) { _skipped.add(experimentId); @@ -739,6 +818,11 @@ public void logSkipped(Logger log) log.error("Support message threads were not found for the following experiment Ids: {}", StringUtils.join(_announcementNotFound, ", ")); } + if (!_publicationSearchFailed.isEmpty()) + { + log.error("Publication search failed for the following experiment Ids: {}. NCBI's reason is in the error logged for each one.", StringUtils.join(_publicationSearchFailed, ", ")); + } + if (!_submitterNotFound.isEmpty()) { log.error("Submitter user was not found for the following experiment Ids: {}", StringUtils.join(_submitterNotFound, ", ")); diff --git a/panoramapublic/src/org/labkey/panoramapublic/view/privateDataRemindersSettingsForm.jsp b/panoramapublic/src/org/labkey/panoramapublic/view/privateDataRemindersSettingsForm.jsp index cf5a738c..4a161bfa 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/view/privateDataRemindersSettingsForm.jsp +++ b/panoramapublic/src/org/labkey/panoramapublic/view/privateDataRemindersSettingsForm.jsp @@ -81,6 +81,58 @@ } window.location = LABKEY.ActionURL.buildURL("panoramapublic", "searchPublications.view", folderPath); } + + function showValidationResult(result, message, detail) + { + while (result.firstChild) + { + result.removeChild(result.firstChild); + } + result.appendChild(document.createTextNode(message)); + + if (!detail) + { + return; + } + + result.appendChild(document.createTextNode(" ")); + const link = document.createElement("a"); + link.href = "#"; + link.textContent = "Details"; + // The handler is attached here rather than with an onclick attribute, which the Content + // Security Policy blocks. NCBI's reply is encoded because it is third party text. + link.addEventListener("click", function (e) + { + e.preventDefault(); + Ext4.Msg.alert("NCBI response", Ext4.String.htmlEncode(detail)); + }); + result.appendChild(link); + } + + function validateNcbiApiKey() + { + const input = document.getElementsByName("ncbiApiKey")[0]; + const result = document.getElementById("ncbiApiKeyValidationResult"); + result.style.color = ""; + result.textContent = "Checking with NCBI..."; + + LABKEY.Ajax.request({ + url: LABKEY.ActionURL.buildURL("panoramapublic", "validateNcbiApiKey.api"), + method: "POST", + // An empty value asks the server to check the saved key, which this form never displays. + jsonData: {ncbiApiKey: input ? input.value : ""}, + success: LABKEY.Utils.getCallbackWrapper(function (response) + { + result.style.color = response.valid ? "green" : "red"; + showValidationResult(result, response.message, response.detail); + }), + failure: LABKEY.Utils.getCallbackWrapper(function () + { + result.style.color = "red"; + showValidationResult(result, "Could not reach the server to validate the key.", null); + }) + }); + } @@ -170,6 +222,25 @@ + + + <%=h(PrivateDataReminderSettings.PROP_NCBI_API_KEY)%> + + + " /> + <%=button("Validate").onClick("validateNcbiApiKey(); return false;")%> + +
+ Optional. An NCBI API key raises the request rate limit for PubMed/PMC searches from 3 to 10 per second. +
+ Create one under Account settings at ncbi.nlm.nih.gov. The saved key is not displayed. Leaving this + blank keeps the key that is already saved. +
+ +
+ + <%=button("Save").submit(true)%> <%=button("Cancel").href(panoramaPublicAdminUrl)%> diff --git a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/NcbiApiKeyTest.java b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/NcbiApiKeyTest.java new file mode 100644 index 00000000..b0c522db --- /dev/null +++ b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/NcbiApiKeyTest.java @@ -0,0 +1,153 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.labkey.test.tests.panoramapublic; + +import org.junit.After; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.labkey.remoteapi.CommandException; +import org.labkey.remoteapi.SimplePostCommand; +import org.labkey.test.BaseWebDriverTest; +import org.labkey.test.Locator; +import org.labkey.test.TestProperties; +import org.labkey.test.categories.External; +import org.labkey.test.categories.MacCossLabModules; + +import java.io.IOException; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +/** + * Covers the NCBI API key on the Private Data Reminder Settings page. The key is a credential, so + * the page stores it in the encrypted property store and never displays it again. + * + * The key is site wide and cannot be read back, so this test cannot save one without destroying + * whatever is already there. It fails rather than skips when a key is saved, so the missing + * coverage is visible instead of silent. + */ +@Category({External.class, MacCossLabModules.class}) +@BaseWebDriverTest.ClassTimeout(minutes = 5) +public class NcbiApiKeyTest extends PanoramaPublicBaseTest +{ + private static final String TEST_API_KEY = "test-ncbi-api-key"; + + private boolean _savedTestApiKey = false; + private boolean _useMockNcbi = false; + + @Test + public void testNcbiApiKeySettings() + { + setupMockNcbiService(); + + assertEquals("An NCBI API key is saved on this server. This test saves its own key and cannot" + + " restore yours, because a saved key is never readable. Remove the key on the" + + " Private Data Reminder Settings page, run this test, then enter the key again.", + "false", getPrivateDataReminderSettings().get("ncbiApiKeySaved")); + + _savedTestApiKey = true; + savePrivateDataReminderSettings("2", "0", "0", true, TEST_API_KEY); + + assertEquals("The saved key must never be rendered into the form", "", + getFormElement(Locator.input("ncbiApiKey"))); + + // Saving with the field left blank must keep the stored key. Every other field on this page + // is edited routinely, so a blank field cannot mean "remove the key". + savePrivateDataReminderSettings("3", "0", "0", true, ""); + assertEquals("A blank key field must leave the saved key alone", "true", + getPrivateDataReminderSettings().get("ncbiApiKeySaved")); + + verifyValidateButton(); + + // Removing a key takes the explicit checkbox. + checkCheckbox(Locator.checkboxByName("clearNcbiApiKey")); + clickButton("Save"); + _savedTestApiKey = false; + assertEquals("Remove the saved key should remove it", "false", + getPrivateDataReminderSettings().get("ncbiApiKeySaved")); + } + + /** + * Covers the button, the request it sends and the message it displays. The mock answers every + * request, so it reports the key as accepted. Only real NCBI rejects one. + */ + private void verifyValidateButton() + { + setFormElement(Locator.input("ncbiApiKey"), "not-a-real-key"); + click(Locator.lkButton("Validate")); + + String expected = _useMockNcbi ? "NCBI accepted this key" : "NCBI rejected this key"; + waitForElement(Locator.id("ncbiApiKeyValidationResult").containing(expected)); + + setFormElement(Locator.input("ncbiApiKey"), ""); + } + + /* + * On TeamCity, route NCBI requests through the mock so this test does not depend on NCBI being + * reachable. On a development machine, use the real service so a key can actually be rejected. + */ + private void setupMockNcbiService() + { + if (!TestProperties.isTestRunningOnTeamCity()) + { + return; + } + + try + { + SimplePostCommand command = new SimplePostCommand("panoramapublic", "setupMockNcbiService"); + command.execute(createDefaultConnection(), "/"); + _useMockNcbi = true; + log("Using mock NCBI service"); + } + catch (IOException | CommandException e) + { + fail("Failed to set up the mock NCBI service: " + e.getMessage()); + } + } + + private void restoreNcbiService() + { + try + { + SimplePostCommand command = new SimplePostCommand("panoramapublic", "restoreNcbiService"); + command.execute(createDefaultConnection(), "/"); + log("Restored real NCBI service"); + } + catch (IOException | CommandException e) + { + log("Warning: Failed to restore NCBI service: " + e.getMessage()); + } + } + + @After + public void removeTestApiKey() + { + if (_useMockNcbi) + { + restoreNcbiService(); + } + + if (_savedTestApiKey) + { + // Remove the key even when the test failed before its own removal step. A key NCBI + // rejects makes every publication search on this server fail, including the next run's. + getPrivateDataReminderSettings(); + checkCheckbox(Locator.checkboxByName("clearNcbiApiKey")); + clickButton("Save"); + } + } +} diff --git a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicBaseTest.java b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicBaseTest.java index c4ee6f4c..679c51f9 100644 --- a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicBaseTest.java +++ b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicBaseTest.java @@ -631,7 +631,7 @@ protected void verifyIsPublicColumn(String panoramaPublicProject, String experim /** * Navigate to the Private Data Reminder Settings page and read the current form values. - * Returns a map with keys: extensionLength, delayUntilFirstReminder, reminderFrequency, enablePublicationSearch, publicationSearchFrequency. + * Returns a map with keys: extensionLength, delayUntilFirstReminder, reminderFrequency, enablePublicationSearch, publicationSearchFrequency, ncbiApiKeySaved. */ protected Map getPrivateDataReminderSettings() { @@ -645,6 +645,9 @@ protected Map getPrivateDataReminderSettings() settings.put("reminderFrequency", getFormElement(Locator.input("reminderFrequency"))); settings.put("enablePublicationSearch", String.valueOf(Locator.checkboxByName("enablePublicationSearch").findElement(getDriver()).isSelected())); settings.put("publicationSearchFrequency", getFormElement(Locator.input("publicationSearchFrequency"))); + // The saved key is never displayed. The placeholder is the only signal that one is stored. + settings.put("ncbiApiKeySaved", String.valueOf( + Locator.input("ncbiApiKey").findElement(getDriver()).getDomAttribute("placeholder").startsWith("A key is saved"))); return settings; } @@ -654,6 +657,14 @@ protected void savePrivateDataReminderSettings(String extensionLength, String de } protected void savePrivateDataReminderSettings(String extensionLength, String delayUntilFirstReminder, String reminderFrequency, boolean enablePublicationSearch) + { + savePrivateDataReminderSettings(extensionLength, delayUntilFirstReminder, reminderFrequency, enablePublicationSearch, null); + } + + /** + * @param ncbiApiKey value to enter in the NCBI API key field. Pass null to leave the field untouched. + */ + protected void savePrivateDataReminderSettings(String extensionLength, String delayUntilFirstReminder, String reminderFrequency, boolean enablePublicationSearch, String ncbiApiKey) { goToAdminConsole().goToSettingsSection(); clickAndWait(Locator.linkWithText("Panorama Public")); @@ -662,6 +673,10 @@ protected void savePrivateDataReminderSettings(String extensionLength, String de setFormElement(Locator.input("delayUntilFirstReminder"), delayUntilFirstReminder); setFormElement(Locator.input("reminderFrequency"), reminderFrequency); setFormElement(Locator.input("extensionLength"), extensionLength); + if (ncbiApiKey != null) + { + setFormElement(Locator.input("ncbiApiKey"), ncbiApiKey); + } if (enablePublicationSearch) { checkCheckbox(Locator.checkboxByName("enablePublicationSearch")); @@ -677,6 +692,13 @@ protected void savePrivateDataReminderSettings(String extensionLength, String de assertEquals(String.valueOf(delayUntilFirstReminder), getFormElement(Locator.input("delayUntilFirstReminder"))); assertEquals(String.valueOf(reminderFrequency), getFormElement(Locator.input("reminderFrequency"))); assertEquals(String.valueOf(extensionLength), getFormElement(Locator.input("extensionLength"))); + assertEquals("The saved publication search setting should be displayed on the form", enablePublicationSearch, + Locator.checkboxByName("enablePublicationSearch").findElement(getDriver()).isSelected()); + if (ncbiApiKey != null) + { + assertEquals("The form should report that a key is saved", "true", + getPrivateDataReminderSettings().get("ncbiApiKeySaved")); + } } protected void goToSendRemindersPage(String projectName) diff --git a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java index 555a193e..25124e57 100644 --- a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java +++ b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java @@ -114,6 +114,10 @@ public void testPublicationSearchAndDismiss() // Step 1: Set up mock NCBI service if running on TeamCity setupMockNcbiService(); + // Capture the existing reminder settings up front so resetAfterTest can put them back. A dev + // machine may have non-default values set. + _originalReminderSettings = getPrivateDataReminderSettings(); + // Step 2: Create dataset 1 folder, submit to Panorama Public, and copy String testProject = getProjectName(); String shortAccessUrl1 = setupFolderSubmitAndCopy(testProject, FOLDER_1, TARGET_FOLDER_1, @@ -190,8 +194,6 @@ public void testPublicationSearchAndDismiss() assertTextPresent("The user has already dismissed the publication suggestion PubMed ID " + PMID_1 + " for this dataset"); // Step 9: Run reminders in TEST MODE — verify DatasetStatus is NOT updated - // Save current settings so they can be restored in doCleanup - _originalReminderSettings = getPrivateDataReminderSettings(); savePrivateDataReminderSettings("2", "0", "0", true); // Post reminders in test mode with publication search enabled @@ -256,8 +258,10 @@ public void testPublicationSearchAndDismiss() assertNotNull("Expected publicationType for dataset 2", dsStatus2AfterPost.get("PublicationType")); assertNotNull("Expected lastReminderDate for dataset 2", dsStatus2AfterPost.get("LastReminderDate")); assertNotNull("Expected citation to be cached for dataset 2", dsStatus2AfterPost.get("Citation")); + } + /* * Navigate to the Panorama Public copy folder and get the experiment ID. */ @@ -486,8 +490,12 @@ public void resetAfterTest() _originalReminderSettings.get("extensionLength"), _originalReminderSettings.get("delayUntilFirstReminder"), _originalReminderSettings.get("reminderFrequency"), - Boolean.parseBoolean(_originalReminderSettings.get("enablePublicationSearch"))); + // A key saved before the run cannot be read back to restore it, so leave the + // key field alone here. Any key the test saved is removed below. + Boolean.parseBoolean(_originalReminderSettings.get("enablePublicationSearch")), + null); } + } @Override