Skip to content
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.

Sign up for GitHub

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

Remove unecessary class constraint on startNew overload #2413

Open
wants to merge 3 commits into
base: v2.x
Choose a base branch
from
Open

Remove unecessary class constraint on startNew overload #2413

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ecd4ac1
Merge pull request #2376 from Azure/dev
davidmrdavid Feb 1, 2023
a27ac20
Merge pull request #2401 from Azure/dev
davidmrdavid Feb 24, 2023
5b05949
remove inconsistent class constraint
davidmrdavid Mar 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/WebJobs.Extensions.DurableTask/ContextInterfaces/IDurableOrchestrationClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,7 @@ Task<string> StartNewAsync(
/// </exception>
Task<string> StartNewAsync<T>(
string orchestratorFunctionName,
T input)
where T : class;
T input);
Copy link
Member

Choose a reason for hiding this comment

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

You may want to check the Git blame history for the previous change that introduced the class constraint. I vaguely remember that there was a reason this constraint was added, and it was to avoid some kind of breaking change related to multiple overloads of this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, that seems to be this PR. The PR itself is to allow for mocking of APIs during testing. Unfortunately, the T : class constraint is added in the first commit and not discussed explicitly on GitHub, so I'm not immediately sure why it was added.

I'll stare at the PR a bit longer, and see if there's any trace of a discussion about this internally;

Copy link
Member

Choose a reason for hiding this comment

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

The PR you're looking at only moved this method signature from one file to another. You'll have to go further back in time, looking at when this method signature was introduced originally in DurableContextExtensions.cs.

Copy link
Contributor Author

@davidmrdavid davidmrdavid Mar 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, you're right - GitHub hid the deletion of content in DurableContextExtensions.cs from me.
This seems to be real the PR that originates this constraint - #986, and then again on V2 here - #991

Context
Brandon describes this very case in the PR itself. In short - this constraint was introduced to allow an orchestration instanceID to be specified without that string being treated as the orchestration input itself.

Before that PR, if you had startNew("MyOrch", "myInstanceId") then "myInstanceId" would become the orchestrator input instead of the instanceID due to an overload of signature startNew(name: string, input: object) triggering.

So the fix was to introduce a generic overload and have that generic overload become what is required when a user wants to specify a string input but not the instanceID. So now, when a user wants to specify a string input but no instanceId, they need the "generic brackets <>and dostartNew("myOrch", "myInput"). and the standard startNew without "generic brackets always treats a second string parameter as the instanceId.

However:
I'm not immediately convinced that the T : class was required to begin with. I think it should suffice just to create the generic-type overload, but I'll need to try it out. I'll make a few local tests on my end to validate. In the meantime, I'd be curious to know if @brandonh-msft remembers why that was added :) . There may be some reason I'm overlooking.

Copy link
Member

@brandonh-msft brandonh-msft Mar 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yup the reason was so that, in the absence of the parameter, we could pass null (can't do that unless you type-constrain the generic to a class). BUT, in the new land of <Nullable> we can/should probably change this to just T? and remove the constraint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! That's a good idea. Enabling nullable analysis in this whole file (I understand the team is trying to do file-wide analyses when we enable nullable-checks) might take a bit of time, potentially making this a larger change. But I'll take a note about trying that out next :) .

Copy link
Member

@brandonh-msft brandonh-msft Mar 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

yeah, converting to nullable usually has cascading effects - understood. Nevertheless, that's why the type constraint was there :) Alternatively, perhaps we could use default instead of null? Up to y'all ;)


/// <summary>
/// Starts a new instance of the specified orchestrator function.
Expand Down