search

fsprojects / SQLProvider

A general F# SQL database erasing type provider, supporting LINQ queries, schema exploration, individuals, CRUD operations and much more besides.
https://fsprojects.github.io/SQLProvider
Other
580 stars 146 forks source link

OnConflict implementation for SQLite provider uses wrong feature of SQLite #796

Open gbtb opened 1 year ago

gbtb commented 1 year ago

Describe the bug I think that implementation of OnConflict uses wrong feature of SQLite. If you check the SQLite docs mentioned in https://fsprojects.github.io/SQLProvider/core/crud.html#OnConflict you'll see that "ON CONFLICT clause applies to UNIQUE, NOT NULL, CHECK, and PRIMARY KEY constraints". SQLite has another feature, specifically for UPSERTs - https://sqlite.org/lang_upsert.html . Docs for that feature mention that "UPSERT in SQLite follows the syntax established by PostgreSQL, with generalizations."

I don't know why this feature of SQLite was chosen back in the day. Even though ON CONFLICT was available in SQLite for decades and UPSERT was only added at 2018, when this feature was added to SQLProvider UPSERT had already been available. Nevertheless, I think it's quite confusing. Behavior of this feature for SQLite is different from other supported providers. I suppose we have two options:

  1. Fix SQLProvider implementation to use proper UPSERTS for SQLite, probably adding a configuration toggle somewhere to enable backward compatibility.
  2. Add a warning to the docs stating that usage of OnConflict feature for SQLite has a broader scope than one could expect.

In my case, I set OnConflict.Ignore to one of my tables, expecting it to only deal with primary key violations. In the meantime, I had an error in my app, one of the non-null column hasn't been filled, but SQLite hasn't complained about it and just silently refused to insert a row into a table. At the end, by debugging raw sql I was able to figure things out.

Thorium commented 1 year ago

I'd expect @piaste focus was PostgreSQL, and SQLite was just implemented to nicely being able to test this feature.

gbtb commented 1 year ago

I'd expect @piaste focus was PostgreSQL, and SQLite was just implemented to nicely being able to test this feature.

I understand :slightly_smiling_face: What do you think about suggested options? I might look into this as well, after fixing SubmitUpdates ordering issue.

Thorium commented 1 year ago

With old .NET Framework and 32 bit IDEs, SQLite used to be the easiest database, one file, no configuration and always working. But then came .NET Core and 64 bit vs 32 bit, and all kind of driver issues.

As I come from finance background, my default go to solution has always been transactions. I've never needed OnConflict.

I know people have different use cases and some think they'll be able to gain bit more performance boost and scalability by not using transaction, with then tolerating some level incorrect program state, or handling with issues manually. Which is fine by me too.

So sure, if you feel it helps your usability, if you make PR, I'll probably accept it.