Backwards compatibility for orchestrationClient

[davidmrdavid]

This PR addresses: #1290

There changes are few and simple (except I apparently pushed other people's commits, more on this later):

1. DurableClientAttribute is no longer a sealed class, so we can subclass it.

2. The declaration of a new dummy subclass of DurableClientAttribute named OrchestrationClientAttribute

3. A new binding rule in DurableTaskExtension.cs that explicitly accepts OrchestrationClientAttribute. This solution is less than ideal, because there's a handful of copy-pasted lines from the rule accepting DurableClientAttribute. Ideally, we would have a single line allowing us to select between the two attributes. If anyone knows how to do this, please let me know

So, while this works, I'm still looking for a solution with less repetition. I'm open to input!

Finally, in #1290 we discuss the possibility of raising warnings on the logs of func host start for JS users using orchestrationClient instead of durableClient. This PR assumes that this is change we would implement exclusively on the JS side of things. If this is not the case, please let me know.

[davidmrdavid]

Hmm, I see a few too many commits being pushed. Perhaps I pulled from the wrong source? Sorry about that, I'll look into fixing that next!

I suppose I should not have merged from dev and master? Oops

[cgillum]

Yeah, that's definitely weird. I typically see it if by branches aren't fully up-to-date. I don't yet really understand what causes it though.

One thing I would suggest is to exclude your changes under the docs directory. This is actually all the auto-generated documentation that existed before we migrated to docs.microsoft.com. We haven't gotten around to deleting this content from our repo. 😅

[cgillum]

Also, it looks like the AppVeyor build is failing due to the usage of an obsolete API.

DurableTaskExtension.cs(259,60): error CS0618: 'OrchestrationClientAttribute' is obsolete: '{OrchestrationClientAttribute is obsolete. Use DurableClientAttribute instead.' [C:\projects\azure-functions-durable-extension\src\WebJobs.Extensions.DurableTask\WebJobs.Extensions.DurableTask.csproj]

We should suppress that warning in the source code since we're doing this intentionally. I think Visual Studio should have some tool tips to help suppress warnings in source code.

[davidmrdavid]

After a lot of work trying to fix my git state, the diff appears correct now. Sorry for the clutter folks, I'll be more careful next time. My mistake was to accidentally pull from master and to repeatedly fail to perform a git diff from local to remote.

[cgillum]

Glad to see everything is working! :)

However, I'm still a little concerned about the Git history represented in this PR. If you're certain that your local branch is good, would you mind locally squashing your changes and doing a force push to make sure that GitHub also thinks things are looking clean?

I imaging the following commands could work for you assuming your branch is in a sane state. They will do the following:

1. Rebase your commits on top of the latest from dev to ensure maximum cleanliness
2. Squash your changes into a single commit
3. Overwrite the history of this PR to match the new history of your local branch

git checkout dev
git pull
git checkout djusto/backwards_compatibility
git rebase dev

git reset --soft 4b81619ddd31043f63a1e351e33367c8de65f368
git commit -m "Backwards compatibility for orchestrationClient"

git push --force

[davidmrdavid]

@cgillum , thanks for the helpful commands and their explanation. I spent a little bit of time today also reading up on how to get out of these bad git states. Anyhow, it should be looking better now :)

[cgillum]

Excellent! This looks perfect.

[amdeel]

LGTM as long as it's tested. :)

I have one question, though.

[ConnorMcMahon]

So historically I have had issues with binding rules and inheritence, but it was a slightly different context, so I think you are OK here.

In that case, I had a GraphTokenAttribute that bound to a string, and then ExcelAttribute which was a subclass of GraphTokenAttribute that bound to a completely different set of type. Based on the order of the rules I configured, it would sometimes try binding ExcelAttribute as GraphTokenAttribute, and error out as the signature did not match.

Since DurableClientAttribute and OrchestrationClientAttribute should bind the exact same way, I think you are safe here. However, as Amanda pointed out, I think that means you can get rid of the backwardsCompRule entirely.

As for testing, I would try to test the following matrix:

Test both DurableClientAttribute and OrchestrationClientAttribute.

Test both "durableClient" and "orchestrationClient" as "type" values in a function.json based function app.

If you test those 4 scenarios, and they all work as expected, then I give this the green light!

[ConnorMcMahon]

Given the new details I learned, I would say this is ready to merge once you have tested the scenarios I outlined in my previous review!

[davidmrdavid]

My latest commit(s) adds rather simple unit tests as a sanity check that my changes work. They are slight variants on already existing tests for the DurableClientAttribute.

I also manually tested the usage of orchestrationClient as a binding in JS and C#, and the samples seem to continue working. All in all, I think this is ready.

I'll wait for another 'lgtm' and them I'll squash-and-merge. Thanks folks!

[ConnorMcMahon]

Still looks good to merge for me. One small nit, but not a blocker.