Remove unecessary class constraint on startNew overload

[davidmrdavid]

Resolves: #1814
Related: #2348

Issue describing the changes in this PR

As highlighted in #1814, there's 2 generic overloads for startNew:

First variant:

azure-functions-durable-extension/src/WebJobs.Extensions.DurableTask/ContextInterfaces/IDurableOrchestrationClient.cs

Lines 142 to 146 in ee2fba7

	Task<string> StartNewAsync<T>(
	string orchestratorFunctionName,
	T input)
	where T : class;

Second variant:

azure-functions-durable-extension/src/WebJobs.Extensions.DurableTask/ContextInterfaces/IDurableOrchestrationClient.cs

Line 163 in ee2fba7

Task<string> StartNewAsync<T>(string orchestratorFunctionName, string instanceId, T input);

You'll notice the first variant puts the constraint T : class whereas the first variant does not. This creates inconsistent type constraints on the same API. In particular, you may pass in a Tuple on the second variant but not in the first, as Tuples are structs not classes.

This PR proposes removing the T : class constraint altogether. I'm using this PR as a conversation starter, given that we have not discussed this change yet.

Pull request checklist

• My changes do not require documentation changes
  • Otherwise: Documentation PR is ready to merge and referenced in pending_docs.md
• My changes should not be added to the release notes for the next release
  • Otherwise: I've added my notes to release_notes.md
• My changes do not need to be backported to a previous version
  • Otherwise: Backport tracked by issue/PR #issue_or_pr
• I have added all required tests (Unit tests, E2E tests)
• My changes do not require any extra work to be leveraged by OutOfProc SDKs
  • Otherwise: That work is being tracked here: #issue_or_pr_in_each_sdk
• My changes do not change the version of the WebJobs.Extensions.DurableTask package
  • Otherwise: major or minor version updates are reflected in /src/Worker.Extensions.DurableTask/AssemblyInfo.cs
• My changes do not add EventIds to our EventSource logs
  • Otherwise: Ensure the EventIds are within the supported range in our existing Windows infrastructure. You may validate this with a deployed app's telemetry. You may also extend the range by completing a PR such as this one.

Still pending:

Add to the release_notes.md. I would like to first get a thumbs up on the change, and see that the CI passes.

[cgillum]

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.

[davidmrdavid]

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;

[cgillum]

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.

[davidmrdavid]

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.

[brandonh-msft]

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.

[davidmrdavid]

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 :) .

[brandonh-msft]

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 ;)