diff --git a/project/build.scala b/project/build.scala index b298711a790..51d29e975e4 100644 --- a/project/build.scala +++ b/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 { diff --git a/src/main/java/mesosphere/marathon/api/validation/PortIndices.java b/src/main/java/mesosphere/marathon/api/validation/PortIndices.java deleted file mode 100644 index 0e6bfa7d594..00000000000 --- a/src/main/java/mesosphere/marathon/api/validation/PortIndices.java +++ /dev/null @@ -1,15 +0,0 @@ -package mesosphere.marathon.api.validation; - -import javax.validation.Constraint; -import javax.validation.Payload; -import java.lang.annotation.*; - -@Target({ ElementType.TYPE, ElementType.ANNOTATION_TYPE }) -@Retention(RetentionPolicy.RUNTIME) -@Constraint(validatedBy = PortIndicesValidator.class) -@Documented -public @interface PortIndices { - String message() default "Health check port indices must address an element of the ports array or container port mappings."; - Class[] groups() default {}; - Class[] payload() default { }; -} diff --git a/src/main/java/mesosphere/marathon/api/validation/PortsArray.java b/src/main/java/mesosphere/marathon/api/validation/PortsArray.java deleted file mode 100644 index 0faafed0149..00000000000 --- a/src/main/java/mesosphere/marathon/api/validation/PortsArray.java +++ /dev/null @@ -1,26 +0,0 @@ -package mesosphere.marathon.api.validation; - -import java.lang.annotation.Documented; -import java.lang.annotation.ElementType; -import java.lang.annotation.Retention; -import java.lang.annotation.RetentionPolicy; -import java.lang.annotation.Target; - -import javax.validation.Constraint; -import javax.validation.Payload; - -@Target({ - ElementType.METHOD, - ElementType.FIELD, - ElementType.ANNOTATION_TYPE, - ElementType.CONSTRUCTOR, - ElementType.PARAMETER -}) -@Documented -@Constraint(validatedBy = PortsArrayValidator.class) -@Retention(RetentionPolicy.RUNTIME) -public @interface PortsArray { - String message() default "Elements must be unique"; - Class[] groups() default {}; - Class[] payload() default {}; -} diff --git a/src/main/java/mesosphere/marathon/api/validation/ValidHealthCheck.java b/src/main/java/mesosphere/marathon/api/validation/ValidHealthCheck.java deleted file mode 100644 index 7fd1e2ddfcb..00000000000 --- a/src/main/java/mesosphere/marathon/api/validation/ValidHealthCheck.java +++ /dev/null @@ -1,15 +0,0 @@ -package mesosphere.marathon.api.validation; - -import javax.validation.Constraint; -import javax.validation.Payload; -import java.lang.annotation.*; - -@Target({ ElementType.TYPE, ElementType.ANNOTATION_TYPE }) -@Retention(RetentionPolicy.RUNTIME) -@Constraint(validatedBy = HealthCheckValidator.class) -@Documented -public @interface ValidHealthCheck { - String message() default "Health check protocol must match supplied fields."; - Class[] groups() default {}; - Class[] payload() default {}; -} diff --git a/src/main/java/mesosphere/marathon/api/validation/ValidV2AppDefinition.java b/src/main/java/mesosphere/marathon/api/validation/ValidV2AppDefinition.java deleted file mode 100644 index 895147098e7..00000000000 --- a/src/main/java/mesosphere/marathon/api/validation/ValidV2AppDefinition.java +++ /dev/null @@ -1,15 +0,0 @@ -package mesosphere.marathon.api.validation; - -import javax.validation.Constraint; -import javax.validation.Payload; -import java.lang.annotation.*; - -@Target({ ElementType.TYPE, ElementType.ANNOTATION_TYPE }) -@Retention(RetentionPolicy.RUNTIME) -@Constraint(validatedBy = V2AppDefinitionValidator.class) -@Documented -public @interface ValidV2AppDefinition { - String message() default "AppDefinition must either contain one of 'cmd' or 'args', and/or a 'container'."; - Class[] groups() default {}; - Class[] payload() default {}; -} diff --git a/src/main/scala/mesosphere/marathon/Exception.scala b/src/main/scala/mesosphere/marathon/Exception.scala index fa59b991bdc..fa388a80213 100644 --- a/src/main/scala/mesosphere/marathon/Exception.scala +++ b/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) + diff --git a/src/main/scala/mesosphere/marathon/api/MarathonExceptionMapper.scala b/src/main/scala/mesosphere/marathon/api/MarathonExceptionMapper.scala index e835c8386d2..d8e677749be 100644 --- a/src/main/scala/mesosphere/marathon/api/MarathonExceptionMapper.scala +++ b/src/main/scala/mesosphere/marathon/api/MarathonExceptionMapper.scala @@ -1,6 +1,5 @@ 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 @@ -8,17 +7,13 @@ 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) { diff --git a/src/main/scala/mesosphere/marathon/api/RestResource.scala b/src/main/scala/mesosphere/marathon/api/RestResource.scala index 2aafd131325..a01e143cb7c 100644 --- a/src/main/scala/mesosphere/marathon/api/RestResource.scala +++ b/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) + } + } } diff --git a/src/main/scala/mesosphere/marathon/api/v2/AppsResource.scala b/src/main/scala/mesosphere/marathon/api/v2/AppsResource.scala index 6563eeea528..a3e6c974f80 100644 --- a/src/main/scala/mesosphere/marathon/api/v2/AppsResource.scala +++ b/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)) @@ -64,26 +67,28 @@ class AppsResource @Inject() ( def create(body: Array[Byte], @DefaultValue("false")@QueryParam("force") force: Boolean, @Context req: HttpServletRequest, @Context resp: HttpServletResponse): Response = { - val app = validateApp(Json.parse(body).as[V2AppDefinition].withCanonizedIds().toAppDefinition) - 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 + 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() + } } } @@ -127,18 +132,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) + } } } @@ -147,17 +151,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))) } } @@ -208,8 +213,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)) @@ -222,13 +227,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)) diff --git a/src/main/scala/mesosphere/marathon/api/v2/BeanValidation.scala b/src/main/scala/mesosphere/marathon/api/v2/BeanValidation.scala deleted file mode 100644 index fe1b66d679f..00000000000 --- a/src/main/scala/mesosphere/marathon/api/v2/BeanValidation.scala +++ /dev/null @@ -1,82 +0,0 @@ -package mesosphere.marathon.api.v2 - -import java.lang.annotation.ElementType -import java.util.Collections -import javax.validation.{ ConstraintViolation, ConstraintViolationException, Validation } -import scala.collection.JavaConverters._ -import scala.reflect.{ classTag, ClassTag } - -import org.hibernate.validator.internal.engine.ConstraintViolationImpl -import org.hibernate.validator.internal.engine.path.PathImpl - -/** - * Bean validation helper trait. - * TODO: we should not use bean validation any longer. - */ -object BeanValidation { - - val validator = Validation.buildDefaultValidatorFactory().getValidator - - def violation[Bean: ClassTag, Prop]( - bean: Bean, - prop: Prop, - path: String, - msg: String): ConstraintViolation[Bean] = { - //scalastyle:off null - ConstraintViolationImpl.forParameterValidation[Bean]( - msg, Collections.emptyMap(), msg, classTag[Bean].runtimeClass.asInstanceOf[Class[Bean]], bean, prop, prop, - PathImpl.createPathFromString(path), - null, ElementType.FIELD, Array()) - //scalastyle:on - } - - def withPath[T: ClassTag](bean: T, e: ConstraintViolation[_], path: String): ConstraintViolation[T] = - ConstraintViolationImpl.forParameterValidation[T]( - e.getMessageTemplate, - Collections.emptyMap(), - e.getMessage, - classTag[T].runtimeClass.asInstanceOf[Class[T]], - bean, - e.getLeafBean, - e.getInvalidValue, - PathImpl.createPathFromString(path + e.getPropertyPath), - e.getConstraintDescriptor, ElementType.FIELD, e.getExecutableParameters) - - def notSet[Bean: ClassTag, Prop](bean: Bean, path: String): ConstraintViolation[Bean] = { - violation(bean, null, path, "Property missing which is mandatory") - } - - def defined[Bean: ClassTag, Prop]( - bean: Bean, - opt: Option[Prop], - path: String, - fn: (Bean, Prop, String) => Iterable[ConstraintViolation[Bean]], - mandatory: Boolean = false): Iterable[ConstraintViolation[Bean]] = { - opt match { - case Some(t) => fn(bean, t, path) - case None if mandatory => List(notSet(bean, path)) - case _ => Nil - } - } - - def isTrue[Bean: ClassTag, Prop]( - bean: Bean, - prop: Prop, - path: String, - message: String, - assertion: => Boolean): Seq[ConstraintViolation[Bean]] = - if (!assertion) List(violation(bean, prop, path, message)) else Nil - - def validate[T: ClassTag]( - bean: T, - violationSets: Iterable[ConstraintViolation[_]]*): Iterable[ConstraintViolation[T]] = { - val beanErrors = validator.validate(bean).asScala - val result = beanErrors ++ violationSets.flatMap(identity) - result.map(withPath(bean, _, "")) - } - - def requireValid[T](errors: Iterable[ConstraintViolation[T]]) { - if (errors.nonEmpty) throw new ConstraintViolationException("Bean is not valid", errors.toSet.asJava) - } -} - diff --git a/src/main/scala/mesosphere/marathon/api/v2/GroupsResource.scala b/src/main/scala/mesosphere/marathon/api/v2/GroupsResource.scala index bbf849faef9..ca01e2e097f 100644 --- a/src/main/scala/mesosphere/marathon/api/v2/GroupsResource.scala +++ b/src/main/scala/mesosphere/marathon/api/v2/GroupsResource.scala @@ -100,14 +100,14 @@ class GroupsResource @Inject() ( body: Array[Byte], @Context req: HttpServletRequest, @Context resp: HttpServletResponse): Response = { doIfAuthorized(req, resp, CreateAppOrGroup, id.toRootPath) { implicit identity => - val update = Json.parse(body).as[V2GroupUpdate] - BeanValidation.requireValid(ModelValidation.checkGroupUpdate(update, needsId = true)) - val effectivePath = update.id.map(_.canonicalPath(id.toRootPath)).getOrElse(id.toRootPath) - val current = result(groupManager.rootGroup()).findGroup(_.id == effectivePath) - if (current.isDefined) - throw ConflictingChangeException(s"Group $effectivePath is already created. Use PUT to change this group.") - val (deployment, path, version) = updateOrCreate(id.toRootPath, update, force) - deploymentResult(deployment, Response.created(new URI(path.toString))) + withValid(Json.parse(body).as[V2GroupUpdate]) { update => + val effectivePath = update.id.map(_.canonicalPath(id.toRootPath)).getOrElse(id.toRootPath) + val current = result(groupManager.rootGroup()).findGroup(_.id == effectivePath) + if (current.isDefined) + throw ConflictingChangeException(s"Group $effectivePath is already created. Use PUT to change this group.") + val (deployment, path, version) = updateOrCreate(id.toRootPath, update, force) + deploymentResult(deployment, Response.created(new URI(path.toString))) + } } } @@ -136,21 +136,21 @@ class GroupsResource @Inject() ( body: Array[Byte], @Context req: HttpServletRequest, @Context resp: HttpServletResponse): Response = { doIfAuthorized(req, resp, UpdateAppOrGroup, id.toRootPath) { implicit identity => - val update = Json.parse(body).as[V2GroupUpdate] - BeanValidation.requireValid(ModelValidation.checkGroupUpdate(update, needsId = false)) - if (dryRun) { - val planFuture = groupManager.group(id.toRootPath).map { maybeOldGroup => - val oldGroup = maybeOldGroup.getOrElse(Group.empty) - Json.obj( - "steps" -> DeploymentPlan(oldGroup, update.apply(V2Group(oldGroup), Timestamp.now()).toGroup()).steps - ) + withValid(Json.parse(body).as[V2GroupUpdate]) { update => + if (dryRun) { + val planFuture = groupManager.group(id.toRootPath).map { maybeOldGroup => + val oldGroup = maybeOldGroup.getOrElse(Group.empty) + Json.obj( + "steps" -> DeploymentPlan(oldGroup, update.apply(V2Group(oldGroup), Timestamp.now()).toGroup()).steps + ) + } + + ok(result(planFuture).toString()) + } + else { + val (deployment, _, _) = updateOrCreate(id.toRootPath, update, force) + deploymentResult(deployment) } - - ok(result(planFuture).toString()) - } - else { - val (deployment, _, _) = updateOrCreate(id.toRootPath, update, force) - deploymentResult(deployment) } } } diff --git a/src/main/scala/mesosphere/marathon/api/v2/ModelValidation.scala b/src/main/scala/mesosphere/marathon/api/v2/ModelValidation.scala deleted file mode 100644 index a6a6e2c0364..00000000000 --- a/src/main/scala/mesosphere/marathon/api/v2/ModelValidation.scala +++ /dev/null @@ -1,319 +0,0 @@ -package mesosphere.marathon.api.v2 - -import java.net.{ HttpURLConnection, URL, URLConnection } -import javax.validation.ConstraintViolation - -import mesosphere.marathon.api.v2.BeanValidation._ -import mesosphere.marathon.api.v2.json.{ V2AppDefinition, V2AppUpdate, V2Group, V2GroupUpdate } -import mesosphere.marathon.state._ - -import scala.reflect.ClassTag -import scala.util.{ Failure, Success, Try } - -/** - * Specific validation helper for specific model classes. - */ -object ModelValidation { - - //scalastyle:off null - - /** - * This regular expression is used to validate each path segment of an ID. - * - * If you change this, please also change "pathType" in AppDefinition.json and - * notify the maintainers of the DCOS CLI. - */ - private[this] val ID_PATH_SEGMENT_PATTERN = - "^(([a-z0-9]|[a-z0-9][a-z0-9\\-]*[a-z0-9])\\.)*([a-z0-9]|[a-z0-9][a-z0-9\\-]*[a-z0-9])|(\\.|\\.\\.)$".r - - // TODO: Re-implement this method on it's own terms - def checkGroup( - group: Group, - path: String, - parent: PathId, - maxApps: Option[Int]): Iterable[ConstraintViolation[V2Group]] = { - - val v2Group = V2Group(group) - val maximumApps = maxApps.filter(group.transitiveApps.size > _).map{ num => - Seq(violation(v2Group, "apps", "", - s"This Marathon instance may only handle up to $num Apps! (Override with command line option --max_apps)")) - }.getOrElse(Seq.empty[ConstraintViolation[V2Group]]) - - checkGroup(v2Group, path, parent) ++ maximumApps - } - - def checkGroup( - group: V2Group, - path: String = "", - parent: PathId = PathId.empty): Iterable[ConstraintViolation[V2Group]] = { - val base = group.id.canonicalPath(parent) - validate(group, - idErrors(group, base, group.id, "id"), - checkPath(group, parent, group.id, path + "id"), - checkApps(group.apps, path + "apps", base), - checkGroups(group.groups, path + "groups", base), - noAppsAndGroupsWithSameName(group, path + "apps", group.apps, group.groups), - noCyclicDependencies(group, path + "dependencies") - ) - } - - def checkGroupUpdate( - group: V2GroupUpdate, - needsId: Boolean, - path: String = "", - parent: PathId = PathId.empty): Iterable[ConstraintViolation[V2GroupUpdate]] = { - if (group == null) { - Seq(violation(group, null, "", "Given group is empty!")) - } - else if (group.version.isDefined || group.scaleBy.isDefined) { - validate(group, - defined( - group, - group.version, - "version", - (b: V2GroupUpdate, t: Timestamp, i: String) => hasOnlyOneDefinedOption(b, t, i), - mandatory = false - ), - defined( - group, - group.scaleBy, - "scaleBy", - (b: V2GroupUpdate, t: Double, i: String) => hasOnlyOneDefinedOption(b, t, i), - mandatory = false - ) - ) - } - else { - val base = group.id.map(_.canonicalPath(parent)).getOrElse(parent) - validate(group, - defined( - group, - group.id, - "id", - (b: V2GroupUpdate, p: PathId, i: String) => idErrors(b, group.groupId.canonicalPath(parent), p, i), - mandatory = needsId - ), - group.id.map(checkPath(group, parent, _, path + "id")).getOrElse(Nil), - group.apps.map(checkApps(_, path + "apps", base)).getOrElse(Nil), - group.groups.map(checkGroupUpdates(_, path + "groups", base)).getOrElse(Nil) - ) - } - } - - private[this] def hasOnlyOneDefinedOption[A <: Product: ClassTag, B](product: A, prop: B, path: String) = { - val definedOptionsCount = product.productIterator.count { - case Some(_) => true - case _ => false - } - isTrue(product, prop, path, "not allowed in conjunction with other properties", definedOptionsCount == 1) - } - - def noAppsAndGroupsWithSameName[T]( - t: T, - path: String, - apps: Set[V2AppDefinition], - groups: Set[V2Group])(implicit ct: ClassTag[T]): Iterable[ConstraintViolation[_]] = { - val groupIds = groups.map(_.id) - val clashingIds = apps.map(_.id).intersect(groupIds) - isTrue( - t, - apps, - path, - s"Groups and Applications may not have the same identifier: ${clashingIds.mkString(", ")}", - clashingIds.isEmpty - ) - } - - def noCyclicDependencies( - group: V2Group, - path: String): Iterable[ConstraintViolation[V2Group]] = { - isTrue( - group, - group.dependencies, - path, - "Dependency graph has cyclic dependencies", - group.toGroup().hasNonCyclicDependencies) - } - - def checkGroupUpdates( - groups: Iterable[V2GroupUpdate], - path: String = "res", - parent: PathId = PathId.empty): Iterable[ConstraintViolation[V2GroupUpdate]] = - groups.zipWithIndex.flatMap { - case (group, pos) => - checkGroupUpdate(group, needsId = true, s"$path[$pos].", parent) - } - - def checkGroups( - groups: Iterable[V2Group], - path: String = "res", - parent: PathId = PathId.empty): Iterable[ConstraintViolation[V2Group]] = - groups.zipWithIndex.flatMap { - case (group, pos) => - checkGroup(group, s"$path[$pos].", parent) - } - - def checkUpdates( - apps: Iterable[V2AppUpdate], - path: String = "res"): Iterable[ConstraintViolation[V2AppUpdate]] = - apps.zipWithIndex.flatMap { - case (app, pos) => - checkUpdate(app, s"$path[$pos].") - } - - def checkPath[T: ClassTag]( - t: T, - parent: PathId, - child: PathId, - path: String): Iterable[ConstraintViolation[T]] = { - val isParent = child.canonicalPath(parent).parent == parent - if (parent != PathId.empty && !isParent) - List(violation(t, child, path, s"identifier $child is not child of $parent. Hint: use relative paths.")) - else Nil - } - - def checkApps( - apps: Iterable[V2AppDefinition], - path: String = "res", - parent: PathId = PathId.empty): Iterable[ConstraintViolation[V2AppDefinition]] = - apps.zipWithIndex.flatMap { - case (app, pos) => - checkAppConstraints(app, parent, s"$path[$pos].") - } - - def checkUpdate( - app: V2AppUpdate, - path: String = "", - needsId: Boolean = false): Iterable[ConstraintViolation[V2AppUpdate]] = { - validate(app, - defined( - app, - app.id, - "id", - (b: V2AppUpdate, p: PathId, i: String) => idErrors(b, PathId.empty, p, i), - needsId - ), - defined( - app, - app.upgradeStrategy, - "upgradeStrategy", - (b: V2AppUpdate, p: UpgradeStrategy, i: String) => upgradeStrategyErrors(b, p, i) - ), - defined( - app, - app.dependencies, - "dependencies", - (b: V2AppUpdate, p: Set[PathId], i: String) => dependencyErrors(b, PathId.empty, p, i) - ), - defined( - app, - app.storeUrls, - "storeUrls", - (b: V2AppUpdate, p: Seq[String], i: String) => urlsCanBeResolved(b, p, i) - ) - ) - } - - def checkAppConstraints(app: V2AppDefinition, parent: PathId, - path: String = ""): Iterable[ConstraintViolation[V2AppDefinition]] = - validate(app, - idErrors(app, parent, app.id, path + "id"), - checkPath(app, parent, app.id, path + "id"), - upgradeStrategyErrors(app, app.upgradeStrategy, path + "upgradeStrategy"), - dependencyErrors(app, parent, app.dependencies, path + "dependencies"), - urlsCanBeResolved(app, app.storeUrls, path + "storeUrls") - ) - - def urlsCanBeResolved[T: ClassTag](t: T, urls: Seq[String], path: String): Iterable[ConstraintViolation[T]] = { - def urlIsValid(url: String): Boolean = Try { - new URL(url).openConnection() match { - case http: HttpURLConnection => - http.setRequestMethod("HEAD") - http.getResponseCode == HttpURLConnection.HTTP_OK - case other: URLConnection => - other.getInputStream - true //if we come here, we could read the stream - } - }.getOrElse(false) - - urls.toList - .zipWithIndex - .collect { - case (url, pos) if !urlIsValid(url) => - violation(t, urls, s"$path[$pos]", s"Can not resolve url $url") - } - } - - def idErrors[T: ClassTag](t: T, base: PathId, id: PathId, path: String): Iterable[ConstraintViolation[T]] = { - val valid = id.path.forall(ID_PATH_SEGMENT_PATTERN.pattern.matcher(_).matches()) - val errors = - if (!valid) - List( - violation( - t, - id, - path, - "path contains invalid characters (allowed: lowercase letters, digits, hyphens, \".\", \"..\")" - ) - ) - else Nil - - Try(id.canonicalPath(base)) match { - case Success(_) => errors - case Failure(_) => violation(t, id, path, s"canonical path can not be computed for $id") :: errors - } - - errors - } - - def dependencyErrors[T: ClassTag]( - t: T, - base: PathId, - set: Set[PathId], - path: String): Iterable[ConstraintViolation[T]] = - set.zipWithIndex.flatMap{ case (id, pos) => idErrors(t, base, id, s"$path[$pos]") } - - def upgradeStrategyErrors[T: ClassTag]( - t: T, - upgradeStrategy: UpgradeStrategy, - path: String): Iterable[ConstraintViolation[T]] = { - if (upgradeStrategy.minimumHealthCapacity < 0) Some("is less than 0") - else if (upgradeStrategy.minimumHealthCapacity > 1) Some("is greater than 1") - else None - }.map { violation(t, upgradeStrategy, path + ".minimumHealthCapacity", _) } - .orElse({ - if (upgradeStrategy.maximumOverCapacity < 0) Some("is less than 0") - else if (upgradeStrategy.maximumOverCapacity > 1) Some("is greater than 1") - else None - }.map { violation(t, upgradeStrategy, path + ".maximumOverCapacity", _) }) - - /** - * Returns a non-empty list of validation messages if the given app definition - * will conflict with existing apps. - */ - def checkAppConflicts(app: AppDefinition, root: Group): Seq[String] = { - app.containerServicePorts.toSeq.flatMap { servicePorts => - checkServicePortConflicts(app.id, servicePorts, root) - } - } - - /** - * Returns a non-empty list of validations messages if the given app definition has service ports - * that will conflict with service ports in other applications. - * - * Does not compare the app definition's service ports with the same deployed app's service ports, as app updates - * may simply restate the existing service ports. - */ - private def checkServicePortConflicts(appId: PathId, requestedServicePorts: Seq[Int], - root: Group): Seq[String] = { - - for { - existingApp <- root.transitiveApps.toList - if existingApp.id != appId // in case of an update, do not compare the app against itself - existingServicePort <- existingApp.portMappings.toList.flatten.map(_.servicePort) - if existingServicePort != 0 // ignore zero ports, which will be chosen at random - if requestedServicePorts contains existingServicePort - } yield s"Requested service port $existingServicePort conflicts with a service port in app ${existingApp.id}" - } - -} diff --git a/src/main/scala/mesosphere/marathon/api/v2/Validation.scala b/src/main/scala/mesosphere/marathon/api/v2/Validation.scala new file mode 100644 index 00000000000..b7af1c3203b --- /dev/null +++ b/src/main/scala/mesosphere/marathon/api/v2/Validation.scala @@ -0,0 +1,122 @@ +package mesosphere.marathon.api.v2 + +import java.net.{ URLConnection, HttpURLConnection, URL } + +import com.wix.accord._ +import mesosphere.marathon.ValidationFailedException + +import play.api.libs.json._ + +import scala.reflect.ClassTag +import scala.util.Try + +object Validation { + + def validate[T](t: T)(implicit validator: Validator[T]): Result = validator.apply(t) + def validateOrThrow[T](t: T)(implicit validator: Validator[T]): T = validate(t) match { + case Success => t + case f: Failure => throw new ValidationFailedException(t, f) + } + + implicit def optional[T](implicit validator: Validator[T]): Validator[Option[T]] = { + new Validator[Option[T]] { + override def apply(option: Option[T]): Result = option.map(validator).getOrElse(Success) + } + } + + implicit def every[T](implicit validator: Validator[T]): Validator[Iterable[T]] = { + new Validator[Iterable[T]] { + override def apply(seq: Iterable[T]): Result = { + + val violations = seq.map(item => (item, validator(item))).zipWithIndex.collect { + case ((item, f: Failure), pos: Int) => GroupViolation(item, "not valid", Some(s"[$pos]"), f.violations) + } + + if (violations.isEmpty) Success + else Failure(Set(GroupViolation(seq, "seq contains elements, which are not valid", None, violations.toSet))) + } + } + } + + implicit lazy val failureWrites: Writes[Failure] = Writes { f => + Json.obj("errors" -> f.violations.flatMap(allRuleViolationsWithFullDescription(_))) + } + + implicit lazy val ruleViolationWrites: Writes[RuleViolation] = Writes { v => + Json.obj( + "attribute" -> v.description, + "error" -> v.constraint + ) + } + + private def allRuleViolationsWithFullDescription(violation: Violation, + parentDesc: Option[String] = None, + attachDot: Boolean = true): Set[RuleViolation] = { + def concatPath(parent: String, child: Option[String], attachDot: Boolean): String = { + child.map(c => parent + { if (attachDot) "." else "" } + c).getOrElse(parent) + } + + violation match { + case r: RuleViolation => Set(parentDesc.map(p => + r.withDescription(concatPath(p, r.description, attachDot))) + .getOrElse(r)) + case g: GroupViolation => g.children.flatMap { c => + parentDesc.map { p => + val desc = concatPath(p, g.description, attachDot) + allRuleViolationsWithFullDescription(c, Some(desc), g.description.isDefined) + } getOrElse { + allRuleViolationsWithFullDescription(c, g.description, g.description.isDefined) + } + } + } + } + + def urlCanBeResolvedValidator: Validator[String] = { + new Validator[String] { + def apply(url: String) = { + Try { + new URL(url).openConnection() match { + case http: HttpURLConnection => + http.setRequestMethod("HEAD") + if (http.getResponseCode == HttpURLConnection.HTTP_OK) Success + else Failure(Set(RuleViolation(url, "url could not be resolved", None))) + case other: URLConnection => + other.getInputStream + Success //if we come here, we could read the stream + } + }.getOrElse( + Failure(Set(RuleViolation(url, "url could not be resolved", None))) + ) + } + } + } + + def elementsAreUnique[A](p: Seq[A] => Seq[A] = { seq: Seq[A] => seq }): Validator[Seq[A]] = { + new Validator[Seq[A]] { + def apply(seq: Seq[A]) = { + val filteredSeq = p(seq) + if (filteredSeq.size == filteredSeq.distinct.size) Success + else Failure(Set(RuleViolation(seq, "Elements must be unique", None))) + } + } + } + + def theOnlyDefinedOptionIn[A <: Product: ClassTag, B](product: A): Validator[Option[B]] = + new Validator[Option[B]] { + def apply(option: Option[B]) = { + option match { + case Some(prop) => + val n = product.productIterator.count { + case Some(_) => true + case _ => false + } + + if (n == 1) + Success + else + Failure(Set(RuleViolation(product, s"not allowed in conjunction with other properties.", None))) + case None => Success + } + } + } +} diff --git a/src/main/scala/mesosphere/marathon/api/v2/json/V2AppDefinition.scala b/src/main/scala/mesosphere/marathon/api/v2/json/V2AppDefinition.scala index 1ca4360f8b0..65926ee2181 100644 --- a/src/main/scala/mesosphere/marathon/api/v2/json/V2AppDefinition.scala +++ b/src/main/scala/mesosphere/marathon/api/v2/json/V2AppDefinition.scala @@ -2,9 +2,10 @@ package mesosphere.marathon.api.v2.json import java.lang.{ Double => JDouble, Integer => JInt } +import com.wix.accord._ import mesosphere.marathon.Protos.Constraint -import mesosphere.marathon.api.validation.FieldConstraints._ -import mesosphere.marathon.api.validation.{ PortIndices, ValidV2AppDefinition } +import mesosphere.marathon.Protos.HealthCheckDefinition.Protocol +import mesosphere.marathon.api.v2.Validation._ import mesosphere.marathon.health.HealthCheck import mesosphere.marathon.state.AppDefinition.VersionInfo.FullVersionInfo import mesosphere.marathon.state._ @@ -13,8 +14,8 @@ import org.apache.mesos.{ Protos => mesos } import scala.collection.immutable.Seq import scala.concurrent.duration._ -@PortIndices -@ValidV2AppDefinition +import com.wix.accord.dsl._ + case class V2AppDefinition( id: PathId = AppDefinition.DefaultId, @@ -27,7 +28,7 @@ case class V2AppDefinition( env: Map[String, String] = AppDefinition.DefaultEnv, - @FieldMin(0) instances: JInt = AppDefinition.DefaultInstances, + instances: JInt = AppDefinition.DefaultInstances, cpus: JDouble = AppDefinition.DefaultCpus, @@ -35,7 +36,7 @@ case class V2AppDefinition( disk: JDouble = AppDefinition.DefaultDisk, - @FieldPattern(regexp = "^(//cmd)|(/?[^/]+(/[^/]+)*)|$") executor: String = AppDefinition.DefaultExecutor, + executor: String = AppDefinition.DefaultExecutor, constraints: Set[Constraint] = AppDefinition.DefaultConstraints, @@ -43,7 +44,7 @@ case class V2AppDefinition( storeUrls: Seq[String] = AppDefinition.DefaultStoreUrls, - @FieldPortsArray ports: Seq[JInt] = AppDefinition.DefaultPorts, + ports: Seq[JInt] = AppDefinition.DefaultPorts, requirePorts: Boolean = AppDefinition.DefaultRequirePorts, @@ -128,4 +129,57 @@ object V2AppDefinition { upgradeStrategy = app.upgradeStrategy, labels = app.labels, acceptedResourceRoles = app.acceptedResourceRoles, ipAddress = app.ipAddress, version = app.version, versionInfo = maybeVersionInfo) } + + /** + * We cannot validate HealthChecks here, because it would break backwards compatibility in weird ways. If users had already + * one invalid app definition, each deployment would cause a complete revalidation of the root group + * including the invalid one. Until the user changed all invalid apps, the user would get weird validation + * errors for every deployment potentially unrelated to the deployed apps. + */ + implicit val appDefinitionValidator = validator[V2AppDefinition] { appDef => + appDef.id is valid + appDef.dependencies is valid + appDef.upgradeStrategy is valid + appDef.storeUrls is every(urlCanBeResolvedValidator) + appDef.ports is elementsAreUnique(filterOutRandomPorts) + appDef.executor should matchRegexFully("^(//cmd)|(/?[^/]+(/[^/]+)*)|$") + appDef is containsCmdArgsContainerValidator + appDef is portIndicesAreValid + appDef.instances.intValue should be >= 0 + } + + def filterOutRandomPorts(ports: scala.Seq[JInt]): scala.Seq[JInt] = { + ports.filterNot(_ == AppDefinition.RandomPortValue) + } + + private def containsCmdArgsContainerValidator: Validator[V2AppDefinition] = { + new Validator[V2AppDefinition] { + override def apply(app: V2AppDefinition): Result = { + val cmd = app.cmd.nonEmpty + val args = app.args.nonEmpty + val container = app.container.exists(_ != Container.Empty) + if ((cmd ^ args) || (!(cmd || args) && container)) Success + else Failure(Set(RuleViolation(app, + "AppDefinition must either contain one of 'cmd' or 'args', and/or a 'container'.", None))) + } + } + } + + private def portIndicesAreValid: Validator[V2AppDefinition] = { + new Validator[V2AppDefinition] { + override def apply(app: V2AppDefinition): Result = { + val appDef = app.toAppDefinition + val validPortIndices = appDef.hostPorts.indices + + if (appDef.healthChecks.forall { hc => + hc.protocol == Protocol.COMMAND || (hc.portIndex match { + case Some(idx) => validPortIndices contains idx + case None => validPortIndices.length == 1 && validPortIndices.head == 0 + }) + }) Success + else Failure(Set(RuleViolation(app, + "Health check port indices must address an element of the ports array or container port mappings.", None))) + } + } + } } diff --git a/src/main/scala/mesosphere/marathon/api/v2/json/V2AppUpdate.scala b/src/main/scala/mesosphere/marathon/api/v2/json/V2AppUpdate.scala index 8d7acf2c17b..5942426c182 100644 --- a/src/main/scala/mesosphere/marathon/api/v2/json/V2AppUpdate.scala +++ b/src/main/scala/mesosphere/marathon/api/v2/json/V2AppUpdate.scala @@ -3,7 +3,6 @@ package mesosphere.marathon.api.v2.json import java.lang.{ Double => JDouble, Integer => JInt } import mesosphere.marathon.Protos.Constraint -import mesosphere.marathon.api.validation.FieldConstraints._ import mesosphere.marathon.health.HealthCheck import mesosphere.marathon.state.{ AppDefinition, Container, IpAddress, PathId, Timestamp, UpgradeStrategy } @@ -38,7 +37,7 @@ case class V2AppUpdate( storeUrls: Option[Seq[String]] = None, - @FieldPortsArray ports: Option[Seq[JInt]] = None, + ports: Option[Seq[JInt]] = None, requirePorts: Option[Boolean] = None, @@ -116,3 +115,15 @@ case class V2AppUpdate( dependencies = dependencies.map(_.map(_.canonicalPath(base))) ) } + +object V2AppUpdate { + import com.wix.accord.dsl._ + import mesosphere.marathon.api.v2.Validation._ + implicit val appUpdateValidator = validator[V2AppUpdate] { appUp => + appUp.id is valid + appUp.dependencies is valid + appUp.upgradeStrategy is valid + appUp.storeUrls is optional(every(urlCanBeResolvedValidator)) + appUp.ports is optional(elementsAreUnique(V2AppDefinition.filterOutRandomPorts)) + } +} diff --git a/src/main/scala/mesosphere/marathon/api/v2/json/V2Group.scala b/src/main/scala/mesosphere/marathon/api/v2/json/V2Group.scala index 9563c003a93..fe6544db131 100644 --- a/src/main/scala/mesosphere/marathon/api/v2/json/V2Group.scala +++ b/src/main/scala/mesosphere/marathon/api/v2/json/V2Group.scala @@ -1,6 +1,9 @@ package mesosphere.marathon.api.v2.json +import com.wix.accord._ +import com.wix.accord.dsl._ import mesosphere.marathon.state.{ Group, PathId, Timestamp } +import mesosphere.marathon.api.v2.Validation._ case class V2Group( id: PathId, @@ -36,4 +39,72 @@ object V2Group { groups = group.groups.map(V2Group(_)), dependencies = group.dependencies, version = group.version) + + implicit val v2GroupValidator: Validator[V2Group] = validator[V2Group] { group => + group.id is valid + group.apps is valid + group.groups is valid + group is noAppsAndGroupsWithSameName + (group.id.isRoot is false) or (group.dependencies is noCyclicDependencies(group)) + + group is validPorts + } + + def v2GroupWithConfigValidator(maxApps: Option[Int])(implicit validator: Validator[V2Group]): Validator[V2Group] = { + new Validator[V2Group] { + override def apply(group: V2Group): Result = { + maxApps.filter(group.toGroup().transitiveApps.size > _).map { num => + Failure(Set(RuleViolation(group, + s"""This Marathon instance may only handle up to $num Apps! + |(Override with command line option --max_apps)""".stripMargin, None))) + } getOrElse Success + } and validator(group) + } + } + + private def noAppsAndGroupsWithSameName: Validator[V2Group] = + new Validator[V2Group] { + def apply(group: V2Group) = { + val groupIds = group.groups.map(_.id) + val clashingIds = group.apps.map(_.id).intersect(groupIds) + + if (clashingIds.isEmpty) Success + else Failure(Set(RuleViolation(group, + s"Groups and Applications may not have the same identifier: ${clashingIds.mkString(", ")}", None))) + } + } + + private def noCyclicDependencies(group: V2Group): Validator[Set[PathId]] = + new Validator[Set[PathId]] { + def apply(dependencies: Set[PathId]) = { + if (group.toGroup().hasNonCyclicDependencies) Success + else Failure(Set(RuleViolation(group, "Dependency graph has cyclic dependencies", None))) + } + } + + private def validPorts: Validator[V2Group] = { + new Validator[V2Group] { + override def apply(group: V2Group): Result = { + val groupViolations = group.apps.flatMap { app => + val ruleViolations = app.toAppDefinition.containerServicePorts.toSeq.flatMap { servicePorts => + for { + existingApp <- group.toGroup().transitiveApps.toList + if existingApp.id != app.id // in case of an update, do not compare the app against itself + existingServicePort <- existingApp.portMappings.toList.flatten.map(_.servicePort) + if existingServicePort != 0 // ignore zero ports, which will be chosen at random + if servicePorts contains existingServicePort + } yield RuleViolation(app.id, + s"Requested service port $existingServicePort conflicts with a service port in app ${existingApp.id}", + None) + } + + if (ruleViolations.isEmpty) None + else Some(GroupViolation(app, "app contains conflicting ports", None, ruleViolations.toSet)) + } + + if (groupViolations.isEmpty) Success + else Failure(groupViolations.toSet) + } + } + } } diff --git a/src/main/scala/mesosphere/marathon/api/v2/json/V2GroupUpdate.scala b/src/main/scala/mesosphere/marathon/api/v2/json/V2GroupUpdate.scala index 1d9c119cea6..8a17a87aac3 100644 --- a/src/main/scala/mesosphere/marathon/api/v2/json/V2GroupUpdate.scala +++ b/src/main/scala/mesosphere/marathon/api/v2/json/V2GroupUpdate.scala @@ -1,8 +1,14 @@ package mesosphere.marathon.api.v2.json import java.lang.{ Double => JDouble } +import com.wix.accord._ +import com.wix.accord.dsl._ import mesosphere.marathon.state._ +import mesosphere.marathon.api.v2.Validation._ + +import scala.reflect.ClassTag + case class V2GroupUpdate( id: Option[PathId], apps: Option[Set[V2AppDefinition]] = None, @@ -58,4 +64,15 @@ object V2GroupUpdate { V2GroupUpdate(Some(id), if (apps.isEmpty) None else Some(apps), if (groups.isEmpty) None else Some(groups)) } def empty(id: PathId): V2GroupUpdate = V2GroupUpdate(Some(id)) + + implicit val v2GroupUpdateValidator: Validator[V2GroupUpdate] = validator[V2GroupUpdate] { group => + group is notNull + + group.version is theOnlyDefinedOptionIn(group) + group.scaleBy is theOnlyDefinedOptionIn(group) + + group.id is valid + group.apps is valid + group.groups is valid + } } diff --git a/src/main/scala/mesosphere/marathon/api/validation/FieldConstraints.scala b/src/main/scala/mesosphere/marathon/api/validation/FieldConstraints.scala deleted file mode 100644 index 330a5e1c185..00000000000 --- a/src/main/scala/mesosphere/marathon/api/validation/FieldConstraints.scala +++ /dev/null @@ -1,16 +0,0 @@ -package mesosphere.marathon.api.validation - -import org.hibernate.validator.constraints.NotEmpty -import javax.validation.constraints.{ Min, Pattern } -import scala.annotation.meta.field - -/** - * Provides type aliases for constraint annotations that target fields - * associated with Scala class constructor arguments. - */ -object FieldConstraints { - type FieldPortsArray = PortsArray @field - type FieldNotEmpty = NotEmpty @field - type FieldPattern = Pattern @field - type FieldMin = Min @field -} diff --git a/src/main/scala/mesosphere/marathon/api/validation/HealthCheckValidator.scala b/src/main/scala/mesosphere/marathon/api/validation/HealthCheckValidator.scala deleted file mode 100644 index 2169e25609c..00000000000 --- a/src/main/scala/mesosphere/marathon/api/validation/HealthCheckValidator.scala +++ /dev/null @@ -1,33 +0,0 @@ -package mesosphere.marathon.api.validation - -import mesosphere.marathon.health.HealthCheck -import mesosphere.marathon.Protos.HealthCheckDefinition.Protocol -import javax.validation.{ ConstraintValidator, ConstraintValidatorContext } - -/** - * FIXME: This is not called when an V2AppDefinition is called because the recursion apparently doesn't work - * with Scala Seq. - * - * We cannot re-enable it because it would break backwards compatibility in weird ways. If users hard already - * one invalid app definition, each deployment would cause a complete revalidation of the root group - * including the invalid one. Until the user changed all invalid apps, the user would get weird validation - * errors for every deployment potentially unrelated to the deployed apps. - */ -class HealthCheckValidator - extends ConstraintValidator[ValidHealthCheck, HealthCheck] { - - def initialize(annotation: ValidHealthCheck): Unit = {} - - def isValid(hc: HealthCheck, context: ConstraintValidatorContext): Boolean = { - def eitherPortIndexOrPort: Boolean = hc.portIndex.isDefined ^ hc.port.isDefined - - val hasCommand = hc.command.isDefined - val hasPath = hc.path.isDefined - hc.protocol match { - case Protocol.COMMAND => hasCommand && !hasPath && hc.port.isEmpty - case Protocol.HTTP => !hasCommand && eitherPortIndexOrPort - case Protocol.TCP => !hasCommand && !hasPath && eitherPortIndexOrPort - case _ => true - } - } -} diff --git a/src/main/scala/mesosphere/marathon/api/validation/PortIndicesValidator.scala b/src/main/scala/mesosphere/marathon/api/validation/PortIndicesValidator.scala deleted file mode 100644 index e224862bc7a..00000000000 --- a/src/main/scala/mesosphere/marathon/api/validation/PortIndicesValidator.scala +++ /dev/null @@ -1,14 +0,0 @@ -package mesosphere.marathon.api.validation - -import mesosphere.marathon.api.v2.json.V2AppDefinition -import javax.validation.{ ConstraintValidator, ConstraintValidatorContext } - -class PortIndicesValidator - extends ConstraintValidator[PortIndices, V2AppDefinition] { - - def initialize(annotation: PortIndices): Unit = {} - - def isValid(app: V2AppDefinition, context: ConstraintValidatorContext): Boolean = - app.portIndicesAreValid() - -} diff --git a/src/main/scala/mesosphere/marathon/api/validation/PortsArrayValidator.scala b/src/main/scala/mesosphere/marathon/api/validation/PortsArrayValidator.scala deleted file mode 100644 index 96b657d3872..00000000000 --- a/src/main/scala/mesosphere/marathon/api/validation/PortsArrayValidator.scala +++ /dev/null @@ -1,30 +0,0 @@ -package mesosphere.marathon.api.validation - -import mesosphere.marathon.state.AppDefinition.RandomPortValue -import mesosphere.util.Logging - -import javax.validation.{ ConstraintValidator, ConstraintValidatorContext } - -/** - * This validator accepts objects of type Iterable[Int] where all of the - * elements are unique, and Option[Iterable[Int]] where the wrapped collection's - * elements are unique. - */ -class PortsArrayValidator - extends ConstraintValidator[PortsArray, Any] - with Logging { - - def initialize(annotation: PortsArray): Unit = {} - - def isValid(obj: Any, context: ConstraintValidatorContext): Boolean = { - - obj match { - case opt: Option[_] => opt.forall { isValid(_, context) } - case it: Iterable[_] => - val withoutRandoms = it.toSeq.filterNot { _ == RandomPortValue } - withoutRandoms.size == withoutRandoms.distinct.size - case _ => false - } - } - -} diff --git a/src/main/scala/mesosphere/marathon/api/validation/V2AppDefinitionValidator.scala b/src/main/scala/mesosphere/marathon/api/validation/V2AppDefinitionValidator.scala deleted file mode 100644 index d33250e395c..00000000000 --- a/src/main/scala/mesosphere/marathon/api/validation/V2AppDefinitionValidator.scala +++ /dev/null @@ -1,20 +0,0 @@ -package mesosphere.marathon.api.validation - -import javax.validation.{ ConstraintValidatorContext, ConstraintValidator } - -import mesosphere.marathon.api.v2.json.V2AppDefinition -import mesosphere.marathon.state.Container - -class V2AppDefinitionValidator extends ConstraintValidator[ValidV2AppDefinition, V2AppDefinition] { - override def initialize(constraintAnnotation: ValidV2AppDefinition): Unit = {} - - override def isValid( - value: V2AppDefinition, - context: ConstraintValidatorContext): Boolean = { - val cmd = value.cmd.nonEmpty - val args = value.args.nonEmpty - val container = value.container.exists(_ != Container.Empty) - (cmd ^ args) || (!(cmd || args) && container) - } - -} diff --git a/src/main/scala/mesosphere/marathon/health/HealthCheck.scala b/src/main/scala/mesosphere/marathon/health/HealthCheck.scala index 6fe5b05e3e8..7772579a4d7 100644 --- a/src/main/scala/mesosphere/marathon/health/HealthCheck.scala +++ b/src/main/scala/mesosphere/marathon/health/HealthCheck.scala @@ -2,15 +2,15 @@ package mesosphere.marathon.health import java.lang.{ Integer => JInt } +import com.wix.accord._ +import com.wix.accord.dsl._ import mesosphere.marathon.Protos import mesosphere.marathon.Protos.HealthCheckDefinition.Protocol -import mesosphere.marathon.api.validation.ValidHealthCheck import mesosphere.marathon.state.{ Command, MarathonState, Timestamp } import org.apache.mesos.{ Protos => MesosProtos } import scala.concurrent.duration._ -@ValidHealthCheck case class HealthCheck( path: Option[String] = HealthCheck.DefaultPath, @@ -122,4 +122,27 @@ object HealthCheck { val DefaultMaxConsecutiveFailures = 3 val DefaultIgnoreHttp1xx = false val DefaultPort = None + + implicit val healthCheck = validator[HealthCheck] { hc => + (hc.portIndex.nonEmpty is true) or (hc.port.nonEmpty is true) + hc is validProtocol + } + + //scalastyle:off + private def validProtocol: Validator[HealthCheck] = { + new Validator[HealthCheck] { + override def apply(hc: HealthCheck): Result = { + def eitherPortIndexOrPort: Boolean = hc.portIndex.isDefined ^ hc.port.isDefined + val hasCommand = hc.command.isDefined + val hasPath = hc.path.isDefined + if (hc.protocol match { + case Protocol.COMMAND => hasCommand && !hasPath && hc.port.isEmpty + case Protocol.HTTP => !hasCommand && eitherPortIndexOrPort + case Protocol.TCP => !hasCommand && !hasPath && eitherPortIndexOrPort + case _ => true + }) Success else Failure(Set(RuleViolation(hc, s"HealthCheck is having parameters violation ${hc.protocol} protocol.", None))) + } + } + } + //scalastyle:on } diff --git a/src/main/scala/mesosphere/marathon/state/AppDefinition.scala b/src/main/scala/mesosphere/marathon/state/AppDefinition.scala index 35a1a4f7e0e..ac0d6aedbbd 100644 --- a/src/main/scala/mesosphere/marathon/state/AppDefinition.scala +++ b/src/main/scala/mesosphere/marathon/state/AppDefinition.scala @@ -5,6 +5,7 @@ import java.lang.{ Double => JDouble, Integer => JInt } import mesosphere.marathon.Protos import mesosphere.marathon.Protos.Constraint import mesosphere.marathon.Protos.HealthCheckDefinition.Protocol +import mesosphere.marathon.api.v2.json.V2AppDefinition import mesosphere.marathon.health.HealthCheck import mesosphere.marathon.state.AppDefinition.VersionInfo import mesosphere.marathon.state.AppDefinition.VersionInfo.{ FullVersionInfo, OnlyVersion } @@ -19,6 +20,8 @@ import scala.collection.JavaConverters._ import scala.collection.immutable.Seq import scala.concurrent.duration._ +import com.wix.accord.dsl._ + case class AppDefinition( id: PathId = AppDefinition.DefaultId, @@ -441,4 +444,8 @@ object AppDefinition { def fromProto(proto: Protos.ServiceDefinition): AppDefinition = AppDefinition().mergeFromProto(proto) + + implicit val appDefinitionValidator = validator[AppDefinition] { appDef => + V2AppDefinition(appDef) is valid + } } diff --git a/src/main/scala/mesosphere/marathon/state/Group.scala b/src/main/scala/mesosphere/marathon/state/Group.scala index edb8abd822c..0a5e3220e02 100644 --- a/src/main/scala/mesosphere/marathon/state/Group.scala +++ b/src/main/scala/mesosphere/marathon/state/Group.scala @@ -1,8 +1,12 @@ package mesosphere.marathon.state +import com.wix.accord.{ RuleViolation, Failure, Result, Validator } import mesosphere.marathon.Protos.GroupDefinition +import mesosphere.marathon.api.v2.json.V2Group import mesosphere.marathon.state.Group._ +import mesosphere.marathon.state.Group.empty import mesosphere.marathon.state.PathId._ +import mesosphere.marathon.state.PathId.empty import org.jgrapht.DirectedGraph import org.jgrapht.alg.CycleDetector import org.jgrapht.graph._ @@ -207,5 +211,11 @@ object Group { def defaultGroups: Set[Group] = Set.empty def defaultDependencies: Set[PathId] = Set.empty def defaultVersion: Timestamp = Timestamp.now() + + def groupWithConfigValidator(maxApps: Option[Int])(implicit validator: Validator[V2Group]): Validator[Group] = { + new Validator[Group] { + override def apply(group: Group): Result = V2Group.v2GroupWithConfigValidator(maxApps).apply(V2Group(group)) + } + } } diff --git a/src/main/scala/mesosphere/marathon/state/GroupManager.scala b/src/main/scala/mesosphere/marathon/state/GroupManager.scala index f3586b61f3b..5506bab1014 100644 --- a/src/main/scala/mesosphere/marathon/state/GroupManager.scala +++ b/src/main/scala/mesosphere/marathon/state/GroupManager.scala @@ -6,8 +6,9 @@ import javax.inject.{ Inject, Named } import akka.event.EventStream import com.google.inject.Singleton + import mesosphere.marathon.Protos.MarathonTask -import mesosphere.marathon.api.v2.{ BeanValidation, ModelValidation } +import mesosphere.marathon.api.v2.json.V2Group import mesosphere.marathon.event.{ EventModule, GroupChangeFailed, GroupChangeSuccess } import mesosphere.marathon.io.PathFun import mesosphere.marathon.io.storage.StorageProvider @@ -22,6 +23,9 @@ import scala.collection.mutable import scala.concurrent.Future import scala.util.{ Failure, Success } +import mesosphere.marathon.api.v2.Validation._ +import mesosphere.marathon.api.v2.json.V2Group._ + /** * The group manager is the facade for all group related actions. * It persists the state of a group and initiates deployments. @@ -155,7 +159,7 @@ class GroupManager @Singleton @Inject() ( from <- rootGroup() (toUnversioned, resolve) <- resolveStoreUrls(assignDynamicServicePorts(from, change(from))) to = GroupVersioningUtil.updateVersionInfoForChangedApps(version, from, toUnversioned) - _ = BeanValidation.requireValid(ModelValidation.checkGroup(to, "", PathId.empty, config.maxApps.get)) + _ = validateOrThrow(to)(validator = Group.groupWithConfigValidator(config.maxApps.get)) plan = DeploymentPlan(from, to, resolve, version, toKill) _ = log.info(s"Computed new deployment plan:\n$plan") _ <- scheduler.deploy(plan, force) diff --git a/src/main/scala/mesosphere/marathon/state/PathId.scala b/src/main/scala/mesosphere/marathon/state/PathId.scala index a41360c00b4..2a05b910040 100644 --- a/src/main/scala/mesosphere/marathon/state/PathId.scala +++ b/src/main/scala/mesosphere/marathon/state/PathId.scala @@ -4,6 +4,9 @@ import mesosphere.marathon.plugin import scala.language.implicitConversions +import com.wix.accord.dsl._ +import com.wix.accord._ + case class PathId(path: List[String], absolute: Boolean = true) extends Ordered[PathId] with plugin.PathId { def root: String = path.headOption.getOrElse("") @@ -88,4 +91,51 @@ object PathId { def toPath: PathId = PathId(stringPath) def toRootPath: PathId = PathId(stringPath).canonicalPath() } + + /** + * This regular expression is used to validate each path segment of an ID. + * + * If you change this, please also change "pathType" in AppDefinition.json and + * notify the maintainers of the DCOS CLI. + */ + private[this] val ID_PATH_SEGMENT_PATTERN = + "^(([a-z0-9]|[a-z0-9][a-z0-9\\-]*[a-z0-9])\\.)*([a-z0-9]|[a-z0-9][a-z0-9\\-]*[a-z0-9])|(\\.|\\.\\.)$".r + + /** + * For external usage. Needed to overwrite the whole description, e.g. id.path -> id. + * @return + */ + implicit def pathIdValidator: Validator[PathId] = { + new Validator[PathId] { + override def apply(pathId: PathId): Result = { + validate(pathId.path)(validator = pathId.path.each should matchRegexFully(ID_PATH_SEGMENT_PATTERN.pattern)) and + validChild(pathId) + } + } + } + + /** + * Check if path's parent, if it exists, is a valid parent indeed. + * @return + */ + private def validChild: Validator[PathId] = { + new Validator[PathId] { + override def apply(pathId: PathId): Result = { + if (pathId.parent == "".toPath) Success + else if (pathId.parent.absolute) { + val p = pathId.canonicalPath(pathId.parent) + if (pathId.parent != PathId.empty && p.parent != pathId.parent) Failure(Set( + RuleViolation(pathId, + s"""identifier $pathId is not child of ${pathId.parent}. + |Actual parent: ${p.parent}. + |Hint: use relative paths.""". + stripMargin, None) + ) + ) + else Success + } + else Failure(Set(RuleViolation(pathId.parent, "path of parent should be absolute", None))) + } + } + } } diff --git a/src/main/scala/mesosphere/marathon/state/UpgradeStrategy.scala b/src/main/scala/mesosphere/marathon/state/UpgradeStrategy.scala index 9810974132a..08efd9c115c 100644 --- a/src/main/scala/mesosphere/marathon/state/UpgradeStrategy.scala +++ b/src/main/scala/mesosphere/marathon/state/UpgradeStrategy.scala @@ -2,6 +2,8 @@ package mesosphere.marathon.state import mesosphere.marathon.Protos._ +import com.wix.accord.dsl._ + case class UpgradeStrategy(minimumHealthCapacity: Double, maximumOverCapacity: Double = 1.0) { def toProto: UpgradeStrategyDefinition = UpgradeStrategyDefinition.newBuilder .setMinimumHealthCapacity(minimumHealthCapacity) @@ -16,4 +18,9 @@ object UpgradeStrategy { upgradeStrategy.getMinimumHealthCapacity, upgradeStrategy.getMaximumOverCapacity ) + + implicit val updateStrategyValidator = validator[UpgradeStrategy] { strategy => + strategy.minimumHealthCapacity is between(0.0, 1.0) + strategy.maximumOverCapacity is between(0.0, 1.0) + } } diff --git a/src/test/scala/mesosphere/marathon/api/MarathonExceptionMapperTest.scala b/src/test/scala/mesosphere/marathon/api/MarathonExceptionMapperTest.scala index fa1d2a495da..f3d20104b2e 100644 --- a/src/test/scala/mesosphere/marathon/api/MarathonExceptionMapperTest.scala +++ b/src/test/scala/mesosphere/marathon/api/MarathonExceptionMapperTest.scala @@ -1,4 +1,5 @@ package mesosphere.marathon.api +/* import javax.validation.ConstraintViolationException @@ -90,3 +91,4 @@ class MarathonExceptionMapperTest extends MarathonSpec with GivenWhenThen with M (firstError \ "error").as[String] should be("AppDefinition must either contain one of 'cmd' or 'args', and/or a 'container'.") } } +*/ diff --git a/src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala b/src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala index f66435789f9..5e44d8ae893 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala +++ b/src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala @@ -1,7 +1,6 @@ package mesosphere.marathon.api.v2 import java.util -import javax.validation.ConstraintViolationException import akka.event.EventStream import mesosphere.marathon._ @@ -64,7 +63,8 @@ class AppsResourceTest extends MarathonSpec with Matchers with Mockito with Give groupManager.updateApp(any, any, any, any, any) returns Future.successful(plan) Then("A constraint violation exception is thrown") - intercept[ConstraintViolationException] { appsResource.create(body, false, auth.request, auth.response) } + val response = appsResource.create(body, false, auth.request, auth.response) + response.getStatus should be(422) } test("Create a new app with float instance count fails") { diff --git a/src/test/scala/mesosphere/marathon/api/v2/ModelValidationTest.scala b/src/test/scala/mesosphere/marathon/api/v2/ModelValidationTest.scala index e6dc89f026d..39626d5c540 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/ModelValidationTest.scala +++ b/src/test/scala/mesosphere/marathon/api/v2/ModelValidationTest.scala @@ -9,6 +9,9 @@ import mesosphere.marathon.state._ import org.apache.mesos.Protos.ContainerInfo.DockerInfo.Network import org.scalatest.{ BeforeAndAfterAll, Matchers, OptionValues } +import mesosphere.marathon.api.v2.Validation._ +import mesosphere.marathon.api.v2.json.V2Group + import scala.collection.immutable.Seq class ModelValidationTest @@ -20,8 +23,7 @@ class ModelValidationTest test("A group update should pass validation") { val update = V2GroupUpdate(id = Some("/a/b/c".toPath)) - val violations = ModelValidation.checkGroupUpdate(update, true) - violations should have size 0 + validate(update).isSuccess should be(true) } test("A group can not be updated to have more than the configured number of apps") { @@ -31,49 +33,46 @@ class ModelValidationTest createServicePortApp("/c".toPath, 0).toAppDefinition )) - val validations = ModelValidation.checkGroup(group, "", PathId.empty, maxApps = Some(2)) - validations should not be Nil - validations.find(_.getMessage.contains("This Marathon instance may only handle up to 2 Apps!")) should be ('defined) + val failedResult = V2Group.v2GroupWithConfigValidator(Some(2)).apply(V2Group(group)) + failedResult.isFailure should be(true) + ValidationHelper.getAllRuleConstrains(failedResult) + .find(v => v.message.contains("This Marathon instance may only handle up to 2 Apps!")) should be ('defined) - val noValidations = ModelValidation.checkGroup(group, "", PathId.empty, maxApps = Some(10)) - noValidations should be('empty) + val successfulResult = V2Group.v2GroupWithConfigValidator(Some(10)).apply(V2Group(group)) + successfulResult.isSuccess should be(true) } test("Model validation should catch new apps that conflict with service ports in existing apps") { - val existingApp = createServicePortApp("/app1".toPath, 3200) - val group = Group(id = PathId.empty, apps = Set(existingApp.toAppDefinition)) - + val existingApp = createServicePortApp("/app1".toPath, 3200).toAppDefinition val conflictingApp = createServicePortApp("/app2".toPath, 3200).toAppDefinition - val validations = ModelValidation.checkAppConflicts(conflictingApp, group) - validations should not be Nil + val group = Group(id = PathId.empty, apps = Set(existingApp, conflictingApp)) + val result = validate(V2Group(group)) + + ValidationHelper.getAllRuleConstrains(result).exists(v => + v.message == "Requested service port 3200 conflicts with a service port in app /app2") should be(true) } test("Model validation should allow new apps that do not conflict with service ports in existing apps") { - val existingApp = createServicePortApp("/app1".toPath, 3200) - val group = Group(id = PathId.empty, apps = Set(existingApp.toAppDefinition)) - + val existingApp = createServicePortApp("/app1".toPath, 3200).toAppDefinition val conflictingApp = createServicePortApp("/app2".toPath, 3201).toAppDefinition - val validations = ModelValidation.checkAppConflicts(conflictingApp, group) - validations should be(Nil) + val group = Group(id = PathId.empty, apps = Set(existingApp, conflictingApp)) + val result = validate(V2Group(group)) + + result.isSuccess should be(true) } test("Model validation should check for application conflicts") { - val existingApp = createServicePortApp("/app1".toPath, 3200) - val group = Group(id = PathId.empty, apps = Set(existingApp.toAppDefinition)) - - val conflictingApp = existingApp.copy(id = "/app2".toPath).toAppDefinition - val validations = ModelValidation.checkAppConflicts(conflictingApp, group) + val existingApp = createServicePortApp("/app1".toPath, 3200).toAppDefinition + val conflictingApp = existingApp.copy(id = "/app2".toPath) - validations should not be Nil - } + val group = Group(id = PathId.empty, apps = Set(existingApp, conflictingApp)) + val result = validate(V2Group(group)) - test("Null groups should be validated correctly") { - val result = ModelValidation.checkGroupUpdate(null, needsId = true) - result should have size 1 - result.head.getMessage should be("Given group is empty!") + ValidationHelper.getAllRuleConstrains(result).exists(v => + v.message == "Requested service port 3200 conflicts with a service port in app /app2") should be(true) } private def createServicePortApp(id: PathId, servicePort: Int) = @@ -88,4 +87,4 @@ class ModelValidationTest )) ) -} +} \ No newline at end of file diff --git a/src/test/scala/mesosphere/marathon/api/v2/ValidationHelper.scala b/src/test/scala/mesosphere/marathon/api/v2/ValidationHelper.scala new file mode 100644 index 00000000000..03e4f892a08 --- /dev/null +++ b/src/test/scala/mesosphere/marathon/api/v2/ValidationHelper.scala @@ -0,0 +1,31 @@ +package mesosphere.marathon.api.v2 + +import com.wix.accord._ + +/** + * Created by alex on 07/01/16. + */ +object ValidationHelper { + case class ViolationMessageAndProperty(message: String, property: Option[String]) { + override def toString: String = s"Property: $property Message: $message" + } + + def getAllRuleConstrains(r: Result): Seq[ViolationMessageAndProperty] = { + def loop(v: Violation, prop: Option[String]): Seq[ViolationMessageAndProperty] = { + v match { + case g: GroupViolation => + g.children.flatMap { c => + val nextProp = prop.map(p => + Some(c.description.map(desc => s"$p.$desc").getOrElse(p))).getOrElse(c.description) + loop(c, nextProp) + }.toSeq + case r: RuleViolation => Seq(ViolationMessageAndProperty(r.constraint, prop)) + } + } + + r match { + case f: Failure => f.violations.flatMap(v => loop(v, v.description)).toSeq + case _ => Seq.empty[ViolationMessageAndProperty] + } + } +} diff --git a/src/test/scala/mesosphere/marathon/api/v2/json/V2AppDefinitionTest.scala b/src/test/scala/mesosphere/marathon/api/v2/json/V2AppDefinitionTest.scala index 0cc310c9313..a9f69d13668 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/json/V2AppDefinitionTest.scala +++ b/src/test/scala/mesosphere/marathon/api/v2/json/V2AppDefinitionTest.scala @@ -4,7 +4,7 @@ import mesosphere.marathon.MarathonSpec import mesosphere.marathon.Protos.Constraint import mesosphere.marathon.Protos.HealthCheckDefinition.Protocol import mesosphere.marathon.api.JsonTestHelper -import mesosphere.marathon.api.v2.ModelValidation +import mesosphere.marathon.api.v2.ValidationHelper import mesosphere.marathon.health.HealthCheck import mesosphere.marathon.state.Container.Docker import mesosphere.marathon.state.DiscoveryInfo.Port @@ -17,32 +17,39 @@ import play.api.libs.json.{ JsError, Json } import scala.collection.immutable.Seq import scala.concurrent.duration._ +import com.wix.accord._ class V2AppDefinitionTest extends MarathonSpec with Matchers { test("Validation") { - def shouldViolate(app: V2AppDefinition, path: String, template: String) = { - val violations = ModelValidation.checkAppConstraints(app, PathId.empty) - assert( - violations.exists { v => - v.getPropertyPath.toString == path && v.getMessageTemplate == template - }, - s"Violations:\n${violations.mkString}" - ) + def shouldViolate(app: V2AppDefinition, path: String, template: String): Unit = { + validate(app) match { + case Success => fail() + case f: Failure => + val violations = ValidationHelper.getAllRuleConstrains(f) + assert(violations.exists { v => + v.property.contains(path) && v.message == template + }, + s"Violations:\n${violations.mkString}" + ) + } } - def shouldNotViolate(app: V2AppDefinition, path: String, template: String) = { - val violations = ModelValidation.checkAppConstraints(app, PathId.empty) - assert( - !violations.exists { v => - v.getPropertyPath.toString == path && v.getMessageTemplate == template - }, - s"Violations:\n${violations.mkString}" - ) + def shouldNotViolate(app: V2AppDefinition, path: String, template: String): Unit = { + validate(app) match { + case Success => + case f: Failure => + val violations = ValidationHelper.getAllRuleConstrains(f) + assert(!violations.exists { v => + v.property.contains(path) && v.message == template + }, + s"Violations:\n${violations.mkString}" + ) + } } var app = V2AppDefinition(id = "a b".toRootPath) - val idError = "path contains invalid characters (allowed: lowercase letters, digits, hyphens, \".\", \"..\")" + val idError = "must fully match regular expression '^(([a-z0-9]|[a-z0-9][a-z0-9\\-]*[a-z0-9])\\.)*([a-z0-9]|[a-z0-9][a-z0-9\\-]*[a-z0-9])|(\\.|\\.\\.)$'" validateJsonSchema(app, false) shouldViolate(app, "id", idError) @@ -70,7 +77,7 @@ class V2AppDefinitionTest extends MarathonSpec with Matchers { ) validateJsonSchema(app, false) - app = V2AppDefinition(id = "test".toPath, ports = Seq(0, 0, 8080)) + app = V2AppDefinition(id = "test".toPath, ports = Seq(0, 0, 8080), cmd = Some("true")) shouldNotViolate( app, "ports", @@ -116,7 +123,7 @@ class V2AppDefinitionTest extends MarathonSpec with Matchers { shouldViolate( app, "executor", - "{javax.validation.constraints.Pattern.message}" + "must fully match regular expression '^(//cmd)|(/?[^/]+(/[^/]+)*)|$'" ) validateJsonSchema(app, false) @@ -124,14 +131,14 @@ class V2AppDefinitionTest extends MarathonSpec with Matchers { shouldViolate( app, "executor", - "{javax.validation.constraints.Pattern.message}" + "must fully match regular expression '^(//cmd)|(/?[^/]+(/[^/]+)*)|$'" ) validateJsonSchema(app, false) app = correct.copy(cmd = Some("command"), args = Some(Seq("a", "b", "c"))) shouldViolate( app, - "", + "value", "AppDefinition must either contain one of 'cmd' or 'args', and/or a 'container'." ) validateJsonSchema(app, false) @@ -139,7 +146,7 @@ class V2AppDefinitionTest extends MarathonSpec with Matchers { app = correct.copy(cmd = None, args = Some(Seq("a", "b", "c"))) shouldNotViolate( app, - "", + "value", "AppDefinition must either contain one of 'cmd' or 'args', and/or a 'container'." ) validateJsonSchema(app) @@ -148,7 +155,7 @@ class V2AppDefinitionTest extends MarathonSpec with Matchers { shouldViolate( app, "upgradeStrategy.minimumHealthCapacity", - "is greater than 1" + "got 1.2, expected between 0.0 and 1.0" ) validateJsonSchema(app, false) @@ -156,7 +163,7 @@ class V2AppDefinitionTest extends MarathonSpec with Matchers { shouldViolate( app, "upgradeStrategy.maximumOverCapacity", - "is greater than 1" + "got 1.2, expected between 0.0 and 1.0" ) validateJsonSchema(app, false) @@ -164,7 +171,7 @@ class V2AppDefinitionTest extends MarathonSpec with Matchers { shouldViolate( app, "upgradeStrategy.minimumHealthCapacity", - "is less than 0" + "got -1.2, expected between 0.0 and 1.0" ) validateJsonSchema(app, false) @@ -172,7 +179,7 @@ class V2AppDefinitionTest extends MarathonSpec with Matchers { shouldViolate( app, "upgradeStrategy.maximumOverCapacity", - "is less than 0" + "got -1.2, expected between 0.0 and 1.0" ) validateJsonSchema(app, false) @@ -218,7 +225,7 @@ class V2AppDefinitionTest extends MarathonSpec with Matchers { ) shouldViolate( app, - "", + "value", "Health check port indices must address an element of the ports array or container port mappings." ) validateJsonSchema(app) diff --git a/src/test/scala/mesosphere/marathon/api/v2/json/V2AppUpdateTest.scala b/src/test/scala/mesosphere/marathon/api/v2/json/V2AppUpdateTest.scala index f6a974cd57f..a873a15741e 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/json/V2AppUpdateTest.scala +++ b/src/test/scala/mesosphere/marathon/api/v2/json/V2AppUpdateTest.scala @@ -1,9 +1,8 @@ package mesosphere.marathon.api.v2.json -import javax.validation.Validation - import mesosphere.marathon.MarathonSpec import mesosphere.marathon.api.JsonTestHelper +import mesosphere.marathon.api.v2.ValidationHelper import mesosphere.marathon.health.HealthCheck import mesosphere.marathon.state.Container._ import mesosphere.marathon.state.DiscoveryInfo.Port @@ -12,27 +11,28 @@ import mesosphere.marathon.state._ import org.apache.mesos.{ Protos => mesos } import play.api.libs.json.Json -import scala.collection.JavaConverters._ import scala.collection.immutable.Seq import scala.concurrent.duration._ +import mesosphere.marathon.api.v2.Validation._ + class V2AppUpdateTest extends MarathonSpec { import Formats._ import mesosphere.marathon.integration.setup.V2TestFormats._ test("Validation") { - val validator = Validation.buildDefaultValidatorFactory().getValidator - - def shouldViolate(update: V2AppUpdate, path: String, template: String) = { - val violations = validator.validate(update).asScala - assert(violations.exists(v => - v.getPropertyPath.toString == path && v.getMessageTemplate == template)) + def shouldViolate(update: V2AppUpdate, path: String, template: String): Unit = { + val violations = validate(update) + assert(violations.isFailure) + assert(ValidationHelper.getAllRuleConstrains(violations).exists(v => + v.property.getOrElse(false) == path && v.message == template + )) } - def shouldNotViolate(update: V2AppUpdate, path: String, template: String) = { - val violations = validator.validate(update).asScala - assert(!violations.exists(v => - v.getPropertyPath.toString == path && v.getMessageTemplate == template)) + def shouldNotViolate(update: V2AppUpdate, path: String, template: String): Unit = { + val violations = validate(update) + assert(!ValidationHelper.getAllRuleConstrains(violations).exists(v => + v.property.getOrElse(false) == path && v.message == template)) } val update = V2AppUpdate() @@ -42,6 +42,12 @@ class V2AppUpdateTest extends MarathonSpec { "ports", "Elements must be unique" ) + + shouldNotViolate( + update.copy(ports = Some(Seq(AppDefinition.RandomPortValue, 8080, AppDefinition.RandomPortValue))), + "ports", + "Elements must be unique" + ) } private[this] def fromJsonString(json: String): V2AppUpdate = { diff --git a/src/test/scala/mesosphere/marathon/api/v2/json/V2GroupUpdateTest.scala b/src/test/scala/mesosphere/marathon/api/v2/json/V2GroupUpdateTest.scala index defc0b735b0..15da5b4faa2 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/json/V2GroupUpdateTest.scala +++ b/src/test/scala/mesosphere/marathon/api/v2/json/V2GroupUpdateTest.scala @@ -1,6 +1,7 @@ package mesosphere.marathon.api.v2.json import mesosphere.marathon.state.{ AppDefinition, Timestamp, PathId, Group } +import mesosphere.marathon.api.v2.Validation._ import org.scalatest.{ GivenWhenThen, Matchers, FunSuite } import PathId._ @@ -15,7 +16,8 @@ class V2GroupUpdateTest extends FunSuite with Matchers with GivenWhenThen { )), V2GroupUpdate( "apps".toPath, - Set(V2AppDefinition("app1".toPath, dependencies = Set("d1".toPath, "../test/foo".toPath, "/test".toPath))) + Set(V2AppDefinition("app1".toPath, Some("foo"), + dependencies = Set("d1".toPath, "../test/foo".toPath, "/test".toPath))) ) ) ) @@ -24,6 +26,8 @@ class V2GroupUpdateTest extends FunSuite with Matchers with GivenWhenThen { When("The update is performed") val result = update(group, timestamp).toGroup() + validate(V2Group(result)).isSuccess should be(true) + Then("The update is applied correctly") result.id should be(PathId.empty) result.groups should have size 2 @@ -41,7 +45,7 @@ class V2GroupUpdateTest extends FunSuite with Matchers with GivenWhenThen { test("A group update can be applied to existing entries") { Given("A group with updates of existing nodes") val actual = V2Group(PathId.empty, groups = Set( - V2Group("/test".toPath, apps = Set(V2AppDefinition("/test/bla".toPath))), + V2Group("/test".toPath, apps = Set(V2AppDefinition("/test/bla".toPath, Some("foo")))), V2Group("/apps".toPath, groups = Set(V2Group("/apps/foo".toPath))) )) val update = V2GroupUpdate( @@ -55,7 +59,8 @@ class V2GroupUpdateTest extends FunSuite with Matchers with GivenWhenThen { ), V2GroupUpdate( "apps".toPath, - Set(V2AppDefinition("app1".toPath, dependencies = Set("d1".toPath, "../test/foo".toPath, "/test".toPath))) + Set(V2AppDefinition("app1".toPath, Some("foo"), + dependencies = Set("d1".toPath, "../test/foo".toPath, "/test".toPath))) ) ) ) @@ -64,6 +69,8 @@ class V2GroupUpdateTest extends FunSuite with Matchers with GivenWhenThen { When("The update is performed") val result: Group = update(actual, timestamp).toGroup() + validate(V2Group(result)).isSuccess should be(true) + Then("The update is applied correctly") result.id should be(PathId.empty) result.groups should have size 2 @@ -85,8 +92,8 @@ class V2GroupUpdateTest extends FunSuite with Matchers with GivenWhenThen { val current = V2Group( "/test".toPath, groups = Set( - V2Group("/test/group1".toPath, Set(V2AppDefinition("/test/group1/app1".toPath))), - V2Group("/test/group2".toPath, Set(V2AppDefinition("/test/group2/app2".toPath))) + V2Group("/test/group1".toPath, Set(V2AppDefinition("/test/group1/app1".toPath, Some("foo")))), + V2Group("/test/group2".toPath, Set(V2AppDefinition("/test/group2/app2".toPath, Some("foo")))) ) ) @@ -95,11 +102,12 @@ class V2GroupUpdateTest extends FunSuite with Matchers with GivenWhenThen { "/test".toPath, Set.empty[V2AppDefinition], Set( - V2GroupUpdate("/test/group1".toPath, Set(V2AppDefinition("/test/group1/app3".toPath))), + V2GroupUpdate("/test/group1".toPath, Set(V2AppDefinition("/test/group1/app3".toPath, Some("foo")))), V2GroupUpdate( "/test/group3".toPath, Set.empty[V2AppDefinition], - Set(V2GroupUpdate("/test/group3/sub1".toPath, Set(V2AppDefinition("/test/group3/sub1/app4".toPath)))) + Set(V2GroupUpdate("/test/group3/sub1".toPath, Set(V2AppDefinition("/test/group3/sub1/app4".toPath, + Some("foo"))))) ) ) ) @@ -107,6 +115,8 @@ class V2GroupUpdateTest extends FunSuite with Matchers with GivenWhenThen { val timestamp = Timestamp.now() val result = update(current, timestamp).toGroup() + validate(V2Group(result)).isSuccess should be(true) + Then("The update is reflected in the current group") result.id.toString should be("/test") result.apps should be('empty) @@ -138,14 +148,16 @@ class V2GroupUpdateTest extends FunSuite with Matchers with GivenWhenThen { val update = V2GroupUpdate(PathId.empty, Set.empty[V2AppDefinition], Set( V2GroupUpdate( "test-group".toPath, - Set(V2AppDefinition("test-app1".toPath), - V2AppDefinition("test-app2".toPath, dependencies = Set("test-app1".toPath))) + Set(V2AppDefinition("test-app1".toPath, Some("foo")), + V2AppDefinition("test-app2".toPath, Some("foo"), dependencies = Set("test-app1".toPath))) ) )) When("The update is performed") val result = update(V2Group(Group.empty), Timestamp.now()).toGroup() + validate(V2Group(result)).isSuccess should be(true) + Then("The update is applied correctly") val group = result.group("test-group".toRootPath) group should be('defined) diff --git a/src/test/scala/mesosphere/marathon/api/validation/V2AppDefinitionValidatorTest.scala b/src/test/scala/mesosphere/marathon/api/validation/V2AppDefinitionValidatorTest.scala deleted file mode 100644 index e8bc823ccde..00000000000 --- a/src/test/scala/mesosphere/marathon/api/validation/V2AppDefinitionValidatorTest.scala +++ /dev/null @@ -1,234 +0,0 @@ -package mesosphere.marathon.api.validation - -import javax.validation.ConstraintValidatorContext - -import mesosphere.marathon.MarathonSpec -import mesosphere.marathon.Protos.HealthCheckDefinition -import mesosphere.marathon.api.v2.{ ModelValidation, BeanValidation } -import mesosphere.marathon.api.v2.json.V2AppDefinition -import mesosphere.marathon.health.HealthCheck -import mesosphere.marathon.state.{ Command, Container, PathId } -import org.scalatest.Matchers - -class V2AppDefinitionValidatorTest extends MarathonSpec with Matchers { - var validator: V2AppDefinitionValidator = _ - - before { - validator = new V2AppDefinitionValidator - } - - test("only cmd") { - val app = V2AppDefinition( - id = PathId("/test"), - cmd = Some("true")) - assert(validator.isValid(app, mock[ConstraintValidatorContext])) - validateJsonSchema(app) - } - - private[this] def testValidId(id: String): Unit = { - val app = V2AppDefinition( - id = PathId(id), - cmd = Some("true")) - - BeanValidation.requireValid(ModelValidation.checkAppConstraints(app, app.id.parent)) - - validateJsonSchema(app) - } - - test("id '/app' is valid") { - testValidId("/app") - } - - test("id '/hy-phenated' is valid") { - testValidId("/hy-phenated") - } - - test("id '/numbered9' is valid") { - testValidId("/numbered9") - } - - test("id '/9numbered' is valid") { - testValidId("/9numbered") - } - - test("id '/num8bered' is valid") { - testValidId("/num8bered") - } - - test("id '/dot.ted' is valid") { - testValidId("/dot.ted") - } - - test("id '/deep/ly/nes/ted' is valid") { - testValidId("/deep/ly/nes/ted") - } - - test("id '/all.to-9gether/now-huh12/nest/nest' is valid") { - testValidId("/all.to-9gether/now-huh12/nest/nest") - } - - test("id '/trailing/' is valid") { - // the trailing slash is apparently ignored by Marathon - testValidId("/trailing/") - } - - test("single dots in id '/test/.' pass schema and validation") { - testInvalid("/test/.") - } - - test("single dots in id '/./not.point.less' pass schema and validation") { - testInvalid("/./not.point.less") - } - - private[this] def testSchemaLessStrictForId(id: String): Unit = { - val app = V2AppDefinition( - id = PathId(id), - cmd = Some("true")) - - an[IllegalArgumentException] should be thrownBy { - ModelValidation.checkAppConstraints(app, app.id.parent) - } - - validateJsonSchema(app) - } - - // non-absolute paths (could be allowed in some contexts) - test(s"relative id 'relative/asd' passes schema but not validation") { - testSchemaLessStrictForId("relative/asd") - } - - // non-absolute paths (could be allowed in some contexts) - test(s"relative id '../relative' passes schema but not validation") { - testSchemaLessStrictForId("../relative") - } - - private[this] def testInvalid(id: String): Unit = { - val app = V2AppDefinition( - id = PathId(id), - cmd = Some("true") - ) - - ModelValidation.checkAppConstraints(app, app.id.parent) should not be empty - - validateJsonSchema(app, valid = false) - } - - test("id '/.../asd' is INVALID") { - testInvalid("/.../asd") - } - - test("id '/app!' is INVALID") { - testInvalid("/app!' i") - } - - test("id '/app[' is INVALID") { - testInvalid("/app[' i") - } - - test("id '/asd/sadf+' is INVALID") { - testInvalid("/asd/sadf+") - } - - test("id '/asd asd' is INVALID") { - testInvalid("/asd asd") - } - - test("id '/app-' is invalid because hyphens and dots are only allowed inside of path fragments") { - testInvalid("/app-") - } - - test("id '/nest./ted' is invalid because hyphens and dots are only allowed inside of path fragments") { - testInvalid("/nest./ted") - } - - test("id '/nest/-ted' is invalid because hyphens and dots are only allowed inside of path fragments") { - testInvalid("/nest/-ted") - } - - test("only cmd + command health check") { - val app = V2AppDefinition( - id = PathId("/test"), - cmd = Some("true"), - healthChecks = Set( - HealthCheck( - protocol = HealthCheckDefinition.Protocol.COMMAND, - command = Some(Command("curl http://localhost:$PORT")) - ) - ) - ) - assert(validator.isValid(app, mock[ConstraintValidatorContext])) - validateJsonSchema(app) - } - - test("only cmd + acceptedResourceRoles") { - val app = V2AppDefinition( - id = PathId("/test"), - cmd = Some("true"), - acceptedResourceRoles = Some(Set("*"))) - assert(validator.isValid(app, mock[ConstraintValidatorContext])) - validateJsonSchema(app) - } - - test("only cmd + acceptedResourceRoles 2") { - val app = V2AppDefinition( - id = PathId("/test"), - cmd = Some("true"), - acceptedResourceRoles = Some(Set("*", "production"))) - assert(validator.isValid(app, mock[ConstraintValidatorContext])) - validateJsonSchema(app) - } - - test("only args") { - val app = V2AppDefinition( - id = PathId("/test"), - args = Some("test" :: Nil)) - assert(validator.isValid(app, mock[ConstraintValidatorContext])) - validateJsonSchema(app) - } - - test("only container") { - val app = V2AppDefinition( - id = PathId("/test"), - container = Some(Container( - docker = Some(Container.Docker(image = "test/image")) - ))) - assert(validator.isValid(app, mock[ConstraintValidatorContext])) - validateJsonSchema(app) - } - - test("empty container is invalid") { - val app = V2AppDefinition( - id = PathId("/test"), - container = Some(Container())) - assert(!validator.isValid(app, mock[ConstraintValidatorContext])) - validateJsonSchema(app) - } - - test("container and cmd") { - val app = V2AppDefinition( - id = PathId("/test"), - cmd = Some("true"), - container = Some(Container())) - assert(validator.isValid(app, mock[ConstraintValidatorContext])) - validateJsonSchema(app) - } - - test("container and args") { - val app = V2AppDefinition( - id = PathId("/test"), - args = Some("test" :: Nil), - container = Some(Container())) - assert(validator.isValid(app, mock[ConstraintValidatorContext])) - validateJsonSchema(app) - } - - test("container, cmd and args is not valid") { - val app = V2AppDefinition( - id = PathId("/test"), - cmd = Some("true"), - args = Some("test" :: Nil), - container = Some(Container())) - assert(!validator.isValid(app, mock[ConstraintValidatorContext])) - validateJsonSchema(app, false) - } -} diff --git a/src/test/scala/mesosphere/marathon/health/HealthCheckTest.scala b/src/test/scala/mesosphere/marathon/health/HealthCheckTest.scala index d780720b621..4980eb3ba49 100644 --- a/src/test/scala/mesosphere/marathon/health/HealthCheckTest.scala +++ b/src/test/scala/mesosphere/marathon/health/HealthCheckTest.scala @@ -1,15 +1,15 @@ package mesosphere.marathon.health -import javax.validation.Validation - import mesosphere.marathon.Protos.HealthCheckDefinition.Protocol +import mesosphere.marathon.api.v2.ValidationHelper import mesosphere.marathon.state.Command import mesosphere.marathon.{ MarathonSpec, Protos } import play.api.libs.json.Json -import scala.collection.JavaConverters._ import scala.concurrent.duration._ +import mesosphere.marathon.api.v2.Validation._ + class HealthCheckTest extends MarathonSpec { test("ToProto") { @@ -263,16 +263,13 @@ class HealthCheckTest extends MarathonSpec { assert(readResult == expected) } - val validator = Validation.buildDefaultValidatorFactory().getValidator - - def shouldBeInvalid(hc: HealthCheck) = { - val violations = validator.validate(hc).asScala - assert(violations.nonEmpty) + def shouldBeInvalid(hc: HealthCheck): Unit = { + assert(validate(hc).isFailure) } - def shouldBeValid(hc: HealthCheck) = { - val violations = validator.validate(hc).asScala - assert(violations.isEmpty, s"violations: $violations") + def shouldBeValid(hc: HealthCheck): Unit = { + val result = validate(hc) + assert(result.isSuccess, s"violations: ${ValidationHelper.getAllRuleConstrains(result)}") } test("A default HealthCheck should be valid") { diff --git a/src/test/scala/mesosphere/marathon/health/MarathonHealthCheckManagerTest.scala b/src/test/scala/mesosphere/marathon/health/MarathonHealthCheckManagerTest.scala index 3b1e18da138..2a5012b2bc0 100644 --- a/src/test/scala/mesosphere/marathon/health/MarathonHealthCheckManagerTest.scala +++ b/src/test/scala/mesosphere/marathon/health/MarathonHealthCheckManagerTest.scala @@ -314,4 +314,4 @@ class MarathonHealthCheckManagerTest extends MarathonSpec with ScalaFutures with } def captureEvents = new CaptureEvents(eventStream) -} +} \ No newline at end of file diff --git a/src/test/scala/mesosphere/marathon/integration/GroupDeployIntegrationTest.scala b/src/test/scala/mesosphere/marathon/integration/GroupDeployIntegrationTest.scala index eff2ff002cd..96d42fd2c32 100644 --- a/src/test/scala/mesosphere/marathon/integration/GroupDeployIntegrationTest.scala +++ b/src/test/scala/mesosphere/marathon/integration/GroupDeployIntegrationTest.scala @@ -257,7 +257,7 @@ class GroupDeployIntegrationTest val result = marathon.createGroup(group) Then("An unsuccessfull response has been posted, with an error indicating cyclic dependencies") - (result.entityJson \\ "error").head.as[String] should include("cyclic dependencies") + (result.entityJson \\ "error").filter(j => j.as[String].contains("cyclic dependencies")) should have size 1 } test("Applications with dependencies get deployed in the correct order") { diff --git a/src/test/scala/mesosphere/marathon/state/EntityStoreCacheTest.scala b/src/test/scala/mesosphere/marathon/state/EntityStoreCacheTest.scala index e0209739488..09fbc3fa48b 100644 --- a/src/test/scala/mesosphere/marathon/state/EntityStoreCacheTest.scala +++ b/src/test/scala/mesosphere/marathon/state/EntityStoreCacheTest.scala @@ -254,4 +254,4 @@ class EntityStoreCacheTest extends MarathonSpec with GivenWhenThen with Matchers Future.successful(true) } } -} +} \ No newline at end of file diff --git a/src/test/scala/mesosphere/marathon/state/GroupManagerTest.scala b/src/test/scala/mesosphere/marathon/state/GroupManagerTest.scala index 868ad2a392c..f9affe9ff5a 100644 --- a/src/test/scala/mesosphere/marathon/state/GroupManagerTest.scala +++ b/src/test/scala/mesosphere/marathon/state/GroupManagerTest.scala @@ -1,7 +1,6 @@ package mesosphere.marathon.state import java.util.concurrent.atomic.AtomicInteger -import javax.validation.ConstraintViolationException import akka.actor.ActorSystem import akka.event.EventStream @@ -10,7 +9,7 @@ import mesosphere.marathon.io.storage.StorageProvider import mesosphere.marathon.state.PathId._ import mesosphere.marathon.test.MarathonActorSupport import mesosphere.marathon.tasks.TaskTrackerImpl -import mesosphere.marathon.{ MarathonConf, MarathonSchedulerService, MarathonSpec, PortRangeExhaustedException } +import mesosphere.marathon._ import mesosphere.util.SerializeExecution import org.mockito.Matchers.any import org.mockito.Mockito.{ times, verify, when } @@ -163,7 +162,7 @@ class GroupManagerTest extends MarathonActorSupport with MockitoSugar with Match when(f.groupRepo.zkRootName).thenReturn(GroupRepository.zkRootName) when(f.groupRepo.group(GroupRepository.zkRootName)).thenReturn(Future.successful(None)) - intercept[ConstraintViolationException] { + intercept[ValidationFailedException] { Await.result(f.manager.update(group.id, _ => group), 3.seconds) }.printStackTrace() diff --git a/src/test/scala/mesosphere/marathon/state/GroupTest.scala b/src/test/scala/mesosphere/marathon/state/GroupTest.scala index 487f3ea1e79..69551cfa4fc 100644 --- a/src/test/scala/mesosphere/marathon/state/GroupTest.scala +++ b/src/test/scala/mesosphere/marathon/state/GroupTest.scala @@ -1,8 +1,7 @@ package mesosphere.marathon.state -import javax.validation.ConstraintViolation - -import mesosphere.marathon.api.v2.ModelValidation +import mesosphere.marathon.api.v2.Validation._ +import mesosphere.marathon.api.v2.ValidationHelper import mesosphere.marathon.api.v2.json.V2Group import mesosphere.marathon.state.AppDefinition.VersionInfo import mesosphere.marathon.state.PathId._ @@ -11,6 +10,8 @@ import org.scalatest.{ FunSpec, GivenWhenThen, Matchers } import scala.collection.JavaConverters._ import scala.collection.immutable.Seq +// import com.wix.accord.scalatest.ResultMatchers + class GroupTest extends FunSpec with GivenWhenThen with Matchers { describe("A Group") { @@ -185,7 +186,7 @@ class GroupTest extends FunSpec with GivenWhenThen with Matchers { changed.transitiveApps.map(_.id.toString) should be(Set("/some/nested")) Then("the resulting group should be valid when represented in the V2 API model") - ModelValidation.checkGroup(V2Group(changed)) should be('empty) + validate(V2Group(changed)).isSuccess should be (true) } it("cannot replace a group with apps by an app definition") { @@ -215,9 +216,10 @@ class GroupTest extends FunSpec with GivenWhenThen with Matchers { changed.transitiveApps.map(_.id.toString) should be(Set("/some/nested", "/some/nested/path2/app")) Then("the conflict will be detected by our V2 API model validation") - val constraintViolations: Iterable[ConstraintViolation[V2Group]] = ModelValidation.checkGroup(V2Group(changed)) - constraintViolations should be('nonEmpty) - constraintViolations.map(_.getMessage) should be(Set("Groups and Applications may not have the same identifier: /some/nested")) + val result = validate(V2Group(changed)) + result.isFailure should be(true) + ValidationHelper.getAllRuleConstrains(result).head + .message should be ("Groups and Applications may not have the same identifier: /some/nested") } it("can marshal and unmarshal from to protos") { diff --git a/src/test/scala/mesosphere/marathon/tasks/TaskTrackerImplTest.scala b/src/test/scala/mesosphere/marathon/tasks/TaskTrackerImplTest.scala index 076071a2ca5..86519576f42 100644 --- a/src/test/scala/mesosphere/marathon/tasks/TaskTrackerImplTest.scala +++ b/src/test/scala/mesosphere/marathon/tasks/TaskTrackerImplTest.scala @@ -429,4 +429,4 @@ class TaskTrackerImplTest extends MarathonActorSupport with MarathonSpec with Ma val keyWithPrefix = TaskRepository.storePrefix + key assert(state.allIds().futureValue.toSet.contains(keyWithPrefix), s"Key $keyWithPrefix was not found in state") } -} +} \ No newline at end of file