Support array result for common action and sequence action by ningyougang · Pull Request #5290 · apache/openwhisk

ningyougang · 2022-07-21T07:04:41Z

Description

POEM document pr: #5244

This feature is: provide support array result feature for common action and sequence action. e.g.

Support array result for common action (support nodejs by default, so let's use nodejs as example)

# cat ~/hello-array.js 
function main(params) {
    console.log("-------------log test---------")
    return [{"key1":"value1"},{"key2":"value2"}];
}

# wsk -i action create hello-array-nodejs --kind nodejs:10 ~/hello-array.js 
# wsk -i action invoke hello-array-nodejs -r -v
REQUEST:
[POST]	http://xxx.xxx.xxx.xxx:10001/api/v1/namespaces/_/actions/hello-array-nodejs?blocking=true&result=true
...
...
Response body size is 37 bytes
Response body received:
[{"key1":"value1"},{"key2":"value2"}]

Support array result for sequence action

# cat ~/split.js
function main(msg) {
    var separator = msg.separator || /\r?\n/;
    var lines = payload.split(separator);
    // below lines result is array, it will be used as next action's input param
    return lines;
}

# cat ~/sort.js
function main(msg) {
    var lines = msg || [];
    lines.sort();
    return lines;
}

# wsk -i action create /whisk.system/utils/split --kind nodejs:10 ~/split.js
# wsk -i action create /whisk.system/utils/sort --kind nodejs:10 ~/sort.js
# wsk -i action create mySequence --sequence /whisk.system/utils/split,/whisk.system/utils/sort
# wsk -i action invoke --result mySequence --param payload "bbb\ncccc\naaaa"  -r -v
REQUEST:
[POST]	http://xxx.xxx.xxx.xxx:10001/api/v1/namespaces/_/actions/mySequence?blocking=true&result=true
...
Response body size is 22 bytes
Response body received:
["aaaa","bbbb","cccc"]

Activation check in elasticsearch

# curl -u xxx:xxx -H "Content-Type: application/json" '[http://xxx.xxx.xxx.xxx:9200/lambda-exp_whisk.system/_search?pretty=true' -d  '{
  "query" : { "match" : { "activationId" : "9cc4dad8d81342fc84dad8d81302fc54" }},
  "from":0,
  "size":1
}'
{
  "took" : 3,
  ...
  "hits" : {
    "total" : 1,
    "max_score" : 5.1628795,
    "hits" : [
      {
        "_index" : "lambda-exp_whisk.system",
          ...
          "logs" : [
            "2022-07-21T06:53:16.543515915Z stdout: -------------log test---------"
          ],
          "name" : "hello-array-nodejs",
          ...
          "response" : {
            "result" : "[{\"key1\":\"value1\"},{\"key2\":\"value2\"}]",
            "size" : 37,
            "statusCode" : 0
          },
          ...
        }
      }
    ]
  }
}

# curl -u xxx:xxx -H "Content-Type: application/json" '[http://10.168.240.79:9200/lambda-exp_whisk.system/_search?pretty=true' -d  '{
  "query" : { "match" : { "activationId" : "9c774fe852de44cdb74fe852de94cd54" }},
  "from":0,
  "size":1
}'
...
          "response" : {
            "result" : "[\"aaaa\",\"bbb\",\"cccc\"]",
            "size" : 21,
            "statusCode" : 0
          }
...

The response.result must use text to store, because different activation's same filed may use different type, e.g.
one activation's result's name filed value is string , e.g. {"name": "jack"}
another activation's result's name filed value is int, e.g. {"name": 12}
Elasticsearch doesn't support this, it store like this, it would report error.

Other works

As above example, we already know, nodejs runtime supports this feature by default.
But other runtimes(e.g. go, java, php, etc) don't support,

After this pr merged, there still has other works for runtime images, need to support subsequent prs for other images.
And in CLI side, still has small mount works to parse the JSON array string to user.

Related issue and scope

I opened an issue to propose and discuss this change (#????)

My changes affect the following components

Types of changes

Bug fix (generally a non-breaking change which closes an issue).
Enhancement or new feature (adds new functionality).
Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

I signed an Apache CLA.
I reviewed the style guides and followed the recommendations (Travis CI will check :).
I added tests to cover my changes.
My changes require further changes to the documentation.
I updated the documentation where necessary.

ningyougang · 2022-07-21T07:12:27Z

common/scala/src/main/scala/org/apache/openwhisk/core/connector/Message.scala

                             rootControllerIndex: ControllerInstanceId,
                             blocking: Boolean,
-                             content: Option[JsObject],
+                             content: Option[JsValue],


For common action to pass parameter as dict, it would use Option[JsObject]

For sequence action to pass parameter as array, it would use Option[JsArray], e.g. the first action's JsArray result as the next action's input param.

ningyougang · 2022-07-21T07:16:09Z

...in/scala/org/apache/openwhisk/core/database/elasticsearch/ElasticSearchActivationStore.scala

              val JsString(data) = r
-              data.parseJson.asJsObject
+              data.parseJson match {
+                case JsArray(elements) => JsArray(elements)


Support store the JsArray result to ElasticSearch.

ningyougang · 2022-07-21T07:18:14Z

tests/dat/actions/split-array.js

+    var payload = msg.payload.toString();
+    var lines = payload.split(separator);
+    // return array as next action's input
+    return lines;


As comment said, return array as next action's input
This script file is used as test case for invoke sequence action which support array result

ningyougang · 2022-07-21T07:18:58Z

tests/src/test/scala/system/basic/WskActionTests.scala

+      activation.response.result shouldBe Some(
+        JsArray(JsObject("key1" -> JsString("value1")), JsObject("key2" -> JsString("value2"))))
+    }
+  }


Because nodejs runtime suports array result by default, add a system test case for return array result for it to test controller/invoker whether works well

ningyougang · 2022-07-21T07:35:26Z

tests/src/test/scala/actionContainers/ActionContainer.scala

 trait ActionContainer {
  def init(value: JsValue): (Int, Option[JsObject])
  def run(value: JsValue): (Int, Option[JsObject])
+  def runForJsArray(value: JsValue): (Int, Option[JsArray])


Normally, need to change def run(...) as below

def run(value: JsValue): (Int, Option[JsValue]) // Option[JsObject] -> Option[JsValue]

But i just add another new method here, two reasons.

If change def run directly, there has a lot of code changes.

It is just for test, so i just add another new method, there has no need to introduce too many code modifications

codecov-commenter · 2022-07-21T08:08:06Z

Codecov Report

Attention: Patch coverage is 69.76744% with 13 lines in your changes missing coverage. Please review.

Project coverage is 75.05%. Comparing base (f915acc) to head (84d220f).
Report is 112 commits behind head on master.

Files with missing lines	Patch %	Lines
...whisk/core/containerpool/AkkaContainerClient.scala	0.00%	5 Missing ⚠️
...isk/core/controller/actions/PrimitiveActions.scala	50.00%	4 Missing ⚠️
...hisk/core/controller/actions/SequenceActions.scala	71.42%	2 Missing ⚠️
.../org/apache/openwhisk/core/connector/Message.scala	66.66%	1 Missing ⚠️
...org/apache/openwhisk/core/controller/Actions.scala	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #5290       +/-   ##
===========================================
+ Coverage   45.04%   75.05%   +30.00%     
===========================================
  Files         238      238               
  Lines       14179    14202       +23     
  Branches      617      608        -9     
===========================================
+ Hits         6387    10659     +4272     
+ Misses       7792     3543     -4249

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

style95

It generally looks good to me.

style95 · 2022-07-26T04:53:17Z

common/scala/src/main/scala/org/apache/openwhisk/core/connector/Message.scala

+        JsHelpers
+          .getFieldPath(JsObject(fields), ERROR_FIELD, "statusCode")
+          .orElse(JsHelpers.getFieldPath(JsObject(fields), "statusCode"))
+      case _ => None


This is for the array result case and there could be no statusCode field in the array result, right?

Yes, for array result, seems can't get the userDefined statusCode

style95 · 2022-07-26T04:54:35Z

common/scala/src/main/scala/org/apache/openwhisk/core/containerpool/AkkaContainerClient.scala

+    implicit logging: Logging,
+    as: ActorSystem,
+    ec: ExecutionContext,
+    tid: TransactionId): (Int, Option[JsArray]) = {


Is there any reason that we can't just change the result type to (Int, Option[JsValue])?

def runForJsArray is added for test case, the return value is (Int, Option[JsArray])

call chain here is: def runForJsArray -> private def syncPostForJsArray -> def postForJsArray

Actually, in order to support test, we can change below method to support return (Int, Option[JsValue]) also

But if we change the run directly, there would be a lot of changes in openwhisk repo and all runtime codes should change as well.

So here, i added another extra method to test return array

This is just for impact the original code as little as possible: #5290 (comment)

style95 · 2022-07-26T04:55:51Z

common/scala/src/main/scala/org/apache/openwhisk/core/entity/ActivationResult.scala

            val sizeOpt = Option(str).map(_.length)
-            Try { str.parseJson.asJsObject } match {
+            Try { str.parseJson } match {
              case scala.util.Success(result @ JsObject(fields)) =>


So even without asJsObject, the result would match here?
Then, it was not necessary in the first place?

If without this pr, i think we can remove .asJsObject.

But with this pr, we need to support JsArray, must remove .asJsObject to match below codes

style95 · 2022-07-26T05:00:42Z

...controller/src/main/scala/org/apache/openwhisk/core/controller/actions/SequenceActions.scala

-    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


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?

So users can't define the explicit error response when the result is a JSON array.
Yes, can't define the explicit error response for a JSON array,
but if we want to define the explicit error response, normally, user should return the JSON-object result.

But there would be no difference in the JSON-object-based action, right?
For the JSON-object-based action, have no any bad influences on these actions, have no any change.

Regarding

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?

I am not sure whether i understand correctly, if libraries return an error or user's codes has some error, normally, there would has catch(....) statement in user's code, and in catch statement, user can return the error or some excetion error with JSON-object result.
If user codes run well without any error or exception, can return array result finally.

style95 · 2022-07-26T05:02:55Z

core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerProxy.scala

+      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))


Suggested change

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))

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?

No, would report error, but there has a optimize point here.
Change JsObject(fields).fields to fields

style95 · 2022-07-26T05:07:39Z

tests/src/test/scala/system/basic/WskSequenceTests.scala

      }
  }

+  it should "invoke a sequence which support array result" in withAssetCleaner(wskprops) { (wp, assetHelper) =>


Suggested change

it should "invoke a sequence which support array result" in withAssetCleaner(wskprops) { (wp, assetHelper) =>

it should "invoke a sequence which supports array result" in withAssetCleaner(wskprops) { (wp, assetHelper) =>

Updated accordingly

style95 · 2022-07-26T05:09:04Z

tests/src/test/scala/org/apache/openwhisk/core/controller/test/ConductorsApiTests.scala

+                      val params = Map("params" -> JsObject(wrappedParams ++ escapedParams))
+                      JsObject(params ++ action ++ state)
+                    }
+                  case _ => JsObject.empty


If we just do this, can we support conductor actions with JSON array results?

hm..i am not sure, if want to support, i think we can submit in next pr.

Couchdb already suports

This test case is just for nodejs

style95 · 2022-07-28T01:31:13Z

It generally looks good to me.
So it is not a breaking change and there would be no issue with the existing features, right?
Do we have to merge this before merging all runtime changes?

ningyougang · 2022-07-28T01:33:26Z

It generally looks good to me. So it is not a breaking change and there would be no issue with the existing features, right? Do we have to merge this before merging all runtime changes?

Yes, has no issue.

Should merge this first, because other runtime prs depend on this pr.

ningyougang · 2022-07-28T10:29:38Z

Have any comment? due to there exist several runtime pr depend on this pr, so i just asked.

style95

I am ok with this change if there is no side effect to existing features.

* Support array result * Make controller accept json array * Make elasticsearch support json array Couchdb already suports * Make go runtime test cases due to depend on this * Add test case for array result for nodejs runtime * Make sequence action to support array result * Optimize sequence action to support array result * Fix test case for sequence action feature * Add test case for sequence action This test case is just for nodejs * Add extra method runForJsArray for runtime tests * Fix build error * Fix review comment

ningyougang commented Jul 21, 2022

View reviewed changes

ningyougang requested review from KeonHee, bdoyle0182, jiangpengcheng, style95 and upgle July 21, 2022 07:20

ningyougang commented Jul 21, 2022

View reviewed changes

style95 reviewed Jul 26, 2022

View reviewed changes

This was referenced Jul 26, 2022

Support array result include sequence action apache/openwhisk-runtime-swift#150

Merged

Support array result include sequence action apache/openwhisk-runtime-deno#14

Merged

ningyougang added 8 commits July 26, 2022 16:56

Support array result

ae9d8d6

Make controller accept json array

1ebc52c

Make elasticsearch support json array

929fe41

Couchdb already suports

Make go runtime test cases due to depend on this

3ba9723

Add test case for array result for nodejs runtime

c15de8f

Make sequence action to support array result

b7235eb

Optimize sequence action to support array result

6ba278a

Fix test case for sequence action feature

a9451f3

ningyougang added 4 commits July 26, 2022 16:56

Add test case for sequence action

c92bce3

This test case is just for nodejs

Add extra method runForJsArray for runtime tests

d7de6e5

Fix build error

d5b6569

Fix review comment

84d220f

ningyougang force-pushed the support-array-result-include-sequence-action branch from 460fdff to 84d220f Compare July 26, 2022 08:57

style95 approved these changes Jul 30, 2022

View reviewed changes

jiangpengcheng approved these changes Aug 1, 2022

View reviewed changes

ningyougang merged commit 62b8a50 into apache:master Aug 1, 2022

ningyougang mentioned this pull request Aug 3, 2022

Upgrade gradle version apache/openwhisk-runtime-go#172

Merged

1 task

This was referenced Aug 3, 2022

Bumping to gradle 6 apache/openwhisk-runtime-nodejs#221

Merged

Fix runtime return type backward compatibility #5306

Merged

ningyougang mentioned this pull request Aug 4, 2022

Support array result apache/openwhisk-cli#516

Closed

1 task

ningyougang mentioned this pull request Aug 20, 2022

Fix deprecated issue & Support array result apache/openwhisk-cli#517

Merged

4 tasks

	it should "invoke a sequence which support array result" in withAssetCleaner(wskprops) { (wp, assetHelper) =>
	it should "invoke a sequence which supports array result" in withAssetCleaner(wskprops) { (wp, assetHelper) =>

Conversation

ningyougang commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

Uh oh!

ningyougang Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ningyougang Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

style95 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ningyougang Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ningyougang Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

style95 commented Jul 28, 2022

Uh oh!

ningyougang commented Jul 28, 2022

Uh oh!

ningyougang commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

style95 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ningyougang commented Jul 21, 2022 •

edited

Loading

ningyougang Jul 21, 2022 •

edited

Loading

ningyougang Jul 21, 2022 •

edited

Loading

codecov-commenter commented Jul 21, 2022 •

edited

Loading

ningyougang Jul 26, 2022 •

edited

Loading

ningyougang Jul 26, 2022 •

edited

Loading

ningyougang commented Jul 28, 2022 •

edited

Loading