Merge pull request #114 from usnistgov/fix/empty-version-string

Replace Empty String with Null for Version Handling in Dataset Caching
usnistgov · Oct 25, 2024 · 7824d70 · 7824d70
2 parents 1e45804 + e8df17a
commit 7824d70
Show file tree

Hide file tree

Showing 4 changed files with 95 additions and 56 deletions.
diff --git a/src/main/java/gov/nist/oar/distrib/service/RPACachingService.java b/src/main/java/gov/nist/oar/distrib/service/RPACachingService.java
@@ -5,6 +5,7 @@
 import gov.nist.oar.distrib.cachemgr.CacheManagementException;
 import gov.nist.oar.distrib.cachemgr.CacheObject;
 import gov.nist.oar.distrib.cachemgr.pdr.PDRDatasetCacheManager;
+import gov.nist.oar.distrib.cachemgr.pdr.PDRCacheManager;
 import gov.nist.oar.distrib.cachemgr.pdr.PDRCacheRoles;
 import gov.nist.oar.distrib.service.rpa.exceptions.MetadataNotFoundException;
 import gov.nist.oar.distrib.service.rpa.exceptions.RequestProcessingException;
@@ -79,7 +80,7 @@ public String cacheAndGenerateRandomId(String datasetID, String version)
 
 
         int prefs = ROLE_RESTRICTED_DATA;
-        if (!version.isEmpty())
+        if (version != null && !version.isEmpty())
             prefs = ROLE_OLD_RESTRICTED_DATA;
 
         // cache dataset

diff --git a/src/main/java/gov/nist/oar/distrib/service/rpa/DefaultRPADatasetCacher.java b/src/main/java/gov/nist/oar/distrib/service/rpa/DefaultRPADatasetCacher.java
@@ -29,7 +29,10 @@ public DefaultRPADatasetCacher(RPACachingService rpaCachingService) {
     public String cache(String datasetId) throws RequestProcessingException {
         String randomId;
         try {
-            randomId = rpaCachingService.cacheAndGenerateRandomId(datasetId, "");
+            // Replaced "" with null because passing an empty string would break the logic downstream. 
+            // An empty string would match the first version available, while in this scenario, 
+            // when the version is not provided, we want to select the latest version. Passing null achieves this.
+            randomId = rpaCachingService.cacheAndGenerateRandomId(datasetId, null);
         } catch (Exception e) {
             this.logCachingException(e);
             throw new RequestProcessingException(e.getMessage());

diff --git a/src/test/java/gov/nist/oar/distrib/service/RPACachingServiceTest.java b/src/test/java/gov/nist/oar/distrib/service/RPACachingServiceTest.java
@@ -29,58 +29,92 @@
 
 public class RPACachingServiceTest {
 
-    private RPACachingService rpaCachingService;
-    private PDRCacheManager pdrCacheManager;
-    private RPAConfiguration rpaConfiguration;
-
-    @Before
-    public void setUp() {
-        pdrCacheManager = mock(PDRCacheManager.class);
-        rpaConfiguration = mock(RPAConfiguration.class);
-        rpaCachingService = new RPACachingService(pdrCacheManager, rpaConfiguration);
-    }
-
-    @Test
-    public void testCacheAndGenerateRandomId_validDatasetID() throws Exception {
-        String datasetID = "mds2-2909";
-
-        String version = "";
-        Set<String> dummyFiles = new HashSet<>();
-        when(pdrCacheManager.cacheDataset(anyString(), eq(version), eq(true), eq(RPACachingService.ROLE_RESTRICTED_DATA), anyString()))
-                .thenReturn(dummyFiles);
-
-        String result = rpaCachingService.cacheAndGenerateRandomId(datasetID, version);
-
-        assertNotNull(result);
-        assertEquals(RPACachingService.RANDOM_ID_LENGTH + 4, result.length()); // 4 for the 'rpa-' prefix
-        assertTrue(result.matches("^rpa-[a-zA-Z0-9]+$")); // Check that the ID starts with 'rpa-' followed by alphanumeric chars
-        verify(pdrCacheManager).cacheDataset(eq("mds2-2909"), eq(version), eq(true), eq(RPACachingService.ROLE_RESTRICTED_DATA), eq(result));
-    }
-
-    @Test
-    public void testCacheAndGenerateRandomId_validDatasetArkID() throws Exception {
-        String datasetID = "ark:/12345/mds2-2909";
-
-        String version = "";
-        Set<String> dummyFiles = new HashSet<>();
-        when(pdrCacheManager.cacheDataset(anyString(), eq(version), eq(true), eq(RPACachingService.ROLE_RESTRICTED_DATA), anyString()))
-                .thenReturn(dummyFiles);
-
-        String result = rpaCachingService.cacheAndGenerateRandomId(datasetID, version);
-
-        assertNotNull(result);
-        assertEquals(RPACachingService.RANDOM_ID_LENGTH + 4, result.length()); // 4 for the 'rpa-' prefix
-        assertTrue(result.matches("^rpa-[a-zA-Z0-9]+$")); // Check that the ID starts with 'rpa-' followed by alphanumeric chars
-        verify(pdrCacheManager).cacheDataset(eq("mds2-2909"), eq(version), eq(true), eq(RPACachingService.ROLE_RESTRICTED_DATA), eq(result));
-    }
-
-    @Test(expected = IllegalArgumentException.class)
-    public void testCacheAndGenerateRandomId_invalidDatasetArkID() throws Exception {
-        String datasetID = "ark:/invalid_ark_id";
-        String version = "";
-
-        rpaCachingService.cacheAndGenerateRandomId(datasetID, version);
-    }
+        private RPACachingService rpaCachingService;
+        private PDRCacheManager pdrCacheManager;
+        private RPAConfiguration rpaConfiguration;
+
+        @Before
+        public void setUp() {
+            pdrCacheManager = mock(PDRCacheManager.class);
+            rpaConfiguration = mock(RPAConfiguration.class);
+            rpaCachingService = new RPACachingService(pdrCacheManager, rpaConfiguration);
+        }
+
+        @Test
+        public void testCacheAndGenerateRandomId_validDatasetID_withEmptyVersion() throws Exception {
+            String datasetID = "mds2-2909";
+
+            String version = ""; // empty version
+            Set<String> dummyFiles = new HashSet<>();
+            when(pdrCacheManager.cacheDataset(anyString(), eq(version), eq(true), eq(RPACachingService.ROLE_RESTRICTED_DATA), anyString()))
+                    .thenReturn(dummyFiles);
+
+            String result = rpaCachingService.cacheAndGenerateRandomId(datasetID, version);
+
+            assertNotNull(result);
+            assertEquals(RPACachingService.RANDOM_ID_LENGTH + 4, result.length()); // 4 for the 'rpa-' prefix
+            assertTrue(result.matches("^rpa-[a-zA-Z0-9]+$")); // Check that the ID starts with 'rpa-' followed by alphanumeric chars
+            verify(pdrCacheManager).cacheDataset(eq("mds2-2909"), eq(version), eq(true), eq(RPACachingService.ROLE_RESTRICTED_DATA), eq(result));
+        }
+
+        @Test
+        public void testCacheAndGenerateRandomId_validDatasetID_withNullVersion() throws Exception {
+            String datasetID = "mds2-2909";
+
+            String version = null; // null version
+            Set<String> dummyFiles = new HashSet<>();
+            when(pdrCacheManager.cacheDataset(anyString(), eq(version), eq(true), eq(RPACachingService.ROLE_RESTRICTED_DATA), anyString()))
+                    .thenReturn(dummyFiles);
+
+            String result = rpaCachingService.cacheAndGenerateRandomId(datasetID, version);
+
+            assertNotNull(result);
+            assertEquals(RPACachingService.RANDOM_ID_LENGTH + 4, result.length()); // 4 for the 'rpa-' prefix
+            assertTrue(result.matches("^rpa-[a-zA-Z0-9]+$")); // Check that the ID starts with 'rpa-' followed by alphanumeric chars
+            verify(pdrCacheManager).cacheDataset(eq("mds2-2909"), eq(version), eq(true), eq(RPACachingService.ROLE_RESTRICTED_DATA), eq(result));
+        }
+
+        @Test
+        public void testCacheAndGenerateRandomId_validDatasetArkID_withEmptyVersion() throws Exception {
+            String datasetID = "ark:/12345/mds2-2909";
+
+            String version = ""; // empty version
+            Set<String> dummyFiles = new HashSet<>();
+            when(pdrCacheManager.cacheDataset(anyString(), eq(version), eq(true), eq(RPACachingService.ROLE_RESTRICTED_DATA), anyString()))
+                    .thenReturn(dummyFiles);
+
+            String result = rpaCachingService.cacheAndGenerateRandomId(datasetID, version);
+
+            assertNotNull(result);
+            assertEquals(RPACachingService.RANDOM_ID_LENGTH + 4, result.length()); // 4 for the 'rpa-' prefix
+            assertTrue(result.matches("^rpa-[a-zA-Z0-9]+$")); // Check that the ID starts with 'rpa-' followed by alphanumeric chars
+            verify(pdrCacheManager).cacheDataset(eq("mds2-2909"), eq(version), eq(true), eq(RPACachingService.ROLE_RESTRICTED_DATA), eq(result));
+        }
+
+        @Test
+        public void testCacheAndGenerateRandomId_validDatasetArkID_withNullVersion() throws Exception {
+            String datasetID = "ark:/12345/mds2-2909";
+
+            String version = null; // null version
+            Set<String> dummyFiles = new HashSet<>();
+            when(pdrCacheManager.cacheDataset(anyString(), eq(version), eq(true), eq(RPACachingService.ROLE_RESTRICTED_DATA), anyString()))
+                    .thenReturn(dummyFiles);
+
+            String result = rpaCachingService.cacheAndGenerateRandomId(datasetID, version);
+
+            assertNotNull(result);
+            assertEquals(RPACachingService.RANDOM_ID_LENGTH + 4, result.length()); // 4 for the 'rpa-' prefix
+            assertTrue(result.matches("^rpa-[a-zA-Z0-9]+$")); // Check that the ID starts with 'rpa-' followed by alphanumeric chars
+            verify(pdrCacheManager).cacheDataset(eq("mds2-2909"), eq(version), eq(true), eq(RPACachingService.ROLE_RESTRICTED_DATA), eq(result));
+        }
+
+        @Test(expected = IllegalArgumentException.class)
+        public void testCacheAndGenerateRandomId_invalidDatasetArkID() throws Exception {
+            String datasetID = "ark:/invalid_ark_id";
+            String version = "";
+
+            rpaCachingService.cacheAndGenerateRandomId(datasetID, version);
+        }    
 
     @Test
     public void testRetrieveMetadata_success() throws Exception  {

diff --git a/src/test/java/gov/nist/oar/distrib/service/rpa/DefaultRPADatasetCacherTest.java b/src/test/java/gov/nist/oar/distrib/service/rpa/DefaultRPADatasetCacherTest.java
@@ -11,6 +11,7 @@
 import org.mockito.junit.MockitoJUnitRunner;
 
 import static org.junit.Assert.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.times;
@@ -30,7 +31,7 @@ public void setUp() {
         MockitoAnnotations.initMocks(this);
         rpaDatasetCacher = new DefaultRPADatasetCacher(rpaCachingService);
         datasetId = "mds2-2909";
-        version = "";
+        version = null;
     }
 
     @Test
@@ -65,7 +66,7 @@ public void testCache_invalidDatasetId() throws Exception {
         String invalidDatasetId = "ark:/invalid_id";
 
          // Throw an IllegalArgumentException for the invalid datasetId when the cacheAndGenerateRandomId method is called
-        when(rpaCachingService.cacheAndGenerateRandomId(eq(invalidDatasetId), anyString()))
+        when(rpaCachingService.cacheAndGenerateRandomId(eq(invalidDatasetId), any()))
                 .thenThrow(new IllegalArgumentException("Invalid dataset ID format: " + invalidDatasetId));
 
         rpaDatasetCacher.cache(invalidDatasetId);