PUT on /v2/apps has a PATCH semantic. (d2iq-archive#5157)

* document that, by default, PUT on /v2/apps has a PATCH semantic: only the fields specified in the app update are applied to the existing app definition.
* add a query parameter, `partialUpdate`, that allows for proper PUT semantics for an app. `false` means "completely replace the existing app with the new one that I'm **fully specifying** here"

changelog.md

@@ -151,6 +151,14 @@ The artifact store was introduced as an easy solution to store and retrieve arti
 There is a variety of tools that can handle this functionality better then Marathon.
 We will remove this functionality from Marathon without replacement.
 
+#### Deprecate PATCH semantic for PUT on /v2/apps
+
+A PUT on /v2/apps has a PATCH like semantic:
+All values that are not defined in the json, will not update existing values.
+This was always the default behaviour in Marathon versions.
+For backward compatibility, we will not change this behaviour, but let users opt in for a proper PUT.
+The next version of Marathon will use PATCH and PUT as two separate actions.
+
 #### Forcefully stop a deployment
 
 Deployments in Marathon can be stopped with force.

docs/docs/rest-api/public/api/v2/apps.raml

@@ -90,6 +90,16 @@
       Note:  This operation will create a deployment. The operation finishes, if the deployment succeeds.
        You can query the deployments endoint to see the status of the deployment.
     is: [ secured, deployable ]
+    queryParameters:
+      partialUpdate:
+        required: false
+        default: true
+        description: |
+          Without specifying this parameter, this method has a patch like semantic:
+          All values that are not defined in the json, will not change existing values.
+          This was the default behaviour in previous Marathon versions.
+          For backward compatibility, we will not change this behaviour, but let users opt in for a proper PUT.
+          Note: We will change the default behaviour in the next Marathon version to support PATCH and PUT as HTTP methods.
     body:
       application/json:
         example: !include examples/apps_create.json
@@ -127,9 +137,6 @@
                   ]
                 }
 
-
-
-
   ###
   # Create application
   #
@@ -182,7 +189,6 @@
                 ]
                 }
 
-
   /{app_id}:
 
     ###
@@ -251,6 +257,16 @@
         Note:  This operation will create a deployment. The operation finishes, if the deployment succeeds.
          You can query the deployments endoint to see the status of the deployment.
       is: [ secured, deployable ]
+      queryParameters:
+        partialUpdate:
+          required: false
+          default: true
+          description: |
+            Without specifying this parameter, this method has a patch like semantic:
+            All values that are not defined in the json, will not change existing values.
+            This was the default behaviour in previous Marathon versions.
+            For backward compatibility, we will not change this behaviour, but let users opt in for a proper PUT.
+            Note: We will change the default behaviour in the next Marathon version to support PATCH and PUT as HTTP methods.
       body:
         application/json:
           example: !include examples/app.json
@@ -293,8 +309,6 @@
                   ]
                 }
 
-
-
     ###
     # Delete a specific app
     #

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

@@ -3,13 +3,14 @@ package mesosphere.marathon.api
 import java.net.URI
 import javax.ws.rs.core.Response
 import javax.ws.rs.core.Response.{ ResponseBuilder, Status }
+
+import akka.http.scaladsl.model.StatusCodes
 import com.wix.accord._
-import mesosphere.marathon.MarathonConf
+import mesosphere.marathon.{ MarathonConf, ValidationFailedException }
 import mesosphere.marathon.api.v2.Validation._
 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.{ Json, Writes }
 
@@ -74,6 +75,19 @@ trait RestResource {
       case Success => fn(t)
     }
   }
+
+  /**
+    * Execute the given function and if any validation errors crop up, generate an UnprocessableEntity
+    * HTTP status code and send the validation error as the response body (in JSON form).
+    */
+  protected def assumeValid(f: => Response): Response =
+    try {
+      f
+    } catch {
+      case vfe: ValidationFailedException =>
+        val entity = Json.toJson(vfe.failure).toString
+        Response.status(StatusCodes.UnprocessableEntity.intValue).entity(entity).build()
+    }
 }
 
 object RestResource {

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

@@ -22,7 +22,7 @@ import mesosphere.marathon.plugin.auth._
 import mesosphere.marathon.state.PathId._
 import mesosphere.marathon.state._
 import mesosphere.marathon.stream._
-import play.api.libs.json.Json
+import play.api.libs.json.{ JsObject, Json }
 
 @Path("v2/apps")
 @Consumes(Array(MediaType.APPLICATION_JSON))
@@ -61,24 +61,26 @@ class AppsResource @Inject() (
     Response.ok(jsonObjString("apps" -> mapped)).build()
   }
 
+  def normalizedApp(appDef: AppDefinition, now: Timestamp): AppDefinition = {
+    appDef.copy(
+      ipAddress = appDef.ipAddress.map { ipAddress =>
+        config.defaultNetworkName.get.collect {
+          case (defaultName: String) if defaultName.nonEmpty && ipAddress.networkName.isEmpty =>
+            ipAddress.copy(networkName = Some(defaultName))
+        }.getOrElse(ipAddress)
+      },
+      versionInfo = VersionInfo.OnlyVersion(now)
+    )
+  }
+
   @POST
   @Timed
   def create(
     body: Array[Byte],
     @DefaultValue("false")@QueryParam("force") force: Boolean,
     @Context req: HttpServletRequest): Response = authenticated(req) { implicit identity =>
     withValid(Json.parse(body).as[AppDefinition].withCanonizedIds()) { appDef =>
-      val now = clock.now()
-      val app = appDef.copy(
-        ipAddress = appDef.ipAddress.map { ipAddress =>
-          config.defaultNetworkName.get.collect {
-            case (defaultName: String) if defaultName.nonEmpty && ipAddress.networkName.isEmpty =>
-              ipAddress.copy(networkName = Some(defaultName))
-          }.getOrElse(ipAddress)
-        },
-        versionInfo = VersionInfo.OnlyVersion(now)
-      )
-
+      val app = normalizedApp(appDef, clock.now())
       checkAuthorization(CreateRunSpec, app)
 
       def createOrThrow(opt: Option[AppDefinition]) = opt
@@ -141,20 +143,66 @@ class AppsResource @Inject() (
     }
   }
 
+  /**
+    * Validate and normalize a single application update submitted via the REST API. Validation exceptions are not
+    * handled here, that's left as an exercise for the caller.
+    *
+    * @param appId used as the id of the generated app update (vs. whatever might be in the JSON body)
+    * @param body is the raw, unparsed JSON
+    * @param partialUpdate true if the JSON should be parsed as a partial application update (all fields optional)
+    *                      or as a wholesale replacement (parsed like an app definition would be)
+    */
+  def canonicalAppUpdateFromJson(appId: PathId, body: Array[Byte], partialUpdate: Boolean): AppUpdate = {
+    if (partialUpdate) {
+      validateOrThrow(Json.parse(body).as[AppUpdate].copy(id = Some(appId)).withCanonizedIds())
+    } else {
+      // this is a complete replacement of the app as we know it, so parse and normalize as if we're dealing
+      // with a brand new app because the rules are different (for example, many fields are non-optional with brand-new apps).
+      // however since this is an update, the user isn't required to specify an ID as part of the definition so we do
+      // some hackery here to pass initial JSON parsing.
+      val jsObj = Json.parse(body).as[JsObject] + ("id" -> Json.toJson(appId.toString))
+      // the version is thrown away in toUpdate so just pass `zero` for now
+      normalizedApp(validateOrThrow(jsObj.as[AppDefinition].withCanonizedIds()), Timestamp.zero).toUpdate
+    }
+  }
+
+  /**
+    * Validate and normalize an array of application updates submitted via the REST API. Validation exceptions are not
+    * handled here, that's left as an exercise for the caller.
+    *
+    * @param body is the raw, unparsed JSON
+    * @param partialUpdate true if the JSON should be parsed as a partial application update (all fields optional)
+    *                      or as a wholesale replacement (parsed like an app definition would be)
+    */
+  def canonicalAppUpdatesFromJson(body: Array[Byte], partialUpdate: Boolean): Seq[AppUpdate] = {
+    if (partialUpdate) {
+      validateOrThrow(Json.parse(body).as[Seq[AppUpdate]].map(_.withCanonizedIds()))
+    } else {
+      // this is a complete replacement of the app as we know it, so parse and normalize as if we're dealing
+      // with a brand new app because the rules are different (for example, many fields are non-optional with brand-new apps).
+      // the version is thrown away in toUpdate so just pass `zero` for now.
+      validateOrThrow(Json.parse(body).as[Seq[AppDefinition]].map(_.withCanonizedIds())).map { app =>
+        normalizedApp(app, Timestamp.zero).toUpdate
+      }
+    }
+  }
+
   @PUT
   @Path("""{id:.+}""")
   @Timed
   def replace(
     @PathParam("id") id: String,
     body: Array[Byte],
     @DefaultValue("false")@QueryParam("force") force: Boolean,
+    @DefaultValue("true")@QueryParam("partialUpdate") partialUpdate: Boolean,
     @Context req: HttpServletRequest): Response = authenticated(req) { implicit identity =>
-    val appId = id.toRootPath
-    val now = clock.now()
 
-    withValid(Json.parse(body).as[AppUpdate].copy(id = Some(appId))) { appUpdate =>
-      val plan = result(groupManager.updateApp(appId, updateOrCreate(appId, _, appUpdate), now, force))
+    val appId = id.toRootPath
 
+    assumeValid {
+      val appUpdate = canonicalAppUpdateFromJson(appId, body, partialUpdate)
+      val version = clock.now()
+      val plan = result(groupManager.updateApp(appId, updateOrCreate(appId, _, appUpdate, partialUpdate), version, force))
       val response = plan.original.app(appId)
         .map(_ => Response.ok())
         .getOrElse(Response.created(new URI(appId.toString)))
@@ -169,14 +217,17 @@ class AppsResource @Inject() (
   @Timed
   def replaceMultiple(
     @DefaultValue("false")@QueryParam("force") force: Boolean,
+    @DefaultValue("true")@QueryParam("partialUpdate") partialUpdate: Boolean,
     body: Array[Byte],
     @Context req: HttpServletRequest): Response = authenticated(req) { implicit identity =>
-    withValid(Json.parse(body).as[Seq[AppUpdate]].map(_.withCanonizedIds())) { updates =>
+
+    assumeValid {
       val version = clock.now()
+      val updates = canonicalAppUpdatesFromJson(body, partialUpdate)
 
       def updateGroup(rootGroup: RootGroup): RootGroup = updates.foldLeft(rootGroup) { (group, update) =>
         update.id match {
-          case Some(id) => group.updateApp(id, updateOrCreate(id, _, update), version)
+          case Some(id) => group.updateApp(id, updateOrCreate(id, _, update, partialUpdate), version)
           case None => group
         }
       }
@@ -232,18 +283,20 @@ class AppsResource @Inject() (
     deploymentResult(restartDeployment)
   }
 
-  private def updateOrCreate(
+  private[v2] def updateOrCreate(
     appId: PathId,
     existing: Option[AppDefinition],
-    appUpdate: AppUpdate)(implicit identity: Identity): AppDefinition = {
+    appUpdate: AppUpdate,
+    partialUpdate: Boolean)(implicit identity: Identity): AppDefinition = {
     def createApp(): AppDefinition = {
       val app = validateOrThrow(appUpdate.empty(appId))
       checkAuthorization(CreateRunSpec, app)
     }
 
     def updateApp(current: AppDefinition): AppDefinition = {
-      val app = validateOrThrow(appUpdate(current))
-      checkAuthorization(UpdateRunSpec, app)
+      val updatedApp = if (partialUpdate) appUpdate(current) else appUpdate.empty(appId)
+      val validatedApp = validateOrThrow(updatedApp)
+      checkAuthorization(UpdateRunSpec, validatedApp)
     }
 
     def rollback(current: AppDefinition, version: Timestamp): AppDefinition = {

src/main/scala/mesosphere/marathon/state/AppDefinition.scala

@@ -9,6 +9,7 @@ import com.wix.accord.dsl._
 import mesosphere.marathon.Protos.Constraint
 import mesosphere.marathon.api.serialization._
 import mesosphere.marathon.api.v2.Validation._
+import mesosphere.marathon.api.v2.json.AppUpdate
 import mesosphere.marathon.core.externalvolume.ExternalVolumes
 import mesosphere.marathon.core.health._
 import mesosphere.marathon.core.plugin.PluginManager
@@ -356,6 +357,47 @@ case class AppDefinition(
     else if (ipAddress.isDefined) fromDiscoveryInfo
     else fromPortDefinitions
   }
+
+  /**
+    * @return an app update generated from this app definition, but without the `version` field (since that can only
+    *         be combined with `id` for app updates)
+    */
+  def toUpdate: AppUpdate =
+    AppUpdate(
+      id = Some(id),
+      cmd = cmd,
+      args = Some(args),
+      user = user,
+      env = Some(env),
+      instances = Some(instances),
+      cpus = Some(resources.cpus),
+      mem = Some(resources.mem),
+      disk = Some(resources.disk),
+      gpus = Some(resources.gpus),
+      executor = Some(executor),
+      constraints = Some(constraints),
+      fetch = Some(fetch),
+      storeUrls = Some(storeUrls),
+      portDefinitions = Some(portDefinitions),
+      requirePorts = Some(requirePorts),
+      backoff = Some(backoffStrategy.backoff),
+      backoffFactor = Some(backoffStrategy.factor),
+      maxLaunchDelay = Some(backoffStrategy.maxLaunchDelay),
+      container = container,
+      healthChecks = Some(healthChecks),
+      readinessChecks = Some(readinessChecks),
+      taskKillGracePeriod = taskKillGracePeriod,
+      dependencies = Some(dependencies),
+      upgradeStrategy = Some(upgradeStrategy),
+      labels = Some(labels),
+      acceptedResourceRoles = Some(acceptedResourceRoles),
+      ipAddress = ipAddress,
+      residency = residency,
+      secrets = Some(secrets),
+      unreachableStrategy = Some(unreachableStrategy),
+      killSelection = Some(killSelection)
+    // purposefully ignore version since it can only be combined with the `id` field
+    )
 }
 
 @SuppressWarnings(Array("IsInstanceOf")) // doesn't work well in the validation macros?!