diff --git a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java index cef3ae75..f8ce5122 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java +++ b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java @@ -95,6 +95,7 @@ import org.labkey.api.query.QueryView; import org.labkey.api.query.ValidationException; import org.labkey.api.security.AdminConsoleAction; +import org.labkey.api.security.Encryption; import org.labkey.api.security.Group; import org.labkey.api.security.LoginManager; import org.labkey.api.security.MutableSecurityPolicy; @@ -158,6 +159,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 +10086,59 @@ 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); + + if (!Encryption.isEncryptionPassPhraseSpecified()) + { + response.put("valid", false); + response.put("message", PrivateDataReminderSettings.NCBI_API_KEY_REQUIRES_ENCRYPTION); + return response; + } + + // An empty field means check the key that is already saved. + 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, LOG); + response.put("valid", check.isValid()); + if (check.isValid()) + { + // Validating does not save the key. Inform the user that the key needs to be saved. + 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 is displayed next to the field. NCBI's own message is displayed separately. + 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 { @@ -10131,6 +10186,33 @@ else if (PrivateDataReminderSettings.parseReminderTime(form.getReminderTime()) = errors.reject(ERROR_MSG, String.format("'Reminder time' could not be parsed. It must be in the format - %s, e.g. %s.", PrivateDataReminderSettings.REMINDER_TIME_FORMAT, PrivateDataReminderSettings.DEFAULT_REMINDER_TIME)); } + if (form.isClearNcbiApiKey() && !StringUtils.isBlank(form.getNcbiApiKey())) + { + errors.reject(ERROR_MSG, "Enter a new NCBI API key or select 'Remove the saved key', not both."); + } + // saveNcbiApiKey throws without an encryption key. Reject here, before handlePost saves the other settings. + if ((form.isClearNcbiApiKey() || !StringUtils.isBlank(form.getNcbiApiKey())) + && !Encryption.isEncryptionPassPhraseSpecified()) + { + errors.reject(ERROR_MSG, PrivateDataReminderSettings.NCBI_API_KEY_REQUIRES_ENCRYPTION); + } + // Save a new key only if NCBI accepts it. + if (!errors.hasErrors() && !form.isClearNcbiApiKey() && !StringUtils.isBlank(form.getNcbiApiKey())) + { + NcbiApiKeyCheck check = NcbiPublicationSearchService.get().checkApiKey(form.getNcbiApiKey().trim(), LOG); + String detail = StringUtils.defaultString(check.getMessage()); + if (check.isRejected()) + { + LOG.warn("NCBI rejected an API key entered on the Private Data Reminder Settings page. {}", detail); + errors.reject(ERROR_MSG, "NCBI rejected this API key, so it was not saved. " + detail); + } + else if (!check.isValid()) + { + LOG.warn("Could not check an API key entered on the Private Data Reminder Settings page. {}", detail); + errors.reject(ERROR_MSG, "Could not check this API key with NCBI, so it was not saved." + + " Try again later. " + detail); + } + } } @Override @@ -10148,6 +10230,9 @@ public ModelAndView getView(PrivateDataReminderSettingsForm form, boolean reshow form.setPublicationSearchFrequency(settings.getPublicationSearchFrequency()); } + // Rendered as an attribute, not an input, so the form does not post it back. Set on every render. + form.setNcbiApiKeySet(PrivateDataReminderSettings.hasNcbiApiKey()); + VBox view = new VBox(); view.addView(new JspView<>("/org/labkey/panoramapublic/view/privateDataRemindersSettingsForm.jsp", form, errors)); view.setTitle("Private Data Reminder Settings"); @@ -10168,6 +10253,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 +10301,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 +10374,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) @@ -11221,12 +11350,13 @@ public static ActionURL getCatalogImageDownloadUrl(ExperimentAnnotations expAnno // ======================== Support actions for Selenium tests ======================== // These actions swap the process-wide NCBI publication search service to a mock so that Selenium tests - // run without calling the live NCBI API. They must never be reachable on a production server (non-dev-mode) - private static void requireDevModeForMockNcbiService() + // run without calling the live NCBI API, and restore the settings a test changed. They must never be + // reachable on a production server (non-dev-mode) + private static void requireDevModeForTestSupport() { if (!AppProps.getInstance().isDevMode()) { - throw new NotFoundException("Mock NCBI publication search service actions are only available on a server running in dev mode."); + throw new NotFoundException("Selenium test support actions are only available on a server running in dev mode."); } } @@ -11236,7 +11366,7 @@ public static class SetupMockNcbiServiceAction extends MutatingApiAction @Override public Object execute(Object form, BindException errors) { - requireDevModeForMockNcbiService(); + requireDevModeForTestSupport(); NcbiPublicationSearchServiceImpl.setInstance(new MockNcbiPublicationSearchService()); return new ApiSimpleResponse("mock", true); } @@ -11248,7 +11378,7 @@ public static class RestoreNcbiServiceAction extends MutatingApiAction @Override public Object execute(Object form, BindException errors) { - requireDevModeForMockNcbiService(); + requireDevModeForTestSupport(); NcbiPublicationSearchServiceImpl.setInstance(new NcbiPublicationSearchServiceImpl()); return new ApiSimpleResponse("restored", true); } @@ -11260,7 +11390,7 @@ public static class RegisterMockPublicationAction extends MutatingApiAction + { + @Override + public Object execute(RestorePrivateDataReminderSettingsForm form, BindException errors) + { + requireDevModeForTestSupport(); + PrivateDataReminderSettings settings = PrivateDataReminderSettings.get(); + if (form.getExtensionLength() != null) + { + settings.setExtensionLength(form.getExtensionLength()); + } + if (form.getDelayUntilFirstReminder() != null) + { + settings.setDelayUntilFirstReminder(form.getDelayUntilFirstReminder()); + } + if (form.getReminderFrequency() != null) + { + settings.setReminderFrequency(form.getReminderFrequency()); + } + if (form.getEnablePublicationSearch() != null) + { + settings.setEnablePublicationSearch(form.getEnablePublicationSearch()); + } + if (form.getPublicationSearchFrequency() != null) + { + settings.setPublicationSearchFrequency(form.getPublicationSearchFrequency()); + } + PrivateDataReminderSettings.save(settings); + // hasNcbiApiKey is false on a server with no encryption key, where saveNcbiApiKey throws. + if (form.isClearNcbiApiKey() && PrivateDataReminderSettings.hasNcbiApiKey()) + { + PrivateDataReminderSettings.saveNcbiApiKey(null); + } + return new ApiSimpleResponse("restored", true); + } + } + + public static class RestorePrivateDataReminderSettingsForm + { + private Integer _extensionLength; + private Integer _delayUntilFirstReminder; + private Integer _reminderFrequency; + private Boolean _enablePublicationSearch; + private Integer _publicationSearchFrequency; + private boolean _clearNcbiApiKey; + + public Integer getExtensionLength() { return _extensionLength; } + public void setExtensionLength(Integer extensionLength) { _extensionLength = extensionLength; } + + public Integer getDelayUntilFirstReminder() { return _delayUntilFirstReminder; } + public void setDelayUntilFirstReminder(Integer delayUntilFirstReminder) { _delayUntilFirstReminder = delayUntilFirstReminder; } + + public Integer getReminderFrequency() { return _reminderFrequency; } + public void setReminderFrequency(Integer reminderFrequency) { _reminderFrequency = reminderFrequency; } + + public Boolean getEnablePublicationSearch() { return _enablePublicationSearch; } + public void setEnablePublicationSearch(Boolean enablePublicationSearch) { _enablePublicationSearch = enablePublicationSearch; } + + public Integer getPublicationSearchFrequency() { return _publicationSearchFrequency; } + public void setPublicationSearchFrequency(Integer publicationSearchFrequency) { _publicationSearchFrequency = publicationSearchFrequency; } + + public boolean isClearNcbiApiKey() { return _clearNcbiApiKey; } + public void setClearNcbiApiKey(boolean clearNcbiApiKey) { _clearNcbiApiKey = clearNcbiApiKey; } + } + public static class TestCase extends AbstractActionPermissionTest { @Override diff --git a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicModule.java b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicModule.java index 1c9ea49b..b5b1f3d6 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,11 +48,14 @@ 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.NcbiHttpClient; import org.labkey.panoramapublic.ncbi.NcbiPublicationSearchServiceImpl; import org.labkey.panoramapublic.model.Journal; import org.labkey.panoramapublic.model.speclib.SpecLibKey; import org.labkey.panoramapublic.pipeline.CopyExperimentPipelineProvider; +import org.labkey.panoramapublic.pipeline.PrivateDataReminderJob; import org.labkey.panoramapublic.pipeline.PxValidationPipelineProvider; import org.labkey.panoramapublic.proteomexchange.ExperimentModificationGetter; import org.labkey.panoramapublic.proteomexchange.Formula; @@ -82,6 +87,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"; @@ -153,6 +160,20 @@ protected void startupAfterSpringConfig(ModuleContext moduleContext) } } + @Override + public void startBackgroundThreads() + { + // Re-establish the reminder schedule on every startup. + try + { + PrivateDataMessageScheduler.getInstance().initialize(PrivateDataReminderSettings.get().isEnableReminders()); + } + catch (RuntimeException e) + { + LOG.error("Failed to schedule the Panorama Public private data reminder job", e); + } + } + @NotNull @Override protected Collection createWebPartFactories() @@ -377,7 +398,9 @@ public Set getSchemaNames() set.add(CatalogEntryManager.TestCase.class); set.add(BlueskyApiClient.TestCase.class); set.add(PrivateDataReminderSettings.TestCase.class); + set.add(PrivateDataReminderJob.TestCase.class); set.add(NcbiPublicationSearchServiceImpl.TestCase.class); + set.add(NcbiHttpClient.TestCase.class); set.add(NcbiUtils.TestCase.class); return set; diff --git a/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java b/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java index a75b411c..955e017d 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java +++ b/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java @@ -15,11 +15,13 @@ */ 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; import org.junit.Test; import org.labkey.api.data.PropertyManager; +import org.labkey.api.security.Encryption; import org.labkey.api.util.DateUtil; import org.labkey.panoramapublic.model.DatasetStatus; @@ -30,6 +32,7 @@ import java.time.format.DateTimeFormatter; import java.time.format.DateTimeParseException; import java.util.Date; +import java.util.Map; public class PrivateDataReminderSettings { @@ -41,6 +44,10 @@ 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"; + public static final String NCBI_API_KEY_REQUIRES_ENCRYPTION = "An NCBI API key cannot be saved or removed because" + + " this server has no encryption key configured."; private static final boolean DEFAULT_ENABLE_REMINDERS = false; public static final String DEFAULT_REMINDER_TIME = "8:00 AM"; @@ -61,6 +68,7 @@ public class PrivateDataReminderSettings private int _extensionLength; private boolean _enablePublicationSearch; private int _publicationSearchFrequency; + private String _ncbiApiKey; public static PrivateDataReminderSettings get() { @@ -113,6 +121,9 @@ public static PrivateDataReminderSettings get() settings.setPublicationSearchFrequency(DEFAULT_PUBLICATION_SEARCH_FREQUENCY); } + // Read the key from its own property set. + settings.setNcbiApiKey(getNcbiApiKeyValue()); + return settings; } @@ -150,6 +161,58 @@ public static void save(PrivateDataReminderSettings settings) settingsMap.save(); } + /** + * Save the NCBI API key, or remove it when the key is blank. The key lives in the encrypted store. + */ + public static void saveNcbiApiKey(@Nullable String apiKey) + { + PropertyManager.WritablePropertyMap credentials; + try + { + credentials = PropertyManager.getEncryptedStore().getWritableProperties(PROP_NCBI_CREDENTIALS, true); + } + catch (Encryption.DecryptionException e) + { + // The saved key cannot be decrypted, for example after the encryption key changed. Delete it and start over. + PropertyManager.getEncryptedStore().deletePropertySet(PROP_NCBI_CREDENTIALS); + 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() + { + // The encrypted store throws when no encryption key is configured. Return null in this case. + if (!Encryption.isEncryptionPassPhraseSpecified()) + { + return null; + } + try + { + Map credentials = + PropertyManager.getEncryptedStore().getProperties(PROP_NCBI_CREDENTIALS); + return credentials.get(PROP_NCBI_API_KEY); + } + catch (Encryption.DecryptionException e) + { + // The key was saved under a different encryption key. Run without it until an admin saves a new one. + return null; + } + } + public void setEnableReminders(boolean enableReminders) { _enableReminders = enableReminders; @@ -225,6 +288,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 +431,129 @@ 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) + private void testExtensionIsValidAsOf(PrivateDataReminderSettings settings, int monthsOffset, + int minutesBeforeExpiry) { - testExtensionIsValid(settings, monthsOffset, minutesOffset, dateFromNow(), true); + testExtensionNearExpiry(settings, monthsOffset, minutesBeforeExpiry, true); } - private void testExtensionIsExpiredAsOf(PrivateDataReminderSettings settings, int monthsOffset, int minutesOffset) + private void testExtensionIsExpiredAsOf(PrivateDataReminderSettings settings, int monthsOffset, + int minutesBeforeExpiry) { - testExtensionIsValid(settings, monthsOffset, minutesOffset, dateFromNow(), false); + testExtensionNearExpiry(settings, monthsOffset, minutesBeforeExpiry, false); } - private void testExtensionIsValid(PrivateDataReminderSettings settings, int monthsOffset, int minutesOffset, - Date currentDate, boolean expectedValid) + /** + * Checks the boundary at the moment an extension expires. The current date comes from the expiry + * the settings calculate, because subtracting months and adding them back does not always return + * to the same day. From 31 August, six months back is 28 February, and six months on is 28 August. + * + * @param minutesBeforeExpiry zero is the expiry itself, and a negative value is after it. + */ + private void testExtensionNearExpiry(PrivateDataReminderSettings settings, int monthsOffset, + int minutesBeforeExpiry, boolean expectedValid) { - Date extensionDate = dateFromNow(monthsOffset, 0, minutesOffset); + DatasetStatus datasetStatus = new DatasetStatus(); + datasetStatus.setExtensionRequestedDate(dateFromNow(monthsOffset, 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, boolean expectedValid) + { DatasetStatus datasetStatus = new DatasetStatus(); - datasetStatus.setExtensionRequestedDate(extensionDate); + datasetStatus.setExtensionRequestedDate(dateFromNow(monthsOffset, 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) + private void testReminderIsRecentAsOf(PrivateDataReminderSettings settings, int monthsOffset, + int minutesBeforeExpiry) { - testReminderIsRecent(settings, monthsOffset, 0, minutesOffset, dateFromNow(), true); + testReminderNearExpiry(settings, monthsOffset, minutesBeforeExpiry, true); } - private void testReminderIsOldAsOf(PrivateDataReminderSettings settings, int monthsOffset, int minutesOffset) + private void testReminderIsOldAsOf(PrivateDataReminderSettings settings, int monthsOffset, + int minutesBeforeExpiry) { - testReminderIsRecent(settings, monthsOffset, 0, minutesOffset, dateFromNow(), false); + testReminderNearExpiry(settings, monthsOffset, minutesBeforeExpiry, 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 expiry, as {@link #testExtensionNearExpiry} explains. + * + * @param minutesBeforeExpiry zero is the expiry itself, and a negative value is after it. + */ + private void testReminderNearExpiry(PrivateDataReminderSettings settings, int monthsOffset, + int minutesBeforeExpiry, boolean expectedRecent) { - Date reminderDate = dateFromNow(monthsOffset, daysOffset, minutesOffset); + DatasetStatus datasetStatus = new DatasetStatus(); + datasetStatus.setLastReminderDate(dateFromNow(monthsOffset, 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)); 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) @@ -464,19 +574,13 @@ private PrivateDataReminderSettings createTestSettings(int extensionLength, int return testSettings; } - private Date dateFromNow() - { - return dateFromNow(0, 0, 0); - } - - private Date dateFromNow(int monthsOffset, int daysOffset, int minutesOffset) + private Date dateFromNow(int monthsOffset, int daysOffset) { return Date.from( LocalDate.now() .plusMonths(monthsOffset) .plusDays(daysOffset) .atStartOfDay(ZoneId.systemDefault()) - .plusMinutes(minutesOffset) .toInstant() ); } diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java index b32c5864..63cbef5b 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java @@ -15,42 +15,56 @@ */ package org.labkey.panoramapublic.ncbi; +import org.apache.hc.client5.http.HttpResponseException; 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; /** * 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 - * verification, citation parsing, and priority filtering run through the real implementation code. + * Used by NcbiApiKeyTest on every server, and by PublicationSearchTest on TeamCity. + * Extends {@link NcbiPublicationSearchServiceImpl} and gives it an {@link NcbiHttpClient} whose + * {@code executeGet()} returns canned responses in place of the real HTTP request. The retry loop in + * {@link NcbiHttpClient#getString}, and 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 - * appropriate responses when the real search logic calls {@code getString()}. + * appropriate responses when the real search logic sends a request. */ public class MockNcbiPublicationSearchService extends NcbiPublicationSearchServiceImpl { - // ESearch: searchKey -> list of IDs (per database) - private final Map> _pmcSearchResults = new HashMap<>(); - private final Map> _pubmedSearchResults = new HashMap<>(); + // The mock responds to a request carrying this API key with a 400, which is NCBI's response to a key it + // does not recognize. NcbiApiKeyTest uses the same value. + public static final String REJECTED_API_KEY = "mock-rejected-ncbi-api-key"; + // The mock responds to a request carrying this API key with a 503, so the key check cannot be completed. + // NcbiApiKeyTest uses the same value. + public static final String UNCHECKED_API_KEY = "mock-unchecked-ncbi-api-key"; - // ESummary: ID -> metadata JSONObject (per database) - private final Map _pmcMetadata = new HashMap<>(); - private final Map _pubmedMetadata = new HashMap<>(); + private final CannedResponses _responses; - // Citations: PMID -> citation string - private final Map _citations = new HashMap<>(); + public MockNcbiPublicationSearchService() + { + this(new CannedResponses()); + } + + private MockNcbiPublicationSearchService(CannedResponses responses) + { + super(responses); + _responses = responses; + } /** * Register a mock article. The mock stores the data in internal lookup maps used by - * {@link #getString(String)}. + * {@link CannedResponses#executeGet(String)}. * @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) @@ -105,13 +119,13 @@ public void register(String database, String id, String searchKey, // Store in appropriate maps if (isPmc) { - _pmcSearchResults.computeIfAbsent(searchKey, k -> new ArrayList<>()).add(id); - _pmcMetadata.put(id, metadata); + _responses._pmcSearchResults.computeIfAbsent(searchKey, k -> new ArrayList<>()).add(id); + _responses._pmcMetadata.put(id, metadata); } else { - _pubmedSearchResults.computeIfAbsent(searchKey, k -> new ArrayList<>()).add(id); - _pubmedMetadata.put(id, metadata); + _responses._pubmedSearchResults.computeIfAbsent(searchKey, k -> new ArrayList<>()).add(id); + _responses._pubmedMetadata.put(id, metadata); } // Store citation keyed by PMID. @@ -122,95 +136,135 @@ public void register(String database, String id, String searchKey, String citationKey = isPmc ? pmid : id; if (citationKey != null) { - _citations.put(citationKey, citation); + _responses._citations.put(citationKey, citation); } } } /** - * Returns canned responses for NCBI API requests based on registered mock data. - * Handles ESearch, ESummary, and Citation Exporter URLs. + * An {@link NcbiHttpClient} that returns canned responses for NCBI API requests based on registered + * mock data. Handles ESearch, ESummary, and Citation Exporter URLs. */ - @Override - protected String getString(String url) throws IOException + private static class CannedResponses extends NcbiHttpClient { - if (url.contains("esearch.fcgi")) - { - return handleESearch(url).toString(); - } - else if (url.contains("esummary.fcgi")) + // ESearch: searchKey -> list of IDs (per database) + private final Map> _pmcSearchResults = new HashMap<>(); + private final Map> _pubmedSearchResults = new HashMap<>(); + + // ESummary: ID -> metadata JSONObject (per database) + private final Map _pmcMetadata = new HashMap<>(); + private final Map _pubmedMetadata = new HashMap<>(); + + // Citations: PMID -> citation string + private final Map _citations = new HashMap<>(); + + @Override + protected String executeGet(String url) throws IOException { - return handleESummary(url).toString(); + if (REJECTED_API_KEY.equals(apiKeyFrom(url))) + { + throw new HttpResponseException(400, "Bad Request - API key invalid"); + } + if (UNCHECKED_API_KEY.equals(apiKeyFrom(url))) + { + throw new HttpResponseException(503, "Service Unavailable"); + } + if (url.contains("esearch.fcgi")) + { + return handleESearch(url).toString(); + } + else if (url.contains("esummary.fcgi")) + { + return handleESummary(url).toString(); + } + else if (url.contains("lit/ctxp")) + { + return handleCitation(url).toString(); + } + throw new IOException("MockNcbiPublicationSearchService: unexpected URL: " + url); } - else if (url.contains("lit/ctxp")) + + private JSONObject handleESearch(String url) { - return handleCitation(url).toString(); - } - throw new IOException("MockNcbiPublicationSearchService: unexpected URL: " + url); - } + boolean isPmc = "pmc".equals(extractQueryParam(url, "db")); + Map> searchMap = isPmc ? _pmcSearchResults : _pubmedSearchResults; - private JSONObject handleESearch(String url) - { - boolean isPmc = url.contains("db=pmc"); - Map> searchMap = isPmc ? _pmcSearchResults : _pubmedSearchResults; + // Match search keys against the decoded ESearch query term, so a key cannot match part of + // another parameter such as tool or email. + String term = extractQueryParam(url, "term"); - JSONArray idList = new JSONArray(); - for (Map.Entry> entry : searchMap.entrySet()) - { - if (url.contains(entry.getKey())) + JSONArray idList = new JSONArray(); + if (term != null) { - entry.getValue().forEach(idList::put); + for (Map.Entry> entry : searchMap.entrySet()) + { + // A key is a substring of the term. searchPmc wraps the PMC term in quotes, e.g. "PXD056793". + if (term.contains(entry.getKey())) + { + entry.getValue().forEach(idList::put); + } + } } + + JSONObject esearchResult = new JSONObject(); + esearchResult.put("idlist", idList); + return new JSONObject().put("esearchresult", esearchResult); } - JSONObject esearchResult = new JSONObject(); - esearchResult.put("idlist", idList); - return new JSONObject().put("esearchresult", esearchResult); - } + private JSONObject handleESummary(String url) + { + boolean isPmc = "pmc".equals(extractQueryParam(url, "db")); + Map metadataMap = isPmc ? _pmcMetadata : _pubmedMetadata; - private JSONObject handleESummary(String url) - { - boolean isPmc = url.contains("db=pmc"); - Map metadataMap = isPmc ? _pmcMetadata : _pubmedMetadata; + // ESummary requests a comma-separated list of IDs in the "id" parameter. Match registered + // IDs against that list 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())) + JSONObject result = new JSONObject(); + for (Map.Entry entry : metadataMap.entrySet()) { - result.put(entry.getKey(), entry.getValue()); + if (requestedIds.contains(entry.getKey())) + { + result.put(entry.getKey(), entry.getValue()); + } } - } - return new JSONObject().put("result", result); - } + return new JSONObject().put("result", result); + } - /** - * Build a citation JSON response matching the NCBI Literature Citation Exporter format. - * The real API returns {@code {"nlm":{"orig":"citation text..."}}}. - * If no citation is registered for the ID, returns an empty JSON object. - */ - 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) + /** + * Build a citation JSON response matching the NCBI Literature Citation Exporter format. + * The real API returns {@code {"nlm":{"orig":"citation text..."}}}. + * If no citation is registered for the ID, returns an empty JSON object. + */ + private JSONObject handleCitation(String url) { - id = url.substring(idIdx + 3); - // Remove any trailing query parameters - int ampIdx = id.indexOf('&'); - if (ampIdx >= 0) + String id = extractQueryParam(url, "id"); + String citation = id != null ? _citations.get(id) : null; + if (citation != null) { - id = id.substring(0, ampIdx); + return new JSONObject().put("nlm", new JSONObject().put("orig", citation)); } + return new JSONObject(); } - String citation = id != null ? _citations.get(id) : null; - if (citation != null) + /** + * 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) { - return new JSONObject().put("nlm", new JSONObject().put("orig", citation)); + 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; } - return new JSONObject(); } } 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..5c4a9ecf --- /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, which means sending NCBI a request that carries the key + * and reading the status it returns. + */ +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/NcbiHttpClient.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiHttpClient.java new file mode 100644 index 00000000..d2cc3950 --- /dev/null +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiHttpClient.java @@ -0,0 +1,458 @@ +/* + * 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.apache.commons.lang3.StringUtils; +import org.apache.hc.client5.http.HttpResponseException; +import org.apache.hc.client5.http.classic.methods.HttpGet; +import org.apache.hc.client5.http.config.ConnectionConfig; +import org.apache.hc.client5.http.config.RequestConfig; +import org.apache.hc.client5.http.impl.classic.CloseableHttpClient; +import org.apache.hc.client5.http.impl.classic.HttpClientBuilder; +import org.apache.hc.client5.http.impl.io.BasicHttpClientConnectionManager; +import org.apache.hc.core5.http.ClassicHttpResponse; +import org.apache.hc.core5.http.NoHttpResponseException; +import org.apache.hc.core5.http.io.entity.EntityUtils; +import org.apache.hc.core5.util.Timeout; +import org.apache.logging.log4j.Logger; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; +import org.junit.Assert; +import org.junit.Test; +import org.labkey.api.util.logging.LogHelper; + +import java.io.IOException; +import java.net.SocketException; +import java.net.SocketTimeoutException; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.List; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * Sends HTTP GET requests to NCBI for {@link NcbiPublicationSearchServiceImpl}. Retries transient + * failures, and removes the NCBI API key from anything it logs or puts in an exception message. + */ +public class NcbiHttpClient +{ + private static final int TIMEOUT_MS = 10000; // 10 seconds + + // NCBI eutils fails intermittently, even well under the rate limit. Retry the transient + // failures listed on isRetryable 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 response when the request rate is exceeded + private static final int MAX_ERROR_BODY_CHARS = 500; + // Read past the logged length by more than the length of an NCBI API key (36 characters), so a key + // that crosses the cut is read whole and can be redacted. + private static final int MAX_ERROR_BODY_READ_CHARS = MAX_ERROR_BODY_CHARS + 100; + + private static final String REDACTED = "REDACTED"; + private static final Pattern API_KEY_PARAM = Pattern.compile("api_key=([^&\\s]*)"); + + /** + * Execute an HTTP GET request and return the response body as a string. Retry warnings are + * written to {@code log}. + * @throws IOException if the request fails or the server returns a non-2xx response + */ + public String getString(String url, @NotNull 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); + 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)) + { + // An interrupted thread cannot wait, so the remaining attempts would run back to + // back. Give up instead. + throw e; + } + } + } + } + + // Overridden in unit tests to drive the retry loop in getString(), and by + // MockNcbiPublicationSearchService to return canned responses, without real HTTP calls. + protected String executeGet(String url) throws IOException + { + ConnectionConfig connectionConfig = ConnectionConfig.custom() + .setConnectTimeout(Timeout.ofMilliseconds(TIMEOUT_MS)) + .setSocketTimeout(Timeout.ofMilliseconds(TIMEOUT_MS)) + .build(); + + RequestConfig requestConfig = RequestConfig.custom() + .setResponseTimeout(Timeout.ofMilliseconds(TIMEOUT_MS)) + .build(); + + BasicHttpClientConnectionManager connectionManager = new BasicHttpClientConnectionManager(); + connectionManager.setConnectionConfig(connectionConfig); + + try (CloseableHttpClient client = HttpClientBuilder.create() + .setDefaultRequestConfig(requestConfig) + .setConnectionManager(connectionManager) + // Without this, the number of requests would be up to twice MAX_HTTP_ATTEMPTS. isRetryable + // has to cover what this turns off. + .disableAutomaticRetries() + .build()) + { + HttpGet getRequest = new HttpGet(url); + return client.execute(getRequest, response -> { + int status = response.getCode(); + if (status < 200 || status >= 300) + { + 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, up to {@link #MAX_ERROR_BODY_READ_CHARS} characters. + * 5xx bodies are large, uninformative HTML error pages, so they are skipped. Returns null if + * the body cannot be read. + */ + 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_READ_CHARS); + } + catch (IOException | org.apache.hc.core5.http.ParseException e) + { + return null; + } + } + + /** + * Build the message for a non-2xx HttpResponseException. A 4xx appends the response body, so + * NCBI's reason (e.g. "API key invalid") reaches the log. Other statuses use only the reason + * phrase. The API key is removed from the body, which is third-party text bound for a log. + */ + static String errorDetail(int status, String reasonPhrase, @Nullable String body, @Nullable String apiKey) + { + if (status >= 400 && status < 500 && !StringUtils.isBlank(body)) + { + return reasonPhrase + " - " + StringUtils.abbreviate(redactApiKey(body.strip(), apiKey), MAX_ERROR_BODY_CHARS); + } + return reasonPhrase; + } + + /** + * Replace the NCBI API key wherever it appears in text bound for a log. + */ + static @Nullable 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, connection resets, closed connections, 5xx responses and 429 are transient NCBI + * failures worth retrying. Other 4xx errors are permanent. + */ + private static boolean isRetryable(IOException e) + { + if (e instanceof SocketTimeoutException || e instanceof SocketException || e instanceof NoHttpResponseException) + { + 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. + 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; + } + } + + public static class TestCase extends Assert + { + private static final Logger LOG = LogHelper.getLogger(TestCase.class, "NcbiHttpClient 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"))); + + // A connection reset or closed by the server is transient -> retry + assertTrue("A connection reset should be retried", isRetryable(new SocketException("Connection reset"))); + assertTrue("A connection closed without a response should be retried", + isRetryable(new NoHttpResponseException("The target server failed to respond"))); + + // 429 is NCBI's response 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("Stream closed"))); + } + + @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")); + + // A key that spans the truncation point must not leave its prefix in the message + String padding = "x".repeat(MAX_ERROR_BODY_CHARS - 10); + String spanning = errorDetail(400, "Bad Request", padding + " SECRET123456789 trailing", "SECRET123456789"); + assertFalse("errorDetail must not put part of the API key in the message", spanning.contains("SECRET")); + } + + @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 testGetStringRetriesTransientFailures() throws IOException + { + // executeGet returns a 5xx twice, then succeeds. getString should retry and return the body. + int[] attempts = {0}; + NoWaitClient client = new NoWaitClient() + { + @Override + protected String executeGet(String url) throws IOException + { + if (++attempts[0] < 3) + throw new HttpResponseException(503, "Service Unavailable"); + return "body"; + } + }; + assertEquals("body", client.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), client.sleeps); + } + + @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}; + NcbiHttpClient client = new NcbiHttpClient() + { + @Override + protected String executeGet(String url) throws IOException + { + attempts[0]++; + Thread.currentThread().interrupt(); + throw new HttpResponseException(503, "Service Unavailable"); + } + }; + + try + { + client.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}; + NoWaitClient client = new NoWaitClient() + { + @Override + protected String executeGet(String url) throws IOException + { + attempts[0]++; + throw new HttpResponseException(500, "Internal Server Error"); + } + }; + try + { + client.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), client.sleeps); + } + + @Test + public void testGetStringDoesNotRetryClientErrors() + { + // A 4xx is permanent. getString should fail immediately without retrying. + int[] attempts = {0}; + NoWaitClient client = new NoWaitClient() + { + @Override + protected String executeGet(String url) throws IOException + { + attempts[0]++; + throw new HttpResponseException(400, "Bad Request"); + } + }; + try + { + client.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", client.sleeps.isEmpty()); + } + + /** + * 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. + */ + static class NoWaitClient extends NcbiHttpClient + { + private final List sleeps = new ArrayList<>(); + + @Override + protected boolean sleepMs(long ms) + { + sleeps.add(ms); + return true; + } + } + } +} diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchService.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchService.java index 0e3de30d..ba33906b 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. Retry warnings are written to {@code logger}, or to + * the service's own logger if it is null. + */ + @NotNull NcbiApiKeyCheck checkApiKey(@Nullable String apiKey, @Nullable Logger logger); + @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 no publication was found and one or more NCBI requests failed. A + * null 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 no publication was found and one or more NCBI requests failed. 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..8342e02f 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java @@ -17,15 +17,7 @@ import org.apache.commons.lang3.StringUtils; import org.apache.commons.text.StringEscapeUtils; -import org.apache.hc.client5.http.classic.methods.HttpGet; -import org.apache.hc.client5.http.config.ConnectionConfig; -import org.apache.hc.client5.http.config.RequestConfig; -import org.apache.hc.client5.http.impl.classic.CloseableHttpClient; -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.io.entity.EntityUtils; -import org.apache.hc.core5.util.Timeout; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -38,10 +30,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 +51,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Predicate; import java.util.stream.Collectors; import static org.labkey.panoramapublic.ncbi.PublicationMatch.MATCH_DOI; @@ -83,6 +78,18 @@ public static void setInstance(NcbiPublicationSearchService impl) _instance = impl; } + private final NcbiHttpClient _httpClient; + + public NcbiPublicationSearchServiceImpl() + { + this(new NcbiHttpClient()); + } + + NcbiPublicationSearchServiceImpl(NcbiHttpClient httpClient) + { + _httpClient = httpClient; + } + // NCBI API endpoints private static final String ESEARCH_URL = "https://eutils.ncbi.nlm.nih.gov/entrez/eutils/esearch.fcgi"; private static final String ESUMMARY_URL = "https://eutils.ncbi.nlm.nih.gov/entrez/eutils/esummary.fcgi"; @@ -92,8 +99,9 @@ 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 TIMEOUT_MS = 10000; // 10 seconds + private static final int RATE_LIMIT_DELAY_MS = 400; // NCBI allows 3 requests/sec without an API key + + private static final int BAD_REQUEST = 400; // NCBI's response when the API key is not recognized // NCBI suggests using the 'tool' and 'email' parameters on E-utilities URLs // https://www.nlm.nih.gov/dataguide/eutilities/utilities.html @@ -156,12 +164,13 @@ 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) { - log.error("Error submitting a request to NCBI Literature Citation Exporter. URL: {}", queryUrl, e); + // A missing citation does not fail the match. The caller displays the publication ID instead. + log.warn("Request to the NCBI Literature Citation Exporter did not complete. URL: {}", queryUrl, e); } return null; } @@ -189,7 +198,7 @@ static String parseCitation(String response, String publicationId, DB database, } catch (JSONException e) { - log.error("Error parsing response from NCBI Literature Citation Exporter for {} ID {}", database.getLabel(), publicationId, e); + log.warn("Error parsing response from NCBI Literature Citation Exporter for {} ID {}", database.getLabel(), publicationId, e); } return null; } @@ -208,19 +217,35 @@ public List searchForPublication(@NotNull ExperimentAnnotation maxResults = Math.max(1, Math.min(maxResults, NcbiPublicationSearchService.MAX_RESULTS)); log.info("Starting publication search for experiment: {}", expAnnotations.getId()); + // An empty result is returned only when every request completed, so a caller can distinguish a search + // that found no publication from one that failed to run. + SearchRequests requests = new SearchRequests(); + // Search PubMed Central first - List matchedArticles = searchPmc(expAnnotations, log); + List matchedArticles = searchPmc(expAnnotations, log, requests); // If no PMC results, fall back to PubMed if (matchedArticles.isEmpty()) { log.info("No PMC articles found, trying PubMed fallback"); - matchedArticles = searchPubMed(expAnnotations, log); + matchedArticles = searchPubMed(expAnnotations, log, requests); + } + + if (!requests.failed.isEmpty()) + { + log.warn("Some NCBI requests did not complete for experiment {}. Failed requests: {}", + expAnnotations.getId(), StringUtils.join(requests.failed, ", ")); } // Build and return result if (matchedArticles.isEmpty()) { + if (!requests.failed.isEmpty()) + { + throw new NcbiSearchException("Publication search for experiment " + expAnnotations.getId() + + " did not complete. Failed requests: " + StringUtils.join(requests.failed, ", "), + !requests.anyCompleted); + } log.info("No publications found"); return Collections.emptyList(); } @@ -244,10 +269,18 @@ public List searchForPublication(@NotNull ExperimentAnnotation return matchedArticles; } + /** NCBI requests that failed during one search, and whether any request completed. */ + private static class SearchRequests + { + private final List failed = new ArrayList<>(); + private boolean anyCompleted = false; + } + /** * Search PubMed Central */ - private @NotNull List searchPmc(@NotNull ExperimentAnnotations expAnnotations, Logger log) + private @NotNull List searchPmc(@NotNull ExperimentAnnotations expAnnotations, Logger log, + SearchRequests requests) { Map searchTermsByStrategy = buildSearchTerms(expAnnotations); @@ -258,11 +291,21 @@ public List searchForPublication(@NotNull ExperimentAnnotation String strategy = entry.getKey(); String searchTerm = entry.getValue(); log.debug("Searching PMC by {}: {}", strategy, searchTerm); - List ids = searchPmc(quote(searchTerm), log); - if (!ids.isEmpty()) + try + { + List ids = searchPmc(quote(searchTerm), log); + requests.anyCompleted = true; + if (!ids.isEmpty()) + { + pmcIdsByStrategy.put(strategy, ids); + log.debug("Found {} PMC articles by {}", ids.size(), strategy); + } + } + catch (NcbiSearchException e) { - pmcIdsByStrategy.put(strategy, ids); - log.debug("Found {} PMC articles by {}", ids.size(), strategy); + // Each strategy is a separate request, so the remaining ones are still worth + // running. Not logged here, executeSearch logs the query and the cause. + requests.failed.add("PMC search by " + strategy); } rateLimit(); } @@ -275,7 +318,17 @@ public List searchForPublication(@NotNull ExperimentAnnotation log.info("Total unique PMC IDs found: {}", idToStrategies.size()); // Fetch and verify PMC articles - List pmcArticles = fetchAndVerifyPmcArticles(idToStrategies.keySet(), idToStrategies, expAnnotations, log); + List pmcArticles; + try + { + pmcArticles = fetchAndVerifyPmcArticles(idToStrategies.keySet(), idToStrategies, expAnnotations, log); + } + catch (NcbiSearchException e) + { + // The IDs cannot be verified without their metadata, so PMC has no usable result. + requests.failed.add("PMC metadata fetch"); + return Collections.emptyList(); + } // Apply priority filtering return applyPriorityFiltering(pmcArticles, expAnnotations.getCreated(), log); @@ -326,7 +379,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"); @@ -339,8 +392,34 @@ 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(); + // One failed request, not the outcome for the dataset. The caller carries on with the rest. + log.warn("Search of {} did not complete for query: {}", database, query, e); + throw new NcbiSearchException("Error searching " + database + " with query: " + query, e); + } + } + + @Override + public @NotNull NcbiApiKeyCheck checkApiKey(@Nullable String apiKey, @Nullable Logger logger) + { + // A minimal ESearch request. getString retries a transient failure before the check gives up. + String url = ESEARCH_URL + "?" + buildCommonParams("pubmed", apiKey) + "&term=labkey&retmax=1&retmode=json"; + try + { + getString(url, logger); + return NcbiApiKeyCheck.valid(); + } + catch (HttpResponseException e) + { + // NCBI rejects an unrecognized key with a 400. Any other status says nothing about the + // key, including a 403 or 404 from a proxy between the server and NCBI. + String message = NcbiHttpClient.redactApiKey(e.getMessage(), apiKey); + return e.getStatusCode() == BAD_REQUEST + ? NcbiApiKeyCheck.rejected(message) + : NcbiApiKeyCheck.unconfirmed(message); + } + catch (IOException e) + { + return NcbiApiKeyCheck.unconfirmed(NcbiHttpClient.redactApiKey(e.getMessage(), apiKey)); } } @@ -349,44 +428,14 @@ 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 + private 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. - * @throws IOException if the request fails or the server returns a non-2xx response - */ - protected String getString(String url) throws IOException + private String getString(String url, @Nullable Logger log) throws IOException { - ConnectionConfig connectionConfig = ConnectionConfig.custom() - .setConnectTimeout(Timeout.ofMilliseconds(TIMEOUT_MS)) - .setSocketTimeout(Timeout.ofMilliseconds(TIMEOUT_MS)) - .build(); - - RequestConfig requestConfig = RequestConfig.custom() - .setResponseTimeout(Timeout.ofMilliseconds(TIMEOUT_MS)) - .build(); - - BasicHttpClientConnectionManager connectionManager = new BasicHttpClientConnectionManager(); - connectionManager.setConnectionConfig(connectionConfig); - - try (CloseableHttpClient client = HttpClientBuilder.create() - .setDefaultRequestConfig(requestConfig) - .setConnectionManager(connectionManager) - .build()) - { - HttpGet getRequest = new HttpGet(url); - return client.execute(getRequest, response -> { - int status = response.getCode(); - if (status < 200 || status >= 300) - { - throw new HttpResponseException(status, response.getReasonPhrase()); - } - return EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8); - }); - } + return _httpClient.getString(url, getLog(log)); } /** @@ -420,7 +469,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(); @@ -438,8 +487,8 @@ private Map fetchMetadata(Collection ids, String dat } catch (IOException | JSONException e) { - log.error("Error fetching {} metadata for IDs: {}", database, ids, e); - return Collections.emptyMap(); + log.warn("Metadata fetch from {} did not complete for IDs: {}", database, ids, e); + throw new NcbiSearchException("Error fetching " + database + " metadata for IDs: " + ids, e); } } @@ -579,7 +628,8 @@ private static int countDataIdMatches(PublicationMatch a) /** * Fall back to PubMed search if PMC finds nothing */ - private List searchPubMed(ExperimentAnnotations expAnnotations, Logger log) + private List searchPubMed(ExperimentAnnotations expAnnotations, Logger log, + SearchRequests requests) { String firstName = expAnnotations.getSubmitterUser() != null ? expAnnotations.getSubmitterUser().getFirstName() : null; @@ -601,7 +651,17 @@ private List searchPubMed(ExperimentAnnotations expAnnotations stripQuerySpecialChars(lastName), stripQuerySpecialChars(firstName), stripQuerySpecialChars(title)); log.debug("PubMed fallback query: {}", query); - List pmids = searchPubMed(query, log); + List pmids; + try + { + pmids = searchPubMed(query, log); + requests.anyCompleted = true; + } + catch (NcbiSearchException e) + { + requests.failed.add("PubMed search"); + return Collections.emptyList(); + } if (pmids.isEmpty()) { @@ -612,7 +672,17 @@ private List searchPubMed(ExperimentAnnotations expAnnotations log.info("PubMed fallback found {} result(s), verifying...", pmids.size()); // Fetch metadata and verify - Map metadata = fetchPubMedMetadata(pmids, log); + Map metadata; + try + { + metadata = fetchPubMedMetadata(pmids, log); + } + catch (NcbiSearchException e) + { + // Metadata is required to confirm that a PMID is a match. + requests.failed.add("PubMed metadata fetch"); + return Collections.emptyList(); + } List articles = new ArrayList<>(); for (String pmid : pmids) @@ -919,16 +989,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(); - } + _httpClient.sleepMs(RATE_LIMIT_DELAY_MS); } /** @@ -943,13 +1006,27 @@ 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 API key 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 +1499,148 @@ public void testPublicationMatchRoundTrip() assertFalse(restored.matchesProteomeXchangeId()); } + // -- Request parameter and API key check tests -- + + @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 testCheckApiKey() + { + // NCBI rejects a key it does not recognize with a 400. 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 403 or 404 can come from a proxy between the server and NCBI, which says nothing about + // the key. Calling either a rejection would stop the reminder job on a key that works. + assertEquals(NcbiApiKeyCheck.Status.UNCONFIRMED, checkApiKeyAgainst(new HttpResponseException(403, "Forbidden")).getStatus()); + assertEquals(NcbiApiKeyCheck.Status.UNCONFIRMED, checkApiKeyAgainst(new HttpResponseException(404, "Not Found")).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")); + + // The service's requests go through the retry loop in NcbiHttpClient.getString + int[] attempts = {0}; + checkApiKeyAgainst(new HttpResponseException(503, "Service Unavailable"), "test-key", attempts); + assertEquals("A 503 from the key check should be tried 3 times", 3, attempts[0]); + attempts[0] = 0; + checkApiKeyAgainst(new HttpResponseException(400, "Bad Request"), "test-key", attempts); + assertEquals("A 400 from the key check should be tried once", 1, attempts[0]); + } + + private NcbiApiKeyCheck checkApiKeyAgainst(IOException failure) + { + return checkApiKeyAgainst(failure, "test-key"); + } + + private NcbiApiKeyCheck checkApiKeyAgainst(IOException failure, String apiKey) + { + return checkApiKeyAgainst(failure, apiKey, new int[1]); + } + + private NcbiApiKeyCheck checkApiKeyAgainst(IOException failure, String apiKey, int[] attempts) + { + NcbiHttpClient client = new NcbiHttpClient.TestCase.NoWaitClient() + { + @Override + protected String executeGet(String url) throws IOException + { + attempts[0]++; + if (failure != null) + { + throw failure; + } + return "{\"esearchresult\":{\"idlist\":[]}}"; + } + }; + return new NcbiPublicationSearchServiceImpl(client).checkApiKey(apiKey, null); + } + + @Test + public void testSearchReportsWhetherAllRequestsFailed() + { + // Two PMC search requests, one per term. No submitter, so there is no PubMed fallback. + ExperimentAnnotations expAnnotations = new ExperimentAnnotations(); + expAnnotations.setPxid("PXD000001"); + expAnnotations.setDoi("10.1000/test"); + + NcbiSearchException allFailed = searchFailure(expAnnotations, url -> true); + assertTrue("A search where every request failed should report it", allFailed.isAllRequestsFailed()); + + NcbiSearchException someFailed = searchFailure(expAnnotations, url -> url.contains("PXD000001")); + assertFalse("A search where a request completed should not report that all requests failed", + someFailed.isAllRequestsFailed()); + } + + /** Returns the exception from a search where requests whose URL matches {@code fails} return a 503. */ + private NcbiSearchException searchFailure(ExperimentAnnotations expAnnotations, Predicate fails) + { + NcbiHttpClient client = new NcbiHttpClient.TestCase.NoWaitClient() + { + @Override + protected String executeGet(String url) throws IOException + { + if (fails.test(url)) + { + throw new HttpResponseException(503, "Service Unavailable"); + } + return "{\"esearchresult\":{\"idlist\":[]}}"; + } + }; + try + { + new NcbiPublicationSearchServiceImpl(client).searchForPublication(expAnnotations, LOG); + fail("A search with a failed request and no match should throw NcbiSearchException"); + return null; + } + catch (NcbiSearchException e) + { + return e; + } + } + + @Test + public void testMockResponses() + { + // The Selenium tests use the mock only on TeamCity. Requests from the service should reach the + // mock's canned responses through NcbiHttpClient.getString. + MockNcbiPublicationSearchService mock = new MockNcbiPublicationSearchService(); + String citation = "Smith J. A test title. J Proteome Res. 2024."; + mock.register("pubmed", "12345", "Smith", null, "A test title", "Smith J", + "2024/01/15 00:00", "J Proteome Res", "Journal of Proteome Research", citation); + + assertEquals("The mock should return the registered citation", citation, mock.getCitation("12345", DB.PubMed)); + assertNull("The mock should return no citation for an ID that was not registered", + mock.getCitation("67890", DB.PubMed)); + assertEquals("The mock should report any API key as valid", NcbiApiKeyCheck.Status.VALID, + mock.checkApiKey("test-key", null).getStatus()); + } + // -- 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..00d67a86 --- /dev/null +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiSearchException.java @@ -0,0 +1,50 @@ +/* + * 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; + +/** + * Lets a caller distinguish a failed NCBI request from a search that found nothing. + * searchForPublication throws it when no publication was found and at least one request failed. + */ +public class NcbiSearchException extends RuntimeException +{ + private final boolean _allRequestsFailed; + + public NcbiSearchException(String message, Throwable cause) + { + super(message, cause); + _allRequestsFailed = false; + } + + public NcbiSearchException(String message) + { + this(message, false); + } + + public NcbiSearchException(String message, boolean allRequestsFailed) + { + super(message); + _allRequestsFailed = allRequestsFailed; + } + + /** + * @return true if every NCBI request for the search failed, which suggests NCBI is unavailable. + */ + public boolean isAllRequestsFailed() + { + return _allRequestsFailed; + } +} diff --git a/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java b/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java index 4b9d8ed6..00642a79 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java +++ b/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java @@ -16,9 +16,13 @@ package org.labkey.panoramapublic.pipeline; import org.apache.commons.lang3.StringUtils; +import org.apache.logging.log4j.Level; import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.core.config.Configurator; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.Assert; +import org.junit.Test; import org.labkey.api.announcements.api.Announcement; import org.labkey.api.announcements.api.AnnouncementService; import org.labkey.api.data.Container; @@ -32,6 +36,7 @@ import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.StringUtilsLabKey; import org.labkey.api.util.URLHelper; +import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.ViewBackgroundInfo; import org.labkey.panoramapublic.PanoramaPublicManager; import org.labkey.panoramapublic.PanoramaPublicNotification; @@ -39,8 +44,11 @@ import org.labkey.panoramapublic.model.DatasetStatus; import org.labkey.panoramapublic.model.ExperimentAnnotations; import org.labkey.panoramapublic.model.Journal; +import org.labkey.panoramapublic.model.JournalExperiment; 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; @@ -59,6 +67,21 @@ public class PrivateDataReminderJob extends PipelineJob { + private static final int MIN_DATASETS_FOR_FAILURE = 3; + private static final double PUBLICATION_SEARCH_FAILURE_THRESHOLD = 0.5; + // Consecutive datasets with all NCBI requests failed before the search is stopped for the run. + private static final int MAX_CONSECUTIVE_ALL_FAILED = 3; + + /** + * @return true when the publication search failure rate reaches the threshold, which suggests a + * problem with searching NCBI rather than with one dataset. + */ + static boolean publicationSearchFailingWidely(int failed, int attempted) + { + return attempted >= MIN_DATASETS_FOR_FAILURE + && failed >= attempted * PUBLICATION_SEARCH_FAILURE_THRESHOLD; + } + private boolean _test; private boolean _forcePublicationCheck; private List _experimentAnnotationsIds; @@ -187,22 +210,24 @@ public static ReminderDecision skip(String reason) { public boolean shouldPost() { return shouldPost; } public String getReason() { return reason; } } - + /** - * Checks for publications associated with the experiment if enabled in settings - * @param expAnnotations The experiment to check - * @param settings The reminder settings - * @param forceCheck Force publication check regardless of global setting - * @param log Logger for diagnostic messages - * @return NcbiArticleMatch with search result + * Finds the publication to report for the experiment, from the dataset's cached match or by + * searching NCBI. + * + * @return null when there is nothing new to report, which includes a search that found only the + * publication the submitter has already dismissed + * @throws NcbiSearchException if a search ran and could not complete */ private PublicationMatch searchForPublication(@NotNull ExperimentAnnotations expAnnotations, @NotNull PrivateDataReminderSettings settings, boolean forceCheck, @NotNull User user, boolean testMode, - @NotNull Logger log) + @NotNull ProcessingResults results) { + Logger log = results._log; + // Check if publication checking is enabled (either globally or forced for this run) if (!forceCheck && !settings.isEnablePublicationSearch()) { @@ -224,11 +249,21 @@ private PublicationMatch searchForPublication(@NotNull ExperimentAnnotations exp return null; } + if (results.isPublicationSearchStopped()) + { + // The search deferral is not reset, so the next run searches again. + log.info("Publication search is stopped for this run. Not re-searching for experiment {}", expAnnotations.getId()); + results.addPublicationSearchSkipped(expAnnotations.getId()); + return null; + } + // Search deferral expired — re-search NCBI log.info("Search deferral expired for experiment {} (dismissed {}); re-searching NCBI", expAnnotations.getId(), dismissedDate); try { + results.addPublicationSearchAttempted(); PublicationMatch newMatch = NcbiPublicationSearchService.get().searchForPublication(expAnnotations, log); + results.addPublicationSearchCompleted(); if (newMatch != null && !newMatch.getPublicationId().equals(datasetStatus.getPotentialPublicationId())) { // Different publication found — return it (caller will save and notify) @@ -251,9 +286,15 @@ private PublicationMatch searchForPublication(@NotNull ExperimentAnnotations exp return null; } } + catch (NcbiSearchException e) + { + // Rethrow so that the caller can record this as a failed NCBI search. + throw e; + } catch (Exception e) { - log.error("Error re-searching publication for experiment {}: {}", expAnnotations.getId(), e.getMessage(), e); + // The reminder is still posted, so this costs the paper information and nothing else. + log.warn("Error re-searching publication for experiment {}: {}", expAnnotations.getId(), e.getMessage(), e); return null; } } @@ -266,15 +307,31 @@ private PublicationMatch searchForPublication(@NotNull ExperimentAnnotations exp } } + if (results.isPublicationSearchStopped()) + { + log.info("Publication search is stopped for this run. Not searching for experiment {}", expAnnotations.getId()); + results.addPublicationSearchSkipped(expAnnotations.getId()); + return null; + } + // Perform the publication search log.info("Searching for publications for experiment {}", expAnnotations.getId()); try { - return NcbiPublicationSearchService.get().searchForPublication(expAnnotations, log); + results.addPublicationSearchAttempted(); + PublicationMatch match = NcbiPublicationSearchService.get().searchForPublication(expAnnotations, log); + results.addPublicationSearchCompleted(); + return match; + } + catch (NcbiSearchException e) + { + // Rethrow so that the caller can record this as a failed NCBI search. + throw e; } catch (Exception e) { - log.error("Error searching for publication for experiment {}: {}", expAnnotations.getId(), e.getMessage(), e); + // The reminder is still posted, so this costs the paper information and nothing else. + log.warn("Error searching for publication for experiment {}: {}", expAnnotations.getId(), e.getMessage(), e); return null; } } @@ -287,21 +344,64 @@ public void run() if (_panoramaPublic == null) { getLogger().error("Panorama Public project does not exist."); + setStatus(TaskStatus.error); return; } - postMessage(_experimentAnnotationsIds, _panoramaPublic); + setStatus(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, getLogger()); + 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) + /** + * @return error when the job could not start, a dataset that should have been sent a reminder was + * not, or the publication search failed for half or more of the datasets it ran for or was stopped. Cancelled + * when the job was cancelled with nothing else to report, and complete otherwise. + */ + private TaskStatus postMessage(List expAnnotationIds, Journal panoramaPublic) { int total = expAnnotationIds.size(); if (total == 0) { getLogger().info("No private datasets were found."); - return; + return TaskStatus.complete; + } + if (!ncbiApiKeyAccepted()) + { + return TaskStatus.error; } Logger log = getLogger(); @@ -309,15 +409,26 @@ private void postMessage(List expAnnotationIds, Journal panoramaPublic) if(!context.isValid()) { context.logErrors(log); - return; + return TaskStatus.error; } ProcessingResults processingResults = new ProcessingResults(expAnnotationIds.size(), log); - processExperiments(_experimentAnnotationsIds, context, processingResults, log); + boolean completed = processExperiments(expAnnotationIds, context, processingResults, log); + // An ERROR logged through the job's logger sets the status to error, and the status set here + // would overwrite it. Report the errors the job recorded instead. + if (processingResults.getTotalErrors() > 0 || processingResults.publicationSearchFailingWidely() + || processingResults.isPublicationSearchStopped()) + { + return TaskStatus.error; + } + return completed ? TaskStatus.complete : TaskStatus.cancelled; } - private void processExperiments(List expAnnotationIds, ProcessingContext context, ProcessingResults processingResults, Logger log) + /** + * @return false if the job was cancelled before every dataset was processed. + */ + private boolean processExperiments(List expAnnotationIds, ProcessingContext context, ProcessingResults processingResults, Logger log) { log.info("Posting reminder message to: {} message threads.", expAnnotationIds.size()); @@ -326,20 +437,29 @@ private void processExperiments(List expAnnotationIds, ProcessingContex { log.info("RUNNING IN TEST MODE - MESSAGES WILL NOT BE POSTED."); } + boolean completed = true; for (Integer experimentAnnotationsId : exptIds) { - try (DbScope.Transaction transaction = PanoramaPublicManager.getSchema().getScope().ensureTransaction()) + if (checkInterrupted() // checkInterrupted is set by Cancel in the pipeline UI. + || Thread.currentThread().isInterrupted()) + { + log.warn("Job was cancelled. Stopping before experiment {}.", experimentAnnotationsId); + completed = false; + break; + } + + try { processExperiment(experimentAnnotationsId, context, processingResults); - transaction.commit(); } catch (Exception e) { - log.error("Error processing experiment {}: {}", experimentAnnotationsId, e.getMessage(), e); + processingResults.addProcessingFailed(experimentAnnotationsId, e); } } processingResults.logResults(log); + return completed; } private void processExperiment(Integer experimentAnnotationsId, ProcessingContext context, ProcessingResults processingResults) @@ -384,14 +504,26 @@ 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. Send a reminder even if the search fails for any reason. + PublicationMatch publicationResult = null; + try + { + publicationResult = searchForPublication(expAnnotations, context.getSettings(), _forcePublicationCheck, getUser(), context.isTestMode(), processingResults); + } + catch (NcbiSearchException e) + { + processingResults.addPublicationSearchFailed(experimentAnnotationsId, e); + } if (!context.isTestMode()) { - postReminderMessage(expAnnotations, submission, announcement, submitter, publicationResult, context); - - updateDatasetStatus(expAnnotations, publicationResult); + // The NCBI requests above run outside the transaction, so it is not held open while they wait. + try (DbScope.Transaction transaction = PanoramaPublicManager.getSchema().getScope().ensureTransaction()) + { + postReminderMessage(expAnnotations, submission, announcement, submitter, publicationResult, context); + updateDatasetStatus(expAnnotations, publicationResult); + transaction.commit(); + } } processingResults.addProcessed(expAnnotations, announcement); @@ -476,6 +608,12 @@ public String getDescription() return "Post private data reminder messages"; } + @Override + protected boolean canInterrupt() + { + return true; + } + private static class ProcessingContext { private final PrivateDataReminderSettings _reminderSettings; @@ -661,7 +799,14 @@ private static class ProcessingResults private final List _experimentNotFound = new ArrayList<>(); private final List _submissionNotFound = new ArrayList<>(); private final List _announcementNotFound = new ArrayList<>(); + private final List _noSupportThread = new ArrayList<>(); private final List _submitterNotFound = new ArrayList<>(); + private final List _publicationSearchFailed = new ArrayList<>(); + private int _publicationSearchAttempted = 0; + private int _consecutiveAllFailed = 0; + private boolean _publicationSearchStopped = false; + private final List _publicationSearchSkipped = new ArrayList<>(); + private final List _processingFailed = new ArrayList<>(); private final List _skipped = new ArrayList<>(); private int _processed = 0; private final int _total; @@ -692,6 +837,13 @@ public void addLatestSubmissionNotFound(Integer experimentId) public void addAnnouncementNotFound(Integer experimentId, JournalSubmission submission, Container announcementsFolder) { + if (submission.getAnnouncementId() == null) + { + // Data submitted before Panorama Public started posting submission requests to a + // message board. + _noSupportThread.add(experimentId); + return; + } _announcementNotFound.add(experimentId); _log.error("Could not find the message thread for experiment Id: {}; announcement Id: {} in the folder {}.", experimentId, submission.getAnnouncementId(), announcementsFolder.getPath()); } @@ -702,6 +854,47 @@ public void addSubmitterNotFound(Integer experimentId) _log.error("Could not find a submitter user for experiment Id: {}.", experimentId); } + public void addPublicationSearchAttempted() + { + _publicationSearchAttempted++; + } + + public void addPublicationSearchCompleted() + { + _consecutiveAllFailed = 0; + } + + public void addPublicationSearchFailed(Integer experimentId, NcbiSearchException e) + { + _publicationSearchFailed.add(experimentId); + _log.warn("Publication search failed for experiment Id: {}. A reminder was still posted. {}", experimentId, e.getMessage(), e); + + // Some requests completing means NCBI is responding, so the failure is intermittent. + _consecutiveAllFailed = e.isAllRequestsFailed() ? _consecutiveAllFailed + 1 : 0; + if (!_publicationSearchStopped && _consecutiveAllFailed >= MAX_CONSECUTIVE_ALL_FAILED) + { + _publicationSearchStopped = true; + _log.error("Every NCBI request failed for {} datasets in a row. The publication search is stopped for the rest of this run. Reminders are still posted.", + _consecutiveAllFailed); + } + } + + public boolean isPublicationSearchStopped() + { + return _publicationSearchStopped; + } + + public void addPublicationSearchSkipped(Integer experimentId) + { + _publicationSearchSkipped.add(experimentId); + } + + public void addProcessingFailed(Integer experimentId, Exception e) + { + _processingFailed.add(experimentId); + _log.error("Error processing experiment {}: {}", experimentId, e.getMessage(), e); + } + public void addSkipped(Integer experimentId, ReminderDecision decision) { _skipped.add(experimentId); @@ -739,6 +932,37 @@ public void logSkipped(Logger log) log.error("Support message threads were not found for the following experiment Ids: {}", StringUtils.join(_announcementNotFound, ", ")); } + if (!_noSupportThread.isEmpty()) + { + log.warn("The following experiment Ids were submitted before Panorama Public posted submission requests to a message board, so they have no support message thread and cannot be sent a reminder: {}", + StringUtils.join(_noSupportThread, ", ")); + } + + if (!_publicationSearchFailed.isEmpty()) + { + String message = "Publication search failed for {} of the {} datasets a search was run for. Experiment Ids: {}. The NCBI requests that failed are logged as warnings above."; + if (publicationSearchFailingWidely()) + { + log.error(message, _publicationSearchFailed.size(), _publicationSearchAttempted, StringUtils.join(_publicationSearchFailed, ", ")); + } + else + { + log.warn(message, _publicationSearchFailed.size(), _publicationSearchAttempted, StringUtils.join(_publicationSearchFailed, ", ")); + } + } + + if (_publicationSearchStopped) + { + log.error("The publication search was stopped after every NCBI request failed for {} datasets in a row. Reminders were still posted. Experiment Ids not searched: {}", + MAX_CONSECUTIVE_ALL_FAILED, + _publicationSearchSkipped.isEmpty() ? "none" : StringUtils.join(_publicationSearchSkipped, ", ")); + } + + if (!_processingFailed.isEmpty()) + { + log.error("Processing failed for the following experiment Ids: {}", StringUtils.join(_processingFailed, ", ")); + } + if (!_submitterNotFound.isEmpty()) { log.error("Submitter user was not found for the following experiment Ids: {}", StringUtils.join(_submitterNotFound, ", ")); @@ -750,12 +974,23 @@ public void logSkipped(Logger log) } } + public boolean publicationSearchFailingWidely() + { + return PrivateDataReminderJob.publicationSearchFailingWidely( + _publicationSearchFailed.size(), _publicationSearchAttempted); + } + + /** + * @return the number of datasets that should have been sent a reminder and were not. Datasets + * whose publication search failed are not counted, because the reminder was still posted. + */ public int getTotalErrors() { return _experimentNotFound.size() + _submissionNotFound.size() + _announcementNotFound.size() + - _submitterNotFound.size(); + _submitterNotFound.size() + + _processingFailed.size(); } public void logSummary(Logger log) { @@ -769,7 +1004,110 @@ public void logSummary(Logger log) log.info("Successfully processed {}.", StringUtilsLabKey.pluralize(_processed, "experiment")); } - log.info("Processing complete: {} total, {} processed, {} skipped, {} errors", _total, _processed, _skipped.size(), getTotalErrors()); + log.info("Processing complete: {} total, {} processed, {} skipped, {} with no support message thread, {} errors", + _total, _processed, _skipped.size(), _noSupportThread.size(), getTotalErrors()); + } + } + + public static class TestCase extends Assert + { + private static final Logger TEST_LOG = LogHelper.getLogger(TestCase.class, "Private data reminder job tests"); + + static + { + // The tests record failures on purpose. Keep their ERROR lines out of the server log. + Configurator.setLevel(TEST_LOG.getName(), Level.OFF); + } + + @Test + public void testPublicationSearchFailingWidely() + { + // Below the floor the rate says too little to act on, even when every search failed. + assertFalse("2 of 2 is under the floor", publicationSearchFailingWidely(2, 2)); + + // At the floor, half or more of the searches that ran. + assertTrue("2 of 3 is over half", publicationSearchFailingWidely(2, 3)); + assertFalse("1 of 3 is under half", publicationSearchFailingWidely(1, 3)); + + // Exactly half is enough. + assertTrue("2 of 4 is exactly half", publicationSearchFailingWidely(2, 4)); + assertFalse("1 of 4 is under half", publicationSearchFailingWidely(1, 4)); + + assertFalse("No failures", publicationSearchFailingWidely(0, 10)); + assertFalse("No searches ran", publicationSearchFailingWidely(0, 0)); + } + + @Test + public void testGetTotalErrors() + { + ProcessingResults results = new ProcessingResults(10, TEST_LOG); + assertEquals("A run with nothing recorded has no errors", 0, results.getTotalErrors()); + + results.addExperimentNotFound(1); + results.addSubmissionNotFound(2); + results.addLatestSubmissionNotFound(3); + results.addSubmitterNotFound(4); + results.addProcessingFailed(5, new IllegalStateException("test")); + assertEquals("Each dataset that missed its reminder is counted once", 5, results.getTotalErrors()); + + // The reminder was still posted for these, so they are not errors. + results.addPublicationSearchFailed(6, new NcbiSearchException("test")); + assertEquals("A failed publication search is not a missed reminder", 5, results.getTotalErrors()); + } + + @Test + public void testPublicationSearchStopped() + { + ProcessingResults results = new ProcessingResults(10, TEST_LOG); + + // Two datasets in a row with every request failed, then a search that completed. + results.addPublicationSearchFailed(1, allRequestsFailed()); + results.addPublicationSearchFailed(2, allRequestsFailed()); + results.addPublicationSearchCompleted(); + results.addPublicationSearchFailed(3, allRequestsFailed()); + assertFalse("A completed search should reset the count", results.isPublicationSearchStopped()); + + // A partial failure resets the count. + results.addPublicationSearchFailed(4, allRequestsFailed()); + results.addPublicationSearchFailed(5, new NcbiSearchException("test", false)); + results.addPublicationSearchFailed(6, allRequestsFailed()); + assertFalse("A search where some requests completed should reset the count", results.isPublicationSearchStopped()); + + results.addPublicationSearchFailed(7, allRequestsFailed()); + assertFalse("Two in a row should not stop the search", results.isPublicationSearchStopped()); + results.addPublicationSearchFailed(8, allRequestsFailed()); + assertTrue("Three in a row with every request failed should stop the search", results.isPublicationSearchStopped()); + + // Stopping the search sets the status, not the error count. + assertEquals("Stopping the search should not count as a missed reminder", 0, results.getTotalErrors()); + } + + private static NcbiSearchException allRequestsFailed() + { + return new NcbiSearchException("test", true); + } + + @Test + public void testAnnouncementNotFoundSplitsOnAnnouncementId() + { + ProcessingResults results = new ProcessingResults(10, TEST_LOG); + Container container = ContainerManager.getRoot(); + + // No announcement id means the data predates the support message board. It cannot be sent + // a reminder and is not an error. + results.addAnnouncementNotFound(1, submissionWithAnnouncementId(null), container); + assertEquals("A dataset that never had a thread is not an error", 0, results.getTotalErrors()); + + // An announcement id whose thread could not be read is worth investigating. + results.addAnnouncementNotFound(2, submissionWithAnnouncementId(99), container); + assertEquals("A thread that went missing is an error", 1, results.getTotalErrors()); + } + + private static JournalSubmission submissionWithAnnouncementId(Integer announcementId) + { + JournalExperiment journalExperiment = new JournalExperiment(); + journalExperiment.setAnnouncementId(announcementId); + return new JournalSubmission(journalExperiment); } } } diff --git a/panoramapublic/src/org/labkey/panoramapublic/view/privateDataRemindersSettingsForm.jsp b/panoramapublic/src/org/labkey/panoramapublic/view/privateDataRemindersSettingsForm.jsp index cf5a738c..7249d894 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/view/privateDataRemindersSettingsForm.jsp +++ b/panoramapublic/src/org/labkey/panoramapublic/view/privateDataRemindersSettingsForm.jsp @@ -16,6 +16,7 @@ */ %> <%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib" %> +<%@ page import="org.labkey.api.security.Encryption" %> <%@ page import="org.labkey.api.view.HttpView" %> <%@ page import="org.labkey.api.view.JspView" %> <%@ page import="org.labkey.api.view.template.ClientDependencies" %> @@ -81,6 +82,57 @@ } 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"; + // NCBI's response 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 requests a check of 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,32 @@ + + + <%=h(PrivateDataReminderSettings.PROP_NCBI_API_KEY)%> + + + <% if (!Encryption.isEncryptionPassPhraseSpecified()) { %> +
<%=h(PrivateDataReminderSettings.NCBI_API_KEY_REQUIRES_ENCRYPTION)%>
+ <% } else { %> + " /> + <%=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. + <% if (form.isNcbiApiKeySet()) { %> +
+ + <% } %> +
+ <% } %> + + <%=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..115dcd59 --- /dev/null +++ b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/NcbiApiKeyTest.java @@ -0,0 +1,176 @@ +/* + * 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.categories.External; +import org.labkey.test.categories.MacCossLabModules; + +import java.io.IOException; +import java.util.Map; + +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"; + // MockNcbiPublicationSearchService.REJECTED_API_KEY. The mock responds to it with a 400. + private static final String REJECTED_API_KEY = "mock-rejected-ncbi-api-key"; + // MockNcbiPublicationSearchService.UNCHECKED_API_KEY. The mock responds to it with a 503. + private static final String UNCHECKED_API_KEY = "mock-unchecked-ncbi-api-key"; + + private boolean _useMockNcbi = false; + private Map _originalReminderSettings; + + @Test + public void testNcbiApiKeySettings() + { + setupMockNcbiService(); + + // Capture the existing reminder settings up front so restoreAfterTest can put them back. A dev + // machine may have non-default values set. + _originalReminderSettings = getPrivateDataReminderSettings(); + + 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", _originalReminderSettings.get("ncbiApiKeySaved")); + + verifyKeyNotSaved(REJECTED_API_KEY, "NCBI rejected this API key, so it was not saved."); + verifyKeyNotSaved(UNCHECKED_API_KEY, "Could not check this API key with NCBI, so it was not saved."); + + 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"); + assertEquals("Remove the saved key should remove it", "false", + getPrivateDataReminderSettings().get("ncbiApiKeySaved")); + assertElementNotPresent("Remove the saved key should be offered only when a key is saved", + Locator.checkboxByName("clearNcbiApiKey")); + } + + /** + * Save checks a new key with NCBI. A key NCBI has not accepted should not be saved, and the form should + * display the reason. + */ + private void verifyKeyNotSaved(String apiKey, String expectedMessage) + { + getPrivateDataReminderSettings(); + setFormElement(Locator.input("ncbiApiKey"), apiKey); + clickButton("Save"); + assertTextPresent(expectedMessage); + assertEquals("A key NCBI has not accepted should not be saved", "false", + getPrivateDataReminderSettings().get("ncbiApiKeySaved")); + } + + /** + * Covers the button, the request it sends and the message it displays for a key the mock accepts and + * a key it rejects. + */ + private void verifyValidateButton() + { + Locator result = Locator.id("ncbiApiKeyValidationResult"); + + setFormElement(Locator.input("ncbiApiKey"), "not-a-real-key"); + click(Locator.lkButton("Validate")); + waitForElement(result.containing("NCBI accepted this key")); + + setFormElement(Locator.input("ncbiApiKey"), REJECTED_API_KEY); + click(Locator.lkButton("Validate")); + waitForElement(result.containing("NCBI rejected this key")); + + setFormElement(Locator.input("ncbiApiKey"), ""); + } + + /* + * Route NCBI requests through the mock, so the test does not depend on NCBI being reachable and a key + * can be rejected on demand. + */ + private void setupMockNcbiService() + { + 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 restoreAfterTest() + { + if (_useMockNcbi) + { + restoreNcbiService(); + } + + if (_originalReminderSettings != null) + { + // The reminder settings are site wide. Restore the values this test overwrote, and remove any key the + // test saved. A key that was saved before the test cannot be read back, so it is left in place. + restorePrivateDataReminderSettings(_originalReminderSettings, + "false".equals(_originalReminderSettings.get("ncbiApiKeySaved"))); + } + } +} 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..1220dd69 100644 --- a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicBaseTest.java +++ b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicBaseTest.java @@ -16,6 +16,7 @@ package org.labkey.test.tests.panoramapublic; import org.apache.commons.collections4.CollectionUtils; +import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.SystemUtils; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -26,6 +27,7 @@ import org.labkey.remoteapi.CommandResponse; import org.labkey.remoteapi.Connection; import org.labkey.remoteapi.SimpleGetCommand; +import org.labkey.remoteapi.SimplePostCommand; import org.labkey.test.Locator; import org.labkey.test.TestFileUtils; import org.labkey.test.TestTimeoutException; @@ -631,7 +633,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 +647,11 @@ 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. data-key-saved on the field reports whether one is stored. + // The field is not rendered on a server with no encryption key, where no key can be saved. + settings.put("ncbiApiKeySaved", Locator.input("ncbiApiKey").findOptionalElement(getDriver()) + .map(field -> field.getDomAttribute("data-key-saved")) + .orElse("false")); return settings; } @@ -654,6 +661,16 @@ protected void savePrivateDataReminderSettings(String extensionLength, String de } protected void savePrivateDataReminderSettings(String extensionLength, String delayUntilFirstReminder, String reminderFrequency, boolean enablePublicationSearch) + { + savePrivateDataReminderSettings(extensionLength, delayUntilFirstReminder, reminderFrequency, enablePublicationSearch, null); + } + + /** + * Saves the site wide Private Data Reminder Settings. A null ncbiApiKey leaves the key field blank, + * which keeps the key already saved on the server. The page never displays a saved key, so a test + * restoring settings it captured earlier cannot restore the key and should pass null. + */ + protected void savePrivateDataReminderSettings(String extensionLength, String delayUntilFirstReminder, String reminderFrequency, boolean enablePublicationSearch, String ncbiApiKey) { goToAdminConsole().goToSettingsSection(); clickAndWait(Locator.linkWithText("Panorama Public")); @@ -662,6 +679,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 +698,40 @@ 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 (!StringUtils.isBlank(ncbiApiKey)) + { + assertEquals("The form should report that a key is saved", "true", + getPrivateDataReminderSettings().get("ncbiApiKeySaved")); + } + } + + /** + * Restores the settings read by getPrivateDataReminderSettings through an API call, which loads no page. For + * use in an @After method, where loading a page would replace the failed page in the failure screenshot. + * @param clearNcbiApiKey true to remove the saved NCBI API key + */ + protected void restorePrivateDataReminderSettings(Map original, boolean clearNcbiApiKey) + { + Map params = new HashMap<>(); + params.put("extensionLength", original.get("extensionLength")); + params.put("delayUntilFirstReminder", original.get("delayUntilFirstReminder")); + params.put("reminderFrequency", original.get("reminderFrequency")); + params.put("enablePublicationSearch", original.get("enablePublicationSearch")); + params.put("publicationSearchFrequency", original.get("publicationSearchFrequency")); + params.put("clearNcbiApiKey", clearNcbiApiKey); + + SimplePostCommand command = new SimplePostCommand("panoramapublic", "restorePrivateDataReminderSettings"); + command.setParameters(params); + try + { + command.execute(createDefaultConnection(), "/"); + } + catch (IOException | CommandException e) + { + throw new RuntimeException("Failed to restore the Private Data Reminder Settings", e); + } } protected void goToSendRemindersPage(String projectName) diff --git a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PrivateDataReminderTest.java b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PrivateDataReminderTest.java index d677ffdd..326fa588 100644 --- a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PrivateDataReminderTest.java +++ b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PrivateDataReminderTest.java @@ -15,6 +15,7 @@ */ package org.labkey.test.tests.panoramapublic; +import org.junit.After; import org.junit.Test; import org.junit.experimental.categories.Category; import org.labkey.test.BaseWebDriverTest; @@ -27,6 +28,7 @@ import org.labkey.test.util.DataRegionTable; import java.util.List; +import java.util.Map; import static org.junit.Assert.assertEquals; import static org.labkey.test.util.PermissionsHelper.READER_ROLE; @@ -49,10 +51,14 @@ public class PrivateDataReminderTest extends PanoramaPublicBaseTest private static final String DELETION_MESSAGE_TITLE = "Title: Data Deletion Requested - "; private static final String MESSAGE_PAGE_TITLE = "Submitted - "; + private Map _originalReminderSettings; @Test public void testPrivateDataReminder() { + // Capture the site wide reminder settings so restoreReminderSettings can put them back. + _originalReminderSettings = getPrivateDataReminderSettings(); + String panoramaPublicProject = PANORAMA_PUBLIC; goToProjectHome(panoramaPublicProject); ApiPermissionsHelper permissionsHelper = new ApiPermissionsHelper(this); @@ -372,6 +378,15 @@ private void postReminders(String projectName, boolean testMode, int expectedExp } + @After + public void restoreReminderSettings() + { + if (_originalReminderSettings != null) + { + restorePrivateDataReminderSettings(_originalReminderSettings, false); + } + } + @Override protected void doCleanup(boolean afterTest) throws TestTimeoutException { 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..cdc4d8c2 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 @@ -482,11 +484,7 @@ public void resetAfterTest() if (_originalReminderSettings != null) { - savePrivateDataReminderSettings( - _originalReminderSettings.get("extensionLength"), - _originalReminderSettings.get("delayUntilFirstReminder"), - _originalReminderSettings.get("reminderFrequency"), - Boolean.parseBoolean(_originalReminderSettings.get("enablePublicationSearch"))); + restorePrivateDataReminderSettings(_originalReminderSettings, false); } }