Implement ListInstanceIDs, GetInstanceHistory, and RerunWorkflowFromEvent workflow RPCs#1738
Conversation
There was a problem hiding this comment.
Pull request overview
Adds the remaining workflow RPCs to the .NET Workflow SDK surface area (and related models/converters), aligning the client with the workflow proto contract.
Changes:
- Exposes
ListInstanceIDsAsync,GetInstanceHistoryAsync, andRerunWorkflowFromEventAsyncacross the public client/interface layers. - Adds new client model types for instance paging, history events, and rerun options, plus proto-to-model conversion helpers.
- Adds logging and unit tests for the new RPCs (client forwarding + gRPC request/response behavior).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Dapr.Workflow.Test/DaprWorkflowClientTests.cs | Adds forwarding/validation tests for the new public client methods. |
| test/Dapr.Workflow.Test/Client/WorkflowGrpcClientTests.cs | Adds gRPC request/response mapping tests for the three new RPCs. |
| test/Dapr.Workflow.Test/Client/ProtoConvertersTests.cs | Adds tests for history event type conversion and event mapping. |
| src/Dapr.Workflow/Logging.cs | Adds structured log helpers for list/history/rerun operations. |
| src/Dapr.Workflow/IDaprWorkflowClient.cs | Extends the public interface with the three missing RPCs. |
| src/Dapr.Workflow/DaprWorkflowClient.cs | Implements new methods by delegating to the inner WorkflowClient. |
| src/Dapr.Workflow/Client/WorkflowInstancePage.cs | Introduces a page model for instance ID listing. |
| src/Dapr.Workflow/Client/WorkflowHistoryEvent.cs | Introduces a history event model + event type enum. |
| src/Dapr.Workflow/Client/RerunWorkflowFromEventOptions.cs | Introduces options for rerunning a workflow from an event. |
| src/Dapr.Workflow/Client/ProtoConverters.cs | Adds conversions for proto history events into SDK models. |
| src/Dapr.Workflow/Client/WorkflowGrpcClient.cs | Implements the three RPCs over gRPC, including request building and logging. |
| src/Dapr.Workflow/Client/WorkflowClient.cs | Extends the abstract base client with the three new RPCs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
JoshVanL
force-pushed
the
implement-workflow-rpcs
branch
from
c1ad8fa to
de0e78a
Compare
…vent workflow RPCs Add the three missing workflow RPCs that are defined in the proto but were not yet exposed through the SDK client. This includes the full stack: abstract base class, gRPC implementation, public interface, public client, model types, logging, and tests. Signed-off-by: joshvanl <me@joshvanl.dev>
JoshVanL
force-pushed
the
implement-workflow-rpcs
branch
from
de0e78a to
911cb4b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
WhitWaldo
left a comment
WhitWaldo
left a comment
There was a problem hiding this comment.
@JoshVanL Looks great - since it's validating operational functionality, could you generate an integration test or two within the Dapr.IntegrationTest.Workflow project within a separate class that just proves it works through and through in addition to the unit tests you've got and the few notes I made?
| /// <returns> | ||
| /// A page containing the instance IDs and an optional continuation token for the next page. | ||
| /// </returns> | ||
| public async Task<WorkflowInstancePage> ListInstanceIDsAsync( |
There was a problem hiding this comment.
Just as a convention, .NET uses "Ids" instead of "IDs" in method names - could you change this?
| /// <returns> | ||
| /// A page containing the instance IDs and an optional continuation token for the next page. | ||
| /// </returns> | ||
| Task<WorkflowInstancePage> ListInstanceIDsAsync( |
There was a problem hiding this comment.
Same thing - please change from "IDs" to "Ids"
| /// <param name="pageSize">The maximum number of instance IDs to return, or <c>null</c> for no limit.</param> | ||
| /// <param name="cancellationToken">A token that can be used to cancel the operation.</param> | ||
| /// <returns>A page of instance IDs and an optional continuation token for the next page.</returns> | ||
| public abstract Task<WorkflowInstancePage> ListInstanceIDsAsync( |
Signed-off-by: joshvanl <me@joshvanl.dev>
WhitWaldo
left a comment
WhitWaldo
left a comment
There was a problem hiding this comment.
Looks good - thank you for putting this together!
Add the three missing workflow RPCs that are defined in the proto but were not yet exposed through the SDK client. This includes the full stack: abstract base class, gRPC implementation, public interface, public client, model types, logging, and tests.