You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kie.apache.org by "fjtirado (via GitHub)" <gi...@apache.org> on 2024/10/29 19:43:20 UTC
[PR] [Fix #3733] Collecting more than one error message [incubator-kie-kogito-runtimes]
fjtirado opened a new pull request, #3756:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3756
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Many thanks for submitting your Pull Request :heart:!
<!-- Please don't forget your Issue link -->
Closes/Fixes/Resolves #ISSUE-NUMBER
<!-- Please provide a short description of what this PR does -->
**Description:**
<!-- Link to related PRs: -->
Please make sure that your PR meets the following requirements:
- [ ] You have read the [contributors guide](CONTRIBUTING.md)
- [ ] Your code is properly formatted according to [this configuration](https://github.com/apache/incubator-kie-kogito-runtimes/tree/main/kogito-build/kogito-ide-config)
- [ ] Pull Request title is properly formatted: `Issue-XYZ Subject`
- [ ] Pull Request title contains the target branch if not targeting main: `[0.9.x] Issue-XYZ Subject`
- [ ] Pull Request contains link to the JIRA issue
- [ ] Pull Request contains link to any dependent or related Pull Request
- [ ] Pull Request contains description of the issue
- [ ] Pull Request does not include fixes for issues other than the main ticket
<details>
<summary>
How to replicate CI configuration locally?
</summary>
Build Chain tool does "simple" maven build(s), the builds are just Maven commands, but because the repositories relates and depends on each other and any change in API or class method could affect several of those repositories there is a need to use [build-chain tool](https://github.com/kiegroup/github-action-build-chain) to handle cross repository builds and be sure that we always use latest version of the code for each repository.
[build-chain tool](https://github.com/kiegroup/github-action-build-chain) is a build tool which can be used on command line locally or in Github Actions workflow(s), in case you need to change multiple repositories and send multiple dependent pull requests related with a change you can easily reproduce the same build by executing it on Github hosted environment or locally in your development environment. See [local execution](https://github.com/kiegroup/github-action-build-chain#local-execution) details to get more information about it.
</details>
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix #3733] Collecting more than one error message [incubator-kie-kogito-runtimes]
Posted by "fjtirado (via GitHub)" <gi...@apache.org>.
fjtirado merged PR #3756:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3756
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix #3733] Collecting more than one error message [incubator-kie-kogito-runtimes]
Posted by "fjtirado (via GitHub)" <gi...@apache.org>.
fjtirado commented on code in PR #3756:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3756#discussion_r1824380574
##########
kogito-serverless-workflow/kogito-serverless-workflow-builder/src/main/java/org/kie/kogito/serverless/workflow/parser/handlers/ActionNodeUtils.java:
##########
@@ -34,16 +34,21 @@ public class ActionNodeUtils {
return embeddedSubProcess.actionNode(context.newId()).name(functionDef.getName());
}
- public static void checkArgs(FunctionRef functionRef, String... requiredArgs) {
+ public static boolean checkArgs(ParserContext context, FunctionRef functionRef, String... requiredArgs) {
JsonNode args = functionRef.getArguments();
+ boolean isOk = true;
if (args == null) {
- throw new IllegalArgumentException("Arguments cannot be null for function " + functionRef.getRefName());
- }
- for (String arg : requiredArgs) {
- if (!args.has(arg)) {
- throw new IllegalArgumentException("Missing mandatory " + arg + " argument for function " + functionRef.getRefName());
+ context.addValidationError("Arguments cannot be null for function " + functionRef.getRefName());
+ isOk = false;
+ } else {
+ for (String arg : requiredArgs) {
+ if (!args.has(arg)) {
+ context.addValidationError("Missing mandatory " + arg + " argument for function " + functionRef.getRefName());
+ isOk = false;
+ }
}
}
+ return isOk;
Review Comment:
all missing mandatory parameters should be collected, that's the reason for the isOk flag.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix #3733] Collecting more than one error message [incubator-kie-kogito-runtimes]
Posted by "fjtirado (via GitHub)" <gi...@apache.org>.
fjtirado commented on code in PR #3756:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3756#discussion_r1824380574
##########
kogito-serverless-workflow/kogito-serverless-workflow-builder/src/main/java/org/kie/kogito/serverless/workflow/parser/handlers/ActionNodeUtils.java:
##########
@@ -34,16 +34,21 @@ public class ActionNodeUtils {
return embeddedSubProcess.actionNode(context.newId()).name(functionDef.getName());
}
- public static void checkArgs(FunctionRef functionRef, String... requiredArgs) {
+ public static boolean checkArgs(ParserContext context, FunctionRef functionRef, String... requiredArgs) {
JsonNode args = functionRef.getArguments();
+ boolean isOk = true;
if (args == null) {
- throw new IllegalArgumentException("Arguments cannot be null for function " + functionRef.getRefName());
- }
- for (String arg : requiredArgs) {
- if (!args.has(arg)) {
- throw new IllegalArgumentException("Missing mandatory " + arg + " argument for function " + functionRef.getRefName());
+ context.addValidationError("Arguments cannot be null for function " + functionRef.getRefName());
+ isOk = false;
+ } else {
+ for (String arg : requiredArgs) {
+ if (!args.has(arg)) {
+ context.addValidationError("Missing mandatory " + arg + " argument for function " + functionRef.getRefName());
+ isOk = false;
+ }
}
}
+ return isOk;
Review Comment:
But I want to collect all missing mandatory parameters, thats the reson for the isOk flag.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix #3733] Collecting more than one error message [incubator-kie-kogito-runtimes]
Posted by "gmunozfe (via GitHub)" <gi...@apache.org>.
gmunozfe commented on code in PR #3756:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3756#discussion_r1824336012
##########
kogito-serverless-workflow/kogito-serverless-workflow-builder/src/main/java/org/kie/kogito/serverless/workflow/parser/handlers/ActionNodeUtils.java:
##########
@@ -34,16 +34,21 @@ public class ActionNodeUtils {
return embeddedSubProcess.actionNode(context.newId()).name(functionDef.getName());
}
- public static void checkArgs(FunctionRef functionRef, String... requiredArgs) {
+ public static boolean checkArgs(ParserContext context, FunctionRef functionRef, String... requiredArgs) {
JsonNode args = functionRef.getArguments();
+ boolean isOk = true;
Review Comment:
Instead of setting `isOk` to false and continuing the checks, you could return `false` immediately after adding a validation error. This will remove the need for the `isOk` flag entirely, making the code more readable. Wdyt?
```suggestion
```
##########
kogito-serverless-workflow/kogito-serverless-workflow-executor-core/src/test/java/org/kie/kogito/serverless/workflow/executor/StaticWorkflowApplicationTest.java:
##########
@@ -107,6 +109,17 @@ void interpolationFile(String fileName, WorkflowFormat format) throws IOExceptio
}
}
+ @Test
+ void testValidationError() throws IOException {
+ try (Reader reader = new InputStreamReader(Thread.currentThread().getContextClassLoader().getResourceAsStream("wrong.sw.json"));
+ StaticWorkflowApplication application = StaticWorkflowApplication.create()) {
+ Workflow workflow = getWorkflow(reader, WorkflowFormat.JSON);
+ ValidationException validationException = catchThrowableOfType(() -> application.process(workflow), ValidationException.class);
+ assertThat(validationException.getErrors()).hasSizeGreaterThanOrEqualTo(4);
Review Comment:
Why also greater? Shouldn't it be equal to 4 always?
##########
kogito-serverless-workflow/kogito-serverless-workflow-builder/src/main/java/org/kie/kogito/serverless/workflow/parser/handlers/ActionNodeUtils.java:
##########
@@ -34,16 +34,21 @@ public class ActionNodeUtils {
return embeddedSubProcess.actionNode(context.newId()).name(functionDef.getName());
}
- public static void checkArgs(FunctionRef functionRef, String... requiredArgs) {
+ public static boolean checkArgs(ParserContext context, FunctionRef functionRef, String... requiredArgs) {
JsonNode args = functionRef.getArguments();
+ boolean isOk = true;
if (args == null) {
- throw new IllegalArgumentException("Arguments cannot be null for function " + functionRef.getRefName());
- }
- for (String arg : requiredArgs) {
- if (!args.has(arg)) {
- throw new IllegalArgumentException("Missing mandatory " + arg + " argument for function " + functionRef.getRefName());
+ context.addValidationError("Arguments cannot be null for function " + functionRef.getRefName());
+ isOk = false;
+ } else {
+ for (String arg : requiredArgs) {
+ if (!args.has(arg)) {
+ context.addValidationError("Missing mandatory " + arg + " argument for function " + functionRef.getRefName());
+ isOk = false;
+ }
}
}
+ return isOk;
Review Comment:
This part without the `isOk` flag would be:
```suggestion
context.addValidationError("Arguments cannot be null for function " + functionRef.getRefName());
return false;
}
for (String arg : requiredArgs) {
if (!args.has(arg)) {
context.addValidationError("Missing mandatory " + arg + " argument for function " + functionRef.getRefName());
return false;
}
}
return true;
```
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix #3733] Collecting more than one error message [incubator-kie-kogito-runtimes]
Posted by "fjtirado (via GitHub)" <gi...@apache.org>.
fjtirado commented on code in PR #3756:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3756#discussion_r1824380574
##########
kogito-serverless-workflow/kogito-serverless-workflow-builder/src/main/java/org/kie/kogito/serverless/workflow/parser/handlers/ActionNodeUtils.java:
##########
@@ -34,16 +34,21 @@ public class ActionNodeUtils {
return embeddedSubProcess.actionNode(context.newId()).name(functionDef.getName());
}
- public static void checkArgs(FunctionRef functionRef, String... requiredArgs) {
+ public static boolean checkArgs(ParserContext context, FunctionRef functionRef, String... requiredArgs) {
JsonNode args = functionRef.getArguments();
+ boolean isOk = true;
if (args == null) {
- throw new IllegalArgumentException("Arguments cannot be null for function " + functionRef.getRefName());
- }
- for (String arg : requiredArgs) {
- if (!args.has(arg)) {
- throw new IllegalArgumentException("Missing mandatory " + arg + " argument for function " + functionRef.getRefName());
+ context.addValidationError("Arguments cannot be null for function " + functionRef.getRefName());
+ isOk = false;
+ } else {
+ for (String arg : requiredArgs) {
+ if (!args.has(arg)) {
+ context.addValidationError("Missing mandatory " + arg + " argument for function " + functionRef.getRefName());
+ isOk = false;
+ }
}
}
+ return isOk;
Review Comment:
But I want to collect all missing mandatory parameters, thats the reason for the isOk flag.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix #3733] Collecting more than one error message [incubator-kie-kogito-runtimes]
Posted by "fjtirado (via GitHub)" <gi...@apache.org>.
fjtirado commented on code in PR #3756:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3756#discussion_r1824400468
##########
kogito-serverless-workflow/kogito-serverless-workflow-builder/src/main/java/org/kie/kogito/serverless/workflow/parser/handlers/ActionNodeUtils.java:
##########
@@ -34,16 +34,21 @@ public class ActionNodeUtils {
return embeddedSubProcess.actionNode(context.newId()).name(functionDef.getName());
}
- public static void checkArgs(FunctionRef functionRef, String... requiredArgs) {
+ public static boolean checkArgs(ParserContext context, FunctionRef functionRef, String... requiredArgs) {
JsonNode args = functionRef.getArguments();
+ boolean isOk = true;
Review Comment:
see response below
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix #3733] Collecting more than one error message [incubator-kie-kogito-runtimes]
Posted by "fjtirado (via GitHub)" <gi...@apache.org>.
fjtirado commented on code in PR #3756:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3756#discussion_r1824384233
##########
kogito-serverless-workflow/kogito-serverless-workflow-executor-core/src/test/java/org/kie/kogito/serverless/workflow/executor/StaticWorkflowApplicationTest.java:
##########
@@ -107,6 +109,17 @@ void interpolationFile(String fileName, WorkflowFormat format) throws IOExceptio
}
}
+ @Test
+ void testValidationError() throws IOException {
+ try (Reader reader = new InputStreamReader(Thread.currentThread().getContextClassLoader().getResourceAsStream("wrong.sw.json"));
+ StaticWorkflowApplication application = StaticWorkflowApplication.create()) {
+ Workflow workflow = getWorkflow(reader, WorkflowFormat.JSON);
+ ValidationException validationException = catchThrowableOfType(() -> application.process(workflow), ValidationException.class);
+ assertThat(validationException.getErrors()).hasSizeGreaterThanOrEqualTo(4);
Review Comment:
The validation errors are a mix of Sonataflow parser errors (which we control) and engine validation errors (which we do not control), so, in order to not tie the unit test too much with the engine implementation, we consider it is ok is there are at least 4 validation errors (one for transition, one for missing error, one for not existing function and another for not connected state) and also check the final exception message contains the four significant strings
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix #3733] Collecting more than one error message [incubator-kie-kogito-runtimes]
Posted by "fjtirado (via GitHub)" <gi...@apache.org>.
fjtirado commented on code in PR #3756:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3756#discussion_r1824380574
##########
kogito-serverless-workflow/kogito-serverless-workflow-builder/src/main/java/org/kie/kogito/serverless/workflow/parser/handlers/ActionNodeUtils.java:
##########
@@ -34,16 +34,21 @@ public class ActionNodeUtils {
return embeddedSubProcess.actionNode(context.newId()).name(functionDef.getName());
}
- public static void checkArgs(FunctionRef functionRef, String... requiredArgs) {
+ public static boolean checkArgs(ParserContext context, FunctionRef functionRef, String... requiredArgs) {
JsonNode args = functionRef.getArguments();
+ boolean isOk = true;
if (args == null) {
- throw new IllegalArgumentException("Arguments cannot be null for function " + functionRef.getRefName());
- }
- for (String arg : requiredArgs) {
- if (!args.has(arg)) {
- throw new IllegalArgumentException("Missing mandatory " + arg + " argument for function " + functionRef.getRefName());
+ context.addValidationError("Arguments cannot be null for function " + functionRef.getRefName());
+ isOk = false;
+ } else {
+ for (String arg : requiredArgs) {
+ if (!args.has(arg)) {
+ context.addValidationError("Missing mandatory " + arg + " argument for function " + functionRef.getRefName());
+ isOk = false;
+ }
}
}
+ return isOk;
Review Comment:
But I want to collect all missing mandatory parameters, thats the reson for the isOk
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org