Return null rather than throwing exception when conversion fail#1222
Conversation
Signed-off-by: fjtirado <ftirados@redhat.com>
fjtirado
force-pushed
the
Return_null_rather_than_throwing_exception
branch
from
19b166c to
1cfd8ec
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces an explicit null-check capability on WorkflowModel and uses it in the Java expression execution path to avoid throwing when a model represents null (notably for typed Java functions).
Changes:
- Add
WorkflowModel#isNull()as a default interface method, with a Jackson-specific override. - Update
JavaExpressionFactoryto returnnullfor typed function arguments when the incomingWorkflowModelis null. - Adjust a Func DSL lambda test invocation to use the typed overload.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
impl/model/src/main/java/io/serverlessworkflow/impl/model/jackson/JacksonModel.java |
Overrides isNull() to use JsonNode#isNull() directly. |
impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowModel.java |
Adds default isNull() implementation based on asJavaObject() == null. |
experimental/lambda/src/main/java/io/serverlessworkflow/impl/expressions/func/JavaExpressionFactory.java |
Routes typed function argument conversion through a new convert() helper that returns null for null models. |
experimental/lambda/src/test/java/io/serverless/workflow/impl/executors/func/FuncDSLDataFlowTransformationHelpersTest.java |
Updates an exportAs call to the typed overload by passing an explicit argument class. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .as(argClass) | ||
| .orElseThrow( | ||
| () -> | ||
| new IllegalArgumentException( |
There was a problem hiding this comment.
The new model.isNull() ? null : ... conversion path isn’t covered by tests. Adding a test that passes a null workflow/task output into a typed Java function and asserts the function receives null (and no exception is thrown) would prevent regressions.
| .exportAs( | ||
| (object, workflowContext, taskContextData) -> { | ||
| Long taskOutput = output(taskContextData, Long.class); | ||
| softly.assertThat(taskOutput).isEqualTo(15L); | ||
| return taskOutput * 2; | ||
| })) | ||
| }, | ||
| Object.class)) |
There was a problem hiding this comment.
This test now forces the typed overload by passing Object.class, but the lambda doesn’t assert what the object argument actually is. Consider using the concrete expected type (e.g., Long.class) and asserting on the object parameter so the test validates the conversion behavior this PR is changing.
| Object value = descriptor.asObject(); | ||
| if (value instanceof Function func) { |
There was a problem hiding this comment.
logger is declared but never used. Either remove it to avoid dead code, or use it (e.g., log conversion failures / null conversions) so it serves a purpose.
| } | ||
|
|
||
| @Override | ||
| public int priority(ExpressionDescriptor descriptor) { |
There was a problem hiding this comment.
The IllegalArgumentException message concatenation is missing a space before the target type (currently produces ... to classclass ...). Consider formatting it as ... to class <type> (e.g., using argClass.getName()), and include a separator after class.
| .as(argClass) | ||
| .orElseThrow( | ||
| () -> | ||
| new IllegalArgumentException( | ||
| "Cannot convert model " + model.asJavaObject() + " to class" + argClass)); | ||
| } | ||
|
|
||
| @Override | ||
| public int priority(ExpressionDescriptor descriptor) { | ||
| Object value = descriptor.asObject(); |
There was a problem hiding this comment.
PR title indicates conversions should return null rather than throw when conversion fails, but this still throws for non-null values when model.as(argClass) is empty (and model.as(...) may also throw). If the intent is truly "return null on conversion failure", consider returning null/Optional here instead of throwing, or explicitly adjust the PR title/behavior to match.
This is a follow up of serverlessworkflow#1222
This is a follow up of serverlessworkflow#1222 Signed-off-by: fjtirado <ftirados@redhat.com>
This is a follow up of serverlessworkflow#1222 Signed-off-by: fjtirado <ftirados@redhat.com>
No description provided.