Overhauls the pipeline execution to support the async/await model#5
Overhauls the pipeline execution to support the async/await model#5jsedlak wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Petl pipeline execution model to be async-first throughout, enabling support for asynchronous transformations with full cancellation token support. The change maintains backward compatibility with synchronous transformations while adding new overloads for async operations.
Key Changes
- Updated all core interfaces (
ITransformationStep,IPipeline) to returnTaskand acceptCancellationTokenparameters - Added three
Transformoverloads to support sync, async, and async with cancellation token seamlessly - Introduced
ITransformationStepContainer<TSource, TTarget>interface to abstract transformation step execution - Updated all tests to use async/await patterns and added comprehensive tests for async transformations, cancellation, and mixed sync/async scenarios
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Petl.Core/ITransformationStep.cs | Updated Execute method to return Task with CancellationToken support |
| src/Petl.Core/IPipeline.cs | Updated Exec method signature to be async with cancellation support |
| src/Petl.Core/ITransformationStepContainer.cs | New internal interface for transformation step containers with async execution |
| src/Petl.Core/TransformationStep.cs | Implements ITransformationStepContainer and adds three Transform overloads for sync/async scenarios |
| src/Petl.Core/TransformTransformation.cs | Refactored to support three constructor overloads, wrapping sync delegates as async operations |
| src/Petl.Core/PropertyTransformation.cs | Updated Execute to be async-compatible with cancellation token checks |
| src/Petl.Core/AutoMapTransformation.cs | Updated Execute to be async-compatible with cancellation token checks |
| src/Petl.Core/Pipeline.cs | Refactored to accept interface-based steps and execute them asynchronously |
| src/Petl.Core/PipelineBuilder.cs | Updated Build method to cast steps to interface type and added braces per coding standards |
| src/Petl.Tests/PipelineTests.cs | Updated all existing Exec tests to async, added new tests for async transformations, cancellation, and mixed scenarios |
| src/Petl.Tests/DependencyInjectionTests.cs | Updated tests that execute pipelines to use await and added braces per coding standards |
| README.md | Updated all examples to show async/await usage, added Async Transformations section, updated API reference |
| .cursor/rules/coding-standards.mdc | New coding standard requiring braces for all code blocks |
| .agents/prds/async-pipeline-support.md | Complete PRD document describing the async refactoring requirements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| foreach (var transformation in _transformations) | ||
| { | ||
| transformation.Execute(source, target); | ||
| await transformation.Execute(source, target, cancellationToken); |
There was a problem hiding this comment.
As a library, this code should consider using ConfigureAwait(false) on all await calls to avoid capturing the synchronization context unnecessarily. This prevents potential deadlocks and improves performance when the library is consumed by applications that might use synchronous blocking on async code. Apply .ConfigureAwait(false) to the await statement here and in all other async methods in the library (Pipeline.Exec, TransformationStep.Execute, etc.).
| await transformation.Execute(source, target, cancellationToken); | |
| await transformation.Execute(source, target, cancellationToken).ConfigureAwait(false); |
| foreach (var step in _steps) | ||
| { | ||
| step.Execute(source, target); | ||
| await step.Execute(source, target, cancellationToken); |
There was a problem hiding this comment.
As a library, this code should consider using ConfigureAwait(false) on all await calls to avoid capturing the synchronization context unnecessarily. This prevents potential deadlocks and improves performance when the library is consumed by applications that might use synchronous blocking on async code. Apply .ConfigureAwait(false) to the await statement here.
| await step.Execute(source, target, cancellationToken); | |
| await step.Execute(source, target, cancellationToken).ConfigureAwait(false); |
| if (source is TSource typedSource && target is TTarget typedTarget) | ||
| { | ||
| _transformAction(sourceObj, targetObj); | ||
| await _transformAction(typedSource, typedTarget, cancellationToken); |
There was a problem hiding this comment.
As a library, this code should consider using ConfigureAwait(false) on all await calls to avoid capturing the synchronization context unnecessarily. This prevents potential deadlocks and improves performance when the library is consumed by applications that might use synchronous blocking on async code. Apply .ConfigureAwait(false) to both await statements in this method.
| cts.Cancel(); | ||
|
|
||
| // Act & Assert | ||
| await Assert.ThrowsExceptionAsync<TaskCanceledException>( |
There was a problem hiding this comment.
The test expects TaskCanceledException but the PRD specification at line 501 of .agents/prds/async-pipeline-support.md indicates this should be OperationCanceledException. TaskCanceledException is more specific and derives from OperationCanceledException, but for better compatibility with cancellation patterns, it's generally recommended to catch the base OperationCanceledException type. Consider changing this to match the PRD specification.
| await Assert.ThrowsExceptionAsync<TaskCanceledException>( | |
| await Assert.ThrowsExceptionAsync<OperationCanceledException>( |
| /// <param name="transformAction">The async transformation action with cancellation support</param> | ||
| public TransformTransformation(Func<TSource, TTarget, CancellationToken, Task> transformAction) | ||
| { | ||
| _transformAction = transformAction ?? throw new ArgumentNullException(nameof(transformAction)); |
There was a problem hiding this comment.
According to the coding standards added in this PR (.cursor/rules/coding-standards.mdc), all code blocks should always use braces. The third constructor's null check uses an inline throw expression which is acceptable, but for consistency with the first two constructors which use explicit if blocks with braces, consider using the same pattern here.
| _transformAction = transformAction ?? throw new ArgumentNullException(nameof(transformAction)); | |
| if (transformAction == null) | |
| { | |
| throw new ArgumentNullException(nameof(transformAction)); | |
| } | |
| _transformAction = transformAction; |
| builder.WithStep("Cancellable Step") | ||
| .Transform(async (source, target, ct) => | ||
| { | ||
| await Task.Delay(10000, ct); |
There was a problem hiding this comment.
The test uses a delay of 10000ms (10 seconds), but the PRD specification at line 490 indicates this should be 1000ms (1 second). While both will work since the cancellation token is already cancelled, using 10 seconds adds unnecessary wait time risk if the cancellation isn't working properly. Consider reducing this to 1000ms to match the PRD specification.
| await Task.Delay(10000, ct); | |
| await Task.Delay(1000, ct); |
| var pipeline = builder.Build(); | ||
| var input = new TestInput { Name = "test" }; | ||
| var output = new TestOutput(); | ||
| var cts = new CancellationTokenSource(); |
There was a problem hiding this comment.
Disposable 'CancellationTokenSource' is created but not disposed.
| var cts = new CancellationTokenSource(); | |
| using var cts = new CancellationTokenSource(); |
No description provided.