Add OnConflict handling for MySQL.

[Choc13]

Proposed Changes

I'd like to propose adding support for OnConflict in the MySQL provider. I believe it can be handled via the ON DUPLICATE KEY UPDATE clause that is available in MySQL.

Types of changes

• [ ] Bugfix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

• [x] Build and tests pass locally
• [ ] I have added tests that prove my fix is effective or that my feature works (if appropriate)
• [x] I have added necessary documentation (if appropriate)

Further comments

I've not written any tests yet as I wanted to check that you open to accepting this new feature first and to also ask a few questions about the implementation, which I will point out in comments on the changes.

[Thorium]

OnConflict is definitely not my favourite thing (I prefer transactions) but yes PRs are accepted!

[Thorium]

Is this work in progress, or waiting for merge?

[Choc13]

Hi, sorry I was busy this last week. It sounds like you're happy to accept the MR, so let me see if I can add some tests if possible.

The build seems to have failed for a transient reason that I didn't understand. Could you advise on how to fix that? Thanks 👍

[Choc13]

OnConflict is definitely not my favourite thing (I prefer transactions) but yes PRs are accepted!

Curious what you meant by "I prefer transactions" here? How would you typically handle upserting rows? Fetching everything that matches and then doing an update on that entity if it already exists or creating a new one if it doesn't?

[Thorium]

Off-topic but as I do financial systems, I try to avoid updates and upserts as it might lose some important historical data. I'd rather just insert a new row and have some kind of order/timestamp to indicate what is the latest and currently valid row. That way I can ask how the system was looking back in time.

[Choc13]

I'm unable to get the build or tests running locally and the AppVeyor build also seems to be failing to setup the SQLServer instance. I'm not too sure how to fix that issue. I can try and investigate, but if you were able to point me in the right direction that would be great. Thanks.

[Thorium]

I'm not very familiar with the test-projects, but definitely as this is MySQL only the MySQL related errors should be relevant and a "System.Data.SqlClient.SqlException (0x80131904): A network-related or instance-specific " is not relevant for this PR.

[Choc13]

Off-topic but as I do financial systems, I try to avoid updates and upserts as it might lose some important historical data. I'd rather just insert a new row and have some kind of order/timestamp to indicate what is the latest and currently valid row. That way I can ask how the system was looking back in time.

Ah yeah, I do that in most places, but in a few places an upsert is all that is needed.

[Choc13]

OK, do you recommend that I comment out the SQLServer tests for the time being then, make sure it passes without them and then uncomment them before merging?

[Thorium]

OK, do you recommend that I comment out the SQLServer tests for the time being then, make sure it passes without them and then uncomment them before merging?

That's fine too, but why you cannot get the build working locally? I'd be keen to update the docs or something to help people to get local build working.

Edit: Just skip the non-mysql tests locally if it's expecting some database you don't have. But it should run the test locally via SQLite. Is there problem with that?

[Choc13]

Ah, it was because I didn't have mono installed. It's getting further now.

[Choc13]

I know you said you don't know much about the test project, but do you know if the scipts/*.fsx test files are supposed to run when invoking dotnet fake run build.fsx -t RunTests. I can't seem to get them to trigger. I see some references to setting up a Postgres instance in build.fsx, but there is no mention of setting up a MySQL instance in there. There is a docker file and some scripts that look like they would initialise a MySQL instance, but I can't seem them being invoked anywhere in the build.

Is the intention that those test scripts are setup and run manually by starting the docker container and running the fsx file?

[Thorium]

The basic test-fsproj executes only .fs files.

The .fsx files are rather for testing manually. They typically need some drivers or connection string settings tweaked first.

I know people have set-up dockers to ease testing with different databases. I'm not so familiar with those. I'd expect there are some .fs files with confitional compilation to execute that part on CI only. But I don't know which databases are included.

SQLite tests run without docker.

[Choc13]

OK, I got the MySQL tests running locally with Docker and tested the conflict behaviour and all seems good. I've not committed the tests as they aren't run automatically anyway and there were some other issues with that file anyway. I can add them back in if you want though.

[Choc13]

I'm happy for this to be merged now if you are.

[Thorium]

Thanks!

[Thorium]

Umm, what is this DoNothing: columnNamesWithValues |> Array.map(fun (c,_) -> sprintf "`%s`=`%s`" c c

Also, if you have multiple columns, then are you missing spaces in the string?

[Choc13]

It is what I mentioned above. It generates something like the following ON DUPLICATE KEY UPDATE col1 = col1, col2 = col2. Which means just set the column to its existing value already in the table. The String.join takes care of comma separating each col=col so I don't think spaces are necessary. I tested this case locally and also inspected the SQL it generated and it looked fine, even with multiple columns in the query.