PR Feedback

backend/src/main/scala/cromwell/backend/standard/callcaching/StandardFileHashingActor.scala

@@ -1,6 +1,5 @@
 package cromwell.backend.standard.callcaching
 
-import java.util.concurrent.TimeoutException
 import akka.actor.{Actor, ActorLogging, ActorRef, Timers}
 import akka.event.LoggingAdapter
 import com.typesafe.config.Config
@@ -16,6 +15,7 @@ import cromwell.core.path.Path
 import net.ceedubs.ficus.Ficus._
 import wom.values.WomFile
 
+import java.util.concurrent.TimeoutException
 import scala.jdk.CollectionConverters._
 import scala.util.{Failure, Success, Try}
 
@@ -99,11 +99,11 @@ abstract class StandardFileHashingActor(standardParams: StandardFileHashingActor
       fsConfigs <- configurationDescriptor.backendConfig.as[Option[Config]]("filesystems").toList
       fsKey <- fsConfigs.root.keySet().asScala
       configKey = s"${fsKey}.caching.hashing-strategy"
-      // TODO this allows users to override with an empty list to prevent caching, is that desirable or a footgun?
       fileHashStrategyFromList = Try(fsConfigs.as[List[String]](configKey)).toOption
       fileHashStrategyFromString = Try(fsConfigs.as[String](configKey)).toOption.map(List(_))
       fileHashStrategy <- fileHashStrategyFromList.orElse(fileHashStrategyFromString)
-    } yield (fsKey, FileHashStrategy.of(fileHashStrategy))
+      nonEmptyFileHashStrategy <- if (fileHashStrategy.nonEmpty) Option(fileHashStrategy) else None
+    } yield (fsKey, FileHashStrategy.of(nonEmptyFileHashStrategy))
 
     defaultHashingStrategies ++ configuredHashingStrategies
 

backend/src/test/scala/cromwell/backend/standard/callcaching/StandardFileHashingActorSpec.scala

@@ -156,7 +156,8 @@ class StandardFileHashingActorSpec
       """filesystems.gcs.caching.hashing-strategy = ["md5", "identity"]
         |filesystems.s3.caching.hashing-strategy = "etag"
         |filesystems.http.some-other-config = "foobar"
-        |filesystems.ftp.caching.hashing-strategy = []""".stripMargin
+        |filesystems.ftp.caching.hashing-strategy = []
+        |filesystems.nfs.caching.hashing-strategy = "bogohash"""".stripMargin
     )
     val config = BackendConfigurationDescriptor(backendConfig, ConfigFactory.empty)
 
@@ -185,12 +186,26 @@ class StandardFileHashingActorSpec
       }
     }
 
+    // Test an actor-defined default overriden by config
     checkHashStrategy("gcs", FileHashStrategy(List(HashType.Md5, HashType.Identity)))
+
+    // Test a strategy only defined in config
     checkHashStrategy("s3", FileHashStrategy.ETag)
-    checkHashStrategy("ftp", FileHashStrategy(List()))
+
+    // Test a strategy defined as an empty list in config, should use fallback
+    checkHashStrategy("ftp", FileHashStrategy(List(HashType.Sha256)))
+
+    // Test a strategy with a default defined in the actor
     checkHashStrategy("drs", FileHashStrategy.Drs)
+
+    // Test a filesystem that has config, but not this config
     checkHashStrategy("http", FileHashStrategy(List(HashType.Sha256)))
+
+    // Test a strategy not defined in config or actor defaults
     checkHashStrategy("blob", FileHashStrategy(List(HashType.Sha256)))
+
+    // Test a strategy with an invalid value in config
+    checkHashStrategy("nfs", FileHashStrategy(List(HashType.Sha256)))
   }
 
 }