fix(jobserver): return 404 for deletion of non existing binary (spark-jobserver#979)

* Return 404 in case of deletion of non existing binary
* Add test for this case

Previous if one tried to delete a non existing library exception was
thrown. This behavior differs from the behavior of contexts and jobs in
the same situation (there 404 is returned)

Author: ecandreev

job-server/src/main/scala/spark/jobserver/BinaryManager.scala

@@ -11,6 +11,7 @@ import java.nio.file.{Files, Paths}
 import scala.concurrent.Future
 import scala.util.{Failure, Success, Try}
 import spark.jobserver.common.akka.InstrumentedActor
+import spark.jobserver.util.NoSuchBinaryException
 
 // Messages to JarManager actor
 
@@ -32,6 +33,7 @@ case class StoreLocalBinaries(localBinaries: Map[String, (BinaryType, String)])
 case object InvalidBinary
 case object BinaryStored
 case object BinaryDeleted
+case object NoSuchBinary
 case class BinaryStorageFailure(ex: Throwable)
 case class BinaryDeletionFailure(ex: Throwable)
 
@@ -109,7 +111,10 @@ class BinaryManager(jobDao: ActorRef) extends InstrumentedActor {
       logger.info(s"Deleting binary $appName")
       deleteBinary(appName).map {
         case Success(_) => BinaryDeleted
-        case Failure(ex) => BinaryDeletionFailure(ex)
+        case Failure(ex) => ex match {
+          case e: NoSuchBinaryException => NoSuchBinary
+          case _ => BinaryDeletionFailure(ex)
+        }
       }.pipeTo(sender)
   }
 }

job-server/src/main/scala/spark/jobserver/WebApi.scala

@@ -267,6 +267,7 @@ class WebApi(system: ActorSystem,
           respondWithMediaType(MediaTypes.`application/json`) { ctx =>
             future.map {
               case BinaryDeleted => ctx.complete(StatusCodes.OK)
+              case NoSuchBinary => notFound(ctx, s"can't find binary with name $appName")
             }.recover {
               case e: Exception => ctx.complete(500, errMap(e, "ERROR"))
             }
@@ -353,7 +354,7 @@ class WebApi(system: ActorSystem,
             case WebUIForContext(name, Some(url)) =>
               ctx.complete(200, Map("context" -> contextName, "url" -> url))
             case WebUIForContext(name, None) => ctx.complete(200, Map("context" -> contextName))
-            case NoSuchContext => ctx.complete(404, s"can't find context with name $contextName")
+            case NoSuchContext => notFound(ctx, s"can't find context with name $contextName")
 
           }.recover {
             case e: Exception => ctx.complete(500, errMap(e, "ERROR"))

job-server/src/main/scala/spark/jobserver/io/JobSqlDAO.scala

@@ -21,6 +21,7 @@ import slick.driver.JdbcProfile
 import slick.lifted.ProvenShape.proveShapeOf
 import spark.jobserver.JobManagerActor.ContextTerminatedException
 import spray.http.ErrorInfo
+import spark.jobserver.util.NoSuchBinaryException
 
 class JobSqlDAO(config: Config) extends JobDAO with FileCacher {
   val slickDriverClass = config.getString("spark.jobserver.sqldao.slick-driver")
@@ -159,7 +160,7 @@ class JobSqlDAO(config: Config) extends JobDAO with FileCacher {
     */
   override def deleteBinary(appName: String): Unit = {
     if (Await.result(deleteBinaryInfo(appName), 60 seconds) == 0) {
-      throw new SlickException(s"Failed to delete binary: $appName from database")
+      throw new NoSuchBinaryException(appName)
     }
     cleanCacheBinaries(appName)
   }

job-server/src/main/scala/spark/jobserver/util/CustomException.scala

@@ -0,0 +1,5 @@
+package spark.jobserver.util
+
+case class NoSuchBinaryException(private val appName: String) extends Exception {
+  private val message = s"can't find binary: $appName in database";
+}

job-server/src/test/scala/spark/jobserver/WebApiMainRoutesSpec.scala

@@ -104,6 +104,12 @@ class WebApiMainRoutesSpec extends WebApiSpec {
         status should be (OK)
       }
     }
+
+    it("should respond with 404 Not Found if binary was not found during deletion") {
+      Delete("/binaries/badbinary") ~> sealRoute(routes) ~> check {
+        status should be (NotFound)
+      }
+    }
   }
 
   describe("list jobs") {

job-server/src/test/scala/spark/jobserver/WebApiSpec.scala

@@ -20,6 +20,7 @@ import scala.concurrent.{Await, Future}
 import scala.util.{Failure, Success}
 import java.util.concurrent.TimeUnit
 import scala.concurrent.duration.Duration
+import spark.jobserver.util.NoSuchBinaryException
 
 // Tests web response codes and formatting
 // Does NOT test underlying Supervisor / JarManager functionality
@@ -136,6 +137,7 @@ with ScalatestRouteTest with HttpService with ScalaFutures with SprayJsonSupport
       case StoreBinary("daofail", _, _) => sender ! BinaryStorageFailure(new Exception("DAO failed to store"))
       case StoreBinary(_, _, _)         => sender ! BinaryStored
 
+      case DeleteBinary("badbinary") => sender ! NoSuchBinary
       case DeleteBinary(_) => sender ! BinaryDeleted
 
       case DataManagerActor.StoreData("errorfileToRemove", _) => sender ! DataManagerActor.Error