From 3880fb8230c0fdf6b63bf3a414ab2d632dd384dd Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Thu, 25 Jun 2026 12:20:46 +0200 Subject: [PATCH 1/3] test: de-flake ORCID cache tests and ZIP-download IT Two independent flaky tests keep turning the dtq-dev pipeline red after #1321: 1. CachingOrcidRestConnectorTest.testCachable / testCacheableWithError still hit the live ORCID sandbox (#1321 only mocked getLabel/search/search_fail). They use the real Spring @Cacheable bean (to exercise the CGLIB caching proxy), so they could not be spied. Point the bean's apiURL at a local MockWebServer serving the canned orcid-expanded-search.xml instead -- keeps the real caching proxy and HTTP transport under test, removes the network dependency. No production change. 2. MetadataBitstreamControllerIT.downloadAllZip compared the response to a locally-built ZIP byte-for-byte; a ZIP entry's DOS timestamp (2s resolution) defaults to "now" on both sides and differs across a 2s boundary. Assert the unzipped entry name + content instead of raw bytes. Test-only changes. Verified locally (ORCID class 8/8 green, repeated offline runs; webapp test module compiles; checkstyle clean). Co-Authored-By: Claude Opus 4.8 --- .../CachingOrcidRestConnectorTest.java | 87 ++++++++++++------- .../rest/MetadataBitstreamControllerIT.java | 51 ++++++----- 2 files changed, 85 insertions(+), 53 deletions(-) diff --git a/dspace-api/src/test/java/org/dspace/external/CachingOrcidRestConnectorTest.java b/dspace-api/src/test/java/org/dspace/external/CachingOrcidRestConnectorTest.java index 7e8cbc6c94fc..47d8af106079 100644 --- a/dspace-api/src/test/java/org/dspace/external/CachingOrcidRestConnectorTest.java +++ b/dspace-api/src/test/java/org/dspace/external/CachingOrcidRestConnectorTest.java @@ -19,7 +19,10 @@ import java.io.IOException; import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; import org.dspace.AbstractDSpaceTest; import org.dspace.external.provider.orcid.xml.ExpandedSearchConverter; import org.dspace.utils.DSpace; @@ -53,6 +56,19 @@ private InputStream cannedResponse(String resource) { return is; } + /** + * Build a canned 200 OK ORCID "expanded-search" response for the mock HTTP server, so the cache-aware + * tests below exercise the real Spring {@code @Cacheable} bean without depending on the live ORCID sandbox. + */ + private MockResponse cannedOrcidResponse() throws IOException { + try (InputStream is = cannedResponse(EXPANDED_SEARCH_XML)) { + return new MockResponse() + .setResponseCode(200) + .setHeader("Content-Type", "application/vnd.orcid+xml") + .setBody(new String(is.readAllBytes(), StandardCharsets.UTF_8)); + } + } + @Before public void setup() { sut = new CachingOrcidRestConnector(); @@ -134,7 +150,7 @@ public void search_fail() throws Exception { } @Test - public void testCachable() { + public void testCachable() throws IOException { CachingOrcidRestConnector c = new DSpace().getServiceManager().getServiceByName( "CachingOrcidRestConnector", CachingOrcidRestConnector.class); @@ -148,43 +164,56 @@ public void testCachable() { verify(c, times(1)).getLabel(orcid); */ - c.setApiURL("https://pub.sandbox.orcid.org/v3.0"); - c.forceAccessToken(sandboxToken); - - String r1 = c.getLabel(orcid); - assertEquals(expectedLabel, r1); - String r2 = c.getLabel(orcid); - assertEquals(expectedLabel, r2); - //get the orcid-labels cache and verify that the label is there - assertEquals(expectedLabel, cache.get(orcid).get()); + // Drive the real Spring @Cacheable bean against a local mock HTTP server instead of the live ORCID + // sandbox, whose dataset is periodically reset and previously caused intermittent CI failures. + try (MockWebServer server = new MockWebServer()) { + // Two responses are enqueued, but with caching working only the FIRST getLabel() hits the server; + // the second is served from the "orcid-labels" cache (asserted via getRequestCount() below). + server.enqueue(cannedOrcidResponse()); + server.enqueue(cannedOrcidResponse()); + + c.setApiURL(server.url("/v3.0").toString()); + c.forceAccessToken(sandboxToken); + + String r1 = c.getLabel(orcid); + assertEquals(expectedLabel, r1); + String r2 = c.getLabel(orcid); + assertEquals(expectedLabel, r2); + //get the orcid-labels cache and verify that the label is there + assertEquals(expectedLabel, cache.get(orcid).get()); + //caching means two getLabel() calls produced a single ORCID API request + assertEquals("Expected getLabel to be cached after the first call", 1, server.getRequestCount()); + } } @Test - public void testCacheableWithError() { + public void testCacheableWithError() throws IOException { CachingOrcidRestConnector c = new DSpace().getServiceManager().getServiceByName( "CachingOrcidRestConnector", CachingOrcidRestConnector.class); Cache cache = prepareCache(); assertNull(cache.get(orcid)); - //skip init - c.forceAccessToken(sandboxToken); - //set bad ApiURL to provoke an error - c.setApiURL("https://api.sandbox.orcid.org/"); - String r1 = c.getLabel(orcid); - //on error, getLabel should return null - assertNull(r1); - //the cache should not contain a value for this id - assertNull(cache.get(orcid)); - - //fix the error - c.setApiURL("https://pub.sandbox.orcid.org/v3.0"); - // the error flipped the initialized flag, this reset it - c.forceAccessToken(sandboxToken); - String r2 = c.getLabel(orcid); - assertEquals(expectedLabel, r2); - //the cache should now contain a value for this id - assertEquals(expectedLabel, cache.get(orcid).get()); + try (MockWebServer server = new MockWebServer()) { + //the (mock) ORCID API returns an error first, then a valid response + server.enqueue(new MockResponse().setResponseCode(500)); + server.enqueue(cannedOrcidResponse()); + + //skip init (force a token so getAccessToken/init never reaches out to the network) + c.forceAccessToken(sandboxToken); + c.setApiURL(server.url("/v3.0").toString()); + String r1 = c.getLabel(orcid); + //on error, getLabel should return null + assertNull(r1); + //a null result must NOT be cached (see @Cacheable(unless = "#result == null")) + assertNull(cache.get(orcid)); + + //the second call gets the valid (200) response; the error never cleared the token, so no re-init needed + String r2 = c.getLabel(orcid); + assertEquals(expectedLabel, r2); + //the cache should now contain a value for this id + assertEquals(expectedLabel, cache.get(orcid).get()); + } } private Cache prepareCache() { diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java index 63f7e62206f8..9925b1b54e29 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java @@ -7,17 +7,20 @@ */ package org.dspace.app.rest; +import static org.junit.Assert.assertEquals; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; -import java.io.ByteArrayOutputStream; +import java.io.ByteArrayInputStream; import java.io.InputStream; -import java.util.zip.Deflater; +import java.nio.charset.StandardCharsets; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; +import java.util.zip.ZipEntry; +import java.util.zip.ZipInputStream; import org.apache.commons.codec.CharEncoding; -import org.apache.commons.compress.archivers.zip.ZipArchiveEntry; -import org.apache.commons.compress.archivers.zip.ZipArchiveOutputStream; import org.apache.commons.io.IOUtils; import org.dspace.app.rest.model.ItemRest; import org.dspace.app.rest.test.AbstractControllerIntegrationTest; @@ -29,7 +32,6 @@ import org.dspace.content.Bitstream; import org.dspace.content.Collection; import org.dspace.content.Item; -import org.dspace.content.service.BitstreamService; import org.junit.Test; import org.springframework.beans.factory.annotation.Autowired; @@ -38,6 +40,7 @@ public class MetadataBitstreamControllerIT extends AbstractControllerIntegration private static final String ALL_ZIP_PATH = "allzip"; private static final String HANDLE_PARAM = "handleId"; private static final String AUTHOR = "Test author name"; + private static final String BITSTREAM_CONTENT = "ThisIsSomeDummyText"; private Collection col; private Item publicItem; @@ -46,9 +49,6 @@ public class MetadataBitstreamControllerIT extends AbstractControllerIntegration @Autowired AuthorizeService authorizeService; - @Autowired - BitstreamService bitstreamService; - @Override public void setUp() throws Exception { @@ -64,7 +64,7 @@ public void setUp() throws Exception { .withAuthor(AUTHOR) .build(); - String bitstreamContent = "ThisIsSomeDummyText"; + String bitstreamContent = BITSTREAM_CONTENT; try (InputStream is = IOUtils.toInputStream(bitstreamContent, CharEncoding.UTF_8)) { bts = BitstreamBuilder. createBitstream(context, publicItem, is) @@ -78,23 +78,26 @@ public void setUp() throws Exception { @Test public void downloadAllZip() throws Exception { - ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream(); - ZipArchiveOutputStream zip = new ZipArchiveOutputStream(byteArrayOutputStream); - zip.setCreateUnicodeExtraFields(ZipArchiveOutputStream.UnicodeExtraFieldPolicy.ALWAYS); - zip.setLevel(Deflater.NO_COMPRESSION); - ZipArchiveEntry ze = new ZipArchiveEntry(bts.getName()); - zip.putArchiveEntry(ze); - InputStream is = bitstreamService.retrieve(context, bts); - org.apache.commons.compress.utils.IOUtils.copy(is, zip); - zip.closeArchiveEntry(); - is.close(); - zip.close(); - String token = getAuthToken(admin.getEmail(), password); - getClient(token).perform(get(METADATABITSTREAM_ENDPOINT + "/" + publicItem.getID() + + byte[] zipBytes = getClient(token).perform(get(METADATABITSTREAM_ENDPOINT + "/" + publicItem.getID() + "/" + ALL_ZIP_PATH).param(HANDLE_PARAM, publicItem.getHandle())) .andExpect(status().isOk()) - .andExpect(content().bytes(byteArrayOutputStream.toByteArray())); + .andReturn().getResponse().getContentAsByteArray(); + + // A ZIP entry stores a DOS last-modified timestamp that defaults to "now" at 2-second resolution, so + // comparing the response byte-for-byte against a locally-built ZIP intermittently failed when the server + // and the test happened to build their entries in different time buckets. Assert the meaningful payload + // instead: the archive must contain exactly the item's bitstream, with the expected content. + Map entries = new HashMap<>(); + try (ZipInputStream zis = new ZipInputStream(new ByteArrayInputStream(zipBytes))) { + ZipEntry entry; + while ((entry = zis.getNextEntry()) != null) { + entries.put(entry.getName(), new String(IOUtils.toByteArray(zis), StandardCharsets.UTF_8)); + zis.closeEntry(); + } + } + assertEquals(Set.of(bts.getName()), entries.keySet()); + assertEquals(BITSTREAM_CONTENT, entries.get(bts.getName())); } } From d10c8333b630ae4c7dbd105ec435ee30ef0d2425 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Thu, 25 Jun 2026 14:38:34 +0200 Subject: [PATCH 2/3] test: assert exactly one ZIP entry in downloadAllZip Address CodeRabbit: a Map keyed by entry name could mask duplicate ZIP entries (same filename overwrites). Track the entry count separately and assert it is 1, so an unexpected extra entry fails the test. Co-Authored-By: Claude Opus 4.8 --- .../org/dspace/app/rest/MetadataBitstreamControllerIT.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java index 9925b1b54e29..7fb2f4cecdf8 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java @@ -89,14 +89,18 @@ public void downloadAllZip() throws Exception { // and the test happened to build their entries in different time buckets. Assert the meaningful payload // instead: the archive must contain exactly the item's bitstream, with the expected content. Map entries = new HashMap<>(); + int entryCount = 0; try (ZipInputStream zis = new ZipInputStream(new ByteArrayInputStream(zipBytes))) { ZipEntry entry; while ((entry = zis.getNextEntry()) != null) { + entryCount++; entries.put(entry.getName(), new String(IOUtils.toByteArray(zis), StandardCharsets.UTF_8)); zis.closeEntry(); } } + // count tracked separately so a duplicate entry name can't be masked by the map + assertEquals(1, entryCount); assertEquals(Set.of(bts.getName()), entries.keySet()); assertEquals(BITSTREAM_CONTENT, entries.get(bts.getName())); } From 27bad9aba93eb50637b325e93330b366c056b8aa Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Thu, 25 Jun 2026 15:44:48 +0200 Subject: [PATCH 3/3] test: de-flake SSR authorization and Solr TopCountries ITs Two more intermittent failures on dtq-dev, both addressed at the root cause (test-only changes): 1. AuthorizationRestRepositoryIT.findByObjectSSRTest flaked with 400 instead of 200. The test sets dspace.server.ssr.url (used by Utils.getBaseObjectRestFromUri to resolve the request URI) and the AlwaysThrowExceptionFeature.turnoff flag via configurationService.setProperty(...). Such in-memory overrides are silently dropped when the combined config is rebuilt by the auto-reload listener (fires on any reloadable cfg file mtime change mid-run). When ssr.url is dropped the URI no longer resolves -> 400; if turnoff were also dropped the /search/object path would let alwaysexception throw -> 500. Fix: set both via JVM system properties (+ reloadConfig) so they sit in the highest-precedence override layer and survive auto-reload, cleared in @After. Same pattern as #1321's Shibboleth fix (AuthenticationRestControllerIT#setAuthenticationMethodSequence). Applied to all three SSR tests. 2. StatisticsRestRepositoryIT.topCountriesReport_Community_Visited flaked with an empty report (points: []). postView() commits with waitSearcher=false, so the just-posted view events can be invisible to the immediately-following report query (and a dropped solr-statistics.autoCommit override would skip the commit entirely). Fix: after posting the view events, force solrLoggerService.commit() (waitSearcher =true) so the events are flushed and visible before the report is queried. Co-Authored-By: Claude Opus 4.8 --- .../rest/AuthorizationRestRepositoryIT.java | 55 ++++++++++++++++--- .../app/rest/StatisticsRestRepositoryIT.java | 7 +++ 2 files changed, 53 insertions(+), 9 deletions(-) diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthorizationRestRepositoryIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthorizationRestRepositoryIT.java index 365e88597d8d..28bbf74a00dd 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthorizationRestRepositoryIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthorizationRestRepositoryIT.java @@ -63,6 +63,7 @@ import org.dspace.eperson.Group; import org.dspace.services.ConfigurationService; import org.hamcrest.Matchers; +import org.junit.After; import org.junit.Before; import org.junit.Test; import org.springframework.beans.factory.annotation.Autowired; @@ -134,6 +135,42 @@ public class AuthorizationRestRepositoryIT extends AbstractControllerIntegration */ private AuthorizationFeature trueForUsersInGroupTest; + private static final String SSR_URL_KEY = "dspace.server.ssr.url"; + private static final String SSR_URL = "http://ssr.example.com/api"; + private static final String ALWAYS_THROW_TURNOFF_KEY = + "org.dspace.app.rest.authorization.AlwaysThrowExceptionFeature.turnoff"; + + /** + * Configure the SSR object-by-URI resolution used by the {@code search/object} tests (disarm the + * AlwaysThrowExceptionFeature and define the SSR base URL), setting both as JVM system properties + * (+ {@link ConfigurationService#reloadConfig()}) instead of {@code configurationService.setProperty(...)}. + * + *

A plain {@code setProperty(...)} override only lives in the in-memory combined-config view and is + * silently dropped whenever that view is rebuilt by the auto-reload listener (which fires when any + * reloadable cfg file's mtime changes, e.g. another test writing {@code local.cfg}). When that rebuild + * lands mid-test the SSR url disappears and {@code search/object} no longer resolves the uri -> 400 (and a + * dropped turnoff would let {@code alwaysexception} throw -> 500). A system property sits in the + * highest-precedence override layer and is re-read on every rebuild, so it survives auto-reload; it is + * cleared in {@link #clearSsrObjectResolution()}. Same pattern as + * AuthenticationRestControllerIT#setAuthenticationMethodSequence.

+ */ + private void enableSsrObjectResolution() { + System.setProperty(ALWAYS_THROW_TURNOFF_KEY, "true"); + System.setProperty(SSR_URL_KEY, SSR_URL); + configurationService.reloadConfig(); + } + + /** + * Remove the system-property overrides set by {@link #enableSsrObjectResolution()} so they do not leak into + * other tests in the same JVM. Runs before the superclass {@code @After}, whose {@code reloadConfig()} then + * restores the on-disk defaults. + */ + @After + public void clearSsrObjectResolution() { + System.clearProperty(SSR_URL_KEY); + System.clearProperty(ALWAYS_THROW_TURNOFF_KEY); + } + @Override @Before public void setUp() throws Exception { @@ -852,9 +889,9 @@ public void findByObjectSSRTest() throws Exception { SiteRest siteRest = siteConverter.convert(site, DefaultProjection.DEFAULT); String siteUri = "http://ssr.example.com/api/core/sites/" + siteRest.getId(); - // disarm the alwaysThrowExceptionFeature - configurationService.setProperty("org.dspace.app.rest.authorization.AlwaysThrowExceptionFeature.turnoff", true); - configurationService.setProperty("dspace.server.ssr.url", "http://ssr.example.com/api"); + // Disarm the alwaysThrowExceptionFeature and define the SSR base URL via system properties so the + // overrides survive a mid-test config auto-reload (see enableSsrObjectResolution). + enableSsrObjectResolution(); String adminToken = getAuthToken(admin.getEmail(), password); String epersonToken = getAuthToken(eperson.getEmail(), password); @@ -969,9 +1006,9 @@ public void findByObjectSSRTest() throws Exception { */ @Test public void findByObjectBadRequestSSRTest() throws Exception { - // disarm the alwaysThrowExceptionFeature - configurationService.setProperty("org.dspace.app.rest.authorization.AlwaysThrowExceptionFeature.turnoff", true); - configurationService.setProperty("dspace.server.ssr.url", "http://ssr.example.com/api"); + // Disarm the alwaysThrowExceptionFeature and define the SSR base URL via system properties so the + // overrides survive a mid-test config auto-reload (see enableSsrObjectResolution). + enableSsrObjectResolution(); String[] invalidUris = new String[] { "invalid-uri", "", @@ -1050,9 +1087,9 @@ public void findByObjectBadRequestSSRTest() throws Exception { public void findByNotExistingObjectSSSTest() throws Exception { String wrongSiteUri = "http://localhost/api/core/sites/" + UUID.randomUUID(); - // disarm the alwaysThrowExceptionFeature - configurationService.setProperty("org.dspace.app.rest.authorization.AlwaysThrowExceptionFeature.turnoff", true); - configurationService.setProperty("dspace.server.ssr.url", "http://ssr.example.com/api"); + // Disarm the alwaysThrowExceptionFeature and define the SSR base URL via system properties so the + // overrides survive a mid-test config auto-reload (see enableSsrObjectResolution). + enableSsrObjectResolution(); String adminToken = getAuthToken(admin.getEmail(), password); diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/StatisticsRestRepositoryIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/StatisticsRestRepositoryIT.java index 6989c208617d..716ccf8b8de6 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/StatisticsRestRepositoryIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/StatisticsRestRepositoryIT.java @@ -938,6 +938,13 @@ public void topCountriesReport_Community_Visited() throws Exception { .contentType(contentType)) .andExpect(status().isCreated()); + // Force a commit that waits for a new searcher so the two just-posted view events are guaranteed to be + // flushed and visible to the report query below. This covers both ways the report could otherwise come + // back empty: postView()'s own commit uses waitSearcher=false (can return before the searcher reopens), + // and if the solr-statistics.autoCommit=false override were dropped by a mid-test config reload then + // postView() skips its commit entirely (Solr's own autoCommit only fires after 10s). + StatisticsServiceFactory.getInstance().getSolrLoggerService().commit(); + // And request that collection's TopCountries report getClient(adminToken).perform( get("/api/statistics/usagereports/" + communityVisited.getID() + "_" + TOP_COUNTRIES_REPORT_ID))