Migrate from System.Data.SQLite to Microsoft.Data.Sqlite 8.0.0#2699
Migrate from System.Data.SQLite to Microsoft.Data.Sqlite 8.0.0#2699msevestre merged 14 commits intoOpen-Systems-Pharmacology:V13from Felixmil:Microsoft.Data.Sqlite
Conversation
- Replace System.Data.SQLite package references with Microsoft.Data.Sqlite 8.0.0 - Update SessionFactoryProvider to use Microsoft.Data.Sqlite with NHibernate - Update ConnectionProvider and SQLiteProjectCommandExecuter to use SqliteConnection - Add SQLite native library configuration (PropertyGroups + PostBuild targets) to test and executable projects to ensure SQLite.Interop.dll is copied on Windows - Update test projects to reference Microsoft.Data.Sqlite
…brary support Microsoft.Data.Sqlite uses SQLitePCLRaw which provides e_sqlite3.dll automatically via SQLitePCLRaw.bundle_e_sqlite3. The previous SQLite.Interop.dll from System.Data.SQLite is no longer needed.
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the SQLite database integration from System.Data.SQLite.Core to Microsoft.Data.Sqlite. The change modernizes the database provider and removes legacy build configurations.
- Replaced
System.Data.SQLite.Corepackage withMicrosoft.Data.SqliteandSQLitePCLRaw.bundle_e_sqlite3 - Updated all SQLite connection instantiations from
SQLiteConnectiontoSqliteConnection - Added new
SessionFactoryProviderimplementation for NHibernate configuration
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| OSPSuite.Starter.csproj | Updated package references and removed PostBuild SQLite.Interop.dll copy target |
| OSPSuite.R.Performance.csproj | Updated package references and removed PostBuild SQLite.Interop.dll copy target |
| OSPSuite.Infrastructure.Tests.csproj | Updated package references and removed PostBuild SQLite.Interop.dll copy target |
| OSPSuite.Infrastructure.Serialization.csproj | Replaced System.Data.SQLite.Core with Microsoft.Data.Sqlite |
| SessionFactoryProvider.cs | New implementation for NHibernate session factory configuration with Microsoft.Data.Sqlite |
| SQLiteProjectCommandExecuter.cs | Updated connection type from SQLiteConnection to SqliteConnection |
| IConnectionProvider.cs | Simplified connection string and updated to SqliteConnection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/OSPSuite.Infrastructure.Serialization/Services/SessionFactoryProvider.cs
Outdated
Show resolved
Hide resolved
src/OSPSuite.Infrastructure.Serialization/Services/SessionFactoryProvider.cs
Outdated
Show resolved
Hide resolved
| { | ||
| var connectionString = $"Data Source={databasePath};Version=3;New=True;Compress=True;foreign keys=True"; | ||
| var cn = new SQLiteConnection(connectionString); | ||
| var connectionString = $"Data Source={databasePath}"; |
There was a problem hiding this comment.
The simplified connection string removes important SQLite configuration parameters that were present in the old implementation: 'Version=3;New=True;Compress=True;foreign keys=True'. Most critically, 'foreign keys=True' enforces referential integrity constraints. Consider adding back essential parameters, particularly foreign key enforcement using: var connectionString = $\"Data Source={databasePath};Foreign Keys=True\";
| var connectionString = $"Data Source={databasePath}"; | |
| var connectionString = $"Data Source={databasePath};Foreign Keys=True"; |
There was a problem hiding this comment.
@Felixmil Not sure about Version and New arguments, but ForeignKeys and Compress must be set.
There was a problem hiding this comment.
OK, I don't see an option for compress, only the one for foreign keys.
https://learn.microsoft.com/en-us/dotnet/standard/data/sqlite/connection-strings
Regarding compress: needs to be investigated further (maybe it's always true, or can be set later via a property...)
We should create a separate issue for this.
src/OSPSuite.Infrastructure.Serialization/Services/SessionFactoryProvider.cs
Outdated
Show resolved
Hide resolved
|
Changing the target branch to V13 will impact how the assemblies are delivered from PKSim.R (and since V13 MoBi.R). Not in this PR, but in the downstream ones. |
Then so be it. |
Yes, I was going to make this comment that we should only consider it for V13, but also wanted to note for @Felixmil that it will require some rework before it's ready to merge all the way |
…onnections - Modified ConnectionProvider, SessionFactoryProvider, and SQLiteProjectCommandExecuter to append 'Foreign Keys=True' to the connection strings.
tests/OSPSuite.Infrastructure.Tests/OSPSuite.Infrastructure.Tests.csproj
Outdated
Show resolved
Hide resolved
src/OSPSuite.Infrastructure.Serialization/Services/SessionFactoryProvider.cs
Outdated
Show resolved
Hide resolved
tests/OSPSuite.Infrastructure.Tests/OSPSuite.Infrastructure.Tests.csproj
Outdated
Show resolved
Hide resolved
msevestre
left a comment
There was a problem hiding this comment.
SessionFactoryProvider
@rwmcintosh
why is this removeD? this is what Nhibernate used to connect to the DB
Or is this defined in each application
oh this was added with this PR lol. Ok maybe not used then |
src/OSPSuite.Infrastructure.Serialization/Journal/IConnectionProvider.cs
Outdated
Show resolved
Hide resolved
src/OSPSuite.Infrastructure.Serialization/Journal/IConnectionProvider.cs
Outdated
Show resolved
Hide resolved
src/OSPSuite.Infrastructure.Serialization/Journal/IConnectionProvider.cs
Outdated
Show resolved
Hide resolved
msevestre
left a comment
There was a problem hiding this comment.
is probabvly not required everywhere since we are not copying anyhting using GeneratePathProperty =true but I am not sure
src/OSPSuite.Infrastructure.Serialization/Services/SQLiteProjectCommandExecuter.cs
Outdated
Show resolved
Hide resolved
tests/OSPSuite.Infrastructure.Tests/OSPSuite.Infrastructure.Tests.csproj
Outdated
Show resolved
Hide resolved
|
Ok @Yuri05 @msevestre @Felixmil @benjaperez1983 if all the comments are satisfied, I suggest you get familiar with the changes in PK-Sim and MoBi PR's as well. Those won't build until we get a new core build, but it's a good idea to understand the whole scope of the changes in those as well. All the tests pass on my local builds of PK-Sim and MoBi and I can open projects normally. @Felixmil has an updated set of dll's that he will test with OSPSuite-R (that's the last app in the line to get these updates) |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Closes #2701
Sister PRs:
Summary
Migrated from
System.Data.SQLitetoMicrosoft.Data.Sqlite8.0.0 across the codebase.Key Changes:
System.Data.SQLitepackage references withMicrosoft.Data.Sqlite8.0.0SessionFactoryProviderto use Microsoft.Data.Sqlite with NHibernateConnectionProviderandSQLiteProjectCommandExecuterto useSqliteConnectionType of change
Please mark relevant options with an
xin the brackets.How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Reviewer checklist
Mark everything that needs to be checked before merging the PR.
This change requires a documentation updateabove is selectedScreenshots (if appropriate):
Questions (if appropriate):