-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix YAML deserializer to bind inherited properties and handle primitive values in custom step types #1395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Fix YAML deserializer to bind inherited properties and handle primitive values in custom step types #1395
Changes from all commits
4189eaf
7d6ac78
d4d385d
4cb4c84
d3ab802
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -222,7 +222,7 @@ private void AttachInputs(StepSourceV1 source, Type dataType, Type stepType, Wor | |||||||||||||||||||||||||||
var dataParameter = Expression.Parameter(dataType, "data"); | ||||||||||||||||||||||||||||
var contextParameter = Expression.Parameter(typeof(IStepExecutionContext), "context"); | ||||||||||||||||||||||||||||
var environmentVarsParameter = Expression.Parameter(typeof(IDictionary), "environment"); | ||||||||||||||||||||||||||||
var stepProperty = stepType.GetProperty(input.Key); | ||||||||||||||||||||||||||||
var stepProperty = stepType.GetProperty(input.Key, BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
if (stepProperty == null) | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
|
@@ -243,6 +243,21 @@ private void AttachInputs(StepSourceV1 source, Type dataType, Type stepType, Wor | |||||||||||||||||||||||||||
continue; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// Handle primitive values (bool, int, etc.) directly | ||||||||||||||||||||||||||||
if (input.Value != null && (input.Value.GetType().IsPrimitive || input.Value is bool || input.Value is int || input.Value is long || input.Value is double || input.Value is decimal)) | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
var primitiveValue = input.Value; | ||||||||||||||||||||||||||||
void primitiveAction(IStepBody pStep, object pData) | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
if (stepProperty.PropertyType.IsAssignableFrom(primitiveValue.GetType())) | ||||||||||||||||||||||||||||
stepProperty.SetValue(pStep, primitiveValue); | ||||||||||||||||||||||||||||
else | ||||||||||||||||||||||||||||
stepProperty.SetValue(pStep, System.Convert.ChangeType(primitiveValue, stepProperty.PropertyType)); | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
step.Inputs.Add(new ActionParameter<IStepBody, object>(primitiveAction)); | ||||||||||||||||||||||||||||
continue; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
throw new ArgumentException($"Unknown type for input {input.Key} on {source.Id}"); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
using WorkflowCore.Interface; | ||
using WorkflowCore.Models; | ||
using WorkflowCore.Primitives; | ||
|
||
namespace WorkflowCore.TestAssets.Steps | ||
{ | ||
public class IterateListStep : Foreach | ||
{ | ||
// This class inherits from Foreach and should be able to use | ||
// the RunParallel property from the base class in YAML definitions | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
using System; | ||
using FakeItEasy; | ||
using WorkflowCore.Interface; | ||
using WorkflowCore.Services.DefinitionStorage; | ||
using Xunit; | ||
|
||
namespace WorkflowCore.UnitTests.Services.DefinitionStorage | ||
{ | ||
/// <summary> | ||
/// Integration test to verify the fix for inherited property binding in YAML definitions. | ||
/// This test specifically reproduces the issue mentioned in GitHub issue #1375. | ||
/// </summary> | ||
public class YamlInheritedPropertyIntegrationTest | ||
{ | ||
[Fact(DisplayName = "Should bind inherited properties like RunParallel from base Foreach class")] | ||
public void ShouldBindInheritedPropertiesInYamlDefinition() | ||
{ | ||
// Arrange | ||
var registry = A.Fake<IWorkflowRegistry>(); | ||
var loader = new DefinitionLoader(registry, new TypeResolver()); | ||
|
||
// This YAML definition uses a custom step (IterateListStep) that inherits from Foreach | ||
// and tries to set the RunParallel property which is defined in the base Foreach class | ||
var yamlWithInheritedProperty = @" | ||
Id: TestInheritedPropertyWorkflow | ||
Version: 1 | ||
Description: Test workflow for inherited property binding | ||
DataType: WorkflowCore.TestAssets.DataTypes.CounterBoard, WorkflowCore.TestAssets | ||
Steps: | ||
- Id: IterateList | ||
StepType: WorkflowCore.TestAssets.Steps.IterateListStep, WorkflowCore.TestAssets | ||
Inputs: | ||
Collection: ""data.DataList"" | ||
RunParallel: false | ||
"; | ||
|
||
// Act & Assert | ||
// Before the fix, this would throw: "Unknown property for input RunParallel on IterateList" | ||
// After the fix, this should succeed | ||
var exception = Record.Exception(() => | ||
loader.LoadDefinition(yamlWithInheritedProperty, Deserializers.Yaml)); | ||
|
||
// Verify no exception was thrown | ||
Assert.Null(exception); | ||
} | ||
|
||
[Fact(DisplayName = "Should still throw exception for truly unknown properties")] | ||
public void ShouldStillThrowForUnknownProperties() | ||
{ | ||
// Arrange | ||
var registry = A.Fake<IWorkflowRegistry>(); | ||
var loader = new DefinitionLoader(registry, new TypeResolver()); | ||
|
||
var yamlWithUnknownProperty = @" | ||
Id: TestInheritedPropertyWorkflow | ||
Version: 1 | ||
Description: Test workflow for inherited property binding | ||
DataType: WorkflowCore.TestAssets.DataTypes.CounterBoard, WorkflowCore.TestAssets | ||
Steps: | ||
- Id: IterateList | ||
StepType: WorkflowCore.TestAssets.Steps.IterateListStep, WorkflowCore.TestAssets | ||
Inputs: | ||
Collection: ""data.DataList"" | ||
NonExistentProperty: false | ||
"; | ||
|
||
// Act & Assert | ||
// This should still throw an exception for truly unknown properties | ||
var exception = Assert.Throws<ArgumentException>(() => | ||
loader.LoadDefinition(yamlWithUnknownProperty, Deserializers.Yaml)); | ||
|
||
Assert.Contains("Unknown property for input NonExistentProperty", exception.Message); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition
input.Value is bool || input.Value is int || input.Value is long || input.Value is double || input.Value is decimal
is redundant since these types are already covered byinput.Value.GetType().IsPrimitive
. Consider simplifying to just checkIsPrimitive
or use a more explicit approach with a dedicated method.Copilot uses AI. Check for mistakes.