Skip to content

Commit

Permalink
[Fix apache#3733] Collecting more than one error message (apache#3756)
Browse files Browse the repository at this point in the history 
* [Fix apache#3733] Collecting more than one error message

* [Fix apache#3733] Including process validation check

* [Fix apache#3733] Adding transition check
  • Loading branch information
@fjtirado @rgdoliveira
fjtirado authored and rgdoliveira committed Nov 7, 2024
1 parent b750843 commit 1e93071
Show file tree
Hide file tree
Showing 18 changed files with 253 additions and 88 deletions.
10 changes: 7 additions & 3 deletions jbpm/jbpm-flow/src/main/java/org/jbpm/ruleflow/core/validation/RuleFlowProcessValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,7 @@ public static RuleFlowProcessValidator getInstance() {
return INSTANCE;
}

public ProcessValidationError[] validateProcess(final RuleFlowProcess process) {
final List<ProcessValidationError> errors = new ArrayList<>();

public List<ProcessValidationError> validateProcess(final RuleFlowProcess process, List<ProcessValidationError> errors) {
if (process.getName() == null) {
errors.add(new ProcessValidationErrorImpl(process,
"Process has no name."));
Expand Down Expand Up @@ -151,6 +149,12 @@ public ProcessValidationError[] validateProcess(final RuleFlowProcess process) {
errors,
process);

return errors;

}

public ProcessValidationError[] validateProcess(final RuleFlowProcess process) {
final List<ProcessValidationError> errors = validateProcess(process, new ArrayList<>());
return errors.toArray(new ProcessValidationError[errors.size()]);
}

Expand Down
21 changes: 14 additions & 7 deletions ...va/org/kie/kogito/serverless/workflow/operationid/AbstractWorkflowOperationIdFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public abstract class AbstractWorkflowOperationIdFactory implements WorkflowOper

@Override
public WorkflowOperationId from(Workflow workflow, FunctionDefinition function, Optional<ParserContext> context) {
ActionResource actionResource = ActionResourceFactory.getActionResource(function);
ActionResource actionResource = ActionResourceFactory.getActionResource(function, context);
Optional<String> convertedUri = convertURI(workflow, context, actionResource.getUri());
final String fileName;
final String uri;
Expand All @@ -56,14 +56,19 @@ public WorkflowOperationId from(Workflow workflow, FunctionDefinition function,
fileName = getFileName(workflow, function, context, uri, actionResource.getOperation(), actionResource.getService());
}
if (fileName == null || fileName.isBlank()) {
throw new IllegalArgumentException(
format("Empty file name for function '%s', please review uri '%s' or consider using a different strategy defined in the kogito.sw.operationIdStrategy property",
function.getName(), uri));
String msg = format("Empty file name for function '%s', please review uri '%s' or consider using a different strategy defined in the kogito.sw.operationIdStrategy property",
function.getName(), uri);
context.ifPresentOrElse(c -> c.addValidationError(msg), () -> {
throw new IllegalArgumentException(msg);
});
}
String packageName = onlyChars(removeExt(fileName.toLowerCase()));
if (packageName.isBlank()) {
throw new IllegalArgumentException(
format("Empty package for file '%s'. A file name should contain at least one letter which is not part of the extension", fileName));
String msg =
format("Empty package for file '%s'. A file name should contain at least one letter which is not part of the extension", fileName);
context.ifPresentOrElse(c -> c.addValidationError(msg), () -> {
throw new IllegalArgumentException(msg);
});
}
return new WorkflowOperationId(uri, actionResource.getOperation(), actionResource.getService(), fileName, packageName);
}
Expand All @@ -80,7 +85,9 @@ private JsonNode getUriDefinitions(Workflow workflow, Optional<ParserContext> co
try {
definitions = uri == null ? NullNode.instance : ObjectMapperFactory.get().readTree(readBytes(uri, workflow, context));
} catch (IOException e) {
throw new UncheckedIOException(e);
context.ifPresentOrElse(c -> c.addValidationError(e.getMessage()), () -> {
throw new UncheckedIOException(e);
});
}
uriDefinitions.setDefinitions(definitions);
}
Expand Down
12 changes: 10 additions & 2 deletions ...rkflow-builder/src/main/java/org/kie/kogito/serverless/workflow/parser/ParserContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,9 @@ public class ParserContext {
private final NodeIdGenerator idGenerator;
private final WorkflowOperationIdFactory operationIdFactory;
private final KogitoBuildContext context;
private final Collection<GeneratedFile> generatedFiles;
private final Collection<GeneratedFile> generatedFiles = new ArrayList<>();
private final AsyncInfoResolver asyncInfoResolver;
private final Collection<String> validationErrors = new ArrayList<>();

public static final String ASYNC_CONVERTER_KEY = "asyncInfoConverter";

Expand All @@ -60,7 +61,6 @@ public ParserContext(NodeIdGenerator idGenerator, RuleFlowProcessFactory factory
this.context = context;
this.operationIdFactory = operationIdFactory;
this.asyncInfoResolver = asyncInfoResolver;
this.generatedFiles = new ArrayList<>();
}

public void add(StateHandler<?> stateHandler) {
Expand Down Expand Up @@ -114,4 +114,12 @@ public void setCompensation() {
public KogitoBuildContext getContext() {
return context;
}

public void addValidationError(String message) {
validationErrors.add(message);
}

public Collection<String> validationErrors() {
return validationErrors;
}
}
22 changes: 17 additions & 5 deletions ...der/src/main/java/org/kie/kogito/serverless/workflow/parser/ServerlessWorkflowParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,24 +20,28 @@

import java.io.IOException;
import java.io.Reader;
import java.io.UncheckedIOException;
import java.net.URI;
import java.net.URL;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;

import org.jbpm.process.core.datatype.impl.type.ObjectDataType;
import org.jbpm.process.core.validation.ProcessValidationError;
import org.jbpm.process.core.validation.impl.ProcessValidationErrorImpl;
import org.jbpm.ruleflow.core.Metadata;
import org.jbpm.ruleflow.core.RuleFlowProcessFactory;
import org.jbpm.ruleflow.core.validation.RuleFlowProcessValidator;
import org.jbpm.workflow.core.WorkflowModelValidator;
import org.kie.kogito.codegen.api.GeneratedInfo;
import org.kie.kogito.codegen.api.context.KogitoBuildContext;
import org.kie.kogito.internal.process.runtime.KogitoWorkflowProcess;
import org.kie.kogito.internal.utils.ConversionUtils;
import org.kie.kogito.jackson.utils.ObjectMapperFactory;
import org.kie.kogito.process.validation.ValidationException;
import org.kie.kogito.serverless.workflow.SWFConstants;
import org.kie.kogito.serverless.workflow.extensions.OutputSchema;
import org.kie.kogito.serverless.workflow.operationid.WorkflowOperationIdFactoryProvider;
Expand Down Expand Up @@ -121,7 +125,7 @@ private ServerlessWorkflowParser(Workflow workflow, KogitoBuildContext context)
}

private GeneratedInfo<KogitoWorkflowProcess> parseProcess() {
WorkflowValidator.validateStart(workflow);

RuleFlowProcessFactory factory = RuleFlowProcessFactory.createProcess(workflow.getId(), !workflow.isKeepActive())
.name(workflow.getName() == null ? DEFAULT_NAME : workflow.getName())
.version(workflow.getVersion() == null ? DEFAULT_VERSION : workflow.getVersion())
Expand All @@ -134,6 +138,7 @@ private GeneratedInfo<KogitoWorkflowProcess> parseProcess() {
.type(KogitoWorkflowProcess.SW_TYPE);
ParserContext parserContext =
new ParserContext(idGenerator, factory, context, WorkflowOperationIdFactoryProvider.getFactory(context.getApplicationProperty(WorkflowOperationIdFactoryProvider.PROPERTY_NAME)));
WorkflowValidator.validateStart(workflow, parserContext);
modelValidator(parserContext, Optional.ofNullable(workflow.getDataInputSchema())).ifPresent(factory::inputValidator);
modelValidator(parserContext, ServerlessWorkflowUtils.getExtension(workflow, OutputSchema.class).map(OutputSchema::getOutputSchema)).ifPresent(factory::outputValidator);
loadConstants(factory, parserContext);
Expand All @@ -145,6 +150,7 @@ private GeneratedInfo<KogitoWorkflowProcess> parseProcess() {
handlers.forEach(StateHandler::handleState);
handlers.forEach(StateHandler::handleTransitions);
handlers.forEach(StateHandler::handleConnections);

if (parserContext.isCompensation()) {
factory.metaData(Metadata.COMPENSATION, true);
factory.metaData(Metadata.COMPENSATE_WHEN_ABORTED, true);
Expand All @@ -170,8 +176,13 @@ private GeneratedInfo<KogitoWorkflowProcess> parseProcess() {
if (!annotations.isEmpty()) {
factory.metaData(Metadata.ANNOTATIONS, annotations);
}

return new GeneratedInfo<>(factory.validate().getProcess(), parserContext.generatedFiles());
factory.link();
List<ProcessValidationError> errors = RuleFlowProcessValidator.getInstance().validateProcess(factory.getProcess(), new ArrayList<>());
parserContext.validationErrors().forEach(m -> errors.add(new ProcessValidationErrorImpl(factory.getProcess(), m)));
if (!errors.isEmpty()) {
throw new ValidationException(factory.getProcess().getId(), errors);
}
return new GeneratedInfo<>(factory.getProcess(), parserContext.generatedFiles());
}

private Optional<WorkflowModelValidator> modelValidator(ParserContext parserContext, Optional<DataInputSchema> schema) {
Expand All @@ -194,7 +205,8 @@ private void loadConstants(RuleFlowProcessFactory factory, ParserContext parserC
try {
constants.setConstantsDef(ObjectMapperFactory.get().readValue(readBytes(constants.getRefValue(), workflow, parserContext), JsonNode.class));
} catch (IOException e) {
throw new UncheckedIOException("Invalid file " + constants.getRefValue(), e);
parserContext.addValidationError("Invalid file " + constants.getRefValue() + e);
return;
}
}
factory.metaData(Metadata.CONSTANTS, constants.getConstantsDef());
Expand Down
17 changes: 11 additions & 6 deletions ...der/src/main/java/org/kie/kogito/serverless/workflow/parser/handlers/ActionNodeUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

private ActionNodeUtils() {
Expand Down
38 changes: 25 additions & 13 deletions ...c/main/java/org/kie/kogito/serverless/workflow/parser/handlers/ActionResourceFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@
package org.kie.kogito.serverless.workflow.parser.handlers;

import java.util.Map;
import java.util.function.Function;
import java.util.Optional;
import java.util.function.BiFunction;

import org.kie.kogito.serverless.workflow.parser.ParserContext;

import io.serverlessworkflow.api.functions.FunctionDefinition;
import io.serverlessworkflow.api.functions.FunctionDefinition.Type;
Expand All @@ -28,38 +31,47 @@

public class ActionResourceFactory {

private static final Map<Type, Function<String, ActionResource>> map =
private static final Map<Type, BiFunction<String, Optional<ParserContext>, ActionResource>> map =
Map.of(FunctionDefinition.Type.REST, ActionResourceFactory::justOperation, FunctionDefinition.Type.ASYNCAPI, ActionResourceFactory::justOperation, FunctionDefinition.Type.RPC,
ActionResourceFactory::withService);

private static ActionResource justOperation(String operationStr) {
String[] tokens = getTokens(operationStr, 2);
private static ActionResource justOperation(String operationStr, Optional<ParserContext> context) {
String[] tokens = getTokens(operationStr, 2, context);
return new ActionResource(tokens[0], tokens[1], null);
}

private static ActionResource withService(String operationStr) {
String[] tokens = getTokens(operationStr, 3);
private static ActionResource withService(String operationStr, Optional<ParserContext> context) {
String[] tokens = getTokens(operationStr, 3, context);
return new ActionResource(tokens[0], tokens[2], tokens[1]);
}

private static String[] getTokens(String operationStr, int expectedTokens) {
private static String[] getTokens(String operationStr, int expectedTokens, Optional<ParserContext> context) {
String[] tokens = operationStr.split(OPERATION_SEPARATOR);
if (tokens.length != expectedTokens) {
throw new IllegalArgumentException(String.format("%s should have just %d %s", operationStr, expectedTokens - 1, OPERATION_SEPARATOR));
String msg = String.format("%s should have just %d %s", operationStr, expectedTokens - 1, OPERATION_SEPARATOR);
context.ifPresentOrElse(c -> c.addValidationError(msg), () -> {
throw new IllegalArgumentException(msg);
});
}
return tokens;
}

public static ActionResource getActionResource(FunctionDefinition function) {
Function<String, ActionResource> factory = map.get(function.getType());
public static ActionResource getActionResource(FunctionDefinition function, Optional<ParserContext> context) {
BiFunction<String, Optional<ParserContext>, ActionResource> factory = map.get(function.getType());
if (factory == null) {
throw new UnsupportedOperationException(function.getType() + " does not support action resources");
String msg = function.getType() + " does not support action resources";
context.ifPresentOrElse(c -> c.addValidationError(msg), () -> {
throw new UnsupportedOperationException(msg);
});
}
String operation = function.getOperation();
if (operation == null) {
throw new IllegalArgumentException("operation string must not be null for function " + function.getName());
String msg = "operation string must not be null for function " + function.getName();
context.ifPresentOrElse(c -> c.addValidationError(msg), () -> {
throw new IllegalArgumentException(msg);
});
}
return factory.apply(operation);
return factory.apply(operation, context);
}

private ActionResourceFactory() {
Expand Down
4 changes: 2 additions & 2 deletions .../java/org/kie/kogito/serverless/workflow/parser/handlers/CompositeContextNodeHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ private MakeNodeResult processActionFilter(RuleFlowNodeContainerFactory<?, ?> em
return filterAndMergeNode(embeddedSubProcess, collectVar, fromExpr, resultExpr, toExpr, useData, shouldMerge,
(factory, inputVar, outputVar) -> addActionMetadata(getActionNode(factory, action.getSubFlowRef(), inputVar, outputVar), action));
} else {
throw new IllegalArgumentException("Action node " + action.getName() + " of state " + state.getName() + " does not have function or event defined");
return faultyNodeResult(embeddedSubProcess, "Action node " + action.getName() + " of state " + state.getName() + " does not have function or event defined");
}
}

Expand Down Expand Up @@ -199,7 +199,7 @@ private TimerNodeFactory<?> createTimerNode(RuleFlowNodeContainerFactory<?, ?> f
.findFirst()
.map(functionDef -> fromFunctionDefinition(embeddedSubProcess, functionDef, functionRef, varInfo))
.or(() -> fromPredefinedFunction(embeddedSubProcess, functionRef, varInfo))
.orElseThrow(() -> new IllegalArgumentException("Cannot find function " + functionName));
.orElseGet(() -> faultyNode(embeddedSubProcess, "Cannot find function " + functionName));
}

private Stream<FunctionDefinition> getFunctionDefStream() {
Expand Down
Loading

0 comments on commit 1e93071

Please sign in to comment.