From e35411c8feb816fbda7a34b3aa9e798673f220d5 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sat, 13 Jun 2026 20:55:08 -0700 Subject: [PATCH 01/12] Fixed Panorama Public reminder scheduling and hardened NCBI publication search - PanoramaPublicModule.startupAfterSpringConfig now re-establishes the daily reminder's Quartz schedule on startup; it was only set when an admin saved the settings form, so a Tomcat restart silently killed the job (Quartz's in-memory job store does not survive a JVM restart). * Added an "NCBI API key" admin field wired into buildCommonParams as &api_key= (raises NCBI rate limit 3->10 req/sec); contextual Logger threaded through getString/getJson so retry warnings follow the job log vs. server log. - NcbiPublicationSearchServiceImpl.getString retries eutils calls (3 attempts, 500/1000ms backoff) on 5xx and read timeouts (~40% transient failure rate); 4xx fails fast, with the NCBI response body included so a bad key's "API key invalid" reaches the log instead of a bare 400. - Added JUnit (retry/backoff, api_key, 4xx body) and Selenium API-key coverage. Follow-up to PR #606. --- .../PanoramaPublicController.java | 13 ++ .../panoramapublic/PanoramaPublicModule.java | 4 + .../message/PrivateDataReminderSettings.java | 15 ++ .../MockNcbiPublicationSearchService.java | 5 +- .../NcbiPublicationSearchServiceImpl.java | 191 ++++++++++++++++-- .../view/privateDataRemindersSettingsForm.jsp | 13 ++ .../PanoramaPublicBaseTest.java | 19 +- .../panoramapublic/PublicationSearchTest.java | 40 +++- 8 files changed, 277 insertions(+), 23 deletions(-) diff --git a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java index cef3ae75..a9dc4619 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java +++ b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java @@ -10146,6 +10146,7 @@ public ModelAndView getView(PrivateDataReminderSettingsForm form, boolean reshow form.setExtensionLength(settings.getExtensionLength()); form.setEnablePublicationSearch(settings.isEnablePublicationSearch()); form.setPublicationSearchFrequency(settings.getPublicationSearchFrequency()); + form.setNcbiApiKey(settings.getNcbiApiKey()); } VBox view = new VBox(); @@ -10166,6 +10167,7 @@ public boolean handlePost(PrivateDataReminderSettingsForm form, BindException er settings.setExtensionLength(form.getExtensionLength()); settings.setEnablePublicationSearch(form.isEnablePublicationSearch()); settings.setPublicationSearchFrequency(form.getPublicationSearchFrequency()); + settings.setNcbiApiKey(form.getNcbiApiKey()); PrivateDataReminderSettings.save(settings); PrivateDataMessageScheduler.getInstance().initialize(settings.isEnableReminders()); @@ -10205,6 +10207,7 @@ public static class PrivateDataReminderSettingsForm private Integer _delayUntilFirstReminder; private boolean _enablePublicationSearch; private Integer _publicationSearchFrequency; + private String _ncbiApiKey; public boolean isEnabled() { @@ -10275,6 +10278,16 @@ public void setPublicationSearchFrequency(Integer publicationSearchFrequency) { _publicationSearchFrequency = publicationSearchFrequency; } + + public String getNcbiApiKey() + { + return _ncbiApiKey; + } + + public void setNcbiApiKey(String ncbiApiKey) + { + _ncbiApiKey = ncbiApiKey; + } } @RequiresPermission(AdminOperationsPermission.class) diff --git a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicModule.java b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicModule.java index 1c9ea49b..c7df83fb 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicModule.java +++ b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicModule.java @@ -46,6 +46,7 @@ import org.labkey.panoramapublic.bluesky.BlueskyApiClient; import org.labkey.panoramapublic.bluesky.PanoramaPublicLogoResourceType; import org.labkey.panoramapublic.catalog.CatalogImageAttachmentType; +import org.labkey.panoramapublic.message.PrivateDataMessageScheduler; import org.labkey.panoramapublic.message.PrivateDataReminderSettings; import org.labkey.panoramapublic.ncbi.NcbiPublicationSearchServiceImpl; import org.labkey.panoramapublic.model.Journal; @@ -151,6 +152,9 @@ protected void startupAfterSpringConfig(ModuleContext moduleContext) { fileContentService.addFileListener(new PanoramaPublicFileListener()); } + + // Start the private data reminder job on server restart if it is enabled. + PrivateDataMessageScheduler.getInstance().initialize(PrivateDataReminderSettings.get().isEnableReminders()); } @NotNull diff --git a/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java b/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java index a75b411c..42cfd43b 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java +++ b/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java @@ -41,6 +41,7 @@ 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"; private static final boolean DEFAULT_ENABLE_REMINDERS = false; public static final String DEFAULT_REMINDER_TIME = "8:00 AM"; @@ -61,6 +62,7 @@ public class PrivateDataReminderSettings private int _extensionLength; private boolean _enablePublicationSearch; private int _publicationSearchFrequency; + private String _ncbiApiKey; public static PrivateDataReminderSettings get() { @@ -101,6 +103,8 @@ public static PrivateDataReminderSettings get() ? DEFAULT_PUBLICATION_SEARCH_FREQUENCY : Integer.valueOf(settingsMap.get(PROP_PUBLICATION_SEARCH_FREQUENCY)); settings.setPublicationSearchFrequency(publicationSearchFrequency); + + settings.setNcbiApiKey(settingsMap.get(PROP_NCBI_API_KEY)); } else { @@ -147,6 +151,7 @@ public static void save(PrivateDataReminderSettings settings) settingsMap.put(PROP_REMINDER_TIME, settings.getReminderTimeFormatted()); settingsMap.put(PROP_ENABLE_PUBLICATION_SEARCH, String.valueOf(settings.isEnablePublicationSearch())); settingsMap.put(PROP_PUBLICATION_SEARCH_FREQUENCY, String.valueOf(settings.getPublicationSearchFrequency())); + settingsMap.put(PROP_NCBI_API_KEY, settings.getNcbiApiKey() != null ? settings.getNcbiApiKey() : ""); settingsMap.save(); } @@ -225,6 +230,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()); diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java index b32c5864..b9995737 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java @@ -15,6 +15,7 @@ */ package org.labkey.panoramapublic.ncbi; +import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.Nullable; import org.json.JSONArray; import org.json.JSONObject; @@ -28,7 +29,7 @@ /** * 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)}, + * Extends {@link NcbiPublicationSearchServiceImpl} and only overrides {@link #getString(String, Logger)}, * 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. * Tests register mock articles via {@link #register}, providing the database, ID, search key, @@ -132,7 +133,7 @@ public void register(String database, String id, String searchKey, * Handles ESearch, ESummary, and Citation Exporter URLs. */ @Override - protected String getString(String url) throws IOException + protected String getString(String url, Logger log) throws IOException { if (url.contains("esearch.fcgi")) { diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java index e6b0bae3..3f6fd727 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java @@ -38,10 +38,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; @@ -95,6 +97,11 @@ public static void setInstance(NcbiPublicationSearchService impl) private static final int RATE_LIMIT_DELAY_MS = 400; // NCBI allows 3 requests/sec private static final int TIMEOUT_MS = 10000; // 10 seconds + // NCBI eutils intermittently returns transient 5xx errors and read timeouts, even well under + // the rate limit. Retry those a few times with exponential backoff before giving up. + private static final int MAX_HTTP_ATTEMPTS = 3; // initial try + 2 retries + private static final int RETRY_BASE_DELAY_MS = 500; // exponential backoff base + // NCBI suggests using the 'tool' and 'email' parameters on E-utilities URLs // https://www.nlm.nih.gov/dataguide/eutilities/utilities.html private static final String TOOL = "PanoramaPublic"; @@ -156,7 +163,7 @@ private static Logger getLog(@Nullable Logger logger) try { - String response = getString(queryUrl); + String response = getString(queryUrl, log); return parseCitation(response, publicationId, database, log); } catch (IOException e) @@ -326,7 +333,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"); @@ -349,16 +356,42 @@ private List executeSearch(String query, String database, Logger log) * @throws IOException if the request fails or the server returns a non-2xx response * @throws JSONException if the response body is not valid JSON */ - protected JSONObject getJson(String url) throws IOException + protected JSONObject getJson(String url, Logger log) throws IOException { - return new JSONObject(getString(url)); + return new JSONObject(getString(url, log)); } /** - * Execute an HTTP GET request and return the response body as a string. + * Execute an HTTP GET request and return the response body as a string. Retry warnings are + * written to {@code log} so they land in the pipeline job log when invoked from the reminder + * job (and in the server log for UI-triggered searches, which pass the static logger). * @throws IOException if the request fails or the server returns a non-2xx response */ - protected String getString(String url) throws IOException + protected String getString(String url, Logger log) throws IOException + { + // Retry transient NCBI failures (5xx, read timeouts) with exponential backoff; + // other failures (e.g. 4xx) are permanent and fail fast. + for (int attempt = 1; ; attempt++) + { + try + { + return executeGet(url); + } + catch (IOException e) + { + if (attempt >= MAX_HTTP_ATTEMPTS || !isRetryable(e)) + { + throw e; + } + long delayMs = retryDelayMs(attempt); + getLog(log).warn("NCBI request failed (attempt {} of {}); retrying in {} ms. URL: {}; cause: {}", + attempt, MAX_HTTP_ATTEMPTS, delayMs, url, e.toString()); + sleepMs(delayMs); + } + } + } + + private String executeGet(String url) throws IOException { ConnectionConfig connectionConfig = ConnectionConfig.custom() .setConnectTimeout(Timeout.ofMilliseconds(TIMEOUT_MS)) @@ -382,13 +415,69 @@ protected String getString(String url) throws IOException int status = response.getCode(); if (status < 200 || status >= 300) { - throw new HttpResponseException(status, response.getReasonPhrase()); + // Read the body only for client errors; 5xx bodies are typically large, + // uninformative HTML error pages. + String body = (status >= 400 && status < 500 && response.getEntity() != null) + ? EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8) + : null; + throw new HttpResponseException(status, errorDetail(status, response.getReasonPhrase(), body)); } return EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8); }); } } + /** + * Builds the message for a non-2xx HttpResponseException. For client errors (4xx) the response + * body is appended (e.g. NCBI's "API key invalid" message) so the cause is visible in the server + * log; for other statuses only the reason phrase is used (5xx bodies are uninformative HTML). + */ + private static String errorDetail(int status, String reasonPhrase, @Nullable String body) + { + if (status >= 400 && status < 500 && !StringUtils.isBlank(body)) + { + return reasonPhrase + " - " + StringUtils.abbreviate(body.strip(), 500); + } + return reasonPhrase; + } + + /** + * Transient NCBI failures worth retrying: read timeouts and 5xx responses. + * 4xx and other errors are treated as permanent and fail fast. + */ + private static boolean isRetryable(IOException e) + { + if (e instanceof SocketTimeoutException) + { + return true; + } + if (e instanceof HttpResponseException hre) + { + return hre.getStatusCode() >= 500; + } + return false; + } + + // Exponential backoff: 500ms after the 1st failure, 1000ms after the 2nd, etc. No jitter is + // needed because both callers (the daily reminder job and the UI search) issue NCBI requests + // sequentially, so a retry never collides with a sibling request from the same caller. + private static long retryDelayMs(int attempt) + { + return (long) RETRY_BASE_DELAY_MS << (attempt - 1); + } + + private static void sleepMs(long ms) + { + try + { + Thread.sleep(ms); + } + catch (InterruptedException e) + { + Thread.currentThread().interrupt(); + } + } + /** * Fetch metadata for PMC articles using ESummary API */ @@ -420,7 +509,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(); @@ -921,14 +1010,7 @@ static List extractTitleKeywords(String title) */ private static void rateLimit() { - try - { - Thread.sleep(RATE_LIMIT_DELAY_MS); - } - catch (InterruptedException e) - { - Thread.currentThread().interrupt(); - } + sleepMs(RATE_LIMIT_DELAY_MS); } /** @@ -947,9 +1029,24 @@ static String stripQuerySpecialChars(String value) */ private static String buildCommonParams(String database) { - return "db=" + URLEncoder.encode(database, StandardCharsets.UTF_8) + + return buildCommonParams(database, PrivateDataReminderSettings.get().getNcbiApiKey()); + } + + // Builds the shared eutils query parameters. The API key is passed in (rather than looked up) + // so this can be unit tested without a running server. + private static String buildCommonParams(String database, @Nullable String apiKey) + { + String params = "db=" + URLEncoder.encode(database, StandardCharsets.UTF_8) + "&tool=" + URLEncoder.encode(TOOL, StandardCharsets.UTF_8) + "&email=" + URLEncoder.encode(EMAIL, StandardCharsets.UTF_8); + + // An NCBI API key (configured in the Private Data Reminder Settings) raises the eutils + // rate limit from 3 to 10 requests/sec. Append it when one has been entered. + if (!StringUtils.isBlank(apiKey)) + { + params += "&api_key=" + URLEncoder.encode(apiKey.trim(), StandardCharsets.UTF_8); + } + return params; } /** @@ -1422,6 +1519,66 @@ public void testPublicationMatchRoundTrip() assertFalse(restored.matchesProteomeXchangeId()); } + // -- HTTP retry tests -- + + @Test + public void testIsRetryable() + { + // Read timeouts and 5xx responses are transient NCBI failures -> retry + assertTrue(isRetryable(new SocketTimeoutException("Read timed out"))); + assertTrue(isRetryable(new HttpResponseException(500, "Internal Server Error"))); + assertTrue(isRetryable(new HttpResponseException(503, "Service Unavailable"))); + + // 4xx and generic IO errors are permanent -> fail fast + assertFalse(isRetryable(new HttpResponseException(400, "Bad Request"))); + assertFalse(isRetryable(new HttpResponseException(404, "Not Found"))); + assertFalse(isRetryable(new IOException("connection reset"))); + } + + @Test + public void testRetryDelayMs() + { + // Exponential backoff: 500ms, 1000ms, 2000ms per attempt. + assertEquals(500, retryDelayMs(1)); + assertEquals(1000, retryDelayMs(2)); + assertEquals(2000, retryDelayMs(3)); + } + + @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\"}"); + 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")); + + // 4xx with blank or null body: just the reason phrase, no trailing separator + assertEquals("Bad Request", errorDetail(400, "Bad Request", "")); + assertEquals("Bad Request", errorDetail(400, "Bad Request", null)); + } + + @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")); + } + // -- Helper methods for building test JSON -- private static JSONObject articleMetadata(String source, String fullJournalName) diff --git a/panoramapublic/src/org/labkey/panoramapublic/view/privateDataRemindersSettingsForm.jsp b/panoramapublic/src/org/labkey/panoramapublic/view/privateDataRemindersSettingsForm.jsp index cf5a738c..95f311fa 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/view/privateDataRemindersSettingsForm.jsp +++ b/panoramapublic/src/org/labkey/panoramapublic/view/privateDataRemindersSettingsForm.jsp @@ -170,6 +170,19 @@ + + + <%=h(PrivateDataReminderSettings.PROP_NCBI_API_KEY)%> + + + +
+ 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. Leave blank to search without a key. +
+ + <%=button("Save").submit(true)%> <%=button("Cancel").href(panoramaPublicAdminUrl)%> 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..c83fa289 100644 --- a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicBaseTest.java +++ b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicBaseTest.java @@ -631,7 +631,7 @@ protected void verifyIsPublicColumn(String panoramaPublicProject, String experim /** * Navigate to the Private Data Reminder Settings page and read the current form values. - * Returns a map with keys: extensionLength, delayUntilFirstReminder, reminderFrequency, enablePublicationSearch, publicationSearchFrequency. + * Returns a map with keys: extensionLength, delayUntilFirstReminder, reminderFrequency, enablePublicationSearch, publicationSearchFrequency, ncbiApiKey. */ protected Map getPrivateDataReminderSettings() { @@ -645,6 +645,7 @@ 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"))); + settings.put("ncbiApiKey", getFormElement(Locator.input("ncbiApiKey"))); return settings; } @@ -654,6 +655,14 @@ protected void savePrivateDataReminderSettings(String extensionLength, String de } protected void savePrivateDataReminderSettings(String extensionLength, String delayUntilFirstReminder, String reminderFrequency, boolean enablePublicationSearch) + { + savePrivateDataReminderSettings(extensionLength, delayUntilFirstReminder, reminderFrequency, enablePublicationSearch, null); + } + + /** + * @param ncbiApiKey value to enter in the NCBI API key field; pass null to leave the field untouched. + */ + protected void savePrivateDataReminderSettings(String extensionLength, String delayUntilFirstReminder, String reminderFrequency, boolean enablePublicationSearch, String ncbiApiKey) { goToAdminConsole().goToSettingsSection(); clickAndWait(Locator.linkWithText("Panorama Public")); @@ -662,6 +671,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 +690,10 @@ 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"))); + if (ncbiApiKey != null) + { + assertEquals(ncbiApiKey, getFormElement(Locator.input("ncbiApiKey"))); + } } protected void goToSendRemindersPage(String projectName) diff --git a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java index 555a193e..cb3e7f7f 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,15 @@ public void testPublicationSearchAndDismiss() // Step 1: Set up mock NCBI service if running on TeamCity setupMockNcbiService(); + // Capture the existing reminder settings up front so doCleanup can restore them exactly. + // The dev machine may already have a real NCBI API key (and other non-default values) set. + _originalReminderSettings = getPrivateDataReminderSettings(); + + // Baseline server error count up front. The only errors this test should produce are the + // deliberate ones from the bad-key search at the end; checkExpectedErrors(baseline + n) + // verifies exactly that count and fails on any unexpected extras (rather than masking them). + int serverErrorCount = getServerErrorCount(); + // 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 +199,6 @@ public void testPublicationSearchAndDismiss() assertTextPresent("The user has already dismissed the publication suggestion PubMed ID " + PMID_1 + " for this dataset"); // Step 9: Run reminders in TEST MODE — verify DatasetStatus is NOT updated - // Save current settings so they can be restored in doCleanup - _originalReminderSettings = getPrivateDataReminderSettings(); savePrivateDataReminderSettings("2", "0", "0", true); // Post reminders in test mode with publication search enabled @@ -256,6 +263,32 @@ public void testPublicationSearchAndDismiss() assertNotNull("Expected publicationType for dataset 2", dsStatus2AfterPost.get("PublicationType")); assertNotNull("Expected lastReminderDate for dataset 2", dsStatus2AfterPost.get("LastReminderDate")); assertNotNull("Expected citation to be cached for dataset 2", dsStatus2AfterPost.get("Citation")); + + // Verify the NCBI API key setting round-trips (set -> save -> re-read). The helper asserts + // the saved value is reflected on the form. doCleanup restores the original settings. + savePrivateDataReminderSettings("2", "0", "0", true, "test-ncbi-api-key"); + + // Verify the configured key actually reaches the live eutils requests. NCBI rejects an + // invalid key with HTTP 400, so re-searching dataset 2 (which found a publication above) + // should now find nothing. Only meaningful against the real NCBI service: on TeamCity the + // mock service bypasses the key, so this runs only when not using the mock (i.e. on dev). + if (!_useMockNcbi) + { + searchPublicationsForDataset(panoramaPublicProject, TARGET_FOLDER_2, exptId2); + assertTextPresent("No publications found for this dataset."); + assertTextNotPresent(PMID_2); + + // The invalid key makes NCBI return HTTP 400. Verify the server log records the cause: + // this proves both that the key reached eutils and that the 400 response body is logged. + assertTrue("Server log should record NCBI's invalid-key error", + getServerErrors().contains("API key invalid")); + + // The bad-key search logs one error per failed NCBI call: 2 PMC strategy searches for + // dataset 2 (ProteomeXchange ID and Panorama URL) plus the PubMed fallback = 3. + // checkExpectedErrors verifies exactly these and clears them; unlike a bare resetErrors() + // it fails the test if any other unexpected errors occurred during the run. + checkExpectedErrors(serverErrorCount + 3); + } } /* @@ -486,7 +519,8 @@ public void resetAfterTest() _originalReminderSettings.get("extensionLength"), _originalReminderSettings.get("delayUntilFirstReminder"), _originalReminderSettings.get("reminderFrequency"), - Boolean.parseBoolean(_originalReminderSettings.get("enablePublicationSearch"))); + Boolean.parseBoolean(_originalReminderSettings.get("enablePublicationSearch")), + _originalReminderSettings.get("ncbiApiKey")); } } From 02022c47be0d2db4944fb4fa93b15954a02503b9 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sat, 13 Jun 2026 21:22:35 -0700 Subject: [PATCH 02/12] Captured PublicationSearchTest's server error-count baseline just before the bad-key search so incidental NCBI 5xx in earlier steps don't make checkExpectedErrors flaky on dev --- .../tests/panoramapublic/PublicationSearchTest.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) 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 cb3e7f7f..c76e0895 100644 --- a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java +++ b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java @@ -118,11 +118,6 @@ public void testPublicationSearchAndDismiss() // The dev machine may already have a real NCBI API key (and other non-default values) set. _originalReminderSettings = getPrivateDataReminderSettings(); - // Baseline server error count up front. The only errors this test should produce are the - // deliberate ones from the bad-key search at the end; checkExpectedErrors(baseline + n) - // verifies exactly that count and fails on any unexpected extras (rather than masking them). - int serverErrorCount = getServerErrorCount(); - // Step 2: Create dataset 1 folder, submit to Panorama Public, and copy String testProject = getProjectName(); String shortAccessUrl1 = setupFolderSubmitAndCopy(testProject, FOLDER_1, TARGET_FOLDER_1, @@ -274,6 +269,12 @@ public void testPublicationSearchAndDismiss() // mock service bypasses the key, so this runs only when not using the mock (i.e. on dev). if (!_useMockNcbi) { + // Capture the server error count immediately before the deliberate bad-key search so the + // assertion below counts only its errors. Capturing at test start would also count any + // incidental transient NCBI errors (5xx) from the earlier real-NCBI steps, which the + // retry logic reduces but cannot eliminate, making the check flaky on a dev machine. + int serverErrorCount = getServerErrorCount(); + searchPublicationsForDataset(panoramaPublicProject, TARGET_FOLDER_2, exptId2); assertTextPresent("No publications found for this dataset."); assertTextNotPresent(PMID_2); From 21a0cc33824c6c79f9afe3a23b14c43c152d8c4f Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sat, 13 Jun 2026 21:31:57 -0700 Subject: [PATCH 03/12] Asserted enablePublicationSearch round-trips through save in PanoramaPublicBaseTest's reminder-settings helper --- .../test/tests/panoramapublic/PanoramaPublicBaseTest.java | 2 ++ 1 file changed, 2 insertions(+) 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 c83fa289..1483ba93 100644 --- a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicBaseTest.java +++ b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicBaseTest.java @@ -690,6 +690,8 @@ 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("enablePublicationSearch should round-trip through save", enablePublicationSearch, + Locator.checkboxByName("enablePublicationSearch").findElement(getDriver()).isSelected()); if (ncbiApiKey != null) { assertEquals(ncbiApiKey, getFormElement(Locator.input("ncbiApiKey"))); From 3c4bdfd0c86a8fccf94f235be244cc8dfd2737ab Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sat, 13 Jun 2026 21:44:43 -0700 Subject: [PATCH 04/12] Scoped MockNcbiPublicationSearchService matching to the decoded db/term/id query params instead of substring-scanning the whole request URL --- .../MockNcbiPublicationSearchService.java | 63 +++++++++++++------ 1 file changed, 43 insertions(+), 20 deletions(-) diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java index b9995737..6f0df488 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java @@ -21,7 +21,10 @@ 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; @@ -152,15 +155,24 @@ else if (url.contains("lit/ctxp")) private JSONObject handleESearch(String url) { - boolean isPmc = url.contains("db=pmc"); + boolean isPmc = "pmc".equals(extractQueryParam(url, "db")); Map> searchMap = isPmc ? _pmcSearchResults : _pubmedSearchResults; + // Match registered search keys against the decoded ESearch query term only, not the whole + // URL, so a key cannot accidentally match part of another parameter (tool/email) or another + // key. The real ESearch term wraps the key in quotes (e.g. "PXD056793"), so contains() on + // the term is the right granularity. + String term = extractQueryParam(url, "term"); + JSONArray idList = new JSONArray(); - for (Map.Entry> entry : searchMap.entrySet()) + if (term != null) { - if (url.contains(entry.getKey())) + for (Map.Entry> entry : searchMap.entrySet()) { - entry.getValue().forEach(idList::put); + if (term.contains(entry.getKey())) + { + entry.getValue().forEach(idList::put); + } } } @@ -171,13 +183,18 @@ private JSONObject handleESearch(String url) private JSONObject handleESummary(String url) { - boolean isPmc = url.contains("db=pmc"); + boolean isPmc = "pmc".equals(extractQueryParam(url, "db")); Map metadataMap = isPmc ? _pmcMetadata : _pubmedMetadata; + // ESummary requests a comma-separated list of IDs in the "id" parameter. Match registered + // IDs against that list (exactly, not by substring), rather than scanning the whole URL. + String idParam = extractQueryParam(url, "id"); + List requestedIds = idParam == null ? List.of() : Arrays.asList(idParam.split(",")); + JSONObject result = new JSONObject(); for (Map.Entry entry : metadataMap.entrySet()) { - if (url.contains(entry.getKey())) + if (requestedIds.contains(entry.getKey())) { result.put(entry.getKey(), entry.getValue()); } @@ -193,20 +210,7 @@ private JSONObject handleESummary(String url) */ private JSONObject handleCitation(String url) { - // Extract the publication ID from the URL (last segment after "id=") - String id = null; - int idIdx = url.indexOf("id="); - if (idIdx >= 0) - { - id = url.substring(idIdx + 3); - // Remove any trailing query parameters - int ampIdx = id.indexOf('&'); - if (ampIdx >= 0) - { - id = id.substring(0, ampIdx); - } - } - + String id = extractQueryParam(url, "id"); String citation = id != null ? _citations.get(id) : null; if (citation != null) { @@ -214,4 +218,23 @@ private JSONObject handleCitation(String url) } return new JSONObject(); } + + /** + * Returns the URL-decoded value of the given query parameter, or null if it is not present. + * Used to scope mock matching to a specific parameter (db, term, id) instead of the whole URL. + */ + private static @Nullable String extractQueryParam(String url, String name) + { + int queryStart = url.indexOf('?'); + String query = queryStart >= 0 ? url.substring(queryStart + 1) : url; + for (String pair : query.split("&")) + { + int eq = pair.indexOf('='); + if (eq > 0 && pair.substring(0, eq).equals(name)) + { + return URLDecoder.decode(pair.substring(eq + 1), StandardCharsets.UTF_8); + } + } + return null; + } } From 0c2058fcdc65f9ae09528d707e40dc9344a4d2a3 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sat, 13 Jun 2026 22:03:35 -0700 Subject: [PATCH 05/12] Added unit tests for the getString retry loop (retry-then-succeed, give-up-after-MAX_HTTP_ATTEMPTS, no-retry-on-4xx); made executeGet protected so a test subclass can drive it - Corrected stale comments in NcbiPublicationSearchServiceImpl and its mock, and tightened the others. --- .../MockNcbiPublicationSearchService.java | 13 +- .../NcbiPublicationSearchServiceImpl.java | 122 ++++++++++++++---- .../PanoramaPublicBaseTest.java | 7 +- .../panoramapublic/PublicationSearchTest.java | 25 ++-- 4 files changed, 120 insertions(+), 47 deletions(-) diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java index 6f0df488..067dabed 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/MockNcbiPublicationSearchService.java @@ -33,7 +33,8 @@ * 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, Logger)}, - * the single method that makes HTTP calls to NCBI. All search logic, filtering, author/title + * the method every NCBI request passes through. The override takes the place of the real HTTP request + * in {@code executeGet()} and the retry loop around it. All search logic, filtering, author/title * verification, citation parsing, and priority filtering run through the real implementation code. * Tests register mock articles via {@link #register}, providing the database, ID, search key, * metadata fields, and citation. The mock builds internal lookup maps from this data and returns @@ -54,7 +55,7 @@ public class MockNcbiPublicationSearchService extends NcbiPublicationSearchServi /** * Register a mock article. The mock stores the data in internal lookup maps used by - * {@link #getString(String)}. + * {@link #getString(String, Logger)}. * @param database "pmc" or "pubmed" — the NCBI database this article is in * @param id the article ID in the given database (numeric ID for pmc or pubmed) * @param searchKey what ESearch query term finds this article (e.g. PXD ID for PMC, author last name for PubMed) @@ -158,10 +159,9 @@ private JSONObject handleESearch(String url) boolean isPmc = "pmc".equals(extractQueryParam(url, "db")); Map> searchMap = isPmc ? _pmcSearchResults : _pubmedSearchResults; - // Match registered search keys against the decoded ESearch query term only, not the whole - // URL, so a key cannot accidentally match part of another parameter (tool/email) or another - // key. The real ESearch term wraps the key in quotes (e.g. "PXD056793"), so contains() on - // the term is the right granularity. + // Match registered search keys against the decoded ESearch query term, so a key cannot + // match part of another parameter such as tool or email. The real ESearch term wraps the + // key in quotes (e.g. "PXD056793"), so contains() on the term is the right granularity. String term = extractQueryParam(url, "term"); JSONArray idList = new JSONArray(); @@ -221,7 +221,6 @@ private JSONObject handleCitation(String url) /** * Returns the URL-decoded value of the given query parameter, or null if it is not present. - * Used to scope mock matching to a specific parameter (db, term, id) instead of the whole URL. */ private static @Nullable String extractQueryParam(String url, String name) { diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java index 3f6fd727..908e442e 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java @@ -94,7 +94,7 @@ public static void setInstance(NcbiPublicationSearchService impl) private static final String PMC_CITATION_EXPORTER_URL = "https://api.ncbi.nlm.nih.gov/lit/ctxp/v1/pmc/?format=citation&id="; // API parameters - private static final int RATE_LIMIT_DELAY_MS = 400; // NCBI allows 3 requests/sec + private static final int RATE_LIMIT_DELAY_MS = 400; // NCBI allows 3 requests/sec without an API key private static final int TIMEOUT_MS = 10000; // 10 seconds // NCBI eutils intermittently returns transient 5xx errors and read timeouts, even well under @@ -363,14 +363,12 @@ protected JSONObject getJson(String url, Logger log) throws IOException /** * Execute an HTTP GET request and return the response body as a string. Retry warnings are - * written to {@code log} so they land in the pipeline job log when invoked from the reminder - * job (and in the server log for UI-triggered searches, which pass the static logger). + * written to {@code log}. The reminder job passes its pipeline job logger, and UI-triggered + * searches pass the static server logger. * @throws IOException if the request fails or the server returns a non-2xx response */ protected String getString(String url, Logger log) throws IOException { - // Retry transient NCBI failures (5xx, read timeouts) with exponential backoff; - // other failures (e.g. 4xx) are permanent and fail fast. for (int attempt = 1; ; attempt++) { try @@ -384,14 +382,15 @@ protected String getString(String url, Logger log) throws IOException throw e; } long delayMs = retryDelayMs(attempt); - getLog(log).warn("NCBI request failed (attempt {} of {}); retrying in {} ms. URL: {}; cause: {}", + getLog(log).warn("NCBI request failed (attempt {} of {}). Retrying in {} ms. URL: {}. Cause: {}", attempt, MAX_HTTP_ATTEMPTS, delayMs, url, e.toString()); sleepMs(delayMs); } } } - private String executeGet(String url) throws IOException + // Overridden in unit tests to drive the retry loop in getString() without real HTTP calls. + protected String executeGet(String url) throws IOException { ConnectionConfig connectionConfig = ConnectionConfig.custom() .setConnectTimeout(Timeout.ofMilliseconds(TIMEOUT_MS)) @@ -415,8 +414,8 @@ private String executeGet(String url) throws IOException int status = response.getCode(); if (status < 200 || status >= 300) { - // Read the body only for client errors; 5xx bodies are typically large, - // uninformative HTML error pages. + // 5xx bodies are large, uninformative HTML error pages, so only client error + // bodies are read. String body = (status >= 400 && status < 500 && response.getEntity() != null) ? EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8) : null; @@ -428,9 +427,9 @@ private String executeGet(String url) throws IOException } /** - * Builds the message for a non-2xx HttpResponseException. For client errors (4xx) the response - * body is appended (e.g. NCBI's "API key invalid" message) so the cause is visible in the server - * log; for other statuses only the reason phrase is used (5xx bodies are uninformative HTML). + * Build the message for a non-2xx HttpResponseException. For client errors (4xx) the response + * body is appended, so NCBI's reason (e.g. "API key invalid") reaches the log. Other statuses + * use only the reason phrase. */ private static String errorDetail(int status, String reasonPhrase, @Nullable String body) { @@ -442,8 +441,8 @@ private static String errorDetail(int status, String reasonPhrase, @Nullable Str } /** - * Transient NCBI failures worth retrying: read timeouts and 5xx responses. - * 4xx and other errors are treated as permanent and fail fast. + * Read timeouts and 5xx responses are transient NCBI failures worth retrying. 4xx and other + * errors are permanent. */ private static boolean isRetryable(IOException e) { @@ -458,9 +457,9 @@ private static boolean isRetryable(IOException e) return false; } - // Exponential backoff: 500ms after the 1st failure, 1000ms after the 2nd, etc. No jitter is - // needed because both callers (the daily reminder job and the UI search) issue NCBI requests - // sequentially, so a retry never collides with a sibling request from the same caller. + // 500ms after the first failure, then doubling. Jitter is not needed. Both callers, the daily + // reminder job and the UI search, issue NCBI requests sequentially, so a retry never collides + // with a sibling request. private static long retryDelayMs(int attempt) { return (long) RETRY_BASE_DELAY_MS << (attempt - 1); @@ -1025,15 +1024,15 @@ 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 buildCommonParams(database, PrivateDataReminderSettings.get().getNcbiApiKey()); } - // Builds the shared eutils query parameters. The API key is passed in (rather than looked up) - // so this can be unit tested without a running server. + // 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) + @@ -1041,7 +1040,7 @@ private static String buildCommonParams(String database, @Nullable String apiKey "&email=" + URLEncoder.encode(EMAIL, StandardCharsets.UTF_8); // An NCBI API key (configured in the Private Data Reminder Settings) raises the eutils - // rate limit from 3 to 10 requests/sec. Append it when one has been entered. + // rate limit from 3 to 10 requests/sec. if (!StringUtils.isBlank(apiKey)) { params += "&api_key=" + URLEncoder.encode(apiKey.trim(), StandardCharsets.UTF_8); @@ -1538,7 +1537,7 @@ public void testIsRetryable() @Test public void testRetryDelayMs() { - // Exponential backoff: 500ms, 1000ms, 2000ms per attempt. + // Exponential backoff of 500ms, 1000ms, 2000ms per attempt. assertEquals(500, retryDelayMs(1)); assertEquals(1000, retryDelayMs(2)); assertEquals(2000, retryDelayMs(3)); @@ -1579,6 +1578,85 @@ public void testBuildCommonParams() assertTrue(buildCommonParams("pmc", " ABC123 ").contains("api_key=ABC123")); } + @Test + public void testGetStringRetriesTransientFailures() throws IOException + { + // executeGet returns a 5xx twice, then succeeds. getString should retry and return the body. + int[] attempts = {0}; + NcbiPublicationSearchServiceImpl service = new NcbiPublicationSearchServiceImpl() + { + @Override + protected String executeGet(String url) throws IOException + { + if (++attempts[0] < 3) + throw new HttpResponseException(503, "Service Unavailable"); + return "body"; + } + }; + assertEquals("body", service.getString("http://test", LOG)); + assertEquals("Should retry until the 3rd attempt succeeds", 3, attempts[0]); + } + + @Test + public void testGetStringGivesUpAfterMaxAttempts() + { + // executeGet always returns a 5xx. getString should try MAX_HTTP_ATTEMPTS times, then rethrow. + int[] attempts = {0}; + NcbiPublicationSearchServiceImpl service = new NcbiPublicationSearchServiceImpl() + { + @Override + protected String executeGet(String url) throws IOException + { + attempts[0]++; + throw new HttpResponseException(500, "Internal Server Error"); + } + }; + try + { + service.getString("http://test", LOG); + fail("Expected HttpResponseException after exhausting retries"); + } + catch (HttpResponseException e) + { + assertEquals(500, e.getStatusCode()); + } + catch (IOException e) + { + fail("Expected HttpResponseException, got " + e); + } + assertEquals("Should attempt exactly MAX_HTTP_ATTEMPTS times", MAX_HTTP_ATTEMPTS, attempts[0]); + } + + @Test + public void testGetStringDoesNotRetryClientErrors() + { + // A 4xx is permanent. getString should fail immediately without retrying. + int[] attempts = {0}; + NcbiPublicationSearchServiceImpl service = new NcbiPublicationSearchServiceImpl() + { + @Override + protected String executeGet(String url) throws IOException + { + attempts[0]++; + throw new HttpResponseException(400, "Bad Request"); + } + }; + try + { + service.getString("http://test", LOG); + fail("Expected HttpResponseException for a 4xx"); + } + catch (HttpResponseException e) + { + assertEquals(400, e.getStatusCode()); + } + catch (IOException e) + { + fail("Expected HttpResponseException, got " + e); + } + assertEquals("4xx must not be retried", 1, attempts[0]); + } + // -- Helper methods for building test JSON -- private static JSONObject articleMetadata(String source, String fullJournalName) 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 1483ba93..3637108c 100644 --- a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicBaseTest.java +++ b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicBaseTest.java @@ -660,7 +660,7 @@ protected void savePrivateDataReminderSettings(String extensionLength, String de } /** - * @param ncbiApiKey value to enter in the NCBI API key field; pass null to leave the field untouched. + * @param ncbiApiKey value to enter in the NCBI API key field. Pass null to leave the field untouched. */ protected void savePrivateDataReminderSettings(String extensionLength, String delayUntilFirstReminder, String reminderFrequency, boolean enablePublicationSearch, String ncbiApiKey) { @@ -690,11 +690,12 @@ 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("enablePublicationSearch should round-trip through save", enablePublicationSearch, + assertEquals("The saved publication search setting should be displayed on the form", enablePublicationSearch, Locator.checkboxByName("enablePublicationSearch").findElement(getDriver()).isSelected()); if (ncbiApiKey != null) { - assertEquals(ncbiApiKey, getFormElement(Locator.input("ncbiApiKey"))); + assertEquals("The saved NCBI API key should be displayed on the form", ncbiApiKey, + getFormElement(Locator.input("ncbiApiKey"))); } } 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 c76e0895..898f64b7 100644 --- a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java +++ b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java @@ -259,35 +259,30 @@ public void testPublicationSearchAndDismiss() assertNotNull("Expected lastReminderDate for dataset 2", dsStatus2AfterPost.get("LastReminderDate")); assertNotNull("Expected citation to be cached for dataset 2", dsStatus2AfterPost.get("Citation")); - // Verify the NCBI API key setting round-trips (set -> save -> re-read). The helper asserts - // the saved value is reflected on the form. doCleanup restores the original settings. + // Verify the NCBI API key setting round-trips (set -> save -> re-read). savePrivateDataReminderSettings("2", "0", "0", true, "test-ncbi-api-key"); - // Verify the configured key actually reaches the live eutils requests. NCBI rejects an - // invalid key with HTTP 400, so re-searching dataset 2 (which found a publication above) - // should now find nothing. Only meaningful against the real NCBI service: on TeamCity the - // mock service bypasses the key, so this runs only when not using the mock (i.e. on dev). + // Verify the configured key reaches the live eutils requests. NCBI rejects an invalid key + // with HTTP 400, so re-searching dataset 2, which found a publication above, should now + // find nothing. The mock service bypasses the key, so this runs only against real NCBI. if (!_useMockNcbi) { // Capture the server error count immediately before the deliberate bad-key search so the - // assertion below counts only its errors. Capturing at test start would also count any - // incidental transient NCBI errors (5xx) from the earlier real-NCBI steps, which the - // retry logic reduces but cannot eliminate, making the check flaky on a dev machine. + // assertion below only counts errors due to the bad-key search. int serverErrorCount = getServerErrorCount(); searchPublicationsForDataset(panoramaPublicProject, TARGET_FOLDER_2, exptId2); assertTextPresent("No publications found for this dataset."); assertTextNotPresent(PMID_2); - // The invalid key makes NCBI return HTTP 400. Verify the server log records the cause: - // this proves both that the key reached eutils and that the 400 response body is logged. + // The invalid key makes NCBI return HTTP 400, and errorDetail appends the response body + // to the logged message. assertTrue("Server log should record NCBI's invalid-key error", getServerErrors().contains("API key invalid")); - // The bad-key search logs one error per failed NCBI call: 2 PMC strategy searches for - // dataset 2 (ProteomeXchange ID and Panorama URL) plus the PubMed fallback = 3. - // checkExpectedErrors verifies exactly these and clears them; unlike a bare resetErrors() - // it fails the test if any other unexpected errors occurred during the run. + // The bad-key search logs one error per failed NCBI call. Dataset 2 runs two PMC + // strategy searches, on ProteomeXchange ID and Panorama URL, plus the PubMed fallback. + // checkExpectedErrors clears exactly that many and fails on any others. checkExpectedErrors(serverErrorCount + 3); } } From 74370ceafc1da52eb68f9a1aa8ab556c54e0cf47 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Mon, 31 Aug 2026 13:46:17 -0700 Subject: [PATCH 06/12] Protected the NCBI API key and made a failed publication search visible * Redacted the API key from the retry warning and from NCBI's 4xx body before either reaches a log * Moved the key to the encrypted property store, and stopped the form displaying or erasing it * Threw NcbiSearchException from executeSearch so a rejected key no longer reads as a dataset with no paper * Added a Validate button that checks a key against NCBI before it is saved * Scheduled the reminder job from startBackgroundThreads so a SchedulerException cannot fail server startup * Added redaction unit tests and reworked the Selenium NCBI API key coverage Co-Authored-By: Claude --- .../PanoramaPublicController.java | 83 +++++++++++++- .../panoramapublic/PanoramaPublicModule.java | 21 +++- .../message/PrivateDataReminderSettings.java | 40 ++++++- .../ncbi/NcbiPublicationSearchService.java | 14 +++ .../NcbiPublicationSearchServiceImpl.java | 101 ++++++++++++++++-- .../ncbi/NcbiSearchException.java | 34 ++++++ .../pipeline/PrivateDataReminderJob.java | 38 ++++++- .../view/privateDataRemindersSettingsForm.jsp | 62 ++++++++++- .../PanoramaPublicBaseTest.java | 8 +- .../panoramapublic/PublicationSearchTest.java | 58 ++++++---- 10 files changed, 413 insertions(+), 46 deletions(-) create mode 100644 panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiSearchException.java diff --git a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java index a9dc4619..c3d0ed24 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java +++ b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java @@ -10084,6 +10084,52 @@ public static ActionURL getViewExperimentModificationsURL(int experimentAnnotati return result; } + @RequiresPermission(AdminOperationsPermission.class) + public static class ValidateNcbiApiKeyAction extends MutatingApiAction + { + @Override + public Object execute(PrivateDataReminderSettingsForm form, BindException errors) + { + ApiSimpleResponse response = new ApiSimpleResponse(); + response.put("success", true); + + // An empty field means check the key that is already saved, since the form never + // displays it. + boolean checkingSavedKey = StringUtils.isBlank(form.getNcbiApiKey()); + String apiKey = checkingSavedKey + ? PrivateDataReminderSettings.get().getNcbiApiKey() + : form.getNcbiApiKey().trim(); + + if (StringUtils.isBlank(apiKey)) + { + response.put("valid", false); + response.put("message", "Enter a key to validate, or save one first."); + return response; + } + + String error = NcbiPublicationSearchService.get().validateApiKey(apiKey); + response.put("valid", error == null); + if (error == null) + { + // Validating does not store anything, so say so. Otherwise "accepted" reads as + // confirmation that the key is now in effect. + response.put("message", checkingSavedKey + ? "NCBI accepted the saved key." + : "NCBI accepted this key. Click Save to store it."); + LOG.info("NCBI accepted an API key entered on the Private Data Reminder Settings page."); + } + else + { + // The short message goes beside the field. NCBI's own words are offered separately, + // since the admin holding the key is the one who has to act on them. + response.put("message", "NCBI rejected this key."); + response.put("detail", error); + LOG.warn("NCBI rejected an API key entered on the Private Data Reminder Settings page. {}", error); + } + return response; + } + } + @RequiresPermission(AdminOperationsPermission.class) public static class PrivateDataReminderSettingsAction extends FormViewAction { @@ -10146,7 +10192,8 @@ public ModelAndView getView(PrivateDataReminderSettingsForm form, boolean reshow form.setExtensionLength(settings.getExtensionLength()); form.setEnablePublicationSearch(settings.isEnablePublicationSearch()); form.setPublicationSearchFrequency(settings.getPublicationSearchFrequency()); - form.setNcbiApiKey(settings.getNcbiApiKey()); + // Do not put the saved key in the form. The JSP shows only whether one is stored. + form.setNcbiApiKeySet(PrivateDataReminderSettings.hasNcbiApiKey()); } VBox view = new VBox(); @@ -10167,9 +10214,19 @@ public boolean handlePost(PrivateDataReminderSettingsForm form, BindException er settings.setExtensionLength(form.getExtensionLength()); settings.setEnablePublicationSearch(form.isEnablePublicationSearch()); settings.setPublicationSearchFrequency(form.getPublicationSearchFrequency()); - settings.setNcbiApiKey(form.getNcbiApiKey()); 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; } @@ -10208,6 +10265,8 @@ public static class PrivateDataReminderSettingsForm private boolean _enablePublicationSearch; private Integer _publicationSearchFrequency; private String _ncbiApiKey; + private boolean _clearNcbiApiKey; + private boolean _ncbiApiKeySet; public boolean isEnabled() { @@ -10288,6 +10347,26 @@ public void setNcbiApiKey(String ncbiApiKey) { _ncbiApiKey = ncbiApiKey; } + + public boolean isClearNcbiApiKey() + { + return _clearNcbiApiKey; + } + + public void setClearNcbiApiKey(boolean clearNcbiApiKey) + { + _clearNcbiApiKey = clearNcbiApiKey; + } + + public boolean isNcbiApiKeySet() + { + return _ncbiApiKeySet; + } + + public void setNcbiApiKeySet(boolean ncbiApiKeySet) + { + _ncbiApiKeySet = ncbiApiKeySet; + } } @RequiresPermission(AdminOperationsPermission.class) diff --git a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicModule.java b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicModule.java index c7df83fb..6942ac20 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicModule.java +++ b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicModule.java @@ -16,6 +16,7 @@ package org.labkey.panoramapublic; +import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.labkey.api.admin.FolderSerializationRegistry; @@ -34,6 +35,7 @@ import org.labkey.api.security.roles.RoleManager; import org.labkey.api.settings.AdminConsole; import org.labkey.api.targetedms.TargetedMSService; +import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.ActionURL; import org.labkey.api.view.BaseWebPartFactory; import org.labkey.api.view.HtmlView; @@ -83,6 +85,8 @@ public class PanoramaPublicModule extends SpringModule { + private static final Logger LOG = LogHelper.getLogger(PanoramaPublicModule.class, "Panorama Public module"); + public static final String NAME = "PanoramaPublic"; public static final String DOWNLOAD_DATA_INFO_WP = "Download Data"; @@ -153,8 +157,21 @@ protected void startupAfterSpringConfig(ModuleContext moduleContext) fileContentService.addFileListener(new PanoramaPublicFileListener()); } - // Start the private data reminder job on server restart if it is enabled. - PrivateDataMessageScheduler.getInstance().initialize(PrivateDataReminderSettings.get().isEnableReminders()); + } + + @Override + public void startBackgroundThreads() + { + // Re-establish the reminder schedule on every startup. Reminder messages contain absolute + // URLs, which are only safe to build once this method is called. + try + { + PrivateDataMessageScheduler.getInstance().initialize(PrivateDataReminderSettings.get().isEnableReminders()); + } + catch (RuntimeException e) + { + LOG.error("Failed to schedule the Panorama Public private data reminder job", e); + } } @NotNull diff --git a/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java b/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java index 42cfd43b..9453b70d 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java +++ b/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java @@ -15,6 +15,7 @@ */ package org.labkey.panoramapublic.message; +import org.apache.commons.lang3.StringUtils; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.junit.Assert; @@ -30,6 +31,7 @@ import java.time.format.DateTimeFormatter; import java.time.format.DateTimeParseException; import java.util.Date; +import java.util.Map; public class PrivateDataReminderSettings { @@ -42,6 +44,7 @@ public class PrivateDataReminderSettings public static final String PROP_ENABLE_PUBLICATION_SEARCH = "Enable publication search"; public static final String PROP_PUBLICATION_SEARCH_FREQUENCY = "Publication search frequency (months)"; public static final String PROP_NCBI_API_KEY = "NCBI API key"; + public static final String PROP_NCBI_CREDENTIALS = "Panorama Public NCBI credentials"; private static final boolean DEFAULT_ENABLE_REMINDERS = false; public static final String DEFAULT_REMINDER_TIME = "8:00 AM"; @@ -104,7 +107,7 @@ public static PrivateDataReminderSettings get() : Integer.valueOf(settingsMap.get(PROP_PUBLICATION_SEARCH_FREQUENCY)); settings.setPublicationSearchFrequency(publicationSearchFrequency); - settings.setNcbiApiKey(settingsMap.get(PROP_NCBI_API_KEY)); + settings.setNcbiApiKey(getNcbiApiKeyValue()); } else { @@ -151,10 +154,43 @@ public static void save(PrivateDataReminderSettings settings) settingsMap.put(PROP_REMINDER_TIME, settings.getReminderTimeFormatted()); settingsMap.put(PROP_ENABLE_PUBLICATION_SEARCH, String.valueOf(settings.isEnablePublicationSearch())); settingsMap.put(PROP_PUBLICATION_SEARCH_FREQUENCY, String.valueOf(settings.getPublicationSearchFrequency())); - settingsMap.put(PROP_NCBI_API_KEY, settings.getNcbiApiKey() != null ? settings.getNcbiApiKey() : ""); + // The API key is a credential and is saved separately, in the encrypted store. Saving the + // rest of the settings must never change it. + settingsMap.remove(PROP_NCBI_API_KEY); settingsMap.save(); } + /** + * Save the NCBI API key, or remove it when the key is blank. The key lives in the encrypted + * store, like the other credentials this module holds. + */ + public static void saveNcbiApiKey(@Nullable String apiKey) + { + PropertyManager.WritablePropertyMap credentials = + PropertyManager.getEncryptedStore().getWritableProperties(PROP_NCBI_CREDENTIALS, true); + if (StringUtils.isBlank(apiKey)) + { + credentials.remove(PROP_NCBI_API_KEY); + } + else + { + credentials.put(PROP_NCBI_API_KEY, apiKey.trim()); + } + credentials.save(); + } + + public static boolean hasNcbiApiKey() + { + return !StringUtils.isBlank(getNcbiApiKeyValue()); + } + + private static @Nullable String getNcbiApiKeyValue() + { + Map credentials = + PropertyManager.getEncryptedStore().getProperties(PROP_NCBI_CREDENTIALS); + return credentials.get(PROP_NCBI_API_KEY); + } + public void setEnableReminders(boolean enableReminders) { _enableReminders = enableReminders; diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchService.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchService.java index 0e3de30d..e0b05baf 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchService.java +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchService.java @@ -38,13 +38,27 @@ static NcbiPublicationSearchService get() @Nullable String getCitation(String publicationId, DB database); + /** + * Send a minimal request to NCBI with the given key. + * @return null if NCBI accepted the key, otherwise the reason it gave. + */ + @Nullable String validateApiKey(@Nullable String apiKey); + @Nullable Pair getPubMedLinkAndCitation(String pubmedId); /** * Searches PMC and PubMed for a publication associated with the experiment. * Returns the top match (highest priority) if multiple matches are found, or null if none. */ + /** + * @throws NcbiSearchException if a request to NCBI fails. An empty result therefore means no + * publication was found, not that the search could not be run. + */ @Nullable PublicationMatch searchForPublication(@NotNull ExperimentAnnotations expAnnotations, @Nullable Logger logger); + /** + * @throws NcbiSearchException if a request to NCBI fails. An empty result therefore means no + * publication was found, not that the search could not be run. + */ List searchForPublication(@NotNull ExperimentAnnotations expAnnotations, int maxResults, @Nullable Logger logger, boolean getCitations); } diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java index 908e442e..a7dae20a 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java @@ -59,6 +59,8 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import java.util.stream.Collectors; import static org.labkey.panoramapublic.ncbi.PublicationMatch.MATCH_DOI; @@ -102,6 +104,9 @@ public static void setInstance(NcbiPublicationSearchService impl) 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 String REDACTED = "REDACTED"; + private static final Pattern API_KEY_PARAM = Pattern.compile("api_key=([^&\\s]*)"); + // NCBI suggests using the 'tool' and 'email' parameters on E-utilities URLs // https://www.nlm.nih.gov/dataguide/eutilities/utilities.html private static final String TOOL = "PanoramaPublic"; @@ -347,7 +352,24 @@ private List executeSearch(String query, String database, Logger log) catch (IOException | JSONException e) { log.error("Error searching {} with query: {}", database, query, e); - return Collections.emptyList(); + throw new NcbiSearchException("Error searching " + database + " with query: " + query, e); + } + } + + @Override + public @Nullable String validateApiKey(@Nullable String apiKey) + { + // A minimal ESearch request. NCBI answers a rejected key with 400 and a reason, which the + // retry loop does not retry, so this returns quickly either way. + String url = ESEARCH_URL + "?" + buildCommonParams("pubmed", apiKey) + "&term=labkey&retmax=1&retmode=json"; + try + { + getString(url, LOG); + return null; + } + catch (IOException e) + { + return redactApiKey(e.getMessage(), apiKey); } } @@ -383,7 +405,7 @@ protected String getString(String url, Logger log) throws IOException } long delayMs = retryDelayMs(attempt); getLog(log).warn("NCBI request failed (attempt {} of {}). Retrying in {} ms. URL: {}. Cause: {}", - attempt, MAX_HTTP_ATTEMPTS, delayMs, url, e.toString()); + attempt, MAX_HTTP_ATTEMPTS, delayMs, redactApiKey(url, apiKeyFrom(url)), e.toString()); sleepMs(delayMs); } } @@ -419,7 +441,8 @@ protected String executeGet(String url) throws IOException String body = (status >= 400 && status < 500 && response.getEntity() != null) ? EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8) : null; - throw new HttpResponseException(status, errorDetail(status, response.getReasonPhrase(), body)); + throw new HttpResponseException(status, + errorDetail(status, response.getReasonPhrase(), body, apiKeyFrom(url))); } return EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8); }); @@ -429,17 +452,46 @@ protected String executeGet(String url) throws IOException /** * Build the message for a non-2xx HttpResponseException. For client errors (4xx) the response * body is appended, so NCBI's reason (e.g. "API key invalid") reaches the log. Other statuses - * use only the reason phrase. + * use only the reason phrase. The body is third-party text on its way to a log, so any + * occurrence of the API key is removed from it. */ - private static String errorDetail(int status, String reasonPhrase, @Nullable String body) + 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(body.strip(), 500); + return reasonPhrase + " - " + redactApiKey(StringUtils.abbreviate(body.strip(), 500), apiKey); } return reasonPhrase; } + /** + * Replace the NCBI API key wherever it appears in text that is about to be logged. The key is a + * query parameter on every eutils URL. When the reminder job runs, the log is the pipeline job + * log, which is readable by anyone with read access to the folder the job ran in. + */ + static String redactApiKey(@Nullable String text, @Nullable String apiKey) + { + if (text == null) + { + return null; + } + String redacted = text.replaceAll("(api_key=)[^&\\s]*", "$1" + REDACTED); + if (!StringUtils.isBlank(apiKey)) + { + redacted = redacted.replace(apiKey.trim(), REDACTED); + } + return redacted; + } + + /** + * Returns the value of the api_key query parameter in the given URL, or null if there is none. + */ + static @Nullable String apiKeyFrom(String url) + { + Matcher matcher = API_KEY_PARAM.matcher(url); + return matcher.find() ? matcher.group(1) : null; + } + /** * Read timeouts and 5xx responses are transient NCBI failures worth retrying. 4xx and other * errors are permanent. @@ -527,7 +579,7 @@ private Map fetchMetadata(Collection ids, String dat catch (IOException | JSONException e) { log.error("Error fetching {} metadata for IDs: {}", database, ids, e); - return Collections.emptyMap(); + throw new NcbiSearchException("Error fetching " + database + " metadata for IDs: " + ids, e); } } @@ -1547,16 +1599,43 @@ public void testRetryDelayMs() 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\"}"); + 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")); + 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", "")); - assertEquals("Bad Request", errorDetail(400, "Bad Request", null)); + assertEquals("Bad Request", errorDetail(400, "Bad Request", "", null)); + assertEquals("Bad Request", errorDetail(400, "Bad Request", null, null)); + + // A body that quotes the request back must not carry the key into the log + String echoed = errorDetail(400, "Bad Request", "invalid key SECRET123 for api_key=SECRET123", "SECRET123"); + assertFalse("errorDetail must not put the API key in the message", echoed.contains("SECRET123")); + } + + @Test + public void testRedactApiKey() + { + // The key is stripped from an eutils URL, and the rest of the URL is left intact + String url = "https://eutils.ncbi.nlm.nih.gov/esearch.fcgi?db=pmc&api_key=SECRET123&term=PXD001"; + String redacted = redactApiKey(url, "SECRET123"); + assertFalse("The redacted URL must not contain the key", redacted.contains("SECRET123")); + assertTrue("The redacted URL must keep its other parameters", redacted.contains("term=PXD001")); + assertTrue(redacted.contains("db=pmc")); + + // The key is stripped even when it appears without the api_key= prefix + assertFalse(redactApiKey("rejected key SECRET123", "SECRET123").contains("SECRET123")); + + // A URL with no key is unchanged, and null text stays null + String noKey = "https://eutils.ncbi.nlm.nih.gov/esearch.fcgi?db=pmc&term=PXD001"; + assertEquals(noKey, redactApiKey(noKey, null)); + assertNull(redactApiKey(null, "SECRET123")); + + // The key is recovered from the URL so callers do not have to read the settings + assertEquals("SECRET123", apiKeyFrom(url)); + assertNull(apiKeyFrom(noKey)); } @Test diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiSearchException.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiSearchException.java new file mode 100644 index 00000000..0e4a8806 --- /dev/null +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiSearchException.java @@ -0,0 +1,34 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.labkey.panoramapublic.ncbi; + +/** + * Thrown when a request to NCBI fails, so that callers can tell a failed search from a search that + * found nothing. Without it a rejected API key and a dataset with no published paper both arrive as + * an empty result. + */ +public class NcbiSearchException extends RuntimeException +{ + public NcbiSearchException(String message, Throwable cause) + { + super(message, cause); + } + + public NcbiSearchException(String message) + { + super(message); + } +} diff --git a/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java b/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java index 4b9d8ed6..e4a8b649 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java +++ b/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java @@ -41,6 +41,7 @@ import org.labkey.panoramapublic.model.Journal; import org.labkey.panoramapublic.model.JournalSubmission; import org.labkey.panoramapublic.ncbi.NcbiPublicationSearchService; +import org.labkey.panoramapublic.ncbi.NcbiSearchException; import org.labkey.panoramapublic.ncbi.PublicationMatch; import org.labkey.panoramapublic.query.DatasetStatusManager; import org.labkey.panoramapublic.query.ExperimentAnnotationsManager; @@ -251,6 +252,12 @@ private PublicationMatch searchForPublication(@NotNull ExperimentAnnotations exp return null; } } + catch (NcbiSearchException e) + { + // A search that could not run must not reset the deferral, which would record it + // as having found no new publication. + throw e; + } catch (Exception e) { log.error("Error re-searching publication for experiment {}: {}", expAnnotations.getId(), e.getMessage(), e); @@ -272,6 +279,12 @@ private PublicationMatch searchForPublication(@NotNull ExperimentAnnotations exp { return NcbiPublicationSearchService.get().searchForPublication(expAnnotations, log); } + catch (NcbiSearchException e) + { + // The caller records this as a failed search. Returning null here would make it + // indistinguishable from a dataset with no published paper. + throw e; + } catch (Exception e) { log.error("Error searching for publication for experiment {}: {}", expAnnotations.getId(), e.getMessage(), e); @@ -384,8 +397,17 @@ private void processExperiment(Integer experimentAnnotationsId, ProcessingContex return; } - // Check for publications if enabled - PublicationMatch publicationResult = searchForPublication(expAnnotations, context.getSettings(), _forcePublicationCheck, getUser(), context.isTestMode(), processingResults._log); + // Check for publications if enabled. A search that could not run still gets a reminder, since + // the reminder is about the data being private, not about the paper. + PublicationMatch publicationResult = null; + try + { + publicationResult = searchForPublication(expAnnotations, context.getSettings(), _forcePublicationCheck, getUser(), context.isTestMode(), processingResults._log); + } + catch (NcbiSearchException e) + { + processingResults.addPublicationSearchFailed(experimentAnnotationsId, e); + } if (!context.isTestMode()) { @@ -662,6 +684,7 @@ private static class ProcessingResults private final List _submissionNotFound = new ArrayList<>(); private final List _announcementNotFound = new ArrayList<>(); private final List _submitterNotFound = new ArrayList<>(); + private final List _publicationSearchFailed = new ArrayList<>(); private final List _skipped = new ArrayList<>(); private int _processed = 0; private final int _total; @@ -702,6 +725,12 @@ public void addSubmitterNotFound(Integer experimentId) _log.error("Could not find a submitter user for experiment Id: {}.", experimentId); } + public void addPublicationSearchFailed(Integer experimentId, Exception e) + { + _publicationSearchFailed.add(experimentId); + _log.error("Publication search failed for experiment Id: {}. A reminder was still posted. {}", experimentId, e.getMessage(), e); + } + public void addSkipped(Integer experimentId, ReminderDecision decision) { _skipped.add(experimentId); @@ -739,6 +768,11 @@ public void logSkipped(Logger log) log.error("Support message threads were not found for the following experiment Ids: {}", StringUtils.join(_announcementNotFound, ", ")); } + if (!_publicationSearchFailed.isEmpty()) + { + log.error("Publication search failed for the following experiment Ids: {}. Check the NCBI API key in the Private Data Reminder Settings.", StringUtils.join(_publicationSearchFailed, ", ")); + } + if (!_submitterNotFound.isEmpty()) { log.error("Submitter user was not found for the following experiment Ids: {}", StringUtils.join(_submitterNotFound, ", ")); diff --git a/panoramapublic/src/org/labkey/panoramapublic/view/privateDataRemindersSettingsForm.jsp b/panoramapublic/src/org/labkey/panoramapublic/view/privateDataRemindersSettingsForm.jsp index 95f311fa..4a161bfa 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/view/privateDataRemindersSettingsForm.jsp +++ b/panoramapublic/src/org/labkey/panoramapublic/view/privateDataRemindersSettingsForm.jsp @@ -81,6 +81,58 @@ } window.location = LABKEY.ActionURL.buildURL("panoramapublic", "searchPublications.view", folderPath); } + + function showValidationResult(result, message, detail) + { + while (result.firstChild) + { + result.removeChild(result.firstChild); + } + result.appendChild(document.createTextNode(message)); + + if (!detail) + { + return; + } + + result.appendChild(document.createTextNode(" ")); + const link = document.createElement("a"); + link.href = "#"; + link.textContent = "Details"; + // The handler is attached here rather than with an onclick attribute, which the Content + // Security Policy blocks. NCBI's reply is encoded because it is third party text. + link.addEventListener("click", function (e) + { + e.preventDefault(); + Ext4.Msg.alert("NCBI response", Ext4.String.htmlEncode(detail)); + }); + result.appendChild(link); + } + + function validateNcbiApiKey() + { + const input = document.getElementsByName("ncbiApiKey")[0]; + const result = document.getElementById("ncbiApiKeyValidationResult"); + result.style.color = ""; + result.textContent = "Checking with NCBI..."; + + LABKEY.Ajax.request({ + url: LABKEY.ActionURL.buildURL("panoramapublic", "validateNcbiApiKey.api"), + method: "POST", + // An empty value asks the server to check the saved key, which this form never displays. + jsonData: {ncbiApiKey: input ? input.value : ""}, + success: LABKEY.Utils.getCallbackWrapper(function (response) + { + result.style.color = response.valid ? "green" : "red"; + showValidationResult(result, response.message, response.detail); + }), + failure: LABKEY.Utils.getCallbackWrapper(function () + { + result.style.color = "red"; + showValidationResult(result, "Could not reach the server to validate the key.", null); + }) + }); + } @@ -175,11 +227,17 @@ <%=h(PrivateDataReminderSettings.PROP_NCBI_API_KEY)%> - + " /> + <%=button("Validate").onClick("validateNcbiApiKey(); return false;")%> +
Optional. An NCBI API key raises the request rate limit for PubMed/PMC searches from 3 to 10 per second.
- Create one under Account settings at ncbi.nlm.nih.gov. Leave blank to search without a key. + 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. +
+
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 3637108c..425860c4 100644 --- a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicBaseTest.java +++ b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicBaseTest.java @@ -645,7 +645,9 @@ protected Map getPrivateDataReminderSettings() settings.put("reminderFrequency", getFormElement(Locator.input("reminderFrequency"))); settings.put("enablePublicationSearch", String.valueOf(Locator.checkboxByName("enablePublicationSearch").findElement(getDriver()).isSelected())); settings.put("publicationSearchFrequency", getFormElement(Locator.input("publicationSearchFrequency"))); - settings.put("ncbiApiKey", getFormElement(Locator.input("ncbiApiKey"))); + // The saved key is never displayed. The placeholder is the only signal that one is stored. + settings.put("ncbiApiKeySaved", String.valueOf( + Locator.input("ncbiApiKey").findElement(getDriver()).getDomAttribute("placeholder").startsWith("A key is saved"))); return settings; } @@ -694,8 +696,8 @@ protected void savePrivateDataReminderSettings(String extensionLength, String de Locator.checkboxByName("enablePublicationSearch").findElement(getDriver()).isSelected()); if (ncbiApiKey != null) { - assertEquals("The saved NCBI API key should be displayed on the form", ncbiApiKey, - getFormElement(Locator.input("ncbiApiKey"))); + assertEquals("The form should report that a key is saved", "true", + getPrivateDataReminderSettings().get("ncbiApiKeySaved")); } } 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 898f64b7..2bd4834a 100644 --- a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java +++ b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java @@ -259,32 +259,44 @@ public void testPublicationSearchAndDismiss() assertNotNull("Expected lastReminderDate for dataset 2", dsStatus2AfterPost.get("LastReminderDate")); assertNotNull("Expected citation to be cached for dataset 2", dsStatus2AfterPost.get("Citation")); - // Verify the NCBI API key setting round-trips (set -> save -> re-read). + verifyNcbiApiKeySettings(); + } + + /** + * The saved key is a credential, so the form reports only whether one is stored. Guards the two + * ways that has gone wrong, displaying the key and erasing it when the field is left blank. + */ + private void verifyNcbiApiKeySettings() + { + if (Boolean.parseBoolean(_originalReminderSettings.get("ncbiApiKeySaved"))) + { + // A saved key cannot be read back, so a test that overwrote it could not put it back. + log("An NCBI API key is already saved on this server. Skipping the key settings checks."); + return; + } + savePrivateDataReminderSettings("2", "0", "0", true, "test-ncbi-api-key"); + assertEquals("The key itself 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")); - // Verify the configured key reaches the live eutils requests. NCBI rejects an invalid key - // with HTTP 400, so re-searching dataset 2, which found a publication above, should now - // find nothing. The mock service bypasses the key, so this runs only against real NCBI. if (!_useMockNcbi) { - // Capture the server error count immediately before the deliberate bad-key search so the - // assertion below only counts errors due to the bad-key search. - int serverErrorCount = getServerErrorCount(); - - searchPublicationsForDataset(panoramaPublicProject, TARGET_FOLDER_2, exptId2); - assertTextPresent("No publications found for this dataset."); - assertTextNotPresent(PMID_2); - - // The invalid key makes NCBI return HTTP 400, and errorDetail appends the response body - // to the logged message. - assertTrue("Server log should record NCBI's invalid-key error", - getServerErrors().contains("API key invalid")); - - // The bad-key search logs one error per failed NCBI call. Dataset 2 runs two PMC - // strategy searches, on ProteomeXchange ID and Panorama URL, plus the PubMed fallback. - // checkExpectedErrors clears exactly that many and fails on any others. - checkExpectedErrors(serverErrorCount + 3); + // Only real NCBI can reject a key. The mock never sends one. + click(Locator.tagWithClass("button", "labkey-button").withText("Validate")); + waitForElement(Locator.id("ncbiApiKeyValidationResult").containing("NCBI rejected this key")); } + + // Removing the key takes the explicit checkbox. + checkCheckbox(Locator.checkboxByName("clearNcbiApiKey")); + clickButton("Save"); + assertEquals("The saved key should be gone after Remove the saved key", "false", + getPrivateDataReminderSettings().get("ncbiApiKeySaved")); } /* @@ -515,8 +527,10 @@ public void resetAfterTest() _originalReminderSettings.get("extensionLength"), _originalReminderSettings.get("delayUntilFirstReminder"), _originalReminderSettings.get("reminderFrequency"), + // The test removes the key it saved, and a key saved before the run cannot be + // read back to restore it, so leave the key field alone here. Boolean.parseBoolean(_originalReminderSettings.get("enablePublicationSearch")), - _originalReminderSettings.get("ncbiApiKey")); + null); } } From 759f5d94899745671b5136521f803439062445b7 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Mon, 31 Aug 2026 16:35:54 -0700 Subject: [PATCH 07/12] Moved the NCBI API key coverage into its own Selenium test * Added NcbiApiKeyTest, which fails with instructions rather than skipping when a key is already saved * Covered the Validate button on TeamCity through the mock NCBI service, so nothing is skipped there * Removed the key coverage from PublicationSearchTest, which no longer saves or removes a site-wide key * Removed the test's own key in @After so a failed run cannot leave one that breaks every later search Co-Authored-By: Claude --- .../tests/panoramapublic/NcbiApiKeyTest.java | 153 ++++++++++++++++++ .../panoramapublic/PublicationSearchTest.java | 42 +---- 2 files changed, 156 insertions(+), 39 deletions(-) create mode 100644 panoramapublic/test/src/org/labkey/test/tests/panoramapublic/NcbiApiKeyTest.java diff --git a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/NcbiApiKeyTest.java b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/NcbiApiKeyTest.java new file mode 100644 index 00000000..b0c522db --- /dev/null +++ b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/NcbiApiKeyTest.java @@ -0,0 +1,153 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.labkey.test.tests.panoramapublic; + +import org.junit.After; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.labkey.remoteapi.CommandException; +import org.labkey.remoteapi.SimplePostCommand; +import org.labkey.test.BaseWebDriverTest; +import org.labkey.test.Locator; +import org.labkey.test.TestProperties; +import org.labkey.test.categories.External; +import org.labkey.test.categories.MacCossLabModules; + +import java.io.IOException; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +/** + * Covers the NCBI API key on the Private Data Reminder Settings page. The key is a credential, so + * the page stores it in the encrypted property store and never displays it again. + * + * The key is site wide and cannot be read back, so this test cannot save one without destroying + * whatever is already there. It fails rather than skips when a key is saved, so the missing + * coverage is visible instead of silent. + */ +@Category({External.class, MacCossLabModules.class}) +@BaseWebDriverTest.ClassTimeout(minutes = 5) +public class NcbiApiKeyTest extends PanoramaPublicBaseTest +{ + private static final String TEST_API_KEY = "test-ncbi-api-key"; + + private boolean _savedTestApiKey = false; + private boolean _useMockNcbi = false; + + @Test + public void testNcbiApiKeySettings() + { + setupMockNcbiService(); + + assertEquals("An NCBI API key is saved on this server. This test saves its own key and cannot" + + " restore yours, because a saved key is never readable. Remove the key on the" + + " Private Data Reminder Settings page, run this test, then enter the key again.", + "false", getPrivateDataReminderSettings().get("ncbiApiKeySaved")); + + _savedTestApiKey = true; + savePrivateDataReminderSettings("2", "0", "0", true, TEST_API_KEY); + + assertEquals("The saved key must never be rendered into the form", "", + getFormElement(Locator.input("ncbiApiKey"))); + + // Saving with the field left blank must keep the stored key. Every other field on this page + // is edited routinely, so a blank field cannot mean "remove the key". + savePrivateDataReminderSettings("3", "0", "0", true, ""); + assertEquals("A blank key field must leave the saved key alone", "true", + getPrivateDataReminderSettings().get("ncbiApiKeySaved")); + + verifyValidateButton(); + + // Removing a key takes the explicit checkbox. + checkCheckbox(Locator.checkboxByName("clearNcbiApiKey")); + clickButton("Save"); + _savedTestApiKey = false; + assertEquals("Remove the saved key should remove it", "false", + getPrivateDataReminderSettings().get("ncbiApiKeySaved")); + } + + /** + * Covers the button, the request it sends and the message it displays. The mock answers every + * request, so it reports the key as accepted. Only real NCBI rejects one. + */ + private void verifyValidateButton() + { + setFormElement(Locator.input("ncbiApiKey"), "not-a-real-key"); + click(Locator.lkButton("Validate")); + + String expected = _useMockNcbi ? "NCBI accepted this key" : "NCBI rejected this key"; + waitForElement(Locator.id("ncbiApiKeyValidationResult").containing(expected)); + + setFormElement(Locator.input("ncbiApiKey"), ""); + } + + /* + * On TeamCity, route NCBI requests through the mock so this test does not depend on NCBI being + * reachable. On a development machine, use the real service so a key can actually be rejected. + */ + private void setupMockNcbiService() + { + if (!TestProperties.isTestRunningOnTeamCity()) + { + return; + } + + try + { + SimplePostCommand command = new SimplePostCommand("panoramapublic", "setupMockNcbiService"); + command.execute(createDefaultConnection(), "/"); + _useMockNcbi = true; + log("Using mock NCBI service"); + } + catch (IOException | CommandException e) + { + fail("Failed to set up the mock NCBI service: " + e.getMessage()); + } + } + + private void restoreNcbiService() + { + try + { + SimplePostCommand command = new SimplePostCommand("panoramapublic", "restoreNcbiService"); + command.execute(createDefaultConnection(), "/"); + log("Restored real NCBI service"); + } + catch (IOException | CommandException e) + { + log("Warning: Failed to restore NCBI service: " + e.getMessage()); + } + } + + @After + public void removeTestApiKey() + { + if (_useMockNcbi) + { + restoreNcbiService(); + } + + if (_savedTestApiKey) + { + // Remove the key even when the test failed before its own removal step. A key NCBI + // rejects makes every publication search on this server fail, including the next run's. + getPrivateDataReminderSettings(); + checkCheckbox(Locator.checkboxByName("clearNcbiApiKey")); + clickButton("Save"); + } + } +} diff --git a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java index 2bd4834a..2a6bff8c 100644 --- a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java +++ b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java @@ -259,45 +259,8 @@ public void testPublicationSearchAndDismiss() assertNotNull("Expected lastReminderDate for dataset 2", dsStatus2AfterPost.get("LastReminderDate")); assertNotNull("Expected citation to be cached for dataset 2", dsStatus2AfterPost.get("Citation")); - verifyNcbiApiKeySettings(); } - /** - * The saved key is a credential, so the form reports only whether one is stored. Guards the two - * ways that has gone wrong, displaying the key and erasing it when the field is left blank. - */ - private void verifyNcbiApiKeySettings() - { - if (Boolean.parseBoolean(_originalReminderSettings.get("ncbiApiKeySaved"))) - { - // A saved key cannot be read back, so a test that overwrote it could not put it back. - log("An NCBI API key is already saved on this server. Skipping the key settings checks."); - return; - } - - savePrivateDataReminderSettings("2", "0", "0", true, "test-ncbi-api-key"); - assertEquals("The key itself 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")); - - if (!_useMockNcbi) - { - // Only real NCBI can reject a key. The mock never sends one. - click(Locator.tagWithClass("button", "labkey-button").withText("Validate")); - waitForElement(Locator.id("ncbiApiKeyValidationResult").containing("NCBI rejected this key")); - } - - // Removing the key takes the explicit checkbox. - checkCheckbox(Locator.checkboxByName("clearNcbiApiKey")); - clickButton("Save"); - assertEquals("The saved key should be gone after Remove the saved key", "false", - getPrivateDataReminderSettings().get("ncbiApiKeySaved")); - } /* * Navigate to the Panorama Public copy folder and get the experiment ID. @@ -527,11 +490,12 @@ public void resetAfterTest() _originalReminderSettings.get("extensionLength"), _originalReminderSettings.get("delayUntilFirstReminder"), _originalReminderSettings.get("reminderFrequency"), - // The test removes the key it saved, and a key saved before the run cannot be - // read back to restore it, so leave the key field alone here. + // A key saved before the run cannot be read back to restore it, so leave the + // key field alone here. Any key the test saved is removed below. Boolean.parseBoolean(_originalReminderSettings.get("enablePublicationSearch")), null); } + } @Override From 2a67949a9c4904546ad8f8b4d5f9c3afb3005131 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Mon, 31 Aug 2026 17:06:53 -0700 Subject: [PATCH 08/12] Hardened the NCBI retry loop against 429, interruption and unreadable error bodies * Retried 429, which is NCBI's answer when the request rate is exceeded and was failing fast as a 4xx * Disabled HttpClient's own retries, which doubled the requests MAX_HTTP_ATTEMPTS names on a persistent 503 * Stopped retrying once the thread is interrupted, since every later sleep throws at once and drops the backoff * Stopped the reminder job starting another experiment after interruption, which would run with no rate limit * Bounded the error body read and caught its ParseException, which could otherwise discard the HTTP status Co-Authored-By: Claude --- .../NcbiPublicationSearchServiceImpl.java | 103 +++++++++++++++--- .../pipeline/PrivateDataReminderJob.java | 8 ++ 2 files changed, 98 insertions(+), 13 deletions(-) diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java index a7dae20a..2953bacc 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java @@ -24,6 +24,7 @@ import org.apache.hc.client5.http.impl.classic.HttpClientBuilder; import org.apache.hc.client5.http.impl.io.BasicHttpClientConnectionManager; import org.apache.hc.client5.http.HttpResponseException; +import org.apache.hc.core5.http.ClassicHttpResponse; import org.apache.hc.core5.http.io.entity.EntityUtils; import org.apache.hc.core5.util.Timeout; import org.apache.logging.log4j.Logger; @@ -104,6 +105,9 @@ public static void setInstance(NcbiPublicationSearchService impl) private static final int MAX_HTTP_ATTEMPTS = 3; // initial try + 2 retries private static final int RETRY_BASE_DELAY_MS = 500; // exponential backoff base + private static final int TOO_MANY_REQUESTS = 429; // NCBI's answer when the request rate is exceeded + private static final int MAX_ERROR_BODY_CHARS = 500; + private static final String REDACTED = "REDACTED"; private static final Pattern API_KEY_PARAM = Pattern.compile("api_key=([^&\\s]*)"); @@ -406,7 +410,12 @@ protected String getString(String url, Logger log) throws IOException long delayMs = retryDelayMs(attempt); getLog(log).warn("NCBI request failed (attempt {} of {}). Retrying in {} ms. URL: {}. Cause: {}", attempt, MAX_HTTP_ATTEMPTS, delayMs, redactApiKey(url, apiKeyFrom(url)), e.toString()); - sleepMs(delayMs); + if (!sleepMs(delayMs)) + { + // The thread was interrupted. Retrying now would run the remaining attempts with + // no delay, because every later sleep throws at once. + throw e; + } } } } @@ -429,6 +438,9 @@ protected String executeGet(String url) throws IOException try (CloseableHttpClient client = HttpClientBuilder.create() .setDefaultRequestConfig(requestConfig) .setConnectionManager(connectionManager) + // HttpClient retries 429 and 503 once on its own, which would make MAX_HTTP_ATTEMPTS + // cost twice the requests it names. getString is the only retry. + .disableAutomaticRetries() .build()) { HttpGet getRequest = new HttpGet(url); @@ -436,19 +448,38 @@ protected String executeGet(String url) throws IOException int status = response.getCode(); if (status < 200 || status >= 300) { - // 5xx bodies are large, uninformative HTML error pages, so only client error - // bodies are read. - String body = (status >= 400 && status < 500 && response.getEntity() != null) - ? EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8) - : null; throw new HttpResponseException(status, - errorDetail(status, response.getReasonPhrase(), body, apiKeyFrom(url))); + errorDetail(status, response.getReasonPhrase(), readErrorBody(status, response), + apiKeyFrom(url))); } return EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8); }); } } + /** + * Read the body of a client error response. 5xx bodies are large, uninformative HTML error + * pages, so only client error bodies are read, and only the first {@link #MAX_ERROR_BODY_CHARS} + * characters. A body that cannot be read or parsed returns null, because losing NCBI's text is + * better than losing the status code the caller decides on. + */ + private static @Nullable String readErrorBody(int status, ClassicHttpResponse response) + { + if (status < 400 || status >= 500 || response.getEntity() == null) + { + return null; + } + + try + { + return EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8, MAX_ERROR_BODY_CHARS); + } + catch (IOException | org.apache.hc.core5.http.ParseException e) + { + return null; + } + } + /** * Build the message for a non-2xx HttpResponseException. For client errors (4xx) the response * body is appended, so NCBI's reason (e.g. "API key invalid") reaches the log. Other statuses @@ -493,7 +524,8 @@ static String redactApiKey(@Nullable String text, @Nullable String apiKey) } /** - * Read timeouts and 5xx responses are transient NCBI failures worth retrying. 4xx and other + * Read timeouts, 5xx responses and 429 are transient NCBI failures worth retrying. NCBI answers + * 429 when the request rate is exceeded, which is the failure backoff exists for. Other 4xx * errors are permanent. */ private static boolean isRetryable(IOException e) @@ -504,7 +536,7 @@ private static boolean isRetryable(IOException e) } if (e instanceof HttpResponseException hre) { - return hre.getStatusCode() >= 500; + return hre.getStatusCode() >= 500 || hre.getStatusCode() == TOO_MANY_REQUESTS; } return false; } @@ -517,15 +549,21 @@ private static long retryDelayMs(int attempt) return (long) RETRY_BASE_DELAY_MS << (attempt - 1); } - private static void sleepMs(long ms) + /** + * @return false if the thread was interrupted, in which case the caller should stop rather than + * carry on without the delay it asked for. + */ + private static boolean sleepMs(long ms) { try { Thread.sleep(ms); + return true; } catch (InterruptedException e) { Thread.currentThread().interrupt(); + return false; } } @@ -1580,7 +1618,10 @@ public void testIsRetryable() assertTrue(isRetryable(new HttpResponseException(500, "Internal Server Error"))); assertTrue(isRetryable(new HttpResponseException(503, "Service Unavailable"))); - // 4xx and generic IO errors are permanent -> fail fast + // 429 is NCBI's answer when the request rate is exceeded, which backoff is for + assertTrue(isRetryable(new HttpResponseException(429, "Too Many Requests"))); + + // Other 4xx and generic IO errors are permanent -> fail fast assertFalse(isRetryable(new HttpResponseException(400, "Bad Request"))); assertFalse(isRetryable(new HttpResponseException(404, "Not Found"))); assertFalse(isRetryable(new IOException("connection reset"))); @@ -1589,10 +1630,13 @@ public void testIsRetryable() @Test public void testRetryDelayMs() { - // Exponential backoff of 500ms, 1000ms, 2000ms per attempt. + // 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(2000, retryDelayMs(3)); + 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 @@ -1676,6 +1720,39 @@ protected String executeGet(String url) throws IOException assertEquals("Should retry until the 3rd attempt succeeds", 3, attempts[0]); } + @Test + public void testGetStringStopsWhenInterrupted() + { + // An interrupted thread cannot wait, so retrying would send the remaining attempts back + // to back. getString should give up after the first failure instead. + int[] attempts = {0}; + NcbiPublicationSearchServiceImpl service = new NcbiPublicationSearchServiceImpl() + { + @Override + protected String executeGet(String url) throws IOException + { + attempts[0]++; + Thread.currentThread().interrupt(); + throw new HttpResponseException(503, "Service Unavailable"); + } + }; + + try + { + service.getString("http://test", LOG); + fail("Expected the interrupted request to be rethrown"); + } + catch (IOException expected) + { + assertEquals("An interrupted request should not be retried", 1, attempts[0]); + } + finally + { + // Clear the flag so it cannot affect the tests that run after this one. + Thread.interrupted(); + } + } + @Test public void testGetStringGivesUpAfterMaxAttempts() { diff --git a/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java b/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java index e4a8b649..282ae46a 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java +++ b/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java @@ -341,6 +341,14 @@ private void processExperiments(List expAnnotationIds, ProcessingContex } for (Integer experimentAnnotationsId : exptIds) { + if (Thread.currentThread().isInterrupted()) + { + // Cancelling the job clears the NCBI rate limiter, because every sleep from here on + // throws at once. Stop instead of running the rest at full speed. + log.warn("Job was interrupted. Stopping before experiment {}.", experimentAnnotationsId); + break; + } + try (DbScope.Transaction transaction = PanoramaPublicManager.getSchema().getScope().ensureTransaction()) { processExperiment(experimentAnnotationsId, context, processingResults); From 89677df542d5ba608eab32d075bc759e598b9095 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Mon, 31 Aug 2026 17:10:07 -0700 Subject: [PATCH 09/12] Fixed two date tests that failed on the last days of a month * Took the current date from the expiry the settings calculate, rather than from today plus an offset * On 31 August, six months back then forward again gives 28 August, so the expiry fell before the date checked * Removed the currentDate and minutesOffset parameters that the change left always null and zero * Left the production arithmetic alone, since adding months and clamping to a shorter month is correct * Corrected two test comments, one naming a map key that was renamed and one naming the wrong cleanup method Co-Authored-By: Claude --- .../message/PrivateDataReminderSettings.java | 108 ++++++++++++------ .../PanoramaPublicBaseTest.java | 2 +- .../panoramapublic/PublicationSearchTest.java | 4 +- 3 files changed, 76 insertions(+), 38 deletions(-) diff --git a/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java b/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java index 9453b70d..376e25e5 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java +++ b/panoramapublic/src/org/labkey/panoramapublic/message/PrivateDataReminderSettings.java @@ -409,92 +409,130 @@ public void testIsLastReminderRecentScenarios() private void testExtensionIsValid(PrivateDataReminderSettings settings, int monthsOffset) { - testExtensionIsValid(settings, monthsOffset, 0, null, true); + testExtensionIsValid(settings, monthsOffset, true); } private void testExtensionIsExpired(PrivateDataReminderSettings settings, int monthsOffset) { - testExtensionIsValid(settings, monthsOffset, 0, null, false); + testExtensionIsValid(settings, monthsOffset, false); } private void testExtensionIsValidAsOf(PrivateDataReminderSettings settings, int monthsOffset, int minutesOffset) { - testExtensionIsValid(settings, monthsOffset, minutesOffset, dateFromNow(), true); + testExtensionAtExpiry(settings, monthsOffset, minutesOffset, true); } - private void testExtensionIsExpiredAsOf(PrivateDataReminderSettings settings, int monthsOffset, int minutesOffset) + /** + * Checks the boundary at the moment an extension expires. The current date comes from the + * expiry the settings calculate, not from today. Subtracting months and adding them back does + * not always return to the same day, because a shorter target month clamps the day, so a date + * built from today can sit on the wrong side of the boundary. On 31 August, subtracting six + * months gives 28 February and adding six back gives 28 August. + * + * @param minutesBeforeExpiry how long before the expiry to check. Zero is the expiry itself, + * and a negative value is after it. + */ + private void testExtensionAtExpiry(PrivateDataReminderSettings settings, int monthsOffset, + int minutesBeforeExpiry, boolean expectedValid) { - testExtensionIsValid(settings, monthsOffset, minutesOffset, dateFromNow(), false); + DatasetStatus datasetStatus = new DatasetStatus(); + datasetStatus.setExtensionRequestedDate(dateFromNow(monthsOffset, 0, 0)); + + Date expiry = settings.getExtensionValidUntilDate(datasetStatus); + Date currentDate = Date.from(expiry.toInstant().minusSeconds(minutesBeforeExpiry * 60L)); + + String failureMessage = String.format( + "Extension is %s; Extension Length: %d; Extension Requested On: %s; Valid Until: %s; Current Date: %s", + expectedValid ? "valid" : "expired", settings.getExtensionLength(), + datasetStatus.getExtensionRequestedDate(), expiry, currentDate); + + boolean isValid = settings.isExtensionValidAsOf(datasetStatus, currentDate); + if (expectedValid) assertTrue(failureMessage, isValid); + else assertFalse(failureMessage, isValid); } - private void testExtensionIsValid(PrivateDataReminderSettings settings, int monthsOffset, int minutesOffset, - Date currentDate, boolean expectedValid) + private void testExtensionIsExpiredAsOf(PrivateDataReminderSettings settings, int monthsOffset, int minutesOffset) { - Date extensionDate = dateFromNow(monthsOffset, 0, minutesOffset); + testExtensionAtExpiry(settings, monthsOffset, minutesOffset, false); + } + private void testExtensionIsValid(PrivateDataReminderSettings settings, int monthsOffset, boolean expectedValid) + { DatasetStatus datasetStatus = new DatasetStatus(); - datasetStatus.setExtensionRequestedDate(extensionDate); + datasetStatus.setExtensionRequestedDate(dateFromNow(monthsOffset, 0, 0)); String failureMessage = String.format("Extension is %s; Extension Length: %d; Extension Requested On: %s; Valid Until: %s", expectedValid ? "valid" : "expired", settings.getExtensionLength(), datasetStatus.getExtensionRequestedDate(), settings.getExtensionValidUntilDate(datasetStatus)); - if (currentDate != null) - { - failureMessage += String.format("; Current Date: %s", currentDate); - } - - boolean isValid = currentDate == null - ? settings.isExtensionValid(datasetStatus) - : settings.isExtensionValidAsOf(datasetStatus, currentDate); + + boolean isValid = settings.isExtensionValid(datasetStatus); if (expectedValid) assertTrue(failureMessage, isValid); else assertFalse(failureMessage, isValid); } private void testReminderIsRecent(PrivateDataReminderSettings settings, int daysOffset) { - testReminderIsRecent(settings, 0, daysOffset, 0, null, true); + testReminderIsRecent(settings, daysOffset, true); } private void testReminderIsOld(PrivateDataReminderSettings settings, int daysOffset) { - testReminderIsRecent(settings, 0, daysOffset, 0, null, false); + testReminderIsRecent(settings, daysOffset, false); } private void testReminderIsRecentAsOf(PrivateDataReminderSettings settings, int monthsOffset, int minutesOffset) { - testReminderIsRecent(settings, monthsOffset, 0, minutesOffset, dateFromNow(), true); + testReminderAtExpiry(settings, monthsOffset, minutesOffset, true); } private void testReminderIsOldAsOf(PrivateDataReminderSettings settings, int monthsOffset, int minutesOffset) { - testReminderIsRecent(settings, monthsOffset, 0, minutesOffset, dateFromNow(), false); + testReminderAtExpiry(settings, monthsOffset, minutesOffset, false); } - private void testReminderIsRecent(PrivateDataReminderSettings settings, int monthsOffset, int daysOffset, int minutesOffset, - Date currentDate, boolean expectedRecent) + /** + * Checks the boundary at the moment a reminder stops counting as recent. The current date + * comes from the date the settings calculate, for the reason given on + * {@link #testExtensionAtExpiry}. + * + * @param minutesBeforeExpiry how long before that date to check. Zero is the date itself, and + * a negative value is after it. + */ + private void testReminderAtExpiry(PrivateDataReminderSettings settings, int monthsOffset, + int minutesBeforeExpiry, boolean expectedRecent) { - Date reminderDate = dateFromNow(monthsOffset, daysOffset, minutesOffset); + DatasetStatus datasetStatus = new DatasetStatus(); + datasetStatus.setLastReminderDate(dateFromNow(monthsOffset, 0, 0)); + + Date expiry = settings.getReminderValidUntilDate(datasetStatus); + Date currentDate = Date.from(expiry.toInstant().minusSeconds(minutesBeforeExpiry * 60L)); + String failureMessage = String.format( + "Reminder is %s; Reminder Frequency: %d; Reminder Sent On: %s; Valid Until: %s; Current Date: %s", + expectedRecent ? "recent" : "old", settings.getReminderFrequency(), + datasetStatus.getLastReminderDate(), expiry, currentDate); + + boolean isRecent = settings.isLastReminderRecentAsOf(datasetStatus, currentDate); + if (expectedRecent) assertTrue(failureMessage, isRecent); + else assertFalse(failureMessage, isRecent); + } + + private void testReminderIsRecent(PrivateDataReminderSettings settings, int daysOffset, boolean expectedRecent) + { DatasetStatus datasetStatus = new DatasetStatus(); - datasetStatus.setLastReminderDate(reminderDate); + datasetStatus.setLastReminderDate(dateFromNow(0, daysOffset, 0)); String failureMessage = String.format("Reminder is %s; Reminder Frequency: %d; Reminder Sent On: %s; Valid Until: %s", expectedRecent ? "recent" : "old", settings.getReminderFrequency(), datasetStatus.getLastReminderDate(), settings.getReminderValidUntilDate(datasetStatus)); - if (currentDate != null) - { - failureMessage += String.format("; Current Date: %s", currentDate); - } - - boolean isValid = currentDate == null - ? settings.isLastReminderRecent(datasetStatus) - : settings.isLastReminderRecentAsOf(datasetStatus, currentDate); - if (expectedRecent) assertTrue(failureMessage, isValid); - else assertFalse(failureMessage, isValid); + + boolean isRecent = settings.isLastReminderRecent(datasetStatus); + if (expectedRecent) assertTrue(failureMessage, isRecent); + else assertFalse(failureMessage, isRecent); } private PrivateDataReminderSettings createTestSettingsExtensionLength(int extensionLength) diff --git a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicBaseTest.java b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicBaseTest.java index 425860c4..679c51f9 100644 --- a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicBaseTest.java +++ b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicBaseTest.java @@ -631,7 +631,7 @@ protected void verifyIsPublicColumn(String panoramaPublicProject, String experim /** * Navigate to the Private Data Reminder Settings page and read the current form values. - * Returns a map with keys: extensionLength, delayUntilFirstReminder, reminderFrequency, enablePublicationSearch, publicationSearchFrequency, ncbiApiKey. + * Returns a map with keys: extensionLength, delayUntilFirstReminder, reminderFrequency, enablePublicationSearch, publicationSearchFrequency, ncbiApiKeySaved. */ protected Map getPrivateDataReminderSettings() { 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 2a6bff8c..25124e57 100644 --- a/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java +++ b/panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PublicationSearchTest.java @@ -114,8 +114,8 @@ public void testPublicationSearchAndDismiss() // Step 1: Set up mock NCBI service if running on TeamCity setupMockNcbiService(); - // Capture the existing reminder settings up front so doCleanup can restore them exactly. - // The dev machine may already have a real NCBI API key (and other non-default values) set. + // 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 From 2bf6107e6c8bac695b67e95b3d557bf5fb0d23f5 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Mon, 31 Aug 2026 19:36:01 -0700 Subject: [PATCH 10/12] Checked the NCBI API key before the reminder job posts anything * PrivateDataReminderJob checks a configured key before any dataset, and errors without posting when NCBI rejects it * checkApiKey replaces validateApiKey and reports VALID, REJECTED or UNCONFIRMED, so an NCBI outage does not stop the run * The Validate button now reports a request that never reached NCBI separately from a rejected key * The end-of-run message no longer tells an admin to check the key when the failure was something else * Added a unit test for the classification, driven through an executeGet override so it needs no network Co-Authored-By: Claude --- .../PanoramaPublicController.java | 16 ++-- .../panoramapublic/ncbi/NcbiApiKeyCheck.java | 74 +++++++++++++++++++ .../ncbi/NcbiPublicationSearchService.java | 3 +- .../NcbiPublicationSearchServiceImpl.java | 53 ++++++++++++- .../pipeline/PrivateDataReminderJob.java | 44 ++++++++++- 5 files changed, 178 insertions(+), 12 deletions(-) create mode 100644 panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiApiKeyCheck.java diff --git a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java index c3d0ed24..d5c6b32f 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java +++ b/panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicController.java @@ -158,6 +158,7 @@ import org.labkey.panoramapublic.message.PrivateDataMessageScheduler; import org.labkey.panoramapublic.message.PrivateDataReminderSettings; import org.labkey.panoramapublic.ncbi.MockNcbiPublicationSearchService; +import org.labkey.panoramapublic.ncbi.NcbiApiKeyCheck; import org.labkey.panoramapublic.ncbi.NcbiPublicationSearchService; import org.labkey.panoramapublic.ncbi.NcbiPublicationSearchServiceImpl; import org.labkey.panoramapublic.ncbi.PublicationMatch; @@ -10107,9 +10108,9 @@ public Object execute(PrivateDataReminderSettingsForm form, BindException errors return response; } - String error = NcbiPublicationSearchService.get().validateApiKey(apiKey); - response.put("valid", error == null); - if (error == null) + NcbiApiKeyCheck check = NcbiPublicationSearchService.get().checkApiKey(apiKey); + response.put("valid", check.isValid()); + if (check.isValid()) { // Validating does not store anything, so say so. Otherwise "accepted" reads as // confirmation that the key is now in effect. @@ -10122,9 +10123,12 @@ public Object execute(PrivateDataReminderSettingsForm form, BindException errors { // The short message goes beside the field. NCBI's own words are offered separately, // since the admin holding the key is the one who has to act on them. - response.put("message", "NCBI rejected this key."); - response.put("detail", error); - LOG.warn("NCBI rejected an API key entered on the Private Data Reminder Settings page. {}", error); + 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; } diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiApiKeyCheck.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiApiKeyCheck.java new file mode 100644 index 00000000..b7bcf95e --- /dev/null +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiApiKeyCheck.java @@ -0,0 +1,74 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.labkey.panoramapublic.ncbi; + +import org.jetbrains.annotations.Nullable; + +/** + * The outcome of checking an NCBI API key. A rejected key is a configuration problem an admin has to + * correct. A check that could not be completed is not, so callers need to tell the two apart. + */ +public class NcbiApiKeyCheck +{ + public enum Status { VALID, REJECTED, UNCONFIRMED } + + private final Status _status; + private final String _message; + + private NcbiApiKeyCheck(Status status, @Nullable String message) + { + _status = status; + _message = message; + } + + public static NcbiApiKeyCheck valid() + { + return new NcbiApiKeyCheck(Status.VALID, null); + } + + public static NcbiApiKeyCheck rejected(@Nullable String message) + { + return new NcbiApiKeyCheck(Status.REJECTED, message); + } + + public static NcbiApiKeyCheck unconfirmed(@Nullable String message) + { + return new NcbiApiKeyCheck(Status.UNCONFIRMED, message); + } + + public Status getStatus() + { + return _status; + } + + /** + * @return NCBI's reason, with the API key removed. Null when the key was accepted. + */ + public @Nullable String getMessage() + { + return _message; + } + + public boolean isValid() + { + return _status == Status.VALID; + } + + public boolean isRejected() + { + return _status == Status.REJECTED; + } +} diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchService.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchService.java index e0b05baf..b18bd913 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchService.java +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchService.java @@ -40,9 +40,8 @@ static NcbiPublicationSearchService get() /** * Send a minimal request to NCBI with the given key. - * @return null if NCBI accepted the key, otherwise the reason it gave. */ - @Nullable String validateApiKey(@Nullable String apiKey); + @NotNull NcbiApiKeyCheck checkApiKey(@Nullable String apiKey); @Nullable Pair getPubMedLinkAndCitation(String pubmedId); diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java index 2953bacc..d0e6fd38 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java @@ -361,7 +361,7 @@ private List executeSearch(String query, String database, Logger log) } @Override - public @Nullable String validateApiKey(@Nullable String apiKey) + public @NotNull NcbiApiKeyCheck checkApiKey(@Nullable String apiKey) { // A minimal ESearch request. NCBI answers a rejected key with 400 and a reason, which the // retry loop does not retry, so this returns quickly either way. @@ -369,11 +369,20 @@ private List executeSearch(String query, String database, Logger log) try { getString(url, LOG); - return null; + return NcbiApiKeyCheck.valid(); + } + catch (HttpResponseException e) + { + // NCBI rejects a key it does not recognise with a 4xx. A 5xx says nothing about the key, + // so the caller is told the key could not be checked rather than that it is bad. + String message = redactApiKey(e.getMessage(), apiKey); + return e.getStatusCode() >= 400 && e.getStatusCode() < 500 + ? NcbiApiKeyCheck.rejected(message) + : NcbiApiKeyCheck.unconfirmed(message); } catch (IOException e) { - return redactApiKey(e.getMessage(), apiKey); + return NcbiApiKeyCheck.unconfirmed(redactApiKey(e.getMessage(), apiKey)); } } @@ -1720,6 +1729,44 @@ protected String executeGet(String url) throws IOException assertEquals("Should retry until the 3rd attempt succeeds", 3, attempts[0]); } + @Test + public void testCheckApiKey() + { + // NCBI rejects a key it does not recognise with a 4xx. The reminder job stops for that, + // so a 5xx has to be reported as a check that could not be completed. + assertEquals(NcbiApiKeyCheck.Status.REJECTED, checkApiKeyAgainst(new HttpResponseException(400, "Bad Request")).getStatus()); + assertEquals(NcbiApiKeyCheck.Status.UNCONFIRMED, checkApiKeyAgainst(new HttpResponseException(503, "Service Unavailable")).getStatus()); + assertEquals(NcbiApiKeyCheck.Status.UNCONFIRMED, checkApiKeyAgainst(new SocketTimeoutException("Read timed out")).getStatus()); + assertEquals(NcbiApiKeyCheck.Status.VALID, checkApiKeyAgainst(null).getStatus()); + + // The key must not travel back to the caller in NCBI's reason + NcbiApiKeyCheck rejected = checkApiKeyAgainst( + new HttpResponseException(400, "Bad Request - invalid key SECRET123"), "SECRET123"); + assertFalse("A rejection must not carry the key", rejected.getMessage().contains("SECRET123")); + } + + private NcbiApiKeyCheck checkApiKeyAgainst(IOException failure) + { + return checkApiKeyAgainst(failure, "test-key"); + } + + private NcbiApiKeyCheck checkApiKeyAgainst(IOException failure, String apiKey) + { + NcbiPublicationSearchServiceImpl service = new NcbiPublicationSearchServiceImpl() + { + @Override + protected String executeGet(String url) throws IOException + { + if (failure != null) + { + throw failure; + } + return "{\"esearchresult\":{\"idlist\":[]}}"; + } + }; + return service.checkApiKey(apiKey); + } + @Test public void testGetStringStopsWhenInterrupted() { diff --git a/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java b/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java index 282ae46a..eaf3971f 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java +++ b/panoramapublic/src/org/labkey/panoramapublic/pipeline/PrivateDataReminderJob.java @@ -40,6 +40,7 @@ import org.labkey.panoramapublic.model.ExperimentAnnotations; import org.labkey.panoramapublic.model.Journal; import org.labkey.panoramapublic.model.JournalSubmission; +import org.labkey.panoramapublic.ncbi.NcbiApiKeyCheck; import org.labkey.panoramapublic.ncbi.NcbiPublicationSearchService; import org.labkey.panoramapublic.ncbi.NcbiSearchException; import org.labkey.panoramapublic.ncbi.PublicationMatch; @@ -303,11 +304,52 @@ public void run() return; } + if (!ncbiApiKeyAccepted()) + { + setStatus(TaskStatus.error); + return; + } + postMessage(_experimentAnnotationsIds, _panoramaPublic); setStatus(TaskStatus.complete); } + /** + * Check a configured NCBI API key before any dataset is touched. A key NCBI rejects would fail the + * publication search for every dataset, so the job stops with nothing posted. The job goes ahead + * when the check could not be completed, since that says nothing about the key. + */ + private boolean ncbiApiKeyAccepted() + { + PrivateDataReminderSettings settings = PrivateDataReminderSettings.get(); + if (!settings.isEnablePublicationSearch() && !_forcePublicationCheck) + { + return true; + } + + String apiKey = settings.getNcbiApiKey(); + if (StringUtils.isBlank(apiKey)) + { + // Searches run without a key, at NCBI's lower request rate. + return true; + } + + NcbiApiKeyCheck check = NcbiPublicationSearchService.get().checkApiKey(apiKey); + if (check.isRejected()) + { + getLogger().error("NCBI rejected the API key, so no reminders were posted. Correct the key on the Private Data Reminder Settings page and run the job again. {}", + check.getMessage()); + return false; + } + + if (!check.isValid()) + { + getLogger().warn("Could not reach NCBI to check the API key. Continuing. {}", check.getMessage()); + } + return true; + } + private void postMessage(List expAnnotationIds, Journal panoramaPublic) { int total = expAnnotationIds.size(); @@ -778,7 +820,7 @@ public void logSkipped(Logger log) if (!_publicationSearchFailed.isEmpty()) { - log.error("Publication search failed for the following experiment Ids: {}. Check the NCBI API key in the Private Data Reminder Settings.", StringUtils.join(_publicationSearchFailed, ", ")); + log.error("Publication search failed for the following experiment Ids: {}. NCBI's reason is in the error logged for each one.", StringUtils.join(_publicationSearchFailed, ", ")); } if (!_submitterNotFound.isEmpty()) From 4981c02f2f5bf36ceeb0349e4a910baf16c920a3 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Tue, 1 Sep 2026 09:54:43 -0700 Subject: [PATCH 11/12] Made the retry loop's waiting overridable so tests can check the delays * sleepMs and rateLimit became instance methods, so a test subclass can replace the waiting * The retry tests now assert the delays the loop used, 500ms then 1000ms, which retryDelayMs alone cannot show * The 4xx test asserts the loop did not wait at all Co-Authored-By: Claude --- .../NcbiPublicationSearchServiceImpl.java | 33 +++++++++++++++---- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java index d0e6fd38..3c32b53c 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java @@ -562,7 +562,7 @@ private static long retryDelayMs(int attempt) * @return false if the thread was interrupted, in which case the caller should stop rather than * carry on without the delay it asked for. */ - private static boolean sleepMs(long ms) + protected boolean sleepMs(long ms) { try { @@ -1106,7 +1106,7 @@ static List extractTitleKeywords(String title) /** * Rate limiting: wait 400ms between API requests */ - private static void rateLimit() + private void rateLimit() { sleepMs(RATE_LIMIT_DELAY_MS); } @@ -1715,7 +1715,7 @@ public void testGetStringRetriesTransientFailures() throws IOException { // executeGet returns a 5xx twice, then succeeds. getString should retry and return the body. int[] attempts = {0}; - NcbiPublicationSearchServiceImpl service = new NcbiPublicationSearchServiceImpl() + NoWaitService service = new NoWaitService() { @Override protected String executeGet(String url) throws IOException @@ -1727,6 +1727,23 @@ protected String executeGet(String url) throws IOException }; assertEquals("body", service.getString("http://test", LOG)); assertEquals("Should retry until the 3rd attempt succeeds", 3, attempts[0]); + assertEquals("The loop should wait 500ms then 1000ms", List.of(500L, 1000L), service.sleeps); + } + + /** + * Runs the retry loop without waiting, and records the delays it asked for. A test can then + * check the delays the loop used, which retryDelayMs on its own cannot show. + */ + private static class NoWaitService extends NcbiPublicationSearchServiceImpl + { + private final List sleeps = new ArrayList<>(); + + @Override + protected boolean sleepMs(long ms) + { + sleeps.add(ms); + return true; + } } @Test @@ -1752,7 +1769,7 @@ private NcbiApiKeyCheck checkApiKeyAgainst(IOException failure) private NcbiApiKeyCheck checkApiKeyAgainst(IOException failure, String apiKey) { - NcbiPublicationSearchServiceImpl service = new NcbiPublicationSearchServiceImpl() + NcbiPublicationSearchServiceImpl service = new NoWaitService() { @Override protected String executeGet(String url) throws IOException @@ -1795,7 +1812,7 @@ protected String executeGet(String url) throws IOException } finally { - // Clear the flag so it cannot affect the tests that run after this one. + // Clear the flag so it cannot reach whatever test runs next. Thread.interrupted(); } } @@ -1805,7 +1822,7 @@ public void testGetStringGivesUpAfterMaxAttempts() { // executeGet always returns a 5xx. getString should try MAX_HTTP_ATTEMPTS times, then rethrow. int[] attempts = {0}; - NcbiPublicationSearchServiceImpl service = new NcbiPublicationSearchServiceImpl() + NoWaitService service = new NoWaitService() { @Override protected String executeGet(String url) throws IOException @@ -1828,6 +1845,7 @@ protected String executeGet(String url) throws IOException fail("Expected HttpResponseException, got " + e); } assertEquals("Should attempt exactly MAX_HTTP_ATTEMPTS times", MAX_HTTP_ATTEMPTS, attempts[0]); + assertEquals("One wait fewer than attempts, and no wait after the last", List.of(500L, 1000L), service.sleeps); } @Test @@ -1835,7 +1853,7 @@ public void testGetStringDoesNotRetryClientErrors() { // A 4xx is permanent. getString should fail immediately without retrying. int[] attempts = {0}; - NcbiPublicationSearchServiceImpl service = new NcbiPublicationSearchServiceImpl() + NoWaitService service = new NoWaitService() { @Override protected String executeGet(String url) throws IOException @@ -1858,6 +1876,7 @@ protected String executeGet(String url) throws IOException fail("Expected HttpResponseException, got " + e); } assertEquals("4xx must not be retried", 1, attempts[0]); + assertTrue("A 4xx must not wait", service.sleeps.isEmpty()); } // -- Helper methods for building test JSON -- From 151992f39bec6c0a837ed3163483b72af2652cb8 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Tue, 1 Sep 2026 10:40:47 -0700 Subject: [PATCH 12/12] Stopped a 429 from the API key check looking like a rejected key * checkApiKey reports a 429 as unconfirmed rather than rejected Co-Authored-By: Claude --- .../ncbi/NcbiPublicationSearchServiceImpl.java | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java index 3c32b53c..fc0a14d3 100644 --- a/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java +++ b/panoramapublic/src/org/labkey/panoramapublic/ncbi/NcbiPublicationSearchServiceImpl.java @@ -363,8 +363,8 @@ private List executeSearch(String query, String database, Logger log) @Override public @NotNull NcbiApiKeyCheck checkApiKey(@Nullable String apiKey) { - // A minimal ESearch request. NCBI answers a rejected key with 400 and a reason, which the - // retry loop does not retry, so this returns quickly either way. + // A minimal ESearch request. A rejected key comes back as a 400 on the first attempt, while + // a 5xx or 429 is retried like any other request before it is called unconfirmed. String url = ESEARCH_URL + "?" + buildCommonParams("pubmed", apiKey) + "&term=labkey&retmax=1&retmode=json"; try { @@ -373,12 +373,12 @@ private List executeSearch(String query, String database, Logger log) } catch (HttpResponseException e) { - // NCBI rejects a key it does not recognise with a 4xx. A 5xx says nothing about the key, - // so the caller is told the key could not be checked rather than that it is bad. + // NCBI rejects a key it does not recognise with a 4xx. A 429 is the request rate and a + // 5xx is NCBI's own failure, so neither counts as a rejection. String message = redactApiKey(e.getMessage(), apiKey); - return e.getStatusCode() >= 400 && e.getStatusCode() < 500 - ? NcbiApiKeyCheck.rejected(message) - : NcbiApiKeyCheck.unconfirmed(message); + boolean rejected = e.getStatusCode() >= 400 && e.getStatusCode() < 500 + && e.getStatusCode() != TOO_MANY_REQUESTS; + return rejected ? NcbiApiKeyCheck.rejected(message) : NcbiApiKeyCheck.unconfirmed(message); } catch (IOException e) { @@ -1753,6 +1753,10 @@ public void testCheckApiKey() // so a 5xx has to be reported as a check that could not be completed. assertEquals(NcbiApiKeyCheck.Status.REJECTED, checkApiKeyAgainst(new HttpResponseException(400, "Bad Request")).getStatus()); assertEquals(NcbiApiKeyCheck.Status.UNCONFIRMED, checkApiKeyAgainst(new HttpResponseException(503, "Service Unavailable")).getStatus()); + + // A 429 is the request rate, not a bad key. Calling it a rejection would stop the + // reminder job and send an admin to correct a key that works. + assertEquals(NcbiApiKeyCheck.Status.UNCONFIRMED, checkApiKeyAgainst(new HttpResponseException(429, "Too Many Requests")).getStatus()); assertEquals(NcbiApiKeyCheck.Status.UNCONFIRMED, checkApiKeyAgainst(new SocketTimeoutException("Read timed out")).getStatus()); assertEquals(NcbiApiKeyCheck.Status.VALID, checkApiKeyAgainst(null).getStatus());