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

[KOGITO-7924] ForEach working with WorkItemHandler #2465

Merged
merged 4 commits into from
Sep 15, 2022

Conversation

fjtirado
Copy link
Contributor

@fjtirado fjtirado commented Sep 14, 2022

ForEach was only working with ActionNodes. This change will allow it to work with any NodeInstance (in theory), In practise, we have either WorkItemNode or ActionNode and this is the scenario a test was added for.

@fjtirado fjtirado force-pushed the KOGITO-7924 branch 2 times, most recently from 0c8a6e0 to 9f0c19c Compare September 14, 2022 12:58
@fjtirado fjtirado changed the title [KOGITO-7924] Input evaluation [KOGITO-7924] ForEach working with WorkItemHandler Sep 14, 2022
@fjtirado fjtirado force-pushed the KOGITO-7924 branch 2 times, most recently from e512f66 to 9a8fb2d Compare September 14, 2022 14:47
@fjtirado fjtirado marked this pull request as ready for review September 14, 2022 14:54
@@ -107,4 +114,31 @@ public static Optional<String> fallbackVarToName(String expr) {
int indexOf = expr.lastIndexOf('.');
return indexOf < 0 ? Optional.empty() : Optional.of(expr.substring(indexOf + 1));
}

public static JsonNode addVariablesFromContext(JsonNode context, KogitoProcessContext processInfo) {
Copy link
Contributor Author

@fjtirado fjtirado Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, it will be better to rather than cloning and adding the engine variables to the model being evaluated, for JQ and JsonPath to read variable value from the process instance,. if needed by the expression.
There are two problems though:

  1. Jq library we are using does not support that, see Change scope constructor from private to protected eiiches/jackson-jq#221
  2. The spec example treats the loop variable as it was part of the workflow model. This is not really true, because it should not be visible. So rather than .item the expression should look $item See ForEach item loop variable usage in expression  serverlessworkflow/specification#682

@apache apache deleted a comment from kie-ci3 Sep 14, 2022
Copy link
Contributor

@hbelmiro hbelmiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I just left 2 nitpicking comments.

@fjtirado fjtirado marked this pull request as draft September 14, 2022 19:07
@fjtirado
Copy link
Contributor Author

Although not strictly related with this issue, I moved to draft because I want to add more test (in particular action data filter, I want to stress for each and I think is a good chance)

Update quarkus/extensions/kogito-quarkus-serverless-workflow-extension/kogito-quarkus-serverless-workflow-integration-test/src/test/java/org/kie/kogito/quarkus/workflows/ForEachRestIT.java

Co-authored-by: Helber Belmiro <[email protected]>
Update kogito-serverless-workflow/kogito-serverless-workflow-builder/src/main/java/org/kie/kogito/serverless/workflow/parser/VariableInfo.java

Co-authored-by: Helber Belmiro <[email protected]>
@fjtirado fjtirado marked this pull request as ready for review September 15, 2022 10:23
"divisor" : ".divisor"
}
},
"actionDataFilter" : {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing an actionDataFilter.
Only results manipulation make sense inside a foreach loop (result are kept in loop variable)

@kie-ci3
Copy link
Contributor

kie-ci3 commented Sep 15, 2022

(tests) - kogito-runtimes job #932 was: UNSTABLE
Possible explanation: This should be test failures

Please look here: https://eng-jenkins-csb-business-automation.apps.ocp-c1.prod.psi.redhat.com/job/KIE/job/kogito/job/main/job/pullrequest/job/kogito-runtimes.tests.kogito-runtimes/932/display/redirect

Test results:

  • PASSED: 2961
  • FAILED: 2

Those are the test failures:

org.kie.kogito.quarkus.workflows.ExpressionRestFromImageIT.testExpressionRest java.lang.AssertionError:
1 expectation failed.
Expected status code <201> but was <500>.

at org.kie.kogito.quarkus.workflows.ExpressionRestFromImageIT.testExpressionRest(ExpressionRestFromImageIT.java:39)
org.kie.kogito.quarkus.workflows.ExpressionRestIT.testExpressionRest java.lang.AssertionError:
1 expectation failed.
Expected status code <201> but was <500>.

at org.kie.kogito.quarkus.workflows.ExpressionRestIT.testExpressionRest(ExpressionRestIT.java:40)

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

61.2% 61.2% Coverage
0.0% 0.0% Duplication

@fjtirado fjtirado merged commit a506da5 into apache:main Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants