From fb9a072eacb3451fd67946acb0077617fe8c7fca Mon Sep 17 00:00:00 2001
From: Dou Mok <douming.mok@gmail.com>
Date: Thu, 5 Sep 2024 14:16:39 -0700
Subject: [PATCH] Refactor 'unTagObject' to always confirm cid before
 proceeding and resolve todo items

---
 .../filehashstore/FileHashStore.java          | 36 +++++++++----------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/src/main/java/org/dataone/hashstore/filehashstore/FileHashStore.java b/src/main/java/org/dataone/hashstore/filehashstore/FileHashStore.java
index e75decb6..e1c34973 100644
--- a/src/main/java/org/dataone/hashstore/filehashstore/FileHashStore.java
+++ b/src/main/java/org/dataone/hashstore/filehashstore/FileHashStore.java
@@ -1725,9 +1725,6 @@ protected void unTagObject(String pid, String cid)
         try {
             ObjectInfo objInfo = findObject(pid);
             String cidToCheck = objInfo.cid();
-            FileHashStoreUtility.ensureNotNull(cidToCheck, "cidRetrieved");
-            FileHashStoreUtility.checkForNotEmptyAndValidString(cidToCheck, "cidRetrieved");
-
             validateCidAndCheckLocked(pid, cid, cidToCheck);
 
             // Get paths to reference files to work on
@@ -1736,21 +1733,22 @@ protected void unTagObject(String pid, String cid)
 
             // Begin deletion process
             addAndRenamePidRefsFileToDeleteList(pid, deleteList, absPidRefsPath);
-
             removePidFromCidRefsAndDetermineDeletion(pid, deleteList, absCidRefsPath);
-
             deleteListOfFilesRenamedForDeletion(pid, cid, deleteList);
+
             logFileHashStore.info("Untagged pid: " + pid + " with cid: " + cid);
 
         } catch (OrphanPidRefsFileException oprfe) {
             // `findObject` throws this exception when the cid refs file doesn't exist,
             // so we only need to delete the pid refs file (pid is already locked)
-            // TODO: Check that the cid found actually matches what has been provided
-
             Path absPidRefsPath = getHashStoreRefsPath(pid, HashStoreIdTypes.pid);
-            addAndRenamePidRefsFileToDeleteList(pid, deleteList, absPidRefsPath);
+            String cidToCheck = new String(Files.readAllBytes(absPidRefsPath));
+            validateCidAndCheckLocked(pid, cid, cidToCheck);
 
+            // Begin deletion process
+            addAndRenamePidRefsFileToDeleteList(pid, deleteList, absPidRefsPath);
             deleteListOfFilesRenamedForDeletion(pid, cid, deleteList);
+
             String warnMsg = "Cid refs file does not exist for pid: " + pid
                 + ". Deleted orphan pid refs file.";
             logFileHashStore.warn(warnMsg);
@@ -1761,19 +1759,15 @@ protected void unTagObject(String pid, String cid)
             // - the pid is found in the cid refs file
             // - but the actual object being referenced by the pid does not exist
             Path absPidRefsPath = getHashStoreRefsPath(pid, HashStoreIdTypes.pid);
+            Path absCidRefsPath = getHashStoreRefsPath(cid, HashStoreIdTypes.cid);
             String cidToCheck = new String(Files.readAllBytes(absPidRefsPath));
-            FileHashStoreUtility.ensureNotNull(cidToCheck, "cidRead");
-            FileHashStoreUtility.checkForNotEmptyAndValidString(cidToCheck, "cidRead");
-            // TODO: Lots of repeated code with the basic scenario, see how to reduce code length
-
             validateCidAndCheckLocked(pid, cid, cidToCheck);
 
+            // Begin deletion process
             addAndRenamePidRefsFileToDeleteList(pid, deleteList, absPidRefsPath);
-
-            Path absCidRefsPath = getHashStoreRefsPath(cidToCheck, HashStoreIdTypes.cid);
             removePidFromCidRefsAndDetermineDeletion(pid, deleteList, absCidRefsPath);
-
             deleteListOfFilesRenamedForDeletion(pid, cid, deleteList);
+
             String warnMsg = "Object with cid: " + cidToCheck
                 + " does not exist, but pid and cid reference file found for pid: " + pid
                 + ". Deleted pid and cid ref files.";
@@ -1782,13 +1776,14 @@ protected void unTagObject(String pid, String cid)
         } catch (PidNotFoundInCidRefsFileException pnficrfe) {
             // `findObject` throws this exception when both the pid and cid refs file exists
             // but the pid is not found in the cid refs file (nothing to change here)
-            // TODO: Still need to check that the cid found matches
-
-            // Only rename pid refs file for deletion
             Path absPidRefsPath = getHashStoreRefsPath(pid, HashStoreIdTypes.pid);
-            addAndRenamePidRefsFileToDeleteList(pid, deleteList, absPidRefsPath);
+            String cidToCheck = new String(Files.readAllBytes(absPidRefsPath));
+            validateCidAndCheckLocked(pid, cid, cidToCheck);
 
+            // Begin deletion process
+            addAndRenamePidRefsFileToDeleteList(pid, deleteList, absPidRefsPath);
             deleteListOfFilesRenamedForDeletion(pid, cid, deleteList);
+
             String warnMsg = "Pid not found in expected cid refs file for pid: " + pid
                 + ". Deleted orphan pid refs file.";
             logFileHashStore.warn(warnMsg);
@@ -1807,7 +1802,6 @@ protected void unTagObject(String pid, String cid)
             }
 
             Path absCidRefsPath = getHashStoreRefsPath(cid, HashStoreIdTypes.cid);
-
             try {
                 if (Files.exists(absCidRefsPath) && isStringInRefsFile(pid, absCidRefsPath)) {
                     updateRefsFile(pid, absCidRefsPath, HashStoreRefUpdateTypes.remove);
@@ -1832,6 +1826,8 @@ protected void unTagObject(String pid, String cid)
      * @param cidToCheck Cid that was retrieved or read
      */
     private static void validateCidAndCheckLocked(String pid, String cid, String cidToCheck) {
+        FileHashStoreUtility.ensureNotNull(cidToCheck, "cidToCheck");
+        FileHashStoreUtility.checkForNotEmptyAndValidString(cidToCheck, "cidToCheck");
         // If the cid retrieved does not match, this untag request is invalid immediately
         if (!cid.equals(cidToCheck)) {
             String errMsg =