csantanapr closed pull request #3333: Make parameters with defined values final URL: https://github.com/apache/incubator-openwhisk/pull/3333
This is a PR merged from a forked repository. As GitHub hides the original diff on merge, it is displayed below for the sake of provenance: As this is a foreign pull request (from a fork), the diff is supplied below (as it won't show otherwise due to GitHub magic): diff --git a/common/scala/src/main/scala/whisk/core/entity/Parameter.scala b/common/scala/src/main/scala/whisk/core/entity/Parameter.scala index 0dd6518584..18ddb6ea7d 100644 --- a/common/scala/src/main/scala/whisk/core/entity/Parameter.scala +++ b/common/scala/src/main/scala/whisk/core/entity/Parameter.scala @@ -41,10 +41,7 @@ protected[core] class Parameters protected[entity] (private val params: Map[Para */ def size = { params - .map { - case (name, value) => - name.size + value.size - } + .map { case (name, value) => name.size + value.size } .foldLeft(0 B)(_ + _) } @@ -77,14 +74,14 @@ protected[core] class Parameters protected[entity] (private val params: Map[Para params.keySet filter (params(_).isDefined) map (_.name) } - protected[core] def toJsArray = + protected[core] def toJsArray = { JsArray(params map { p => JsObject("key" -> p._1.name.toJson, "value" -> p._2.value.toJson) } toSeq: _*) - protected[core] def toJsObject = - JsObject(params map { p => - (p._1.name -> p._2.value.toJson) - }) + } + + protected[core] def toJsObject = JsObject(params.map(p => (p._1.name -> p._2.value.toJson))) + override def toString = toJsArray.compactPrint /** diff --git a/core/controller/src/main/scala/whisk/core/controller/Actions.scala b/core/controller/src/main/scala/whisk/core/controller/Actions.scala index 1e26a6684c..1fb47c02ff 100644 --- a/core/controller/src/main/scala/whisk/core/controller/Actions.scala +++ b/core/controller/src/main/scala/whisk/core/controller/Actions.scala @@ -226,38 +226,17 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with onComplete(entitleReferencedEntitiesMetaData(user, Privilege.ACTIVATE, Some(action.exec))) { case Success(_) => val actionWithMergedParams = env.map(action.inherit(_)) getOrElse action - val waitForResponse = if (blocking) Some(waitOverride) else None - onComplete(invokeAction(user, actionWithMergedParams, payload, waitForResponse, cause = None)) { - case Success(Left(activationId)) => - // non-blocking invoke or blocking invoke which got queued instead - complete(Accepted, activationId.toJsObject) - case Success(Right(activation)) => - val response = if (result) activation.resultAsJson else activation.toExtendedJson - - if (activation.response.isSuccess) { - complete(OK, response) - } else if (activation.response.isApplicationError) { - // actions that result is ApplicationError status are considered a 'success' - // and will have an 'error' property in the result - the HTTP status is OK - // and clients must check the response status if it exists - // NOTE: response status will not exist in the JSON object if ?result == true - // and instead clients must check if 'error' is in the JSON - // PRESERVING OLD BEHAVIOR and will address defect in separate change - complete(BadGateway, response) - } else if (activation.response.isContainerError) { - complete(BadGateway, response) - } else { - complete(InternalServerError, response) - } - case Failure(t: RecordTooLargeException) => - logging.debug(this, s"[POST] action payload was too large") - terminate(RequestEntityTooLarge) - case Failure(RejectRequest(code, message)) => - logging.debug(this, s"[POST] action rejected with code $code: $message") - terminate(code, message) - case Failure(t: Throwable) => - logging.error(this, s"[POST] action activation failed: ${t.getMessage}") - terminate(InternalServerError) + + // incoming parameters may not override final parameters (i.e., parameters with already defined values) + // on an action once its parameters are resolved across package and binding + val allowInvoke = payload + .map(_.fields.keySet.forall(key => !actionWithMergedParams.immutableParameters.contains(key))) + .getOrElse(true) + + if (allowInvoke) { + doInvoke(user, actionWithMergedParams, payload, blocking, waitOverride, result) + } else { + terminate(BadRequest, Messages.parametersNotAllowed) } case Failure(f) => @@ -268,6 +247,47 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with } } + private def doInvoke(user: Identity, + actionWithMergedParams: WhiskActionMetaData, + payload: Option[JsObject], + blocking: Boolean, + waitOverride: FiniteDuration, + result: Boolean)(implicit transid: TransactionId): RequestContext => Future[RouteResult] = { + val waitForResponse = if (blocking) Some(waitOverride) else None + onComplete(invokeAction(user, actionWithMergedParams, payload, waitForResponse, cause = None)) { + case Success(Left(activationId)) => + // non-blocking invoke or blocking invoke which got queued instead + complete(Accepted, activationId.toJsObject) + case Success(Right(activation)) => + val response = if (result) activation.resultAsJson else activation.toExtendedJson + + if (activation.response.isSuccess) { + complete(OK, response) + } else if (activation.response.isApplicationError) { + // actions that result is ApplicationError status are considered a 'success' + // and will have an 'error' property in the result - the HTTP status is OK + // and clients must check the response status if it exists + // NOTE: response status will not exist in the JSON object if ?result == true + // and instead clients must check if 'error' is in the JSON + // PRESERVING OLD BEHAVIOR and will address defect in separate change + complete(BadGateway, response) + } else if (activation.response.isContainerError) { + complete(BadGateway, response) + } else { + complete(InternalServerError, response) + } + case Failure(t: RecordTooLargeException) => + logging.debug(this, s"[POST] action payload was too large") + terminate(RequestEntityTooLarge) + case Failure(RejectRequest(code, message)) => + logging.debug(this, s"[POST] action rejected with code $code: $message") + terminate(code, message) + case Failure(t: Throwable) => + logging.error(this, s"[POST] action activation failed: ${t.getMessage}") + terminate(InternalServerError) + } + } + /** * Deletes action. * diff --git a/core/controller/src/main/scala/whisk/core/controller/WebActions.scala b/core/controller/src/main/scala/whisk/core/controller/WebActions.scala index cd3c68ca37..a692530c89 100644 --- a/core/controller/src/main/scala/whisk/core/controller/WebActions.scala +++ b/core/controller/src/main/scala/whisk/core/controller/WebActions.scala @@ -91,7 +91,7 @@ private case class Context(propertyMap: WebApiDirectives, // returns true iff the attached query and body parameters contain a property // that conflicts with the given reserved parameters - def overrides(reservedParams: Set[String]): Set[String] = { + def overrides(reservedParams: Set[String]): Boolean = { val queryParams = queryAsMap.keySet val bodyParams = body .map { @@ -100,7 +100,7 @@ private case class Context(propertyMap: WebApiDirectives, } .getOrElse(Set.empty) - (queryParams ++ bodyParams) intersect reservedParams + (queryParams ++ bodyParams).forall(key => !reservedParams.contains(key)) } // attach the body to the Context @@ -612,8 +612,7 @@ trait WhiskWebActionsApi extends Directives with ValidateRequestSize with PostAc // since these are system properties, the action should not define them, and if it does, // they will be overwritten if (isRawHttpAction || context - .overrides(webApiDirectives.reservedProperties ++ action.immutableParameters) - .isEmpty) { + .overrides(webApiDirectives.reservedProperties ++ action.immutableParameters)) { val content = context.toActionArgument(onBehalfOf, isRawHttpAction) invokeAction(actionOwnerIdentity, action, Some(JsObject(content)), maxWaitForWebActionResult, cause = None) } else { diff --git a/tests/src/test/scala/whisk/core/controller/test/ActionsApiTests.scala b/tests/src/test/scala/whisk/core/controller/test/ActionsApiTests.scala index fb001847ea..380c5f0b76 100644 --- a/tests/src/test/scala/whisk/core/controller/test/ActionsApiTests.scala +++ b/tests/src/test/scala/whisk/core/controller/test/ActionsApiTests.scala @@ -832,6 +832,23 @@ class ActionsApiTests extends ControllerTestCommon with WhiskActionsApi { } } + it should "not invoke an action when final parameters are redefined" in { + implicit val tid = transid() + val annotations = Parameters(WhiskActionMetaData.finalParamsAnnotationName, JsBoolean(true)) + val parameters = Parameters("a", "A") ++ Parameters("empty", JsNull) + val action = WhiskAction(namespace, aname(), jsDefault("??"), parameters = parameters, annotations = annotations) + put(entityStore, action) + Seq((Parameters("a", "B"), BadRequest), (Parameters("empty", "C"), Accepted)).foreach { + case (p, code) => + Post(s"$collectionPath/${action.name}", p.toJsObject) ~> Route.seal(routes(creds)) ~> check { + status should be(code) + if (code == BadRequest) { + responseAs[ErrorResponse].error shouldBe Messages.parametersNotAllowed + } + } + } + } + it should "invoke an action, blocking with default timeout" in { implicit val tid = transid() val action = WhiskAction( ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services