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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
