microsoft / durabletask-mssql

Microsoft SQL storage provider for Durable Functions and the Durable Task Framework
MIT License
87 stars 32 forks source link

Bug fixup #111

Closed matei-dorian closed 2 years ago

matei-dorian commented 2 years ago

I was working on a project using the Durable Task Framework and I was encountering some unexpected errors when using Microsoft SQL Provider for it. Upon some investigations I found an issue with a script in here, fixed it, and I would also like to open a PR for that, but I do not have the permission to push my changes in a separate branch.

matei-dorian commented 2 years ago

The issue is that there is a runtime error when calling the CreateIfNotExistAsync() method while working on a case sensitive database. The error message says that @InstanceID is not set or defined. The problem is that in some of the stored procedures InstanceID is sometimes used as InstanceId, and in a case sensitive database these are two different variables

cgillum commented 2 years ago

Hi @matei-dorian, the way to submit a PR as an external user is to first create a fork of this repo in GitHub, apply your changes to your fork, and then you can submit a PR from your fork back to this repo, which we can then approve and merge.

We definitely want to support case sensitive databases (it's the default setup, so I'm surprised to hear that we missed a case) so thank you for pointing this out! It would be great if we could get a PR from you for this since you've already identified the fix!

cgillum commented 2 years ago

Hi @matei-dorian, thanks for your PR, which fixes the issue. Indeed, our tests are not catching this issue, which surprised me. Can you tell me more about how you configured your database to be case sensitive? I thought that creating SQL databases with the Latin1_General_100_BIN2_UTF8 collation would do this, but apparently I was wrong.

matei-dorian commented 2 years ago

Hey! When I discovered the problem I was using a database with the SQL_Latin1_General_CP1_CS_AS collation.

cgillum commented 2 years ago

Thanks @matei-dorian. I opened a new issue to track adding a new automated test to catch these kinds of accidental regressions before they happen.