Merge branch 'master' into submit_collection_endpoint

Author: patrick-austin

.github/workflows/ci-build.yml

@@ -13,9 +13,7 @@ jobs:
       matrix:
         java_version:
           - 11
-          # Errors with `java.lang.IllegalArgumentException: ArquillianServletRunnerEE9 not found. Could not determine ContextRoot from ProtocolMetadata, please contact DeployableContainer developer.`
-          # Bumping various Arquillian dependencies does not fix, leave as just Java 11
-          # - 17  
+          - 21
 
     steps:
       - name: Setup Java

.github/workflows/release-build.yml

@@ -16,6 +16,7 @@ jobs:
       - name: Set up JDK 11
         uses: actions/setup-java@v4
         with:
+          distribution: 'temurin'
           java-version: 11
 
       - name: Cache maven dependencies
@@ -64,7 +65,7 @@ jobs:
         if: ${{ github.ref == 'refs/heads/master' }}
 
       - name: Create/update release
-        uses: johnwbyrd/update-release@v1.0.0
+        uses: IsaacShelton/update-existing-release@v1.3.4
         with:
           token: ${{ secrets.GITHUB_TOKEN }}
           files: ./datagateway-download-api-${{ env.VERSION }}-distro.zip

pom.xml

@@ -126,7 +126,7 @@
 			<dependency>
 				<groupId>org.jboss.arquillian</groupId>
 				<artifactId>arquillian-bom</artifactId>
-				<version>1.7.0.Final</version>
+				<version>1.9.5.Final</version>
 				<scope>import</scope>
 				<type>pom</type>
 			</dependency>
@@ -153,14 +153,14 @@
 			<version>1.2.13</version>
 		</dependency>
 		<dependency>
-			<groupId>junit</groupId>
-			<artifactId>junit</artifactId>
-			<version>4.13.2</version>
+			<groupId>org.junit.jupiter</groupId>
+			<artifactId>junit-jupiter</artifactId>
+			<version>5.13.1</version>
 			<scope>test</scope>
 		</dependency>
 		<dependency>
-			<groupId>org.jboss.arquillian.junit</groupId>
-			<artifactId>arquillian-junit-container</artifactId>
+			<groupId>org.jboss.arquillian.junit5</groupId>
+			<artifactId>arquillian-junit5-container</artifactId>
 			<scope>test</scope>
 		</dependency>
 		<!-- required for @Transactional annotation -->
@@ -208,13 +208,13 @@
 				<dependency>
 					<groupId>org.omnifaces.arquillian</groupId>
 					<artifactId>arquillian-glassfish-server-embedded</artifactId>
-					<version>1.4</version>
+					<version>1.8</version>
 					<scope>test</scope>
 				</dependency>
 				<dependency>
 					<groupId>org.glassfish.main.extras</groupId>
 					<artifactId>glassfish-embedded-all</artifactId>
-					<version>7.0.5</version>
+					<version>7.0.25</version>
 					<scope>test</scope>
 				</dependency>
 				<dependency>
@@ -242,9 +242,11 @@
 						GlassFish to retarget the derby log file -->
 					<plugin>
 						<artifactId>maven-surefire-plugin</artifactId>
-						<version>2.19.1</version>
+						<version>3.5.3</version>
 						<configuration>
-							<argLine>-Xmx768m -XX:MaxMetaspaceSize=256m</argLine>
+							<!-- Required for jdk 17+ -->
+							<argLine>--add-opens java.base/java.lang=ALL-UNNAMED</argLine>
+
 							<systemPropertyVariables>
 								<java.util.logging.config.file>
 									${project.build.testOutputDirectory}/logging.properties

src/main/java/org/icatproject/topcat/repository/CacheRepository.java

@@ -27,7 +27,7 @@ public class CacheRepository {
 	@PersistenceContext(unitName = "topcat")
 	EntityManager em;
 
-	private static final Logger logger = LoggerFactory.getLogger(ConfVarRepository.class);
+	private static final Logger logger = LoggerFactory.getLogger(CacheRepository.class);
 
 	public Object get(String key, Long seconds){
 		Cache cache = getCache(key);
@@ -80,27 +80,32 @@ public void remove(String key) {
 
 	@Schedule(hour="*", minute="0")
 	public void prune(){
-		Properties properties = Properties.getInstance();
-      	Integer maxCacheSize = Integer.valueOf(properties.getProperty("maxCacheSize", "10000"));
-
-		TypedQuery<Cache> query = em.createQuery("select cache from Cache cache order by cache.lastAccessTime desc", Cache.class);
-		query.setMaxResults(maxCacheSize);
-
-		List<Cache> caches;
-		int page = 1;
-		while(true){
-			query.setFirstResult(maxCacheSize * page);
-			caches = query.getResultList();
-			if(caches.size() > 0){
-				for(Cache cache : caches){
-					em.remove(cache);
+		try {
+			Properties properties = Properties.getInstance();
+			Integer maxCacheSize = Integer.valueOf(properties.getProperty("maxCacheSize", "10000"));
+
+			TypedQuery<Cache> query = em.createQuery("select cache from Cache cache order by cache.lastAccessTime desc", Cache.class);
+			query.setMaxResults(maxCacheSize);
+
+			List<Cache> caches;
+			int page = 1;
+			while(true){
+				query.setFirstResult(maxCacheSize * page);
+				caches = query.getResultList();
+				if(caches.size() > 0){
+					for(Cache cache : caches){
+						em.remove(cache);
+					}
+				} else {
+					break;
 				}
-			} else {
-				break;
 			}
-		}
 
-		em.flush();
+			em.flush();
+		} catch (RuntimeException e) {
+			// Catch exceptions to prevent the EJBTimerService from crashing
+			logger.error("Unhandled exception in prune()", e);
+		}
 	}
 
 	private Cache getCache(String key){

src/test/java/org/icatproject/topcat/AdminResourceTest.java

@@ -1,41 +1,46 @@
 package org.icatproject.topcat;
 
-import java.util.*;
+import java.net.URLEncoder;
+import java.util.ArrayList;
 import java.util.Date;
-import java.io.File;
-import java.lang.reflect.*;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import jakarta.inject.Inject;
+import jakarta.json.JsonObject;
+import jakarta.ejb.EJB;
+import jakarta.ws.rs.core.Response;
 
 import org.jboss.arquillian.container.test.api.Deployment;
-import org.jboss.arquillian.junit.Arquillian;
+import org.jboss.arquillian.junit5.container.annotation.ArquillianTest;
 import org.jboss.shrinkwrap.api.ShrinkWrap;
 import org.jboss.shrinkwrap.api.asset.EmptyAsset;
 import org.jboss.shrinkwrap.api.spec.JavaArchive;
 
-import static org.junit.Assert.*;
-import org.junit.*;
-import org.junit.runner.RunWith;
-import jakarta.inject.Inject;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.fail;
 
-import jakarta.json.*;
-import jakarta.ws.rs.core.Response;
-import jakarta.ejb.EJB;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
 
 import org.icatproject.topcat.httpclient.HttpClient;
-import org.icatproject.topcat.domain.*;
-import org.icatproject.topcat.exceptions.BadRequestException;
+import org.icatproject.topcat.domain.Download;
+import org.icatproject.topcat.domain.DownloadStatus;
+import org.icatproject.topcat.domain.DownloadType;
 import org.icatproject.topcat.exceptions.ForbiddenException;
-
-import java.net.URLEncoder;
-
 import org.icatproject.topcat.repository.CacheRepository;
 import org.icatproject.topcat.repository.ConfVarRepository;
 import org.icatproject.topcat.repository.DownloadRepository;
 import org.icatproject.topcat.repository.DownloadTypeRepository;
 import org.icatproject.topcat.web.rest.AdminResource;
 
-import java.sql.*;
-
-@RunWith(Arquillian.class)
+@ArquillianTest
 public class AdminResourceTest {
 
 	/*
@@ -71,12 +76,12 @@ public static JavaArchive createDeployment() {
 	private static String adminSessionId;
 	private static String nonAdminSessionId;
 
-	@BeforeClass
+	@BeforeAll
 	public static void beforeAll() {
 		TestHelpers.installTrustManager();
 	}
 
-	@Before
+	@BeforeEach
 	public void setup() throws Exception {
 		HttpClient httpClient = new HttpClient("https://localhost:8181/icat");
 
@@ -194,7 +199,7 @@ public void testDownloadAPI() throws Exception {
 			downloads = (List<Download>) response.getEntity();
 
 			testDownload = findDownload(downloads, testDownload.getId());
-			assertTrue(testDownload.getIsDeleted() != currentDeleted);
+			assertNotEquals(testDownload.getIsDeleted(), currentDeleted);
 
 			// Test that getDownloadStatus() etc. produce an error response for a non-admin
 			// user
@@ -206,7 +211,6 @@ public void testDownloadAPI() throws Exception {
 						+ (String) response.getEntity());
 				fail("AdminResource.getDownloads did not raise exception for non-admin user");
 			} catch (ForbiddenException fe) {
-				assertTrue(true);
 			}
 
 			try {
@@ -217,7 +221,6 @@ public void testDownloadAPI() throws Exception {
 						+ (String) response.getEntity());
 				fail("AdminResource.setDownloadStatus did not raise exception for non-admin user");
 			} catch (ForbiddenException fe) {
-				assertTrue(true);
 			}
 
 			try {
@@ -228,7 +231,6 @@ public void testDownloadAPI() throws Exception {
 						+ (String) response.getEntity());
 				fail("AdminResource.deleteDownload did not raise exception for non-admin user");
 			} catch (ForbiddenException fe) {
-				assertTrue(true);
 			}
 		} finally {
 			// Remove the test download from the repository
@@ -308,7 +310,7 @@ public void testSetDownloadTypeStatus() throws Exception {
 		if (dt != null) {
 			System.out.println(
 					"DEBUG: AdminRT final download type status is {" + dt.getDisabled() + "," + dt.getMessage() + "}");
-			assertTrue(disabled != dt.getDisabled());
+			assertNotEquals(disabled, dt.getDisabled());
 			assertEquals(message, dt.getMessage());
 		}
 
@@ -322,7 +324,6 @@ public void testSetDownloadTypeStatus() throws Exception {
 					+ (String) response.getEntity());
 			fail("AdminResource.setDownloadTypeStatus did not raise exception for non-admin user");
 		} catch (ForbiddenException fe) {
-			assertTrue(true);
 		}
 
 		// Finally, ought to reset the disabled status to the original value!
@@ -406,7 +407,6 @@ public void testClearCachedSize() throws Exception {
 					+ (String) response.getEntity());
 			fail("AdminResource.clearCachedSize did not raise exception for non-admin user");
 		} catch (ForbiddenException fe) {
-			assertTrue(true);
 		}
 	}
 
@@ -437,7 +437,6 @@ public void testSetConfVar() throws Exception {
 					+ (String) response.getEntity());
 			fail("AdminResource.setConfVar did not raise exception for non-admin user");
 		} catch (ForbiddenException fe) {
-			assertTrue(true);
 		}
 	}
 

src/test/java/org/icatproject/topcat/CacheRepositoryTest.java

@@ -1,25 +1,22 @@
 package org.icatproject.topcat;
 
-import java.util.*;
-import java.lang.reflect.*;
+import jakarta.inject.Inject;
 
 import org.jboss.arquillian.container.test.api.Deployment;
-import org.jboss.arquillian.junit.Arquillian;
+import org.jboss.arquillian.junit5.container.annotation.ArquillianTest;
 import org.jboss.shrinkwrap.api.ShrinkWrap;
 import org.jboss.shrinkwrap.api.asset.EmptyAsset;
 import org.jboss.shrinkwrap.api.spec.JavaArchive;
-import static org.junit.Assert.*;
-import org.junit.*;
-import org.junit.runner.RunWith;
-import jakarta.inject.Inject;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
 
-import jakarta.ejb.EJB;
+import org.junit.jupiter.api.Test;
 
 import org.icatproject.topcat.domain.Cache;
 import org.icatproject.topcat.repository.CacheRepository;
 
-@RunWith(Arquillian.class)
+@ArquillianTest
 public class CacheRepositoryTest {
 
     @Deployment
@@ -44,6 +41,6 @@ public void testRemove() {
 		String key = "test:remove";
 		cacheRepository.put(key, "Hello World");
 		cacheRepository.remove(key);
-		assertEquals(null,cacheRepository.get(key));
+		assertNull(cacheRepository.get(key));
 	}
-}
+}

src/test/java/org/icatproject/topcat/FacilityMapTest.java

@@ -1,13 +1,14 @@
 package org.icatproject.topcat;
 
-import static org.junit.Assert.*;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import java.util.HashMap;
 import java.util.Map;
 
 import org.icatproject.topcat.exceptions.InternalException;
-import org.junit.*;
-import org.junit.function.ThrowingRunnable;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.function.Executable;
 
 public class FacilityMapTest {
 
@@ -153,7 +154,7 @@ public void testGetIcatUrlFailure() throws InternalException{
 
 		FacilityMap facilityMap = new FacilityMap(props);
 
-		ThrowingRunnable runnable = () -> {facilityMap.getIcatUrl(null);};
+		Executable runnable = () -> {facilityMap.getIcatUrl(null);};
 		assertThrows(InternalException.class, runnable);
 	}
 }