You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openwhisk.apache.org by GitBox <gi...@apache.org> on 2022/07/26 05:09:30 UTC
[GitHub] [openwhisk] style95 commented on a diff in pull request #5290: Support array result for common action and sequence action
style95 commented on code in PR #5290:
URL: https://github.com/apache/openwhisk/pull/5290#discussion_r929523068
##########
common/scala/src/main/scala/org/apache/openwhisk/core/containerpool/AkkaContainerClient.scala:
##########
@@ -201,6 +201,19 @@ object AkkaContainerClient {
result
}
+ /** A helper method to post one single request to a connection. Used for container tests. */
+ def postForJsArray(host: String, port: Int, endPoint: String, content: JsValue, timeout: FiniteDuration)(
+ implicit logging: Logging,
+ as: ActorSystem,
+ ec: ExecutionContext,
+ tid: TransactionId): (Int, Option[JsArray]) = {
Review Comment:
Is there any reason that we can't just change the result type to `(Int, Option[JsValue])`?
##########
common/scala/src/main/scala/org/apache/openwhisk/core/connector/Message.scala:
##########
@@ -380,9 +380,13 @@ object Activation extends DefaultJsonProtocol {
/** Get "StatusCode" from result response set by action developer * */
def userDefinedStatusCode(result: Option[JsValue]): Option[Int] = {
- val statusCode = JsHelpers
- .getFieldPath(result.get.asJsObject, ERROR_FIELD, "statusCode")
- .orElse(JsHelpers.getFieldPath(result.get.asJsObject, "statusCode"))
+ val statusCode: Option[JsValue] = result match {
+ case Some(JsObject(fields)) =>
+ JsHelpers
+ .getFieldPath(JsObject(fields), ERROR_FIELD, "statusCode")
+ .orElse(JsHelpers.getFieldPath(JsObject(fields), "statusCode"))
+ case _ => None
Review Comment:
This is for the array result case and there could be no `statusCode` field in the array result, right?
##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerProxy.scala:
##########
@@ -1082,25 +1082,28 @@ object ContainerProxy {
* @param initArgs set of parameters to treat as initialization arguments
* @return A partition of the arguments into an environment variables map and the JsObject argument to the action
*/
- def partitionArguments(content: Option[JsObject], initArgs: Set[String]): (Map[String, JsValue], JsObject) = {
+ def partitionArguments(content: Option[JsValue], initArgs: Set[String]): (Map[String, JsValue], JsValue) = {
content match {
- case None => (Map.empty, JsObject.empty)
- case Some(js) if initArgs.isEmpty => (Map.empty, js)
- case Some(js) =>
- val (env, args) = js.fields.partition(k => initArgs.contains(k._1))
+ case None => (Map.empty, JsObject.empty)
+ case Some(JsObject(fields)) if initArgs.isEmpty => (Map.empty, JsObject(fields))
+ case Some(JsObject(fields)) =>
+ val (env, args) = JsObject(fields).fields.partition(k => initArgs.contains(k._1))
(env, JsObject(args))
+ case Some(JsArray(elements)) => (Map.empty, JsArray(elements))
Review Comment:
```suggestion
case None => (Map.empty, JsObject.empty)
case Some(JsArray(elements)) => (Map.empty, JsArray(elements))
case Some(fields) if initArgs.isEmpty => (Map.empty, fields)
case Some(fields) =>
val (env, args) = fields.fields.partition(k => initArgs.contains(k._1))
(env, JsObject(args))
```
This one can be changed like this?
##########
tests/src/test/scala/system/basic/WskSequenceTests.scala:
##########
@@ -541,6 +541,31 @@ class WskSequenceTests extends TestHelpers with WskTestHelpers with StreamLoggin
}
}
+ it should "invoke a sequence which support array result" in withAssetCleaner(wskprops) { (wp, assetHelper) =>
Review Comment:
```suggestion
it should "invoke a sequence which supports array result" in withAssetCleaner(wskprops) { (wp, assetHelper) =>
```
##########
tests/src/test/scala/org/apache/openwhisk/core/controller/test/ConductorsApiTests.scala:
##########
@@ -345,26 +351,39 @@ class ConductorsApiTests extends ControllerTestCommon with WhiskActionsApi {
case `echo` => // echo action
Future(Right(respond(action, msg, args)))
case `conductor` => // see tests/dat/actions/conductor.js
- val result =
- if (args.fields.get("error") isDefined) args
- else {
- val action = args.fields.get("action") map { action =>
- Map("action" -> action)
- } getOrElse Map.empty
- val state = args.fields.get("state") map { state =>
- Map("state" -> state)
- } getOrElse Map.empty
- val wrappedParams = args.fields.getOrElse("params", JsObject.empty).asJsObject.fields
- val escapedParams = args.fields - "action" - "state" - "params"
- val params = Map("params" -> JsObject(wrappedParams ++ escapedParams))
- JsObject(params ++ action ++ state)
+ val result = {
+ args match {
+ case JsObject(fields) =>
+ if (fields.get("error") isDefined) args
+ else {
+ val action = fields.get("action") map { action =>
+ Map("action" -> action)
+ } getOrElse Map.empty
+ val state = fields.get("state") map { state =>
+ Map("state" -> state)
+ } getOrElse Map.empty
+ val wrappedParams = fields.getOrElse("params", JsObject.empty).asJsObject.fields
+ val escapedParams = fields - "action" - "state" - "params"
+ val params = Map("params" -> JsObject(wrappedParams ++ escapedParams))
+ JsObject(params ++ action ++ state)
+ }
+ case _ => JsObject.empty
Review Comment:
If we just do this, can we support conductor actions with JSON array results?
##########
common/scala/src/main/scala/org/apache/openwhisk/core/entity/ActivationResult.scala:
##########
@@ -203,7 +203,7 @@ protected[core] object ActivationResponse extends DefaultJsonProtocol {
truncated match {
case None =>
val sizeOpt = Option(str).map(_.length)
- Try { str.parseJson.asJsObject } match {
+ Try { str.parseJson } match {
case scala.util.Success(result @ JsObject(fields)) =>
Review Comment:
So even without `asJsObject`, the result would match here?
Then, it was not necessary in the first place?
##########
core/controller/src/main/scala/org/apache/openwhisk/core/controller/actions/SequenceActions.scala:
##########
@@ -460,9 +465,10 @@ protected[actions] case class SequenceAccounting(atomicActionCnt: Int,
// check conditions on payload that may lead to interrupting the execution of the sequence
// short-circuit the execution of the sequence iff the payload contains an error field
// and is the result of an action return, not the initial payload
- val outputPayload = activation.response.result.map(_.asJsObject)
- val payloadContent = outputPayload getOrElse JsObject.empty
- val errorField = payloadContent.fields.get(ActivationResponse.ERROR_FIELD)
+ val errorField: Option[JsValue] = activation.response.result match {
+ case Some(JsObject(fields)) => fields.get(ActivationResponse.ERROR_FIELD)
+ case _ => None
Review Comment:
So users can't define the explicit error response when the result is a JSON array.
But there would be no difference in the JSON-object-based action, right?
Sometimes, libraries return an `error` filed and OW considers that as an error of an action as well.
Since there would be no behavioral difference in the JSON-object-based actions, there would be no difference in existing semantics, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org