Skip to content

Add New Property Properties to TaskOrchestrationContext #415

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

Merged
merged 12 commits into from
May 2, 2025

Conversation

nytian
Copy link
Contributor

@nytian nytian commented Apr 22, 2025

This PR adds a new property IDictionay<string, object?> Properties at TaskOrchestrationContext, which allows DurableTaskOptions from WebJobs.Extensions.DurableTask can be passed through Microsoft.DurableTask.Protobuf.OrchestratorRequest.

Related PR:

@nytian nytian changed the title Add Properties to TaskOrchestrationContext Add New Property Properties to TaskOrchestrationContext Apr 22, 2025
@nytian nytian marked this pull request as ready for review April 22, 2025 17:45
@nytian nytian requested a review from jviau April 24, 2025 20:41
throw new ArgumentNullException(nameof(properties));
}

this.Properties = properties.ToDictionary(pair => pair.Key, pair => pair.Value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converting from IEnumerable<KVP> to a dictionary is dangerous because IEnumerable<KVP> allows duplicate keys whereas dictionary does not. We should change the properties parameter to be a dictionary instead of IEnumerable<KVP> to avoid these kinds of problems.

Copy link
Member

@jviau jviau Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is safe if you use a foreach loop and set the key each time, so last key wins. My preference for IEnumerable is only to follow the "take in least derived, return most derived" principal. But I am not unmoving in that. If you disagree and want IDictionary, that is fine.

Copy link
Contributor Author

@nytian nytian Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Chris and Jacob. I updated this to be Dictionary. Because to write a foreach loop, I have to either grant Properties private set, or adding a new field to store this and then pass the value to Properties. Just feel like using Dictionary directly here seems like a more straightforward and cleaner way in this case. Let me know if you prefer others.

Copy link
Member

@jviau jviau Apr 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay with going back to dictionary, but I don't see why you need a private set. The following is valid without any setter on Properties:

this.Properties = new();
foreach ((string key, object? value) in properties)
{
    this.Properties[key] = value;
}

or if this.Properties is IReadOnlyDictionary<string, object?>, then you can do this:

Dictionary<string, object?> props = new();
foreach ((string key, object? value) in properties)
{
    props[key] = value;
}

this.Properties = props;

@nytian nytian requested a review from cgillum April 25, 2025 21:07
@nytian nytian requested a review from jviau April 29, 2025 19:49
Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of minor nits

@nytian nytian merged commit 78a46df into main May 2, 2025
4 checks passed
@nytian nytian deleted the nytian/context-add-config branch May 2, 2025 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants