-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding ability to configure extensions for Sqlite #428
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 9 changed files in this pull request and generated 2 comments.
Files not reviewed (4)
- src/CommunityToolkit.Aspire.Hosting.Sqlite/PublicAPI.Unshipped.txt: Language not supported
- src/CommunityToolkit.Aspire.Microsoft.Data.Sqlite/PublicAPI.Unshipped.txt: Language not supported
- src/CommunityToolkit.Aspire.Hosting.Sqlite/SqliteResource.cs: Evaluated as low risk
- src/CommunityToolkit.Aspire.Hosting.Sqlite/SqliteResourceBuilderExtensions.cs: Evaluated as low risk
src/CommunityToolkit.Aspire.Microsoft.Data.Sqlite/SqliteConnectionSettings.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Aspire.Microsoft.Data.Sqlite/AspireSqliteExtensions.cs
Outdated
Show resolved
Hide resolved
ca34e67
to
35ddfec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaronpowell, I understand that writing functional tests for this situation is challenging, but I believe we need to proceed with it. For now, we could create tests that run only on Windows, but we should definitely include some functional tests. Additionally, we might want to update the example project to utilize the one extension, which I could then attempt to test locally.
src/CommunityToolkit.Aspire.Microsoft.Data.Sqlite/SqliteConnectionSettings.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Aspire.Hosting.Sqlite/SqliteResourceBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
I'll have to think about a functional test because yes, it needs to run on Windows only (which means I can't actually run the tests myself). |
… feature as experimental
…erExtensions.cs Co-authored-by: Alireza Baloochi <[email protected]>
48c44eb
to
1d0398e
Compare
@Alirexaa I've created a test that runs on Windows only to verify the extension loading, but as you can see, it doesn't pass on GitHub Actions. Running locally, the test passes: Can you run the test on your machine and see if it passes? I'm not sure why it's failing, looking at the logging I've added it's finding the right location to load the extension from (it comes from |
I'll try. |
Apparently you can load extensions from an absolute path, so I'll try that instead as it might simplify the code and maybe fix our issue. |
Going to put this back to draft, as the package that I'm using seems to be really out of date and it's not meant to be used the way that I was thinking. I actually am more interested in https://github.com/asg017/sqlite-vec but we depend on asg017/sqlite-vec#193 |
@aaronpowell, test passed locally. I think there is a limitation in the runner. |
That's my guess. Are you ok if we merge without tests, as I'm going to be looking into the sqlite-vec plugin and shipping it via NuGet which I'd then migrate to instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep this test and skip it for now. Overall, it looks good to me.
Minimum allowed line rate is |
@aaronpowell I believe EnsureLoadableFromNuGet won't be needed after dotnet/efcore#35617 is merged |
Nice - I'll keep an eye on that being released. |
Adding the ability to specify SQLite extensions that you want to load on the database. Extensions can be shipped as NuGet packages (https://www.nuget.org/packages/mod_spatialite is an example) or you can specify a path to the extension binary (an absolute path).
This is done in the app host and the info is projected as part of the connection string for the client.
In the client library, it'll read the extensions from the connection string (then delete that value so it is a valid connection string) and load the extensions using the SqliteConnection. Since SQLite needs the extension to be in PATH (see this doc) there is a bit of code to ensure the path for the binary is loaded correctly.
I've added some tests for observing that the extension loads, but it's a hard one to write tests for as you have to have a bunch of different binaries checked into the repo (I'd use mod_spatialite but it only ships Windows binaries in the nuget package). Because if this, I've marked the feature as experimental.
This doesn't support the EF SQLite integration as there's no way I can find using the EF API in which you can add extensions, you have to do it manually when the DbContext is created.