fix: Exception on loop after agent call#1289
fix: Exception on loop after agent call#1289matheusandre1 wants to merge 2 commits intoserverlessworkflow:mainfrom
Conversation
Signed-off-by: Matheus Andre <matheusandr2@gmail> Signed-off-by: Matheus Andre <matheusandr2@gmail.com>
...l/lambda/src/main/java/io/serverlessworkflow/impl/executors/func/JavaForExecutorBuilder.java
Outdated
Show resolved
Hide resolved
…zation tests Signed-off-by: Matheus Andre <matheusandr2@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #824 by addressing a NullPointerException that occurred when looping after an agent call. The root cause was that ForTaskFunction.getForClass() and other Optional fields could return null, causing NPE when calling .isPresent() on them. The fix ensures all Optional fields are properly initialized and maintained as non-null throughout the object's lifecycle, including after serialization/deserialization.
Changes:
- Initialize Optional fields with
Optional.empty()as defaults and add defensive normalization in constructor and deserialization - Add
readObject()method to handle deserialization safely - Update
JavaForExecutorBuilderto use safer Optional patterns (.map().orElse()instead of.isPresent()) - Add comprehensive regression tests covering initialization, serialization, and execution scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| experimental/types/src/main/java/io/serverlessworkflow/api/types/func/ForTaskFunction.java | Added default initialization of Optional fields, constructor, normalization logic, and readObject method for deserialization |
| experimental/lambda/src/main/java/io/serverlessworkflow/impl/executors/func/JavaForExecutorBuilder.java | Updated collectionFilterObject method to use safer Optional pattern |
| experimental/lambda/src/test/java/io/serverless/workflow/impl/executors/func/ForTaskFunctionRegressionTest.java | New test file with regression tests for Optional field initialization, serialization, and for-loop execution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
experimental/types/src/main/java/io/serverlessworkflow/api/types/func/ForTaskFunction.java
Show resolved
Hide resolved
| return taskFunctions | ||
| .getForClass() | ||
| .<Object>map( | ||
| forClass -> (Object) new TypedFunction(taskFunctions.getCollection(), (Class) forClass)) |
There was a problem hiding this comment.
| forClass -> (Object) new TypedFunction(taskFunctions.getCollection(), (Class) forClass)) | |
| forClass -> new TypedFunction(taskFunctions.getCollection(), (Class) forClass)) |
You can avoid that casting
| this.whileClass = modelClass != null ? modelClass : Optional.empty(); | ||
| this.itemClass = itemClass != null ? itemClass : Optional.empty(); |
There was a problem hiding this comment.
I do not think modelClass or itemClass can be null here and this method is private.
This is again defensive programming we should avoid ;)
Note: If this method was public, there will be a chance the parameters were null, but that will be a failure of the caller, and in that case, rather than switching the value provided, you should use Objects.requireNonNull to throw the exception the earlier the better.
| private void normalizeOptionalFields() { | ||
| if (whileClass == null) { | ||
| whileClass = Optional.empty(); | ||
| } | ||
| if (itemClass == null) { | ||
| itemClass = Optional.empty(); | ||
| } | ||
| if (forClass == null) { | ||
| forClass = Optional.empty(); | ||
| } | ||
| } | ||
|
|
||
| private void readObject(ObjectInputStream input) throws IOException, ClassNotFoundException { | ||
| input.defaultReadObject(); | ||
| normalizeOptionalFields(); | ||
| } |
There was a problem hiding this comment.
I think this deserve a comment.
The situation you are trying to prevent is an already serialized definition with null values, because before this PR the fields might not be initialized.
| private Function<?, Collection<?>> collection; | ||
|
|
||
| public ForTaskFunction() { | ||
| normalizeOptionalFields(); |
There was a problem hiding this comment.
You do not need to call this method here (whileClass, itemClass and forClass will be never null because you are now initiliazing them), just for deserialization
Many thanks for submitting your Pull Request ❤️!
What this PR does / why we need it: Fixes: #824
Special notes for reviewers: modified validations and I set it to optional.empty by default.
Additional information (if needed):