Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serverless workflow: actionDataFilter failed to merge nested objects #2686

Closed
GlobeFishNG opened this issue Dec 13, 2022 · 11 comments
Closed
Assignees

Comments

@GlobeFishNG
Copy link
Contributor

GlobeFishNG commented Dec 13, 2022

Describe the bug

Below is my action and the actionDataFilter ${ .collections.test } didn't work and it only gave me an output like { "test": "sth" }.

{
    "name": "test",
    "type": "operation",
    "actions": [{
            "name": "Collect Inputs",
            "functionRef": {
                "refName": "collect",
                "arguments": {
                    "stepName": "test",
                    "stepVersion": "1.0.0",
                    "subject": "globefish",
                    "bucket": ".destination.bucket",
                    "prefix": ".destination.key",
                    "action": "input"
                }
            },
            "actionDataFilter": {
                "toStateData": "${ .collections.test }"
            }
        }
    ],
    "end": true
}

Expected behavior

The workflow should be { "collections": { "test": "sth" } }.

Actual behavior

{ "test": "sth" }

How to Reproduce?

  1. set actionDataFilter.toStateData with a nested path like a.b
  2. run the workflow
  3. Get the response

Output of uname -a or ver

Linux ubuntu 5.15.79.1-microsoft-standard-WSL2 #1 SMP Wed Nov 23 01:01:46 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Output of java -version

openjdk version "17.0.5" 2022-10-18
OpenJDK Runtime Environment Temurin-17.0.5+8 (build 17.0.5+8)
OpenJDK 64-Bit Server VM Temurin-17.0.5+8 (build 17.0.5+8, mixed mode, sharing)

GraalVM version (if different from Java)

No response

Kogito version or git rev (or at least Quarkus version if you are using Kogito via Quarkus platform BOM)

Quarkus 2.14.3.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)
Maven home: /home/littlefox/.m2/wrapper/dists/apache-maven-3.8.6-bin/67568434/apache-maven-3.8.6
Java version: 17.0.5, vendor: Eclipse Adoptium, runtime: /usr/lib/jvm/temurin-17-jdk-amd64
Default locale: en, platform encoding: UTF-8 OS name: "linux", version: "5.15.79.1-microsoft-standard-wsl2", arch: "amd64", family: "unix"

Additional information

No response

@fjtirado fjtirado self-assigned this Dec 13, 2022
@fjtirado
Copy link
Contributor

I was not able to fully understand the scenario
with toStateData you are saying that what should be merged into the workflow models is "collection.test" property of the json response.
To fully understand the issue I would need you input model and the function json response.
As an example,
If the flow input is "{"name": "pepe"}, and your function json response is {"collections":{"test":{"name":"pepa"}}}, the result workflow model after the action execution (using the same expression that you show) will be {"name":"pepa"}

@ufonion
Copy link

ufonion commented Dec 13, 2022

Let me try to make it clear. BTW, this is from another account of @GlobeFishNG.

  1. The function json response is {"name":"pepa"} as your mentioned.
  2. My expectation is to merge the response into the field collections.test as below.
{
  "collections": { "test": { "name": "pepa"} }
}
  1. So I tried to set toStateData as .collections.test or ${ .collections.test }, but finally I got this one
{
  "test":  { "name": "pepa"} 
}

When I tried to merge it into the root using only collections or test without dot ., it worked. It seems like it would omit the any parent field.

@fjtirado
Copy link
Contributor

fjtirado commented Dec 13, 2022

Ok, got it now.
Non existing nested attributes is not currently supported (supporting it, although possible, is tricky and I think the spec should be changed, because is mixing selection with assignment and it is messy)
To achieve a similar output you can write, "data" : "${ collections.test: . }"
That will set the data that should be merged to the flow to {
"collections": { "test": { "name": "pepa"} }
}

@fjtirado
Copy link
Contributor

fjtirado commented Dec 13, 2022

Another point that is not clearly set by the spec is what happens is your data is an array and there is already an array in collection.tests, how do you merge them?
In Kogito we decided to not merge arrays and replace the value, if existing ( think of merging [1,2,3,4] and [5,4,3,2,1], which will be the expected result?

@ufonion
Copy link

ufonion commented Dec 13, 2022

@fjtirado T

Ok, got it now. Non existing nested attributes is not currently supported (supporting it, although possible, is tricky and I think the spec should be changed, because is mixing selection with assignment and it is messy) To achieve a similar output you can write, "data" : "${ collections.test: . }" That will set the data that should be merged to the flow to { "collections": { "test": { "name": "pepa"} } }

I will try it. Thanks a lot.

@fjtirado
Copy link
Contributor

In any case, Ill to support "toStateData" : "${collection.test}" by making it equivalent to "data":"${collections.test:.}" for completeness, because this will happen to more people, thanks for the feedback ;)

@ricardozanini
Copy link
Member

@ufonion @fjtirado feel free to bring this use case to the spec. I do believe we need a more clear way of defining merging rules.

@ufonion
Copy link

ufonion commented Dec 13, 2022

@fjtirado Found another issue :)
As you mentioned, i tried to merge to the exsiting state data but got duplicated data finally.
The sw json is below.

{
  "id": "hello_world",
  "version": "1.0",
  "specVersion": "0.8",
  "name": "Hello World Workflow",
  "description": "JSON based hello world workflow",
  "start": "Inject",
  "functions": [
    {
     "name": "greetingFunction",
     "operation": "http://localhost:3000/api-json#GreetingsController_create"
    }
  ],
  "states": [
    {
      "name": "Inject",
      "type": "inject",
      "data": {
        "a": {
          "b": {
            "m": "m"
          }
        }
      },
      "transition": "Greet"
    },
    {
      "name":"Greet",
     "type":"operation",
     "actions":[
        {
           "functionRef": {
              "refName": "greetingFunction",
              "arguments": {
                "person": ".person"
              }
           },
           "actionDataFilter": {
              "results": "{ greeting: .greeting }",
              "toStateData": "${ .a.b }"
           }
        }
     ],
     "end": true
    }
  ]
}

And the result is

~ http -vv POST localhost:8080/hello_world person:='{"name":"John"}'
POST /hello_world HTTP/1.1
Accept: application/json, */*;q=0.5
Accept-Encoding: gzip, deflate
Connection: keep-alive
Content-Length: 28
Content-Type: application/json
Host: localhost:8080
User-Agent: HTTPie/3.2.1

{
    "person": {
        "name": "John"
    }
}


HTTP/1.1 201 Created
Content-Type: application/json
Location: http://localhost:8080/hello_world/0a72bd9f-7c0d-4eeb-807e-2a3b3303a947
content-length: 223

{
    "id": "0a72bd9f-7c0d-4eeb-807e-2a3b3303a947",
    "workflowdata": {
        "a": {
            "b": {
                "greeting": "Welcome to Serverless Workflow, John!",
                "m": "m"
            }
        },
        "b": {
            "greeting": "Welcome to Serverless Workflow, John!",
            "m": "m"
        },
        "person": {
            "name": "John"
        }
    }
}

@fjtirado
Copy link
Contributor

fjtirado commented Dec 14, 2022

It is the "same" issue (different perceiverd result, but same underlying issue) , toStateData is using b to merge and ignoring a (so mergde b object, containing initial m plus function returned greeting is now in two places, under root and under a)
Use results instead of toStateData. So results looks like "${a:{b:{greeting : .greeting}}}" and there is no toStateData.
Im going to modify toStateData to behave properly, but till then, use results (which can do esentially the same than toStateData)

@fjtirado
Copy link
Contributor

Created https://issues.redhat.com/browse/KOGITO-8360 to change toStateData to behave properly

@GlobeFishNG
Copy link
Contributor Author

Created https://issues.redhat.com/browse/KOGITO-8360 to change toStateData to behave properly

Great!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants