From 62f392deb14ee76595a98c9fe7a7fd14765c75c3 Mon Sep 17 00:00:00 2001 From: milanmajchrak <90026355+milanmajchrak@users.noreply.github.com> Date: Wed, 17 Jun 2026 12:26:12 +0200 Subject: [PATCH 1/2] Fix flaky tests in IT pipeline (#1321) * Fixed integration tests because they use to fail sometimes * test: stabilize flaky CI tests (Hibernate cleanup retry, Shibboleth auth sequence reset, ORCID assertion hardening) * test: fix flaky ITs at the source (live ORCID, Shibboleth config-reload) + Hibernate CME diagnostics ORCID CachingOrcidRestConnectorTest no longer hits the live ORCID sandbox: search/getLabel/search_fail mock the HTTP layer (httpGet made protected) with a canned expanded-search response, so they are deterministic instead of asserting against fluctuating sandbox data. Shibboleth WWW-Authenticate flakiness: add a test-only config-definition.xml with config-reload=false. Runtime setProperty(...AuthenticationMethod...) overrides were silently discarded whenever the auto-reload listener rebuilt the combined config (restoring clarin-dspace.cfg's [Password, ClarinShib] default), intermittently leaking 'password realm' into the header. Verified: with auto-reload off the override survives; the explicit reloadConfig() reset in @After still works. Hibernate ConcurrentModificationException in @After cleanup: the per-session JDBC ResourceRegistry is not thread-safe, so the CME means two threads touch one Session. Capture a full thread dump on CME (target/cme-dumps/) to identify the colliding thread in CI; keep a resilient retry so an already-passed test isn't failed by this teardown race. (Context.finalize() ruled out: sessions are thread-local.) Co-Authored-By: Claude Opus 4.8 * test: revert IT-env config-reload=false override Disabling config auto-reload globally in the test environment broke AuthorizeConfigIT.testReloadConfiguration, which deliberately verifies that AuthorizeConfiguration picks up live changes written to local.cfg via the auto-reload mechanism. Auto-reload is a tested feature here, so it must not be disabled to work around the Shibboleth WWW-Authenticate flakiness. The Shibboleth flakiness (runtime setProperty override discarded when the combined config is rebuilt) needs a reload-safe fix in the auth test instead; tracked separately. Co-Authored-By: Claude Opus 4.8 * test: make Shibboleth auth-sequence override reload-safe (fix WWW-Authenticate flakiness) The flaky 'password realm' leak in AuthenticationRestControllerIT had this root cause: configurationService.setProperty(plugin.sequence...AuthenticationMethod, ...) only updates the in-memory view of the combined configuration. That view is discarded whenever it is rebuilt, and the auto-reload listener rebuilds it as soon as any reloadable cfg file's mtime changes mid-run (e.g. another test writing local.cfg). When that rebuild lands between the override and the request, clarin-dspace.cfg's default [PasswordAuthentication, ClarinShibAuthentication] returns and 'password realm' leaks into the header. The previous clear-then-set helper did not help (it is equivalent to a plain setProperty). Fix: set the sequence via a JVM system property (highest-precedence override layer, re-read on every rebuild) + reloadConfig(), and clear it in @After. This survives auto-reload without disabling it (so AuthorizeConfigIT, which verifies auto-reload, still passes). Verified in the real /api/authn/status endpoint: an explicit reloadConfig() after a setProperty override reproduces the leak, while the system-property approach keeps the header Shibboleth-only across rebuilds. Full AuthenticationRestControllerIT (43 tests) passes, and running it alongside ClarinAuthenticationRestControllerIT / AnonymousAdditionalAuthorizationFilterIT confirms the property does not leak across classes. Co-Authored-By: Claude Opus 4.8 * test: add Hibernate concurrency monitor + CI upload to pinpoint @After CME The intermittent ConcurrentModificationException in @After cleanup is a genuine cross-thread data race on Hibernate's per-session, non-thread-safe JDBC ResourceRegistry (xref): a second thread mutates the test thread's session while it commits/rolls back. Verified against hibernate-core-5.6.15 sources that the releaseResources forEach lambda never touches xref, so single-thread re-entrancy is impossible (this disproves the earlier HHH-15116 single-thread theory). The window is microseconds, so it does not reproduce locally even with deliberate cross-thread session sharing; it only surfaces under CI load. A live thread dump of a running IT JVM shows NO legitimate background thread ever touches Hibernate (all are Solr/HTTP/Jetty/JVM). So the culprit is a transient thread, and any non-test thread caught inside Hibernate JDBC/session code is by definition the offender. - HibernateConcurrencyMonitor: JVM-wide background sampler that records (de-duped) any non-test thread found inside org.hibernate.{resource.jdbc,engine.jdbc, internal.SessionImpl}; flushed to target/cme-dumps/ on CME and at JVM shutdown. Pure observer, never changes test behaviour. - AbstractIntegrationTestWithDatabase: start the monitor and mark the JUnit thread in setUp; flush it alongside the existing thread dump on a captured CME. - build.yml: always-upload **/target/cme-dumps/** (not gated on failure) so a successful cleanup retry no longer hides the diagnostic. Co-Authored-By: Claude Opus 4.8 * fix: don't close iterate() Hibernate stream from a finalize() (root cause of flaky CME) Root cause of the intermittent ConcurrentModificationException in @After integration-test cleanup, identified via the HibernateConcurrencyMonitor CI dumps: the GC Finalizer thread, running org.dspace.core.AbstractHibernateDAO$1.finalize(), closed the Hibernate Stream returned by AbstractHibernateDAO.iterate(). Closing a stream closes its ScrollableResults, which mutates the owning Session's per-session, non-thread-safe JDBC ResourceRegistry (xref) - but on the Finalizer thread, concurrently with the thread that owns the session. When that collided with the owning thread's commit/rollback (releaseResources -> xref.forEach), it threw ConcurrentModificationException. The CI dumps showed this exact finalizer stack as the only non-test thread inside Hibernate in dspace-api, and present in dspace-server-webapp too. This was confirmed genuine cross-thread access (not the previously assumed single-thread/HHH bug): verified against hibernate-core-5.6.15 sources that the releaseResources forEach lambda never touches xref, so single-thread re-entrancy is impossible. Fix: close the backing stream on the owning thread when iteration is exhausted, and remove the finalize() override. An iterator abandoned before exhaustion is released safely when its Context/Session is closed (releaseResources then runs on the owning thread). Adds AbstractHibernateDAOIteratorIT to guard against reintroducing a stream-closing finalizer. Co-Authored-By: Claude Opus 4.8 * fix: remove broken Context.finalize() that leaked finalizer-thread sessions Context.finalize() ran on the GC Finalizer thread and called dbConnection.isTransActionAlive()/abort(), which resolve sessionFactory.getCurrentSession() to a brand-new session bound to the Finalizer thread - never the (now-unreachable) thread that opened the Context. So it could not roll back the Context's transaction anyway; it only opened and leaked a throwaway Hibernate session on the Finalizer thread, and threw IllegalStateException once the SessionFactory was closed (seen in the CI thread dumps used to diagnose the flaky integration-test ConcurrentModificationException). Abandoned Contexts are cleaned up safely when their owning thread's session ends; callers already close Contexts via complete()/abort()/try-with-resources (Context is AutoCloseable). Removes the now-redundant ContextTest.testFinalize (close()/abort() are covered by testClose/testAbort/testAbort2). Co-Authored-By: Claude Opus 4.8 * test: remove flaky-CME diagnostic scaffolding and teardown retry (root cause fixed) The intermittent @After ConcurrentModificationException is now fixed at its source (AbstractHibernateDAO.iterate no longer closes its Hibernate stream from a finalizer; broken Context.finalize() removed). The temporary diagnostics that pinpointed it are no longer needed: - Restore AbstractIntegrationTestWithDatabase.destroy() to its plain form (drop the 3x cleanup retry and the per-CME thread dump) and remove the HibernateConcurrencyMonitor wiring. - Delete HibernateConcurrencyMonitor. - Revert the build.yml always-upload of target/cme-dumps. CI keeps -Dfailsafe.rerunFailingTestsCount=2 as the generic flaky-test safety net. Co-Authored-By: Claude Opus 4.8 * revert: keep Context.finalize() (out of scope, not the CME cause) Reverts the Context.finalize() removal (and the ContextTest.testFinalize deletion). The flaky @After ConcurrentModificationException is fully fixed by the AbstractHibernateDAO.iterate() change alone; Context.finalize() runs on a single GC Finalizer thread against its own finalizer-thread session and provably cannot cause that cross-thread xref race. Removing a finalizer from this core, widely-used class is a riskier change that does not belong in a flaky-test fix, so leave Context untouched. The (pre-existing, harmless) finalizer-thread session it opens can be addressed separately if desired. Co-Authored-By: Claude Opus 4.8 * test: address review comments on flaky-test fix - AbstractHibernateDAOIteratorIT: add Javadoc to the test method and walk the iterator's full class hierarchy (up to Object) when asserting no finalize() override, so a finalizer reintroduced on a superclass/helper is also caught (per CodeRabbit review). - AuthenticationRestControllerIT: wrap an over-length (122 char) Javadoc line. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 (cherry picked from commit 8f4b80d514a59a59b1c5032800da873e7ce4606c) --- .../org/dspace/core/AbstractHibernateDAO.java | 20 ++++- .../external/CachingOrcidRestConnector.java | 4 +- .../core/AbstractHibernateDAOIteratorIT.java | 76 +++++++++++++++++++ .../CachingOrcidRestConnectorTest.java | 46 +++++++++-- .../dspace/external/orcid-expanded-search.xml | 22 ++++++ .../rest/AuthenticationRestControllerIT.java | 76 ++++++++++++++----- 6 files changed, 214 insertions(+), 30 deletions(-) create mode 100644 dspace-api/src/test/java/org/dspace/core/AbstractHibernateDAOIteratorIT.java create mode 100644 dspace-api/src/test/resources/org/dspace/external/orcid-expanded-search.xml diff --git a/dspace-api/src/main/java/org/dspace/core/AbstractHibernateDAO.java b/dspace-api/src/main/java/org/dspace/core/AbstractHibernateDAO.java index 32ad747d765e..498c52c27f46 100644 --- a/dspace-api/src/main/java/org/dspace/core/AbstractHibernateDAO.java +++ b/dspace-api/src/main/java/org/dspace/core/AbstractHibernateDAO.java @@ -306,11 +306,23 @@ public Iterator iterate(Query query) { return new AbstractIterator () { @Override protected T computeNext() { - return iter.hasNext() ? iter.next() : endOfData(); - } - @Override - public void finalize() { + if (iter.hasNext()) { + return iter.next(); + } + // Close the backing ScrollableResults / JDBC cursor as soon as the iteration is exhausted, on + // the thread that owns the Hibernate Session (the caller's thread). + // + // This MUST NOT be done from a finalize() override (as it previously was): finalize() runs on + // the GC Finalizer thread, so closing the stream there mutates the Session's per-session, + // non-thread-safe JDBC ResourceRegistry (xref) concurrently with the owning thread. That is a + // genuine data race which intermittently throws ConcurrentModificationException from + // ResourceRegistryStandardImpl.releaseResources during an unrelated commit/rollback (observed + // as flaky integration-test failures in AbstractIntegrationTestWithDatabase teardown). + // + // An iterator abandoned before exhaustion no longer leaks: its open statement is released + // safely when the owning Context/Session is closed (releaseResources runs on the owning thread). stream.close(); + return endOfData(); } }; } diff --git a/dspace-api/src/main/java/org/dspace/external/CachingOrcidRestConnector.java b/dspace-api/src/main/java/org/dspace/external/CachingOrcidRestConnector.java index a1b529169566..90e307dcedf0 100644 --- a/dspace-api/src/main/java/org/dspace/external/CachingOrcidRestConnector.java +++ b/dspace-api/src/main/java/org/dspace/external/CachingOrcidRestConnector.java @@ -179,7 +179,9 @@ protected String getAccessToken(String clientSecret, String clientId, String OAU } } - private InputStream httpGet(String path, String accessToken) throws IOException { + // Package/sub-class visible so tests can stub the HTTP layer (see CachingOrcidRestConnectorTest) + // and avoid hitting the live ORCID sandbox. + protected InputStream httpGet(String path, String accessToken) throws IOException { String trimmedPath = path.replaceFirst("^/+", "").replaceFirst("/+$", ""); String fullPath = apiURL + '/' + trimmedPath; diff --git a/dspace-api/src/test/java/org/dspace/core/AbstractHibernateDAOIteratorIT.java b/dspace-api/src/test/java/org/dspace/core/AbstractHibernateDAOIteratorIT.java new file mode 100644 index 000000000000..fcc00434d620 --- /dev/null +++ b/dspace-api/src/test/java/org/dspace/core/AbstractHibernateDAOIteratorIT.java @@ -0,0 +1,76 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +package org.dspace.core; + +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.fail; + +import java.util.Iterator; + +import org.dspace.AbstractIntegrationTestWithDatabase; +import org.dspace.content.MetadataValue; +import org.dspace.content.factory.ContentServiceFactory; +import org.dspace.content.service.MetadataValueService; +import org.junit.Test; + +/** + * Regression test for the intermittent {@code ConcurrentModificationException} thrown during + * {@code @After} integration-test cleanup and traced to + * {@link AbstractHibernateDAO#iterate(javax.persistence.Query)}. + * + *

That method used to close its Hibernate {@code Stream} from a {@code finalize()} override. {@code finalize()} + * runs on the GC Finalizer thread, so closing the stream there mutated the owning {@code Session}'s per-session, + * non-thread-safe JDBC {@code ResourceRegistry} (xref) concurrently with the thread that owns the session. That + * is a genuine data race which intermittently threw {@code ConcurrentModificationException} from + * {@code ResourceRegistryStandardImpl.releaseResources} during an unrelated commit/rollback. The fix closes the + * stream on the owning thread once the iteration is exhausted. This test guards against reintroducing any + * stream-closing finalizer on the returned iterator.

+ */ +public class AbstractHibernateDAOIteratorIT extends AbstractIntegrationTestWithDatabase { + + private final MetadataValueService metadataValueService = + ContentServiceFactory.getInstance().getMetadataValueService(); + + /** + * Verifies that the iterator returned by {@link AbstractHibernateDAO#iterate(javax.persistence.Query)} + * (exercised here through {@code MetadataValueService.findByValueLike}) does not close its backing Hibernate + * stream from a {@code finalize()} override anywhere in its class hierarchy, and that it still iterates to + * exhaustion (closing its cursor on the owning thread) without error. No matching rows are required - the + * wrapper iterator is created regardless of the result count. + * + * @throws Exception passed through. + */ + @Test + public void iterateIteratorMustNotCloseStreamFromFinalizer() throws Exception { + Iterator iterator = + metadataValueService.findByValueLike(context, "no-such-metadata-value-" + System.nanoTime()); + assertNotNull(iterator); + + // The returned iterator - and every class in its hierarchy up to Object - MUST NOT declare a finalize() + // override: closing the backing Hibernate Stream from the GC Finalizer thread is exactly the cross-thread + // access to the non-thread-safe per-session JDBC ResourceRegistry that caused the flaky + // ConcurrentModificationException. Walking the hierarchy also catches a finalizer reintroduced on a + // superclass/helper rather than on the anonymous leaf class. + for (Class type = iterator.getClass(); type != null && type != Object.class; type = type.getSuperclass()) { + try { + type.getDeclaredMethod("finalize"); + fail("AbstractHibernateDAO.iterate() iterator must not declare a finalize() override (found on " + + type.getName() + ") - closing the Hibernate Stream on the GC Finalizer thread races the " + + "owning thread's non-thread-safe JDBC ResourceRegistry and intermittently throws " + + "ConcurrentModificationException."); + } catch (NoSuchMethodException expected) { + // good: no stream-closing finalizer on this class + } + } + + // It must still iterate to exhaustion and close its cursor on THIS (the owning) thread without error. + while (iterator.hasNext()) { + assertNotNull(iterator.next()); + } + } +} 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 a323a6026975..fb325a8f1bbb 100644 --- a/dspace-api/src/test/java/org/dspace/external/CachingOrcidRestConnectorTest.java +++ b/dspace-api/src/test/java/org/dspace/external/CachingOrcidRestConnectorTest.java @@ -13,9 +13,13 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import java.io.IOException; +import java.io.InputStream; + import org.dspace.AbstractDSpaceTest; import org.dspace.external.provider.orcid.xml.ExpandedSearchConverter; import org.dspace.utils.DSpace; @@ -34,8 +38,22 @@ public class CachingOrcidRestConnectorTest extends AbstractDSpaceTest { private static final String orcid = "0000-0002-9150-2529"; private static final String expectedLabel = "Connor, John"; + // Canned ORCID "expanded-search" response (num-found=1725, first result -> "Connor, John"). + // Used to mock the HTTP layer so the tests don't depend on the live ORCID sandbox. + private static final String EXPANDED_SEARCH_XML = "org/dspace/external/orcid-expanded-search.xml"; + private CachingOrcidRestConnector sut; + /** + * Load a canned API response from the test classpath as a fresh InputStream. + * (A new stream is returned on every call because the connector consumes/closes it.) + */ + private InputStream cannedResponse(String resource) { + InputStream is = getClass().getClassLoader().getResourceAsStream(resource); + assertNotNull("Missing test resource: " + resource, is); + return is; + } + @Before public void setup() { sut = new CachingOrcidRestConnector(); @@ -61,40 +79,54 @@ public void getAccessToken() { @Ignore @Test - public void getLabel() { + public void getLabel() throws Exception { sut = Mockito.spy(sut); sut.setApiURL("https://pub.sandbox.orcid.org/v3.0"); //Mock the CachingOrcidRestConnector so that getAccessToken returns sandboxToken doReturn(sandboxToken).when(sut).getAccessToken(Mockito.anyString(), Mockito.anyString(), Mockito.anyString()); + //Mock the HTTP layer with a canned response so we don't depend on the live ORCID sandbox. + doReturn(cannedResponse(EXPANDED_SEARCH_XML)).when(sut).httpGet(Mockito.anyString(), Mockito.anyString()); String label = sut.getLabel(orcid); assertEquals(expectedLabel, label); } @Test - public void search() { + public void search() throws Exception { sut = Mockito.spy(sut); sut.setApiURL("https://pub.sandbox.orcid.org/v3.0"); //Mock the CachingOrcidRestConnector so that getAccessToken returns sandboxToken doReturn(sandboxToken).when(sut).getAccessToken(Mockito.anyString(), Mockito.anyString(), Mockito.anyString()); + //Mock the HTTP layer with a canned ORCID expanded-search response. Previously this test hit the live + //ORCID sandbox and asserted numFound() > 1000, which flaked whenever the sandbox dataset was reset/shrunk. + //Mocking the transport keeps the real parsing + edismax wildcard query-building path under test, but makes + //the result deterministic. + doReturn(cannedResponse(EXPANDED_SEARCH_XML)).when(sut).httpGet(Mockito.anyString(), Mockito.anyString()); ExpandedSearchConverter.Results search = sut.search("joh", 0, 1); - //Should match all Johns also, because edismax with wildcard - assertTrue(search.numFound() > 1000); + assertTrue("Expected a successful ORCID response, got: " + search, search.isOk()); + //'joh' is alphabetic, so the connector turns it into an edismax wildcard query ("joh || joh*") that matches + //many authors; the canned response carries num-found=1725. + assertEquals("Unexpected num-found for the canned ORCID response", 1725L, (long) search.numFound()); + assertEquals("Connor, John", search.results().get(0).label()); } @Test - public void search_fail() { + public void search_fail() throws Exception { sut = Mockito.spy(sut); sut.setApiURL("https://pub.sandbox.orcid.org/v3.0"); - //Mock the CachingOrcidRestConnector so that getAccessToken returns and invalid token + //Mock the CachingOrcidRestConnector so that getAccessToken returns an invalid token doReturn("FAKE").when(sut).getAccessToken(Mockito.anyString(), Mockito.anyString(), Mockito.anyString()); + //Simulate the ORCID API rejecting the (fake) token: every httpGet fails. Done via the mocked HTTP layer + //so the test is deterministic and doesn't rely on the live sandbox returning a 401. + doThrow(new IOException("simulated ORCID auth failure")).when(sut) + .httpGet(Mockito.anyString(), Mockito.anyString()); ExpandedSearchConverter.Results search = sut.search("joh", 0, 1); assertFalse(search.isOk()); - //Further calls fail too, token is stored + //Further calls fail too, token is stored (so getAccessToken is only resolved once) search = sut.search("joh", 0, 1); assertFalse(search.isOk()); diff --git a/dspace-api/src/test/resources/org/dspace/external/orcid-expanded-search.xml b/dspace-api/src/test/resources/org/dspace/external/orcid-expanded-search.xml new file mode 100644 index 000000000000..582731a42038 --- /dev/null +++ b/dspace-api/src/test/resources/org/dspace/external/orcid-expanded-search.xml @@ -0,0 +1,22 @@ + + + + + 0000-0002-9150-2529 + John + Connor + + + 0000-0002-1208-2352 + John + Kendrew + John Kendrew + + diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthenticationRestControllerIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthenticationRestControllerIT.java index 63318926d752..b23811f27f17 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthenticationRestControllerIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthenticationRestControllerIT.java @@ -71,6 +71,7 @@ import org.dspace.orcid.model.OrcidTokenResponseDTO; import org.dspace.services.ConfigurationService; import org.hamcrest.Matchers; +import org.junit.After; import org.junit.Before; import org.junit.Ignore; import org.junit.Test; @@ -131,6 +132,45 @@ public class AuthenticationRestControllerIT extends AbstractControllerIntegratio private final String feature = CanChangePasswordFeature.NAME; + /** + * Configuration key for the ordered list of active AuthenticationMethod plugins. + */ + private static final String AUTH_PLUGIN_KEY = + "plugin.sequence.org.dspace.authenticate.AuthenticationMethod"; + + /** + * Replace the active AuthenticationMethod plugin sequence. + * + *

This sets the sequence via a JVM system property (plus an explicit + * {@link org.dspace.services.ConfigurationService#reloadConfig()} so the change is visible + * immediately) rather than via {@link org.dspace.services.ConfigurationService#setProperty(String, Object)}.

+ * + *

A plain {@code setProperty(...)} override only lives in the in-memory view of the combined + * configuration and is silently discarded whenever that view is rebuilt. The auto-reload listener + * rebuilds it as soon as any reloadable cfg file's last-modified timestamp changes (which happens + * intermittently during a CI run, e.g. another test writing {@code local.cfg}). When that rebuild lands + * between this call and the request under test, the on-disk default returns -- in CLARIN that default is + * {@code [PasswordAuthentication, ClarinShibAuthentication]} -- and a stray {@code password realm} leaks + * into the {@code WWW-Authenticate} header even though only e.g. Shibboleth was requested. A system + * property sits in the highest-precedence (override) section of the combined config and is re-read on + * every rebuild, so it survives auto-reload. It is cleared again in + * {@link #clearAuthenticationMethodSequence()}.

+ */ + private void setAuthenticationMethodSequence(String[] methods) { + System.setProperty(AUTH_PLUGIN_KEY, String.join(",", methods)); + configurationService.reloadConfig(); + } + + /** + * Remove the system-property override set by {@link #setAuthenticationMethodSequence(String[])} so it + * does not leak into other test classes running in the same JVM. Runs before the superclass @After, + * whose {@code reloadConfig()} then restores the on-disk default. + */ + @After + public void clearAuthenticationMethodSequence() { + System.clearProperty(AUTH_PLUGIN_KEY); + } + @Before public void setup() throws Exception { super.setUp(); @@ -140,7 +180,7 @@ public void setup() throws Exception { authorization = new Authorization(eperson, canChangePasswordFeature, ePersonRest); // Default all tests to Password Authentication only - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", PASS_ONLY); + setAuthenticationMethodSequence(PASS_ONLY); } @Test @@ -198,7 +238,7 @@ public void testStatusGetSpecialGroups() throws Exception { .withName("specialGroupIP") .build(); - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", PASS_AND_IP); + setAuthenticationMethodSequence(PASS_AND_IP); configurationService.setProperty("authentication-password.login.specialgroup","specialGroupPwd"); configurationService.setProperty("authentication-ip.specialGroupIP", "123.123.123.123"); context.restoreAuthSystemState(); @@ -338,7 +378,7 @@ public void testStatusNotAuthenticated() throws Exception { // @Test // public void testStatusShibAuthenticatedWithCookie() throws Exception { // //Enable Shibboleth login only -// configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_ONLY); +// setAuthenticationMethodSequence(SHIB_ONLY); // // String uiURL = configurationService.getProperty("dspace.ui.url"); // @@ -458,7 +498,7 @@ public void testStatusNotAuthenticated() throws Exception { // @Test // public void testShibbolethEndpointCannotBeUsedWithShibDisabled() throws Exception { // // Enable only password login -// configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", PASS_ONLY); +// setAuthenticationMethodSequence(PASS_ONLY); // // String uiURL = configurationService.getProperty("dspace.ui.url"); // @@ -977,7 +1017,7 @@ public void testLoginGetRequest() throws Exception { public void testShibbolethLoginURLWithDefaultLazyURL() throws Exception { context.turnOffAuthorisationSystem(); //Enable Shibboleth login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_ONLY); + setAuthenticationMethodSequence(SHIB_ONLY); //Create a reviewers group Group reviewersGroup = GroupBuilder.createGroup(context) @@ -1001,7 +1041,7 @@ public void testShibbolethLoginURLWithDefaultLazyURL() throws Exception { public void testShibbolethLoginURLWithServerURLContainingPort() throws Exception { context.turnOffAuthorisationSystem(); //Enable Shibboleth login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_ONLY); + setAuthenticationMethodSequence(SHIB_ONLY); configurationService.setProperty("dspace.server.url", "http://localhost:8080/server"); configurationService.setProperty("authentication-shibboleth.lazysession.secure", false); @@ -1027,7 +1067,7 @@ public void testShibbolethLoginURLWithServerURLContainingPort() throws Exception public void testShibbolethLoginURLWithConfiguredLazyURL() throws Exception { context.turnOffAuthorisationSystem(); //Enable Shibboleth login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_ONLY); + setAuthenticationMethodSequence(SHIB_ONLY); configurationService.setProperty("authentication-shibboleth.lazysession.loginurl", "http://shibboleth.org/Shibboleth.sso/Login"); @@ -1053,7 +1093,7 @@ public void testShibbolethLoginURLWithConfiguredLazyURL() throws Exception { public void testShibbolethLoginURLWithConfiguredLazyURLWithPort() throws Exception { context.turnOffAuthorisationSystem(); //Enable Shibboleth login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_ONLY); + setAuthenticationMethodSequence(SHIB_ONLY); configurationService.setProperty("authentication-shibboleth.lazysession.loginurl", "http://shibboleth.org:8080/Shibboleth.sso/Login"); @@ -1081,7 +1121,7 @@ public void testShibbolethLoginURLWithConfiguredLazyURLWithPort() throws Excepti public void testShibbolethLoginRequestAttribute() throws Exception { context.turnOffAuthorisationSystem(); //Enable Shibboleth login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_ONLY); + setAuthenticationMethodSequence(SHIB_ONLY); //Create a reviewers group Group reviewersGroup = GroupBuilder.createGroup(context) @@ -1137,7 +1177,7 @@ public void testShibbolethLoginRequestAttribute() throws Exception { @Ignore // Ignored until an endpoint is added to return all groups public void testShibbolethLoginRequestHeaderWithIpAuthentication() throws Exception { - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_AND_IP); + setAuthenticationMethodSequence(SHIB_AND_IP); configurationService.setProperty("authentication-ip.Administrator", "123.123.123.123"); @@ -1210,7 +1250,7 @@ public void testShibbolethLoginRequestHeaderWithIpAuthentication() throws Except @Test public void testShibbolethAndPasswordAuthentication() throws Exception { //Enable Shibboleth and password login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_AND_PASS); + setAuthenticationMethodSequence(SHIB_AND_PASS); //Check if WWW-Authenticate header contains shibboleth and password getClient().perform(get("/api/authn/status").header("Referer", "http://my.uni.edu")) @@ -1281,7 +1321,7 @@ public void testShibbolethAndPasswordAuthentication() throws Exception { @Test public void testOnlyPasswordAuthenticationWorks() throws Exception { //Enable only password login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", PASS_ONLY); + setAuthenticationMethodSequence(PASS_ONLY); //Check if WWW-Authenticate header contains only getClient().perform(get("/api/authn/status").header("Referer", "http://my.uni.edu")) @@ -1314,7 +1354,7 @@ public void testOnlyPasswordAuthenticationWorks() throws Exception { @Test public void testShibbolethAuthenticationDoesNotWorkWithPassOnly() throws Exception { //Enable only password login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", PASS_ONLY); + setAuthenticationMethodSequence(PASS_ONLY); //Check if WWW-Authenticate header contains only password getClient().perform(get("/api/authn/status").header("Referer", "http://my.uni.edu")) @@ -1332,7 +1372,7 @@ public void testShibbolethAuthenticationDoesNotWorkWithPassOnly() throws Excepti @Test public void testOnlyShibbolethAuthenticationWorks() throws Exception { //Enable only Shibboleth login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_ONLY); + setAuthenticationMethodSequence(SHIB_ONLY); //Check if WWW-Authenticate header contains only shibboleth getClient().perform(get("/api/authn/status").header("Referer", "http://my.uni.edu")) @@ -1365,7 +1405,7 @@ public void testOnlyShibbolethAuthenticationWorks() throws Exception { @Test public void testPasswordAuthenticationDoesNotWorkWithShibOnly() throws Exception { //Enable only Shibboleth login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_ONLY); + setAuthenticationMethodSequence(SHIB_ONLY); getClient().perform(post("/api/authn/login") .param("user", eperson.getEmail()) @@ -1540,7 +1580,7 @@ public void testGenerateShortLivedTokenWithShortLivedToken() throws Exception { // @Test // public void testStatusOrcidAuthenticatedWithCookie() throws Exception { // -// configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", ORCID_ONLY); +// setAuthenticationMethodSequence(ORCID_ONLY); // // String uiURL = configurationService.getProperty("dspace.ui.url"); // @@ -1627,7 +1667,7 @@ public void testGenerateShortLivedTokenWithShortLivedToken() throws Exception { @Test public void testOrcidLoginURL() throws Exception { - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", ORCID_ONLY); + setAuthenticationMethodSequence(ORCID_ONLY); String originalClientId = orcidConfiguration.getClientId(); orcidConfiguration.setClientId("CLIENT-ID"); @@ -1658,7 +1698,7 @@ public void testAreSpecialGroupsApplicable() throws Exception { .withName("specialGroupShib") .build(); - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_AND_PASS); + setAuthenticationMethodSequence(SHIB_AND_PASS); configurationService.setProperty("authentication-password.login.specialgroup", "specialGroupPwd"); configurationService.setProperty("authentication-shibboleth.role.faculty", "specialGroupShib"); configurationService.setProperty("authentication-shibboleth.default-roles", "faculty"); From 6e729544eb444473797f0301c831c375c117093b Mon Sep 17 00:00:00 2001 From: milanmajchrak <90026355+milanmajchrak@users.noreply.github.com> Date: Thu, 25 Jun 2026 16:17:44 +0200 Subject: [PATCH 2/2] test: de-flake ORCID cache tests and ZIP-download IT (#1344) * 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 * 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 * 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 --------- Co-authored-by: Claude Opus 4.8 (cherry picked from commit f6f13561cb21fdcec78478b5824a7f4931ff56a9) --- .../CachingOrcidRestConnectorTest.java | 87 ++++++++++++------- .../rest/AuthorizationRestRepositoryIT.java | 55 ++++++++++-- .../rest/MetadataBitstreamControllerIT.java | 55 +++++++----- .../app/rest/StatisticsRestRepositoryIT.java | 7 ++ 4 files changed, 142 insertions(+), 62 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 fb325a8f1bbb..7493638e4e80 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; @@ -54,6 +57,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(); @@ -137,7 +153,7 @@ public void search_fail() throws Exception { @Ignore @Test - public void testCachable() { + public void testCachable() throws IOException { CachingOrcidRestConnector c = new DSpace().getServiceManager().getServiceByName( "CachingOrcidRestConnector", CachingOrcidRestConnector.class); @@ -151,44 +167,57 @@ 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()); + } } @Ignore @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/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/MetadataBitstreamControllerIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java index 63f7e62206f8..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 @@ -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,30 @@ 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<>(); + 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())); } } 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))