Switch schedule entities to numeric keys and rebuild schema#6
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the schedule application from GUID-based identifiers to integer identity columns across all entities. The changes include database schema updates with auto-generated integer IDs, a schema validation mechanism that recreates the database when columns are incompatible, and updated service methods that return created entities to support ID-dependent workflows in the UI.
- Models updated from
Guidtointfor all entity IDs and foreign keys - Database context configured with
ValueGeneratedOnAdd()for integer identity columns and unique constraint on subject codes - Service layer enhanced to return created entities and validate schema compatibility before initialization
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| BlazorApp1/Models/Subject.cs | Changed Id from Guid to int |
| BlazorApp1/Models/Role.cs | Changed Id from Guid to int |
| BlazorApp1/Models/Person.cs | Changed Id and RoleId from Guid to int |
| BlazorApp1/Models/LessonType.cs | Changed Id from Guid to int |
| BlazorApp1/Models/LessonStudent.cs | Changed LessonId and PersonId from Guid to int |
| BlazorApp1/Models/Lesson.cs | Changed Id, LessonTypeId, LecturerId, and StudentIds from Guid to int |
| BlazorApp1/Models/Department.cs | Changed Id from Guid to int |
| BlazorApp1/Models/Classroom.cs | Changed Id from Guid to int |
| BlazorApp1/Data/ScheduleDbContext.cs | Added ValueGeneratedOnAdd for integer IDs and unique index for subject codes |
| BlazorApp1/Services/ScheduleService.cs | Added schema validation, updated methods to return entities, adjusted seeding to save after each entity type |
| BlazorApp1/Components/Pages/SubjectDetails.razor | Updated dictionary key types from Guid to int |
| BlazorApp1/Components/Pages/Schedule.razor | Updated dictionary key types from Guid to int |
| BlazorApp1/Components/Pages/Management.razor | Updated to use returned entities for ID references and check for 0 instead of Guid.Empty |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await db.SaveChangesAsync(); | ||
|
|
||
| if (studentIds.Any()) | ||
| { | ||
| db.LessonStudents.AddRange(studentIds.Select(s => new LessonStudent | ||
| { | ||
| LessonId = lesson.Id, | ||
| PersonId = s | ||
| })); | ||
| await db.SaveChangesAsync(); | ||
| } |
There was a problem hiding this comment.
The AddLessonAsync method saves changes twice: once after adding the Lesson (line 172) and again after adding LessonStudents (line 181). While this ensures the Lesson.Id is available for the LessonStudent records, it creates two database round trips. Consider using a single transaction or documenting why two saves are necessary here.
| modelBuilder.Entity<Department>().Property(d => d.Code).IsRequired(); | ||
| modelBuilder.Entity<Department>().Property(d => d.Name).IsRequired(); |
There was a problem hiding this comment.
The Department entity is missing the ValueGeneratedOnAdd configuration for its Id property, while all other entities (Classroom, Subject, LessonType, Role, Person, Lesson) have this configuration. This inconsistency could lead to issues if the Id is expected to be auto-generated. Add the configuration for consistency.
| var connection = db.Database.GetDbConnection(); | ||
| await connection.OpenAsync(); | ||
| await using var command = connection.CreateCommand(); | ||
| command.CommandText = "PRAGMA table_info('People');"; | ||
| var columns = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); | ||
|
|
||
| await using var reader = await command.ExecuteReaderAsync(); | ||
| while (await reader.ReadAsync()) | ||
| { | ||
| var name = reader.GetString(1); | ||
| var type = reader.GetString(2); | ||
| columns[name] = type; | ||
| } | ||
|
|
||
| await connection.CloseAsync(); |
There was a problem hiding this comment.
The connection is not being disposed properly. After opening the connection with OpenAsync(), the connection should be closed even if an exception occurs. Consider wrapping the connection usage in a try-finally block or using the connection disposal pattern to ensure proper cleanup.
| private static async Task<bool> HasExpectedColumnsAsync(ScheduleDbContext db) | ||
| { | ||
| var connection = db.Database.GetDbConnection(); | ||
| await connection.OpenAsync(); | ||
| await using var command = connection.CreateCommand(); | ||
| command.CommandText = "PRAGMA table_info('People');"; | ||
| var columns = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); | ||
|
|
||
| await using var reader = await command.ExecuteReaderAsync(); | ||
| while (await reader.ReadAsync()) | ||
| { | ||
| var name = reader.GetString(1); | ||
| var type = reader.GetString(2); | ||
| columns[name] = type; | ||
| } | ||
|
|
||
| await connection.CloseAsync(); | ||
|
|
||
| var expectedColumns = new[] { "Id", "FirstName", "LastName", "Title", "Affiliation", "RoleId" }; | ||
| if (expectedColumns.Any(c => !columns.ContainsKey(c))) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| return columns.TryGetValue("Id", out var idType) | ||
| && idType.Contains("INT", StringComparison.OrdinalIgnoreCase); | ||
| } |
There was a problem hiding this comment.
The HasExpectedColumnsAsync method only checks the 'People' table schema but is used to determine whether to recreate the entire database. This means if other tables (Departments, Roles, Lessons, etc.) have schema mismatches, they won't be detected. Either check all relevant tables or rename the method to indicate it only validates the People table schema.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
[WIP] Switch schedule entities to numeric keys and rebuild schema
Summary
Testing
Codex Task