fix: #1344 convert destObj to the correct property type before settin… #1346
Conversation
…efore setting value
|
Thank you for this contribution. This is really great. Could you please add some test coverage for this scenario to the |
…kflow - Added `should_execute_json_workflow_with_complex_input_type` test to verify handling of complex input properties. - Ensured `AssignTask` step correctly sets `Assignee` information in `FlowData`. - Included JSON workflow definition with complex input property. - Verified `Assignee` and `UnitInfo` properties are correctly set in the test.
I’ve added the requested integration tests. Let me know if any further changes are needed. |
|
Hi @danielgerla , I recently made a pull request and have since added a few more commits to support list of complex input type. Could you please review the latest changes when you have a moment? I’d appreciate your thoughts and any suggestions for further improvement. Thanks for your time! |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issue #1344 by improving type conversion for complex input properties in workflow definitions. The change eliminates the need for explicit data type specification when working with complex objects and collections.
Key changes:
- Enhanced type conversion for complex objects and collections in workflow step inputs
- Added support for processing lists/arrays of complex objects
- Implemented proper type conversion using
ToObject()method
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/WorkflowCore.DSL/Services/DefinitionLoader.cs |
Core fix: added list processing support and improved type conversion for complex objects |
test/WorkflowCore.TestAssets/Steps/AssigneeInfo.cs |
Added test data classes for complex object scenarios |
test/WorkflowCore.TestAssets/Steps/AssignTask.cs |
Added test step implementation for complex input handling |
test/WorkflowCore.TestAssets/DataTypes/FlowData.cs |
Added workflow data type for testing complex scenarios |
test/WorkflowCore.TestAssets/Utils.cs |
Added utility methods to load new test definitions |
test/WorkflowCore.TestAssets/WorkflowCore.TestAssets.csproj |
Updated project file to include new test assets |
test/WorkflowCore.TestAssets/def-complex-input-property.json |
Test workflow definition for single complex object input |
test/WorkflowCore.TestAssets/def-list-of-complex-input-property.json |
Test workflow definition for list/array of complex objects |
test/WorkflowCore.IntegrationTests/Scenarios/StoredJsonScenario.cs |
Added integration tests for complex input scenarios |
| @@ -253,7 +261,7 @@ private void AttachDirectlyOutput(KeyValuePair<string, string> output, WorkflowS | |||
|
|
|||
| Action<IStepBody, object> acn = (pStep, pData) => | |||
| { | |||
| object resolvedValue = sourceExpr.Compile().DynamicInvoke(pStep); ; | |||
| object resolvedValue = sourceExpr.Compile().DynamicInvoke(pStep); | |||
There was a problem hiding this comment.
Removed unnecessary semicolon after the statement. This appears to be a formatting cleanup that's unrelated to the main fix.
Copilot uses AI. Check for mistakes.
| @@ -306,7 +314,7 @@ private void AttachNestedOutput(KeyValuePair<string, string> output, WorkflowSte | |||
| { | |||
| var targetExpr = Expression.Lambda(memberExpression, dataParameter); | |||
| object data = targetExpr.Compile().DynamicInvoke(pData); | |||
| object resolvedValue = sourceExpr.Compile().DynamicInvoke(pStep); ; | |||
| object resolvedValue = sourceExpr.Compile().DynamicInvoke(pStep); | |||
There was a problem hiding this comment.
Removed unnecessary semicolon after the statement. This appears to be a formatting cleanup that's unrelated to the main fix.
Copilot uses AI. Check for mistakes.
|
|
||
| var itemType = stepProperty.PropertyType.IsArray | ||
| ? stepProperty.PropertyType.GetElementType() | ||
| : stepProperty.PropertyType.GetGenericArguments().FirstOrDefault(); |
There was a problem hiding this comment.
Using FirstOrDefault() could return null for non-generic types, but the null check on line 406 should handle this. However, consider using First() if you expect generic arguments to always exist for non-array collection types.
Copilot uses AI. Check for mistakes.
| { | ||
| var listInstance = Activator.CreateInstance(typeof(List<>).MakeGenericType(itemType)); | ||
| var addMethod = listInstance.GetType().GetMethod("Add"); | ||
| foreach (var item in processedItems) |
There was a problem hiding this comment.
Consider using processedItems.ToArray() directly instead of manually copying items when creating arrays, as this would be more efficient and cleaner.
Copilot uses AI. Check for mistakes.
| "unitType": 1 | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Extra trailing whitespace after the closing brace. Consider removing the trailing spaces for consistency.
| } | |
| } |
Copilot uses AI. Check for mistakes.
There is no longer a need to specify the input data type. A minor change for Issue #1344 now allows it to function without requiring data type information for the input.