Fall back to non-RETURNING updates with old Sqlite #28911
Conversation
|
@bricelam could you take a look at the test failure? CSharpRuntimeModelCodeGeneratorTest.Sqlite fails because of: (because only the core package is referenced and we now call SQLitePCL.raw.sqlite3_libversion_number). Trying to reference EFCore.Sqlite from EFCore.Design.Tests fails with: |
|
Maybe a quirk switch in addition to the version test? |
|
A quirk is easy enough to do, but any particular reason in mind? We usually do quirks for patches, and I'm not sure this Sqlite change is particularly risky. |
|
@roji Only if some depending on winsqlite would like a predictable behaviour (if it ever gets updated) ? |
|
I think we can add quirk to allow user to opt-in in particular behavior for whatever reason they want to differ in case our check is not accurate enough for them. Not necessarily the change is risky, just a user configurable behavior. We should discuss with team though as we have never added such switch in framework before. (Patch quirks only live for that major release). |
@ErikEJ hopefully the visible end-user behavior is predictable and the same across both versions, with only the SQL (and performance) varying; otherwise that would mean there's some sort of bug/issue 😅 But sure, we'll discuss and I have no problem adding this if needed. |
There was a problem hiding this comment.
Are we aware of a situation where different versions of Sqlite could be used?
No. It's very difficult and error-prone to load multiple versions of SQLite into a process.
add a quirk to allow user to opt-in in particular behavior
No need for a quirk. Just use ReplaceService to get the behavior you want.
| // Detect which version we're using, and fall back to the older INSERT/UPDATE+SELECT behavior on legacy versions. | ||
| var dependencies = sp.GetRequiredService<UpdateSqlGeneratorDependencies>(); | ||
|
|
||
| return SQLitePCL.raw.sqlite3_libversion_number() < 3035000 |
There was a problem hiding this comment.
Hmm, configuring services is a bit early for calling this. Use new SqliteConnection().ServerVersion instead to ensure the SQLitePCLRaw provider is set.
There was a problem hiding this comment.
Thanks, that makes it look better - but CSharpRuntimeModelCodeGeneratorTest.Sqlite is still failing in the same way for me...
There was a problem hiding this comment.
Ah, so that wasn't the reason it was failing. It really is just because that project doesn't reference a provider.
Hmm, I really hate how early in the stack (configuring services) we need to make this check. Is it possible to push this decision down into the actual service?
There was a problem hiding this comment.
You can revert the change to use SqliteConnection if you want.
If we can't defer the check until later, you can just reference SQLitePCLRaw.bundle_e_sqlite3 from EFCore.Design.Tests to fix the failure.
There was a problem hiding this comment.
No I think using SqliteConnection is better.
I think it's nice to have the behavior switch happen at the wiring up of services, and among other things it makes it very simple to swap the services around as you suggested... Referencing SQLitePCLRaw.bundle_e_sqlite3 seems simple enough (and we do it in many other test projects), so I'll do that.
There was a problem hiding this comment.
BTW will centralize the version to SqlitePCLRawVersion in Versions.props
There was a problem hiding this comment.
BTW will centralize the version to SqlitePCLRawVersion in Versions.props
Not sure Dependabot will like that, but I’ll revert it if it has trouble.
There was a problem hiding this comment.
Oh OK, sorry about that.
BTW hopefully we'll move to nuget's centralized package management at some point, it works well with dependabot.
There was a problem hiding this comment.
Yes, I filed an issue on Arcade the day it finally released.
@bricelam I ended up using SQLitePCL.raw.sqlite3_libversion_number() to find out which Sqlite version we're using, since the UpdateSqlGenerator is set up in a singleton context and it's a bit of a hassle to get a SqliteConnection etc. Are we aware of a situation where different versions of Sqlite could be used based on the connection string (seems unlikely)? If so and you think it's important, I can look into using SqliteConnection.ServerVersion instead.
Test-wise, I've verified manually that the old SQL gets properly generated on Windows (where the version is 3034001, using SQLitePCLRaw.bundle_winsqlite3); there's also a new legacy test suite which I've verified artifically by hard-coding an old version. But since all our tests pass in CI, I'm assuming we always run with the bundled version which is new; we could do the extra legwork to e.g. use the system Windows version for tests, but that seems low-value to me...
Fixes #28776