Skip to content

workflow: apply PR #1825 review-thread fixes for propagated history lookups and conversion performance#1828

Closed
Copilot wants to merge 9 commits into
masterfrom
copilot/fix-code-for-review-comments
Closed

workflow: apply PR #1825 review-thread fixes for propagated history lookups and conversion performance#1828
Copilot wants to merge 9 commits into
masterfrom
copilot/fix-code-for-review-comments

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 22, 2026 •
edited
Loading

Description

Applied the actionable comments from review thread #pullrequestreview-4344832175 with minimal scope: propagated history lookup APIs now match SDK case-insensitive name/app-id behavior, and raw history conversion avoids repeated full-list scans. The changes stay focused on the reviewed files and preserve the existing public API shape.

  • Case-insensitive propagated-history queries

    • Updated PropagatedHistory and PropagatedHistoryEntry lookup methods to use OrdinalIgnoreCase for app/workflow/activity/child-workflow name matching.
    • Updated app-id de-duplication in GetAppIds() to be case-insensitive.

  • History conversion complexity reduction

    • Refactored WorkflowOrchestrationContext.ConvertChunk to pre-index completion/failure events by scheduled ID and resolve activities/child workflows with O(1) lookups instead of repeated O(n) scans.

  • Targeted test updates

    • Adjusted workflow propagation unit tests to assert case-insensitive behavior.
    • Updated a test summary XML comment to reflect current type semantics.
return _workflows
    .Where(w => string.Equals(w.Name, name, StringComparison.OrdinalIgnoreCase))
    .ToList();

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #[issue number]

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

nelson-parente and others added 8 commits May 22, 2026 11:03
@nelson-parente
feat(workflow): align history propagation API with go-sdk
b3f6b23 
Cassie (durabletask-go author) flagged the .NET surface for cross-SDK
divergence post-merge of dotnet-sdk#1802 / #1818. This rewrites the
public history-propagation API to match the go-sdk shape — same one the
python-sdk just adopted (python-sdk#1047). Issue dotnet-sdk#1801 was
closed before her review; this PR delivers what the issue originally
described.

Three concrete gaps closed:

1. Activity-level opt-in (was missing entirely)
   - PropagationScope moved from ChildWorkflowTaskOptions to base
     WorkflowTaskOptions; ChildWorkflowTaskOptions inherits it.
   - WithHistoryPropagation() extension method added on the base record.
   - scheduleTaskAction.HistoryPropagationScope is now wired in
     WorkflowOrchestrationContext.CallActivityInternalAsync so activities
     can opt into propagation, matching CallChildWorkflowInternalAsync.
   - Without this, the Go SDK's reference example (SettlePayment activity
     using PropagateOwnHistory) literally cannot be ported to .NET.

2. Read API rewritten as high-level resolvers (was lossy FilterBy* + a
   PropagatedHistoryEvent record that dropped input/output/failure
   payloads)
   - PropagatedHistory.FilterByAppId/InstanceId/WorkflowName removed.
   - PropagatedHistory now exposes GetWorkflows(), GetWorkflowsByName(),
     GetLastWorkflowByName(), GetAppIds(), GetWorkflowsByAppId(),
     GetWorkflowsByInstanceId().
   - New WorkflowResult class with InstanceId/AppId/Name plus
     GetActivitiesByName(), GetLastActivityByName(),
     GetChildWorkflowsByName(), GetLastChildWorkflowByName() — mirrors
     durabletask-go's GetLastWorkflowByName / GetLastActivityByName /
     GetLastChildWorkflowByName renames from durabletask-go#105.
   - New ActivityResult record carries Name, Started, Completed, Failed,
     Input, Output, FailureDetails — matching the Go/Python equivalents
     so chain-of-custody patterns line up.
   - New ChildWorkflowResult record with the equivalent shape.

3. Event payload preserved internally (was discarded by ConvertChunk)
   - ConvertChunk in WorkflowOrchestrationContext now parses raw events,
     walks them to resolve TaskScheduled <-> TaskCompleted/Failed and
     ChildWorkflowInstanceCreated <-> ChildWorkflowInstanceCompleted/
     Failed by scheduleId, and produces fully-populated ActivityResult /
     ChildWorkflowResult instances. SDK retries reuse TaskExecutionId so
     matching is on scheduleId (matching Go/Python semantics).
   - Public API does not leak the proto HistoryEvent type — resolution
     happens at construction time inside Dapr.Workflow.

Additional surface additions:

- PropagationNotFoundException for missing-name lookups (mirrors
  Python's PropagationNotFoundError / Go's error returns).
- Static WorkflowHistory.PropagateLineage() / PropagateOwnHistory()
  factory helpers for go-sdk call-site parity.

Removed (clean break — 1.18 unreleased): PropagatedHistoryEntry,
PropagatedHistoryEvent, HistoryEventKind, FilterByAppId,
FilterByInstanceId, FilterByWorkflowName.

Tests:

- WorkflowHistoryPropagationTests.cs rewritten end-to-end to cover the
  new resolvers, query helpers, factory helpers, activity-level scope
  wiring, and child-workflow-level scope wiring.
- HistoryPropagationWorkflowTests.cs (integration) updated to use
  GetWorkflows().Count in place of Entries.Count.

Refs: #1801, dapr/durabletask-go#105, dapr/go-sdk#823,
dapr/python-sdk#1047

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>

Co-authored-by: nelson-parente <20144601+nelson-parente@users.noreply.github.com>
@nelson-parente
fix(workflow): address code-review feedback on history-propagation al…
b4aebad 
…ignment

- Document the `new`-hiding contract on ChildWorkflowTaskOptions
  .WithHistoryPropagation and add a regression test that asserts the
  returned type is ChildWorkflowTaskOptions (not the base record), so
  InstanceId survives the with-expression.
- Add the standard `()`, `(string)`, and `(string, Exception)` constructors
  on PropagationNotFoundException so callers can wrap inner exceptions.
- Alias StringValue alongside the existing Timestamp alias in
  WorkflowOrchestrationContext so the propagation helper signature stays
  consistent with the rest of the file.

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>

Co-authored-by: nelson-parente <20144601+nelson-parente@users.noreply.github.com>
@nelson-parente
test(workflow): clarify chunk-order test variable names
4b508d6 
Renames the test fixtures in GetPropagatedHistory_PreservesChunkOrder so the
variable order matches the documented oldest-first chunk ordering (index 0 is
the oldest ancestor, the last chunk is the immediate parent). No behavior change.

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>

Co-authored-by: nelson-parente <20144601+nelson-parente@users.noreply.github.com>
@nelson-parente
fix(workflow): pass StringValue-wrapped fields directly as strings
d49ad8d 
protoc unwraps google.protobuf.StringValue to a plain string in the
generated C# (only the wire codec uses the wrapper). The
StringValueOrNull(StringValue?) helper added in this branch expected
the wrapper type, breaking the build with CS1503 at the three call
sites in ResolveActivity / ResolveChildWorkflow. Drop the helper and
pass the generated string fields straight through — they are already
nullable at runtime and ActivityResult/ChildWorkflowResult accept
string? for Input/Output.

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>

Co-authored-by: nelson-parente <20144601+nelson-parente@users.noreply.github.com>
@nelson-parente
test(workflow): assign string directly to wrapper-typed fields
312b7e0 
Same StringValue mismatch as the production fix — protoc-generated
properties for google.protobuf.StringValue fields are plain string,
not the wrapper. Drop the new StringValue { Value = ... } wrappers
in the test helpers.

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>

Co-authored-by: nelson-parente <20144601+nelson-parente@users.noreply.github.com>
@nelson-parente
refactor(workflow): rename propagation types and adopt TryGet pattern
bf28b54 
Addresses Whit's review on #1825:

- Rename ActivityResult -> PropagatedHistoryActivityResult
- Rename ChildWorkflowResult -> PropagatedHistoryChildWorkflowResult
- Rename WorkflowResult -> PropagatedHistoryEntry (primary constructor)
- Drop WorkflowHistory static class; callers pass HistoryPropagationScope directly
- Switch GetLast*ByName to TryGet*ByName + drop PropagationNotFoundException
- Drop chunk/chain terminology from public XML docs
- Surface malformed propagated event bytes via InvalidProtocolBufferException
  instead of silently skipping
- Update unit tests to new names and TryGet asserts

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>

Co-authored-by: nelson-parente <20144601+nelson-parente@users.noreply.github.com>
@nelson-parente
test(workflow): rename propagation test cases to match renamed types
bfd4a57 
Test names previously embedded the old type names (ActivityResult,
ChildWorkflowResult); rename to the more general Activity_/ChildWorkflow_
prefix to avoid confusion with the renamed public types.

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>

Co-authored-by: nelson-parente <20144601+nelson-parente@users.noreply.github.com>
@nelson-parente
fix(workflow): address PR #1825 copilot review comments
e30f507 
Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/b8e98abe-cc48-40ba-ac78-f1cd82018a30

Co-authored-by: nelson-parente <20144601+nelson-parente@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix code based on review comments workflow: apply PR #1825 review-thread fixes for propagated history lookups and conversion performance May 22, 2026
Copilot AI requested a review from nelson-parente May 22, 2026 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nelson-parente nelson-parente Awaiting requested review from nelson-parente

Assignees

@nelson-parente nelson-parente

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nelson-parente