#2804 Introducing new way of model validation

project/build.scala

@@ -268,6 +268,7 @@ object Dependencies {
     graphite % "compile",
     datadog % "compile",
     marathonApiConsole % "compile",
+    wixAccord % "compile",
 
     // test
     Test.diffson % "test",
@@ -304,6 +305,7 @@ object Dependency {
     val Graphite = "3.1.2"
     val DataDog = "1.1.3"
     val Logback = "1.1.3"
+    val WixAccord = "0.5"
 
     // test deps versions
     val Mockito = "1.9.5"
@@ -341,6 +343,7 @@ object Dependency {
   val marathonApiConsole = "mesosphere.marathon" % "api-console" % V.MarathonApiConsole
   val graphite = "io.dropwizard.metrics" % "metrics-graphite" % V.Graphite
   val datadog = "org.coursera" % "dropwizard-metrics-datadog" % V.DataDog exclude("ch.qos.logback", "logback-classic")
+  val wixAccord = "com.wix" %% "accord-core" % V.WixAccord
 
 
   object Test {

src/main/scala/mesosphere/marathon/Exception.scala

@@ -1,5 +1,6 @@
 package mesosphere.marathon
 
+import com.wix.accord.Failure
 import mesosphere.marathon.state.PathId
 
 //scalastyle:off null
@@ -29,6 +30,13 @@ case class CanceledActionException(msg: String) extends Exception(msg)
 
 case class ConflictingChangeException(msg: String) extends Exception(msg)
 
+/**
+  * Is thrown if an object validation is not successful.
+  * @param obj object which is not valid
+  * @param failure validation information kept in a Failure object
+  */
+case class ValidationFailedException(obj: Any, failure: Failure) extends Exception("Validation failed")
+
 /*
  * Task upgrade specific exceptions
  */
@@ -56,3 +64,4 @@ class ResolveArtifactsCanceledException(msg: String) extends DeploymentFailedExc
  */
 class StoreCommandFailedException(msg: String, cause: Throwable = null) extends Exception(msg, cause)
 class MigrationFailedException(msg: String, cause: Throwable = null) extends Exception(msg, cause)
+

src/main/scala/mesosphere/marathon/api/MarathonExceptionMapper.scala

@@ -1,24 +1,19 @@
 package mesosphere.marathon.api
 
-import javax.validation.{ ConstraintViolation, ConstraintViolationException }
 import javax.ws.rs.ext.{ Provider, ExceptionMapper }
 import javax.ws.rs.core.{ MediaType, Response }
 import com.fasterxml.jackson.databind.JsonMappingException
 import com.google.inject.Singleton
 import org.slf4j.LoggerFactory
 
 import scala.concurrent.TimeoutException
-import mesosphere.marathon.{
-  AppLockedException,
-  BadRequestException,
-  ConflictingChangeException,
-  UnknownAppException
-}
+import mesosphere.marathon.{ Exception => _, _ }
 import com.sun.jersey.api.NotFoundException
 import com.fasterxml.jackson.core.JsonParseException
 import javax.ws.rs.WebApplicationException
 import javax.ws.rs.core.Response.Status
-import play.api.libs.json.{ Json, JsObject, JsResultException }
+import play.api.libs.json.{ JsValue, Json, JsObject, JsResultException }
+import mesosphere.marathon.api.v2.Validation._
 
 @Provider
 @Singleton
@@ -45,22 +40,22 @@ class MarathonExceptionMapper extends ExceptionMapper[Exception] {
   //scalastyle:off magic.number cyclomatic.complexity
   private def statusCode(exception: Exception): Int = exception match {
     //scalastyle:off magic.number
-    case e: IllegalArgumentException     => 422 // Unprocessable entity
-    case e: TimeoutException             => 503 // Service Unavailable
-    case e: UnknownAppException          => 404 // Not found
-    case e: AppLockedException           => 409 // Conflict
-    case e: ConflictingChangeException   => 409 // Conflict
-    case e: BadRequestException          => 400 // Bad Request
-    case e: JsonParseException           => 400 // Bad Request
-    case e: JsResultException            => 400 // Bad Request
-    case e: ConstraintViolationException => 422 // Unprocessable entity
-    case e: JsonMappingException         => 400 // Bad Request
-    case e: WebApplicationException      => e.getResponse.getStatus
-    case _                               => 500 // Internal server error
+    case e: TimeoutException           => 503 // Service Unavailable
+    case e: UnknownAppException        => 404 // Not found
+    case e: AppLockedException         => 409 // Conflict
+    case e: ConflictingChangeException => 409 // Conflict
+    case e: BadRequestException        => 400 // Bad Request
+    case e: JsonParseException         => 400 // Bad Request
+    case e: JsResultException          => 400 // Bad Request
+    case e: JsonMappingException       => 400 // Bad Request
+    case e: IllegalArgumentException   => 422 // Unprocessable entity
+    case e: ValidationFailedException  => 422 // Unprocessable Entity
+    case e: WebApplicationException    => e.getResponse.getStatus
+    case _                             => 500 // Internal server error
     //scalastyle:on
   }
 
-  private def entity(exception: Exception): JsObject = exception match {
+  private def entity(exception: Exception): JsValue = exception match {
     case e: NotFoundException =>
       Json.obj("message" -> s"URI not found: ${e.getNotFoundUri.getRawPath}")
     case e: AppLockedException =>
@@ -86,19 +81,7 @@ class MarathonExceptionMapper extends ExceptionMapper[Exception] {
         "message" -> s"Invalid JSON",
         "details" -> errors
       )
-    case e: ConstraintViolationException =>
-      def violationToError(violation: ConstraintViolation[_]): JsObject = {
-        Json.obj(
-          "attribute" -> violation.getPropertyPath.toString,
-          "error" -> violation.getMessage
-        )
-      }
-
-      import scala.collection.JavaConverters._
-      Json.obj(
-        "message" -> e.getMessage,
-        "errors" -> e.getConstraintViolations.asScala.map(violationToError)
-      )
+    case ValidationFailedException(obj, failure) => Json.toJson(failure)
     case e: WebApplicationException =>
       //scalastyle:off null
       if (Status.fromStatusCode(e.getResponse.getStatus) != null) {

src/main/scala/mesosphere/marathon/api/RestResource.scala

@@ -9,7 +9,10 @@ import mesosphere.marathon.api.v2.json.Formats._
 import mesosphere.marathon.state.{ PathId, Timestamp }
 import mesosphere.marathon.upgrade.DeploymentPlan
 import play.api.libs.json.Json.JsValueWrapper
-import play.api.libs.json.{ Writes, JsArray, JsObject, Json }
+import play.api.libs.json.{ Writes, Json }
+
+import com.wix.accord._
+import mesosphere.marathon.api.v2.Validation._
 
 import scala.concurrent.{ Await, Awaitable }
 
@@ -47,4 +50,26 @@ trait RestResource {
   protected def jsonArrString(fields: JsValueWrapper*): String = Json.stringify(Json.arr(fields: _*))
 
   protected def result[T](fn: Awaitable[T]): T = Await.result(fn, config.zkTimeoutDuration)
+
+  //scalastyle:off
+  /**
+    * Checks if the implicit validator yields a valid result.
+    * @param t object to validate
+    * @param description optional description which might be injected into the failure message
+    * @param fn function to execute after successful validation
+    * @param validator validator to use
+    * @tparam T type of object
+    * @return returns a 422 response if there is a failure due to validation. Executes fn function if successful.
+    */
+  protected def withValid[T](t: T, description: Option[String] = None)(fn: T => Response)(implicit validator: Validator[T]): Response = {
+    //scalastyle:on
+    validator(t) match {
+      case f: Failure =>
+        val entity = Json.toJson(description.map(f.withDescription).getOrElse(f)).toString
+        //scalastyle:off magic.number
+        Response.status(422).entity(entity).build()
+      //scalastyle:on
+      case Success => fn(t)
+    }
+  }
 }

src/main/scala/mesosphere/marathon/api/v2/AppsResource.scala

@@ -4,6 +4,7 @@ import java.net.URI
 import javax.inject.{ Inject, Named }
 import javax.servlet.http.{ HttpServletResponse, HttpServletRequest }
 import javax.ws.rs._
+import javax.ws.rs.core.Response.Status
 import javax.ws.rs.core.{ Context, MediaType, Response }
 
 import akka.event.EventStream
@@ -24,6 +25,8 @@ import play.api.libs.json.Json
 import scala.collection.immutable.Seq
 import scala.concurrent.Future
 
+import Validation._
+
 @Path("v2/apps")
 @Consumes(Array(MediaType.APPLICATION_JSON))
 @Produces(Array(MarathonMediaType.PREFERRED_APPLICATION_JSON))
@@ -65,28 +68,28 @@ class AppsResource @Inject() (
              @DefaultValue("false")@QueryParam("force") force: Boolean,
              @Context req: HttpServletRequest, @Context resp: HttpServletResponse): Response = {
     val now = clock.now()
-    val app = validateApp(Json.parse(body).as[V2AppDefinition].withCanonizedIds().toAppDefinition)
-      .copy(versionInfo = AppDefinition.VersionInfo.OnlyVersion(now))
-
-    doIfAuthorized(req, resp, CreateAppOrGroup, app.id) { identity =>
-      def createOrThrow(opt: Option[AppDefinition]) = opt
-        .map(_ => throw new ConflictingChangeException(s"An app with id [${app.id}] already exists."))
-        .getOrElse(app)
-
-      val plan = result(groupManager.updateApp(app.id, createOrThrow, app.version, force))
-
-      val appWithDeployments = AppInfo(
-        app,
-        maybeCounts = Some(TaskCounts.zero),
-        maybeTasks = Some(Seq.empty),
-        maybeDeployments = Some(Seq(Identifiable(plan.id)))
-      )
-
-      maybePostEvent(req, appWithDeployments.app)
-      Response
-        .created(new URI(app.id.toString))
-        .entity(jsonString(appWithDeployments))
-        .build()
+    withValid(Json.parse(body).as[V2AppDefinition].withCanonizedIds()) { appDef =>
+      val app = appDef.toAppDefinition.copy(versionInfo = AppDefinition.VersionInfo.OnlyVersion(now))
+      doIfAuthorized(req, resp, CreateAppOrGroup, app.id) { identity =>
+        def createOrThrow(opt: Option[AppDefinition]) = opt
+          .map(_ => throw new ConflictingChangeException(s"An app with id [${app.id}] already exists."))
+          .getOrElse(app)
+
+        val plan = result(groupManager.updateApp(app.id, createOrThrow, app.version, force))
+
+        val appWithDeployments = AppInfo(
+          app,
+          maybeCounts = Some(TaskCounts.zero),
+          maybeTasks = Some(Seq.empty),
+          maybeDeployments = Some(Seq(Identifiable(plan.id)))
+        )
+
+        maybePostEvent(req, appWithDeployments.app)
+        Response
+          .created(new URI(app.id.toString))
+          .entity(jsonString(appWithDeployments))
+          .build()
+      }
     }
   }
 
@@ -130,18 +133,17 @@ class AppsResource @Inject() (
     @DefaultValue("false")@QueryParam("force") force: Boolean,
     @Context req: HttpServletRequest, @Context resp: HttpServletResponse): Response = {
     val appId = id.toRootPath
-    doIfAuthorized(req, resp, UpdateAppOrGroup, appId) { identity =>
-      val appUpdate = Json.parse(body).as[V2AppUpdate].copy(id = Some(appId))
-      BeanValidation.requireValid(ModelValidation.checkUpdate(appUpdate, needsId = false))
-
-      val now = clock.now()
-      val plan = result(groupManager.updateApp(appId, updateOrCreate(appId, _, appUpdate, now), now, force))
-
-      val response = plan.original.app(appId)
-        .map(_ => Response.ok())
-        .getOrElse(Response.created(new URI(appId.toString)))
-      maybePostEvent(req, plan.target.app(appId).get)
-      deploymentResult(plan, response)
+    withValid(Json.parse(body).as[V2AppUpdate].copy(id = Some(appId))) { app =>
+      doIfAuthorized(req, resp, UpdateAppOrGroup, appId) { identity =>
+        val now = clock.now()
+        val plan = result(groupManager.updateApp(appId, updateOrCreate(appId, _, app, now), now, force))
+
+        val response = plan.original.app(appId)
+          .map(_ => Response.ok())
+          .getOrElse(Response.created(new URI(appId.toString)))
+        maybePostEvent(req, plan.target.app(appId).get)
+        deploymentResult(plan, response)
+      }
     }
   }
 
@@ -150,17 +152,18 @@ class AppsResource @Inject() (
   def replaceMultiple(@DefaultValue("false")@QueryParam("force") force: Boolean,
                       body: Array[Byte],
                       @Context req: HttpServletRequest, @Context resp: HttpServletResponse): Response = {
-    val updates = Json.parse(body).as[Seq[V2AppUpdate]].map(_.withCanonizedIds())
-    BeanValidation.requireValid(ModelValidation.checkUpdates(updates))
-    doIfAuthorized(req, resp, UpdateAppOrGroup, updates.flatMap(_.id): _*) { identity =>
-      val version = clock.now()
-      def updateGroup(root: Group): Group = updates.foldLeft(root) { (group, update) =>
-        update.id match {
-          case Some(id) => group.updateApp(id, updateOrCreate(id, _, update, version), version)
-          case None     => group
+
+    withValid(Json.parse(body).as[Seq[V2AppUpdate]].map(_.withCanonizedIds())) { updates =>
+      doIfAuthorized(req, resp, UpdateAppOrGroup, updates.flatMap(_.id): _*) { identity =>
+        val version = clock.now()
+        def updateGroup(root: Group): Group = updates.foldLeft(root) { (group, update) =>
+          update.id match {
+            case Some(id) => group.updateApp(id, updateOrCreate(id, _, update, version), version)
+            case None     => group
+          }
         }
+        deploymentResult(result(groupManager.update(PathId.empty, updateGroup, version, force)))
       }
-      deploymentResult(result(groupManager.update(PathId.empty, updateGroup, version, force)))
     }
   }
 
@@ -211,8 +214,8 @@ class AppsResource @Inject() (
                              existing: Option[AppDefinition],
                              appUpdate: V2AppUpdate,
                              newVersion: Timestamp): AppDefinition = {
-    def createApp() = validateApp(appUpdate(AppDefinition(appId)))
-    def updateApp(current: AppDefinition) = validateApp(appUpdate(current))
+    def createApp() = validateOrThrow(V2AppDefinition(appUpdate(AppDefinition(appId)))).toAppDefinition
+    def updateApp(current: AppDefinition) = validateOrThrow(V2AppDefinition(appUpdate(current))).toAppDefinition
     def rollback(version: Timestamp) = service.getApp(appId, version).getOrElse(throw new UnknownAppException(appId))
     def updateOrRollback(current: AppDefinition) = appUpdate.version.map(rollback).getOrElse(updateApp(current))
 
@@ -225,13 +228,6 @@ class AppsResource @Inject() (
     }
   }
 
-  private def validateApp(app: AppDefinition): AppDefinition = {
-    BeanValidation.requireValid(ModelValidation.checkAppConstraints(V2AppDefinition(app), app.id.parent))
-    val conflicts = ModelValidation.checkAppConflicts(app, result(groupManager.rootGroup()))
-    if (conflicts.nonEmpty) throw new ConflictingChangeException(conflicts.mkString(","))
-    app
-  }
-
   private def maybePostEvent(req: HttpServletRequest, app: AppDefinition) =
     eventBus.publish(ApiPostEvent(req.getRemoteAddr, req.getRequestURI, app))