-
-
Notifications
You must be signed in to change notification settings - Fork 235
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
Add support for importing pre-populated SQLite file to Besql (#9813) #9814
Conversation
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request refactors multiple synchronization methods across the codebase. Method names in the storage-related classes and interfaces have been updated to explicitly indicate the synchronization direction between .NET and browser cache storage. Additionally, conditional logic has been introduced in the database context factory to determine which synchronization method to call based on file existence. UI and static file serving modifications have been applied in the demo client, and a new method in the pooled context factory enforces asynchronous context creation. Changes
Sequence Diagram(s)sequenceDiagram
participant Factory as DbContextFactory
participant FS as FileSystem
participant Storage as IBesqlStorage
Factory->>FS: Check if _fileName exists
alt File does not exist
Factory->>Storage: SyncFromBrowserCacheStorageToDotNet(_fileName)
else File exists
Factory->>Storage: (Proceed with standard initialization)
end
Factory-->>Factory: Return new DbContext
sequenceDiagram
participant Interceptor as DbContextInterceptor
participant Storage as IBesqlStorage
participant JS as Browser (via BitBesql JS)
Interceptor->>Storage: SyncFromDotNetToBrowserCacheStorage(fileName)
Storage->>JS: Invoke syncFromDotNetToBrowserCacheStorage(fileName)
JS-->>Storage: Response/Error
Storage-->>Interceptor: Completion
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/Besql/Demo/Bit.Besql.Demo.Client/Extensions/ServiceCollectionExtensions.cs (1)
18-18
: Consider using HttpClientFactory instead of direct HttpClient registration.While registering HttpClient directly works, using HttpClientFactory via
AddHttpClient()
is recommended as it provides better lifecycle management and features like automatic DNS refresh.- services.AddScoped(sp => new HttpClient()); + services.AddHttpClient();src/Besql/Bit.Besql/wwwroot/bit-besql.js (2)
4-4
: Update function declaration name to match the property name.The function declaration still uses the old name 'init' while the property uses the new name 'syncFromBrowserCacheStorageToDotNet'.
-BitBesql.syncFromBrowserCacheStorageToDotNet = async function init(fileName) { +BitBesql.syncFromBrowserCacheStorageToDotNet = async function syncFromBrowserCacheStorageToDotNet(fileName) {
21-21
: Update function declaration name to match the property name.The function declaration still uses the old name 'persist' while the property uses the new name 'syncFromDotNetToBrowserCacheStorage'.
-BitBesql.syncFromDotNetToBrowserCacheStorage = async function persist(fileName) { +BitBesql.syncFromDotNetToBrowserCacheStorage = async function syncFromDotNetToBrowserCacheStorage(fileName) {src/Besql/Demo/Bit.Besql.Demo.Client/Pages/Weather.razor (3)
7-7
: Consider configuring HttpClient with retry policies and timeouts.The injected HttpClient is used for downloading pre-populated database files. Consider configuring it with appropriate retry policies, timeouts, and error handling to ensure robust operation.
-@inject HttpClient HttpClient; +@inject IHttpClientFactory HttpClientFactory;Then in the
UsePrePopulatedDatabaseSample
method:var client = HttpClientFactory.CreateClient("PrePopulatedDb"); client.Timeout = TimeSpan.FromMinutes(5);
13-17
: Add loading states and error handling to UI operations.The UI lacks feedback mechanisms during async operations. Consider:
- Adding loading indicators while operations are in progress
- Displaying error messages when operations fail
- Adding confirmation dialogs for destructive operations like deletion
-<button class="btn btn-primary" @onclick="DeleteSomeForecastsSample">Delete some forecasts</button> +<button class="btn btn-primary" @onclick="DeleteSomeForecastsSample" disabled="@isLoading"> + @if (isLoading) + { + <span class="spinner-border spinner-border-sm" role="status" aria-hidden="true"></span> + } + Delete some forecasts +</button> + +@if (errorMessage != null) +{ + <div class="alert alert-danger" role="alert"> + @errorMessage + </div> +}
36-37
: Reduce test data size and move to a constant.The test data contains an extremely long string literal that makes the code hard to read. Consider moving it to a constant or generating it programmatically.
+private const string LongTestSummary = new string('A', 1000); + -Summary = "A B C D E F G H I J K L M N O P Q R S T U V W X Y Z ... [truncated]", +Summary = LongTestSummary,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/Besql/Demo/Bit.Besql.Demo/wwwroot/Pre-Populated-Offline-Client.db
is excluded by!**/*.db
📒 Files selected for processing (12)
src/Besql/Bit.Besql/BesqlDbContextInterceptor.cs
(1 hunks)src/Besql/Bit.Besql/BesqlPooledDbContextFactory.cs
(1 hunks)src/Besql/Bit.Besql/BrowserCacheBesqlStorage.cs
(2 hunks)src/Besql/Bit.Besql/IBesqlStorage.cs
(1 hunks)src/Besql/Bit.Besql/NoopBesqlStorage.cs
(1 hunks)src/Besql/Bit.Besql/PooledDbContextFactoryBase.cs
(1 hunks)src/Besql/Bit.Besql/wwwroot/bit-besql.js
(2 hunks)src/Besql/Demo/Bit.Besql.Demo.Client/Extensions/ServiceCollectionExtensions.cs
(1 hunks)src/Besql/Demo/Bit.Besql.Demo.Client/Pages/Weather.razor
(3 hunks)src/Besql/Demo/Bit.Besql.Demo.Client/Program.cs
(1 hunks)src/Besql/Demo/Bit.Besql.Demo/Components/_Imports.razor
(1 hunks)src/Besql/Demo/Bit.Besql.Demo/Program.cs
(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/Besql/Demo/Bit.Besql.Demo.Client/Program.cs
- src/Besql/Demo/Bit.Besql.Demo/Components/_Imports.razor
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build and test
🔇 Additional comments (12)
src/Besql/Bit.Besql/NoopBesqlStorage.cs (2)
3-5
: LGTM! Documentation is properly inherited.The XML documentation properly inherits from the interface using
<inheritdoc>
.
8-25
: LGTM! Clear no-op implementations with descriptive method names.The no-op implementations are correctly implemented, and the method names have been improved to clearly indicate the synchronization direction between .NET and browser cache storage.
src/Besql/Bit.Besql/BrowserCacheBesqlStorage.cs (3)
5-7
: LGTM! Documentation is properly inherited.The XML documentation properly inherits from the interface using
<inheritdoc>
.
12-26
: LGTM! Proper async/await usage with ConfigureAwait(false).The sync methods are correctly implemented with proper async/await patterns and ConfigureAwait(false) to avoid potential deadlocks.
33-45
: LGTM! Proper error handling and state management.The resume method correctly handles invalid states and properly cleans up the paused files list after processing.
src/Besql/Bit.Besql/wwwroot/bit-besql.js (1)
23-25
: LGTM! Proper initialization of dbCache.The code correctly initializes dbCache if it's null, ensuring the cache is always available.
src/Besql/Bit.Besql/BesqlPooledDbContextFactory.cs (1)
35-38
: LGTM! The file existence check enables pre-populated db support.The conditional sync from browser cache only when the file doesn't exist aligns well with the PR objectives, allowing pre-populated SQLite db files to be used without being overwritten by browser cache data.
src/Besql/Bit.Besql/IBesqlStorage.cs (2)
5-17
: Great documentation for sync methods!The XML documentation clearly explains the WebAssembly context and the purpose of each sync direction. The method names are now more explicit about their functionality.
19-30
: Clear documentation for pause/resume functionality.The documentation effectively explains the throttling mechanism and when to use pause/resume functionality.
src/Besql/Bit.Besql/PooledDbContextFactoryBase.cs (1)
24-27
: Good enforcement of async operations.The override correctly prevents synchronous context creation and provides a clear error message directing users to the async alternative, which is crucial for WebAssembly operations.
src/Besql/Bit.Besql/BesqlDbContextInterceptor.cs (1)
108-108
: LGTM! Method call updated to match new interface.The change to use
SyncFromDotNetToBrowserCacheStorage
is consistent with the interface changes and maintains the same functionality.src/Besql/Demo/Bit.Besql.Demo/Program.cs (1)
29-32
: Consider security implications of serving unknown file types.While enabling
ServeUnknownFileTypes
is necessary for serving SQLite database files, it could potentially expose sensitive files. Consider these security measures:
- Restrict file extensions to only those needed (e.g., .db, .sqlite)
- Place database files in a specific directory
- Implement proper access controls
Here's a script to check if there are any sensitive files that could be exposed:
closes #9813
Summary by CodeRabbit