Add support for Run.script (Javascript) #962
Conversation
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
There was a problem hiding this comment.
I know this is a draft but I think is good I wrote a long general comment, since this PR requires some deep refactor.
The goal should be to not add Javascript or Python dependencies to the core.
With that spirit, you need a RunScriptTask in the core modules that selects either the JavaScript or the Python implementation (use the language id to select the right implementation, all implementation being loaded through service loading to not create dependencies)
Then you have another module with the java script specific code plus the javascript library (the java script implementation class being loaded thorugh service loader)
About this specific module, we need to split better between init (parsing) and apply (runtime). rather than having an Executor that does all the parsing at runtime at the end, you will need several "functor" classes that give you the source, the environmment and so on.
Finally, JavaScript should be executed on embedded mode, not running an external process.
I will try to comment on specific cases.
| String value = | ||
| ExpressionUtils.isExpr(entry.getValue()) | ||
| ? WorkflowUtils.buildStringResolver( | ||
| application, | ||
| entry.getValue().toString(), | ||
| taskContext.input().asJavaObject()) | ||
| .apply(workflowContext, taskContext, taskContext.input()) | ||
| : entry.getValue().toString(); |
There was a problem hiding this comment.
This is an example of functor I mention in the general comment.
The idea is that you generate a Map<String,WorkflowValueResolver during init, by using Workfloutils.buildStringFilter (WorkflowApplication appl, String expression), and at runtime you actually invoke the apply (passing the Workflowcontext, TaskContext and WorkflowModel passed to apply method)
| if (scriptUnion.getInlineScript() != null) { | ||
| source = scriptUnion.getInlineScript().getCode(); | ||
| } else if (scriptUnion.getExternalScript() == null) { | ||
| throw new WorkflowException( | ||
| WorkflowError.runtime( | ||
| taskContext, new IllegalStateException("No script source defined.")) | ||
| .build()); | ||
| } else { | ||
| source = | ||
| definition | ||
| .resourceLoader() | ||
| .load( | ||
| scriptUnion.getExternalScript().getSource(), | ||
| ResourceLoaderUtils::readString, | ||
| workflowContext, | ||
| taskContext, | ||
| taskContext.input()); | ||
| } |
There was a problem hiding this comment.
This is also another example of "functor" that should be defined at init and later caller at runtime.
You basically generate a WorkflowValueResolver, so the if are invoked during it, that either give you the source inline or use the workflow and task context to load the resource containing the string
| <artifactId>serverlessworkflow-impl-openapi</artifactId> | ||
| <version>${project.version}</version> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
I think graalvm is a the right choice for javascript library, however, as indicated in the general comment, this dependecy should be in an specific javascript module, not in the core one
| WorkflowModelFactory modelFactory = application.modelFactory(); | ||
|
|
||
| // GraalVM does not provide exit code, assuming 0 for successful execution | ||
| int statusCode = 0; | ||
|
|
||
| return switch (taskConfiguration.getReturn()) { | ||
| case ALL -> | ||
| modelFactory.fromAny( | ||
| new ProcessResult(statusCode, stdout.toString(), stderr.toString())); | ||
| case NONE -> modelFactory.fromNull(); | ||
| case CODE -> modelFactory.from(statusCode); | ||
| case STDOUT -> modelFactory.from(stdout.toString().trim()); | ||
| case STDERR -> modelFactory.from(stderr.toString().trim()); | ||
| }; | ||
| } catch (PolyglotException e) { | ||
| if (e.getExitStatus() != 0 || e.isSyntaxError()) { | ||
| throw new WorkflowException(WorkflowError.runtime(taskContext, e).build()); | ||
| } else { | ||
| WorkflowModelFactory modelFactory = definition.application().modelFactory(); | ||
| return switch (taskConfiguration.getReturn()) { | ||
| case ALL -> | ||
| modelFactory.fromAny( | ||
| new ProcessResult( | ||
| e.getExitStatus(), stdout.toString().trim(), buildStderr(e, stderr))); | ||
| case NONE -> modelFactory.fromNull(); | ||
| case CODE -> modelFactory.from(e.getExitStatus()); | ||
| case STDOUT -> modelFactory.from(stdout.toString().trim()); | ||
| case STDERR -> modelFactory.from(buildStderr(e, stderr)); | ||
| }; | ||
| } |
There was a problem hiding this comment.
I thing that rather than an external context, GraalVM supports embedded call
Take a look to
https://www.graalvm.org/latest/reference-manual/js/Modules/#ecmascript-modules-esm
There was a problem hiding this comment.
Let me elaborate a bit more on this. The spec states this
To ensure cross-compatibility, Serverless Workflow strictly limits the versions of supported scripting languages. These versions may evolve with future releases. If you wish to use a different version of a language, you may do so by utilizing the container process.
So this clearly implies that the supported script execution should be embedded. If you want to execute a different version that the supported one, then you use external process. Therefore calling external process to run a supported script is not the way to go.
Great comment, I was inclined to create a separate module, especially since the dependency is quite large and really specific. Thanks, I will change it in the next commits. |
2 participants
Many thanks for submitting your Pull Request ❤️!
What this PR does / why we need it:
Special notes for reviewers:
Additional information (if needed):