Refactor Issue31751 of ef core across projects (#9790)#9793
Refactor Issue31751 of ef core across projects (#9790)#9793msynk merged 1 commit intobitfoundation:developfrom yasmoradi:9790
Conversation
WalkthroughThe pull request updates three components. In the Besql module, the Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application Startup
participant OS as Operating System Check
participant Config as AppContext Switch
App->>OS: Check OS (Android or Browser)
alt OS condition met
OS->>Config: Set "Microsoft.EntityFrameworkCore.Issue31751" switch
else
Note over App,OS: No switch set
end
sequenceDiagram
participant Service as ClientCoreService registration
participant Factory as DbContext Factory Initialization
participant DB as Database Migration
Service->>Factory: Call AddBesqlDbContextFactory
Factory->>Factory: Wrap migration task with Task.Run
Factory->>DB: Invoke MigrateAsync asynchronously
DB-->>Factory: Migration process completed
Factory-->>Service: Return initialization result
Poem
Tip 🌐 Web search-backed reviews and chat
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Besql/Bit.Besql/BesqlDbContextInterceptor.cs (1)
51-51: LGTM! Good enhancement for extensibility.Making
IsTargetedCommandprotected virtual allows derived classes to customize command targeting logic while maintaining the core functionality.Consider documenting the method's contract to guide implementers on:
- Expected behavior for custom implementations
- Impact on the interceptor's functionality
- Performance considerations when overriding
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Besql/Bit.Besql/BesqlDbContextInterceptor.cs(1 hunks)src/Besql/Bit.Besql/IServiceCollectionBesqlExtentions.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/IClientCoreServiceCollectionExtensions.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build and test
🔇 Additional comments (2)
src/Besql/Bit.Besql/IServiceCollectionBesqlExtentions.cs (1)
22-25: Good consolidation of platform-specific configuration.The AppContext switch for Issue31751 is now consistently applied across both Android and Browser platforms.
Please verify the impact of this configuration by checking the latest EF Core documentation:
✅ Verification successful
Consolidated Platform-Specific Configuration Verified
- The GitHub GraphQL query confirms that EF Core Issue31751 is closed.
- This indicates that the workaround applied via the AppContext switch is aligned with the current state of EF Core.
- The configuration correctly consolidates settings for both Android and Browser platforms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if Issue31751 is still relevant in the latest EF Core version # Get the latest EF Core version from NuGet curl -s https://api.nuget.org/v3/registration5-gz-semver2/microsoft.entityframeworkcore/index.json | jq -r '.items[0].upper' # Search for Issue31751 in EF Core's GitHub repository gh api graphql -f query=' { repository(owner: "dotnet", name: "efcore") { issue(number: 31751) { title state url comments(last: 1) { nodes { updatedAt body } } } } }'Length of output: 686
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/IClientCoreServiceCollectionExtensions.cs (1)
121-121: Consider the implications of using Task.Run with async operations.While wrapping the migration in
Task.Runcan prevent blocking the main thread, be cautious as:
- Using Task.Run with async operations can lead to unnecessary thread pool usage
- Database migrations are already asynchronous operations
Consider if this change is necessary by checking if the migration was causing UI blocking issues. If you're experiencing such issues, you might want to:
- Profile the migration performance
- Consider implementing a progress indicator
- Evaluate if the migration can be deferred or performed in smaller batches
✅ Verification successful
Review Verification: Consistent Task.Run Usage
- Our search confirms that wrapping async operations—including the call to
Database.MigrateAsync—insideTask.Runis a recurring pattern across the repository.- Although
Database.MigrateAsyncis inherently asynchronous, usingTask.Runmight be intended to ensure that any intensive migration work does not block the main thread.- It’s recommended to profile the migration’s impact; if no UI thread blocking is observed, consider simplifying to avoid unnecessary thread pool usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for similar patterns in the codebase to understand the common practice # Look for other Task.Run usages with async operations ast-grep --pattern 'Task.Run(async () => $_)' # Look for Database.MigrateAsync calls to compare implementations ast-grep --pattern 'Database.MigrateAsync()'Length of output: 8266
msynk
changed the title
Issue31751 across projects (#9790)2 participants
closes #9790
Summary by CodeRabbit