V1 Done and working#5
Conversation
|
@DMiradakis Any update to my Pull Request? - is it obsolete by now? |
Fix foreign key constraint on FlowRun table
--- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/Trubador/didact-engine?shareId=XXXX-XXXX-XXXX-XXXX).
Fix foreign key constraint issue in FlowRun table
|
|
- Introduced ExecutionMode entity and updated related configurations. - Added FlowRunRepository for managing flow runs. - Updated database context to include ExecutionModes DbSet. - Refactored database initialization logic to seed ExecutionModes. - Updated application to use SQLite as the default database provider. - Enhanced error handling and logging in various components.
There was a problem hiding this comment.
Pull Request Overview
This PR implements the initial V1 version of the Didact workflow engine with a complete working system. The engine transitions from SQL Server to SQLite for local development and adds core functionality for flow management, execution scheduling, and background processing.
Key changes:
- Migrated database provider from SQL Server to SQLite with proper configuration handling
- Implemented comprehensive flow and flow run management with repositories and controllers
- Enhanced task scheduling with efficient thread pool management and graceful shutdown capabilities
Reviewed Changes
Copilot reviewed 51 out of 69 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| DidactEngine/appsettings.json | Database configuration updated to support SQLite for local development |
| DidactEngine/TaskSchedulers/DidactThreadPoolScheduler.cs | Enhanced thread pool scheduler with proper synchronization and shutdown handling |
| DidactEngine/Services/FlowRunRepository.cs | New repository for flow run CRUD operations |
| DidactEngine/Services/FlowRepository.cs | New repository for flow management and configuration storage |
| DidactEngine/Services/Database/DatabaseInitializer.cs | Database initialization service with seeding capabilities |
| DidactEngine/Services/Contexts/DidactDbContext.cs | Database context updated for SQLite compatibility |
| DidactEngine/Program.cs | Application startup configuration with dependency injection setup |
Files not reviewed (17)
- .idea/.idea.DidactEngine/.idea/.gitignore: Language not supported
- .idea/.idea.DidactEngine/.idea/.name: Language not supported
- .idea/.idea.DidactEngine/.idea/indexLayout.xml: Language not supported
- .idea/.idea.DidactEngine/.idea/vcs.xml: Language not supported
- DidactEngine/Migrations/20230717051620_InitialSnapshot.Designer.cs: Language not supported
- DidactEngine/Migrations/20231205064918_RenamedStandardQueuesToHyperQueues.Designer.cs: Language not supported
- DidactEngine/Migrations/20231206055336_AddFlowRunsConfigurations.Designer.cs: Language not supported
- DidactEngine/Migrations/20231206071100_AddFlowRunsToQueueInbounds.Designer.cs: Language not supported
- DidactEngine/Migrations/20231209040238_RemoveFlowRefsFromQueueInboundConfigurations.Designer.cs: Language not supported
- DidactEngine/Migrations/20231209041434_AddWhoAuditColumnsToOrganization.Designer.cs: Language not supported
- DidactEngine/Migrations/20231209044908_AddBlockRunConfig.Designer.cs: Language not supported
- DidactEngine/Migrations/20231210080814_AddEngineConfiguration.Designer.cs: Language not supported
- DidactEngine/Migrations/20231210081256_AddEngineUniqueNameConstraint.Designer.cs: Language not supported
- DidactEngine/Migrations/20231210202046_AddFlowNameUniqueConstraint.Designer.cs: Language not supported
- DidactEngine/Migrations/20240107100141_AddTypeRefToFlow.Designer.cs: Language not supported
- DidactEngine/Migrations/20250604162806_InitialCreate.Designer.cs: Language not supported
- DidactEngine/Migrations/20250604165914_AddExecutionModesDbSet.Designer.cs: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| "AllowedHosts": "*", | ||
| "ConnectionStrings": { | ||
| "Didact": "Data Source=localhost;Initial Catalog=Didact;Integrated Security=true" | ||
| "Didact": "Data source=localhost;Initial Catalog=Didact;User Id=sa;Password=9b9i!B8PJrec56rEo@T8T3LB#5a3rnaKd@m?hBcP;" // running in docker sql server container |
There was a problem hiding this comment.
Database password is exposed in plain text in the configuration file. Consider using environment variables, user secrets, or a secure configuration provider for sensitive connection string information.
| "Didact": "Data source=localhost;Initial Catalog=Didact;User Id=sa;Password=9b9i!B8PJrec56rEo@T8T3LB#5a3rnaKd@m?hBcP;" // running in docker sql server container | |
| "Didact": "Data source=localhost;Initial Catalog=Didact;User Id=sa;Password=${DIDACT_DB_PASSWORD};" // running in docker sql server container; set DIDACT_DB_PASSWORD as an environment variable |
| private readonly ThreadLocal<string> _currentThreadName = new(); | ||
|
|
||
| private readonly int _maxDegreeOfParallelism; | ||
| private readonly ThreadLocal<bool> _currentThreadIsExecuting = new(false); private readonly ThreadLocal<string> _currentThreadName = new(); |
There was a problem hiding this comment.
[nitpick] Multiple field declarations should be on separate lines for better readability and maintainability.
| public DidactThreadPoolScheduler(ILogger<DidactThreadPoolScheduler> logger, int maxDegreeOfParallelism) | ||
| private readonly AutoResetEvent _workAvailable = new(false); | ||
|
|
||
| private volatile bool _shutdown = false; public DidactThreadPoolScheduler(ILogger<DidactThreadPoolScheduler> logger, int maxDegreeOfParallelism) |
There was a problem hiding this comment.
[nitpick] Field declaration and constructor should be on separate lines for better code readability.
| } | ||
|
|
||
| private void ThreadExecutionLoop() | ||
| } private void ThreadExecutionLoop() |
There was a problem hiding this comment.
[nitpick] Missing line break between method declarations. Add proper spacing for better code readability.
| // This blocks the thread instead of busy-waiting, dramatically reducing CPU usage | ||
| _workAvailable.WaitOne(1000); // Wait up to 1 second for work | ||
| } | ||
| } catch (ThreadInterruptedException ex) |
There was a problem hiding this comment.
[nitpick] Missing line break between closing brace and catch block. Add proper spacing for better code readability.
| private async Task SeedInitialDataAsync() | ||
| { // Add any initial seeding logic here | ||
| // For example, ensure there's at least one organization if (!await _dbContext.Organizations.AnyAsync()) |
There was a problem hiding this comment.
[nitpick] Inconsistent indentation and formatting in comments and code. Ensure proper line breaks and consistent spacing.
| { // Add any initial seeding logic here | ||
| // For example, ensure there's at least one organization if (!await _dbContext.Organizations.AnyAsync()) | ||
| { _dbContext.Organizations.Add(new DidactCore.Entities.Organization |
There was a problem hiding this comment.
[nitpick] Inconsistent indentation. The opening brace should be properly aligned with the if statement.
| { // Add any initial seeding logic here | |
| // For example, ensure there's at least one organization if (!await _dbContext.Organizations.AnyAsync()) | |
| { _dbContext.Organizations.Add(new DidactCore.Entities.Organization | |
| { | |
| // Add any initial seeding logic here | |
| // For example, ensure there's at least one organization | |
| if (!await _dbContext.Organizations.AnyAsync()) | |
| { | |
| _dbContext.Organizations.Add(new DidactCore.Entities.Organization |
| _flowExecutor = flowExecutor ?? throw new ArgumentNullException(nameof(flowExecutor)); | ||
| //_flowRepository = flowRepository ?? throw new ArgumentNullException(nameof(flowRepository)); | ||
| //_flowExecutor = flowExecutor ?? throw new ArgumentNullException(nameof(flowExecutor)); | ||
| } |
There was a problem hiding this comment.
Extra closing brace at the end of file that doesn't match any opening brace.
| using Microsoft.Extensions.DependencyInjection; | ||
| using DidactEngine.Services; | ||
| using Microsoft.Extensions.Hosting; | ||
| using Microsoft.Extensions.Hosting; |
There was a problem hiding this comment.
Duplicate using statement for Microsoft.Extensions.Hosting should be removed.
| using Microsoft.Extensions.Hosting; |
| /// </summary> | ||
| /// <returns></returns> | ||
| [HttpPost("/flows")] | ||
| [SwaggerResponse(StatusCodes.Status202Accepted)] public async Task<IActionResult> CreateFlowAsync() |
There was a problem hiding this comment.
[nitpick] Missing line break between attribute and method declaration. Add proper spacing for better code readability.
| [SwaggerResponse(StatusCodes.Status202Accepted)] public async Task<IActionResult> CreateFlowAsync() | |
| [SwaggerResponse(StatusCodes.Status202Accepted)] | |
| public async Task<IActionResult> CreateFlowAsync() |
No description provided.