Refactors course data sources and rebuild workflow#161
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR refactors the course rebuild workflow by introducing new raw data tables ( ChangesCourse Rebuild Refactor with Raw Tables and Description Replacement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tacos.core/Migrations/20260522162307_AlignCourseDescriptionWithRaw.cs`:
- Around line 133-191: The Down method currently only reverts some columns but
does not restore the primary key, altered column definitions, indexes or
constraints changed in Up; update Down (in
20260522162307_AlignCourseDescriptionWithRaw.cs) to fully reverse Up by: adding
back any dropped columns with their original data types and nullability,
repopulating values where Up transformed data (use CONVERT/CAST as needed),
renaming columns back to their original names if Up renamed them, and recreating
the original primary key and any indexes/foreign keys that Up removed or changed
(reference the Down and Up migration SQL blocks and the [CourseDescription]
primary key/index names used in Up to ensure exact reversal).
- Around line 112-124: Add a fail-fast validation step before applying the
TRY_CONVERT update and before dropping the source columns in the migration: in
the migration method (where migrationBuilder.Sql is used for the ALTER/UPDATE),
insert a preceding SQL block that checks for any rows where CreatedOn or
UpdatedOn is non-NULL but TRY_CONVERT(datetime2(6), ...) returns NULL (e.g. IF
EXISTS (SELECT 1 FROM [dbo].[CourseDescription] WHERE [CreatedOn] IS NOT NULL
AND TRY_CONVERT(datetime2(6), [CreatedOn]) IS NULL OR [UpdatedOn] IS NOT NULL
AND TRY_CONVERT(datetime2(6), [UpdatedOn]) IS NULL) THROW 51000, 'Invalid
timestamp(s) in CourseDescription: conversion to datetime2(6) failed', 1;), so
the migration fails fast rather than silently losing data; place this guard SQL
before the UPDATE that sets CreatedOnRaw/UpdatedOnRaw and before the ALTER TABLE
DROP COLUMN statements.
In `@src/tacos.core/Migrations/20260522162926_ReorderCourseDescriptionLikeRaw.cs`:
- Around line 166-168: The Down(MigrationBuilder migrationBuilder) method is
empty but Up performs a destructive table rebuild; implement Down to reverse Up
by restoring the original table schema and data: identify the original table and
column layout used before the rebuild (as in Up), create a
temporary/original-named table with the prior columns, copy data back from the
new table using INSERT INTO ... SELECT (handling renamed columns and
nullability), re-create original indexes and foreign keys, then drop the rebuilt
table (or rename the temp back) so the database returns to its prior state;
ensure you use the MigrationBuilder API (CreateTable, DropTable, RenameTable,
AddColumn/DropColumn, CreateIndex, AddForeignKey, and raw SQL via
migrationBuilder.Sql where needed) and preserve transactions and constraints to
make the downgrade safe and reproducible.
- Around line 64-159: The migration currently drops and renames tables then
creates PK_CourseDescription, which will fail late if there are duplicate
composite keys; add a pre-check in the migration
(20260522162926_ReorderCourseDescriptionLikeRaw) before dropping/renaming
CourseDescription: run a SELECT on [dbo].[CourseDescription] (or on the source
used to INSERT into CourseDescription_Rebuild) grouping by SubjectCode,
CourseNumber, Status HAVING COUNT(*)>1, and if any rows are returned throw/raise
an error and abort the migration (or log and fail) so duplicates are detected
early and the migration does not leave a partially-applied state; reference the
tables CourseDescription, CourseDescription_Rebuild and the constraint name
PK_CourseDescription when implementing the check and failing fast.
In `@src/tacos.core/Migrations/20260526111811_DropCourseOfferingsRaw.cs`:
- Around line 23-25: The Down(MigrationBuilder) in the DropCourseOfferingsRaw
migration currently no-ops which hides an unsupported rollback and leaves
dbo.CourseOfferingsRaw missing; update the Down method in the
DropCourseOfferingsRaw migration class to either (A) recreate
dbo.CourseOfferingsRaw with the same schema and indexes removed in Up (mirror
the Up's DropTable by using migrationBuilder.CreateTable and
migrationBuilder.CreateIndex calls matching the original
columns/types/constraints), or (B) explicitly fail fast by throwing a
NotSupportedException (or call throw new InvalidOperationException with a clear
message) to mark the downgrade as unsupported; implement one of these approaches
in the Down(MigrationBuilder migrationBuilder) method referenced in the diff.
In
`@src/tacos.core/Migrations/20260526112751_DeduplicateCourseDescriptionsFromRawProcedure.cs`:
- Around line 190-192: The Down method in the
DeduplicateCourseDescriptionsFromRawProcedure migration is empty so rolling back
will leave the deduplicating procedure in place; restore the previous
stored-procedure body (the SQL that was replaced in Up) inside protected
override void Down(MigrationBuilder migrationBuilder) by reapplying the original
CREATE/ALTER PROCEDURE or by executing the prior SQL via
migrationBuilder.Sql(...), or if downgrades are intentionally unsupported throw
a clear exception (e.g., throw new InvalidOperationException) from Down to fail
fast; update the Down method in the migration class accordingly.
- Around line 88-92: The ROW_NUMBER() ordering in the migration's procedure is
nondeterministic because dbo.CourseDescriptionRaw lacks a stable per-row
tiebreaker; modify the migration so the raw table has a stable PK/IDENTITY
(e.g., add an identity column to CourseDescriptionRaw) and include that identity
column in the ORDER BY clause used to compute ROW_NUMBER() in the procedure (or
alternatively add explicit detection and THROW when ties remain), and also
implement the migration's Down() in
20260526112751_DeduplicateCourseDescriptionsFromRawProcedure to restore the
previous procedure definition (or DROP the altered procedure) so downgrades
revert the change.
In `@src/tacos.mvc/Services/CourseRebuildService.cs`:
- Around line 100-101: Summary: ReplaceCourseDescriptionsFromRawAsync commits
before RebuildCoursesAsync, leaving snapshot inconsistent if
usp_RebuildCoursesFromProcessingWindow fails; make both steps atomic. Fix:
modify the SQL gateway to provide a single operation (e.g.,
ReplaceAndRebuildCoursesFromRawAsync or a transaction-aware RebuildCoursesAsync
overload) that opens one DB connection and begins a transaction, calls the
replacement logic (currently in ReplaceCourseDescriptionsFromRawAsync) and then
calls the rebuild stored proc (usp_RebuildCoursesFromProcessingWindow via
RebuildCoursesAsync) on the same connection/transaction, committing only after
both succeed (or alternatively move the replace logic into the stored proc
itself); update CourseRebuildService to call the new atomic gateway method
instead of calling ReplaceCourseDescriptionsFromRawAsync and RebuildCoursesAsync
separately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee7ff6b9-7fde-4217-8fa0-af7536cb63a1
📒 Files selected for processing (24)
CONTEXT.mdTest/Services/CourseRebuildServiceTests.csdatamart/stored-procedures/usp_LoadCourseDescriptionRaw.sqldatamart/stored-procedures/usp_LoadCoursesRaw.sqlsrc/tacos.core/Data/CourseDescription.cssrc/tacos.core/Migrations/20260522152058_UseCoursesRawSourceTable.cssrc/tacos.core/Migrations/20260522155158_CreateCourseDescriptionRaw.cssrc/tacos.core/Migrations/20260522162307_AlignCourseDescriptionWithRaw.cssrc/tacos.core/Migrations/20260522162926_ReorderCourseDescriptionLikeRaw.cssrc/tacos.core/Migrations/20260526111118_ReplaceCourseDescriptionsFromRawProcedure.cssrc/tacos.core/Migrations/20260526111811_DropCourseOfferingsRaw.cssrc/tacos.core/Migrations/20260526112751_DeduplicateCourseDescriptionsFromRawProcedure.cssrc/tacos.core/Migrations/20260526120500_MaterializeCourseRebuildSpanOptions.cssrc/tacos.core/Migrations/TacoDbContextModelSnapshot.cssrc/tacos.core/createMigrationsrc/tacos.core/updateDbFromMigrationssrc/tacos.mvc/ClientApp/css/site.scsssrc/tacos.mvc/ClientApp/pages/ManageCourseRebuild.tsxsrc/tacos.mvc/Services/CourseRebuildService.cssrc/tacos.mvc/Startup.cssrc/tacos.sql/dbo/Tables/CourseDescription.sqlsrc/tacos.sql/dbo/Tables/CourseDescriptionRaw.sqlsrc/tacos.sql/dbo/Tables/CoursesRaw.sqlsrc/tacos.sql/tacos.sql.sqlproj
Comment on lines
+112
to
+124
| UPDATE [dbo].[CourseDescription] | ||
| SET | ||
| [CreatedOnRaw] = TRY_CONVERT(datetime2(6), [CreatedOn]), | ||
| [UpdatedOnRaw] = TRY_CONVERT(datetime2(6), [UpdatedOn]); | ||
| """, | ||
| suppressTransaction: true | ||
| ); | ||
|
|
||
| migrationBuilder.Sql( | ||
| """ | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [CreatedOn]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [UpdatedOn]; | ||
|
|
Contributor
There was a problem hiding this comment.
Guard timestamp conversion before dropping source columns.
TRY_CONVERT can silently turn invalid non-null timestamps into NULL, and the original columns are dropped immediately after. Add a fail-fast validation before the update/drop sequence.
Suggested SQL guard
+IF EXISTS
+(
+ SELECT 1
+ FROM [dbo].[CourseDescription]
+ WHERE ([CreatedOn] IS NOT NULL AND TRY_CONVERT(datetime2(6), [CreatedOn]) IS NULL)
+ OR ([UpdatedOn] IS NOT NULL AND TRY_CONVERT(datetime2(6), [UpdatedOn]) IS NULL)
+)
+BEGIN
+ THROW 50018, 'CourseDescription contains non-convertible CreatedOn/UpdatedOn values.', 1;
+END;
+
UPDATE [dbo].[CourseDescription]
SET
[CreatedOnRaw] = TRY_CONVERT(datetime2(6), [CreatedOn]),
[UpdatedOnRaw] = TRY_CONVERT(datetime2(6), [UpdatedOn]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tacos.core/Migrations/20260522162307_AlignCourseDescriptionWithRaw.cs`
around lines 112 - 124, Add a fail-fast validation step before applying the
TRY_CONVERT update and before dropping the source columns in the migration: in
the migration method (where migrationBuilder.Sql is used for the ALTER/UPDATE),
insert a preceding SQL block that checks for any rows where CreatedOn or
UpdatedOn is non-NULL but TRY_CONVERT(datetime2(6), ...) returns NULL (e.g. IF
EXISTS (SELECT 1 FROM [dbo].[CourseDescription] WHERE [CreatedOn] IS NOT NULL
AND TRY_CONVERT(datetime2(6), [CreatedOn]) IS NULL OR [UpdatedOn] IS NOT NULL
AND TRY_CONVERT(datetime2(6), [UpdatedOn]) IS NULL) THROW 51000, 'Invalid
timestamp(s) in CourseDescription: conversion to datetime2(6) failed', 1;), so
the migration fails fast rather than silently losing data; place this guard SQL
before the UPDATE that sets CreatedOnRaw/UpdatedOnRaw and before the ALTER TABLE
DROP COLUMN statements.
Comment on lines
+133
to
+191
| protected override void Down(MigrationBuilder migrationBuilder) | ||
| { | ||
| migrationBuilder.Sql( | ||
| """ | ||
| IF COL_LENGTH(N'[dbo].[CourseDescription]', N'CreatedOnText') IS NULL | ||
| ALTER TABLE [dbo].[CourseDescription] ADD [CreatedOnText] [nvarchar](50) NULL; | ||
| IF COL_LENGTH(N'[dbo].[CourseDescription]', N'UpdatedOnText') IS NULL | ||
| ALTER TABLE [dbo].[CourseDescription] ADD [UpdatedOnText] [nvarchar](50) NULL; | ||
| """, | ||
| suppressTransaction: true | ||
| ); | ||
|
|
||
| migrationBuilder.Sql( | ||
| """ | ||
| UPDATE [dbo].[CourseDescription] | ||
| SET | ||
| [CreatedOnText] = CONVERT(nvarchar(50), [CreatedOn], 121), | ||
| [UpdatedOnText] = CONVERT(nvarchar(50), [UpdatedOn], 121); | ||
| """, | ||
| suppressTransaction: true | ||
| ); | ||
|
|
||
| migrationBuilder.Sql( | ||
| """ | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [CreatedOn]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [UpdatedOn]; | ||
|
|
||
| EXEC sp_rename N'[dbo].[CourseDescription].[CreatedOnText]', N'CreatedOn', N'COLUMN'; | ||
| EXEC sp_rename N'[dbo].[CourseDescription].[UpdatedOnText]', N'UpdatedOn', N'COLUMN'; | ||
| """, | ||
| suppressTransaction: true | ||
| ); | ||
|
|
||
| migrationBuilder.Sql( | ||
| """ | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [FirstContactHoursPeriod]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [SecondContactHoursPeriod]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [ThirdContactHoursPeriod]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [FourthContactHoursPeriod]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [Ge2ArtsHumanities]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [Ge2ScienceEngineering]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [Ge2SocialSciences]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [Ge2Diversity]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [Ge2WritingExperience]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [Ge3ArtsHumanities]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [Ge3ScienceEngineering]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [Ge3SocialSciences]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [Ge3AmericanCultures]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [Ge3DomesticDiversity]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [Ge3OralLiteracy]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [Ge3QuantitativeLiteracy]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [Ge3ScientificLiteracy]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [Ge3VisualLiteracy]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [Ge3WorldCultures]; | ||
| ALTER TABLE [dbo].[CourseDescription] DROP COLUMN [Ge3WritingExperience]; | ||
| """, | ||
| suppressTransaction: true | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Down migration is not a full inverse of Up.
Rollback currently does not restore the primary key/state altered in Up (including the key shape and other altered column definitions), so downgrade can leave schema drift.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tacos.core/Migrations/20260522162307_AlignCourseDescriptionWithRaw.cs`
around lines 133 - 191, The Down method currently only reverts some columns but
does not restore the primary key, altered column definitions, indexes or
constraints changed in Up; update Down (in
20260522162307_AlignCourseDescriptionWithRaw.cs) to fully reverse Up by: adding
back any dropped columns with their original data types and nullability,
repopulating values where Up transformed data (use CONVERT/CAST as needed),
renaming columns back to their original names if Up renamed them, and recreating
the original primary key and any indexes/foreign keys that Up removed or changed
(reference the Down and Up migration SQL blocks and the [CourseDescription]
primary key/index names used in Up to ensure exact reversal).
Comment on lines
+64
to
+159
| INSERT INTO [dbo].[CourseDescription_Rebuild] | ||
| ( | ||
| [Course], | ||
| [SubjectCode], | ||
| [CourseNumber], | ||
| [CrossListing], | ||
| [Title], | ||
| [AbbreviatedTitle], | ||
| [CourseDescription], | ||
| [College], | ||
| [Department], | ||
| [Status], | ||
| [CreatedOn], | ||
| [UpdatedOn], | ||
| [FirstLearningActivity], | ||
| [FirstContactHoursPeriod], | ||
| [SecondLearningActivity], | ||
| [SecondContactHoursPeriod], | ||
| [ThirdLearningActivity], | ||
| [ThirdContactHoursPeriod], | ||
| [FourthLearningActivity], | ||
| [FourthContactHoursPeriod], | ||
| [Ge2ArtsHumanities], | ||
| [Ge2ScienceEngineering], | ||
| [Ge2SocialSciences], | ||
| [Ge2Diversity], | ||
| [Ge2WritingExperience], | ||
| [Ge3ArtsHumanities], | ||
| [Ge3ScienceEngineering], | ||
| [Ge3SocialSciences], | ||
| [Ge3AmericanCultures], | ||
| [Ge3DomesticDiversity], | ||
| [Ge3OralLiteracy], | ||
| [Ge3QuantitativeLiteracy], | ||
| [Ge3ScientificLiteracy], | ||
| [Ge3VisualLiteracy], | ||
| [Ge3WorldCultures], | ||
| [Ge3WritingExperience], | ||
| [Quarters], | ||
| [QuartersOffered], | ||
| [EffectiveTerm], | ||
| [Effective] | ||
| ) | ||
| SELECT | ||
| [Course], | ||
| [SubjectCode], | ||
| [CourseNumber], | ||
| [CrossListing], | ||
| [Title], | ||
| [AbbreviatedTitle], | ||
| [CourseDescription], | ||
| [College], | ||
| [Department], | ||
| [Status], | ||
| [CreatedOn], | ||
| [UpdatedOn], | ||
| [FirstLearningActivity], | ||
| [FirstContactHoursPeriod], | ||
| [SecondLearningActivity], | ||
| [SecondContactHoursPeriod], | ||
| [ThirdLearningActivity], | ||
| [ThirdContactHoursPeriod], | ||
| [FourthLearningActivity], | ||
| [FourthContactHoursPeriod], | ||
| [Ge2ArtsHumanities], | ||
| [Ge2ScienceEngineering], | ||
| [Ge2SocialSciences], | ||
| [Ge2Diversity], | ||
| [Ge2WritingExperience], | ||
| [Ge3ArtsHumanities], | ||
| [Ge3ScienceEngineering], | ||
| [Ge3SocialSciences], | ||
| [Ge3AmericanCultures], | ||
| [Ge3DomesticDiversity], | ||
| [Ge3OralLiteracy], | ||
| [Ge3QuantitativeLiteracy], | ||
| [Ge3ScientificLiteracy], | ||
| [Ge3VisualLiteracy], | ||
| [Ge3WorldCultures], | ||
| [Ge3WritingExperience], | ||
| [Quarters], | ||
| [QuartersOffered], | ||
| [EffectiveTerm], | ||
| [Effective] | ||
| FROM [dbo].[CourseDescription]; | ||
|
|
||
| DROP TABLE [dbo].[CourseDescription]; | ||
| EXEC sp_rename N'[dbo].[CourseDescription_Rebuild]', N'CourseDescription'; | ||
|
|
||
| ALTER TABLE [dbo].[CourseDescription] | ||
| ADD CONSTRAINT [PK_CourseDescription] PRIMARY KEY CLUSTERED | ||
| ( | ||
| [SubjectCode] ASC, | ||
| [CourseNumber] ASC, | ||
| [Status] ASC | ||
| ); |
Contributor
There was a problem hiding this comment.
Fail fast on duplicate composite keys before table swap.
PK creation happens after drop/rename. Duplicate (SubjectCode, CourseNumber, Status) rows will fail late and leave a partially-applied migration state.
Suggested pre-check before rebuild/drop
+IF EXISTS
+(
+ SELECT 1
+ FROM [dbo].[CourseDescription]
+ GROUP BY [SubjectCode], [CourseNumber], [Status]
+ HAVING COUNT(*) > 1
+)
+BEGIN
+ THROW 50019, 'Duplicate SubjectCode/CourseNumber/Status rows block PK creation.', 1;
+END;
+
DROP TABLE IF EXISTS [dbo].[CourseDescription_Rebuild];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tacos.core/Migrations/20260522162926_ReorderCourseDescriptionLikeRaw.cs`
around lines 64 - 159, The migration currently drops and renames tables then
creates PK_CourseDescription, which will fail late if there are duplicate
composite keys; add a pre-check in the migration
(20260522162926_ReorderCourseDescriptionLikeRaw) before dropping/renaming
CourseDescription: run a SELECT on [dbo].[CourseDescription] (or on the source
used to INSERT into CourseDescription_Rebuild) grouping by SubjectCode,
CourseNumber, Status HAVING COUNT(*)>1, and if any rows are returned throw/raise
an error and abort the migration (or log and fail) so duplicates are detected
early and the migration does not leave a partially-applied state; reference the
tables CourseDescription, CourseDescription_Rebuild and the constraint name
PK_CourseDescription when implementing the check and failing fast.
Comment on lines
+166
to
+168
| protected override void Down(MigrationBuilder migrationBuilder) | ||
| { | ||
| } |
Contributor
There was a problem hiding this comment.
Add a real rollback path for this migration.
Down is empty even though Up performs a destructive table rebuild. This makes downgrade behavior unsafe and non-reproducible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tacos.core/Migrations/20260522162926_ReorderCourseDescriptionLikeRaw.cs`
around lines 166 - 168, The Down(MigrationBuilder migrationBuilder) method is
empty but Up performs a destructive table rebuild; implement Down to reverse Up
by restoring the original table schema and data: identify the original table and
column layout used before the rebuild (as in Up), create a
temporary/original-named table with the prior columns, copy data back from the
new table using INSERT INTO ... SELECT (handling renamed columns and
nullability), re-create original indexes and foreign keys, then drop the rebuilt
table (or rename the temp back) so the database returns to its prior state;
ensure you use the MigrationBuilder API (CreateTable, DropTable, RenameTable,
AddColumn/DropColumn, CreateIndex, AddForeignKey, and raw SQL via
migrationBuilder.Sql where needed) and preserve transactions and constraints to
make the downgrade safe and reproducible.
Comment on lines
+23
to
+25
| protected override void Down(MigrationBuilder migrationBuilder) | ||
| { | ||
| } |
Contributor
There was a problem hiding this comment.
Don't silently noop on downgrade.
Rolling back past this migration will still leave dbo.CourseOfferingsRaw dropped, so the database no longer matches the target migration even though the downgrade appears to succeed. Please either recreate the table here or explicitly fail fast to mark the rollback unsupported.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tacos.core/Migrations/20260526111811_DropCourseOfferingsRaw.cs` around
lines 23 - 25, The Down(MigrationBuilder) in the DropCourseOfferingsRaw
migration currently no-ops which hides an unsupported rollback and leaves
dbo.CourseOfferingsRaw missing; update the Down method in the
DropCourseOfferingsRaw migration class to either (A) recreate
dbo.CourseOfferingsRaw with the same schema and indexes removed in Up (mirror
the Up's DropTable by using migrationBuilder.CreateTable and
migrationBuilder.CreateIndex calls matching the original
columns/types/constraints), or (B) explicitly fail fast by throwing a
NotSupportedException (or call throw new InvalidOperationException with a clear
message) to mark the downgrade as unsupported; implement one of these approaches
in the Down(MigrationBuilder migrationBuilder) method referenced in the diff.
Comment on lines
+88
to
+92
| ROW_NUMBER() OVER | ||
| ( | ||
| PARTITION BY [SubjectCode], [CourseNumber], [Status] | ||
| ORDER BY [UpdatedOn] DESC, [CreatedOn] DESC, TRY_CONVERT(int, [EffectiveTerm]) DESC, [Course] | ||
| ) AS [RowNumber] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -i 'CourseDescriptionRaw.sql' . -x sed -n '1,240p' {}
echo
rg -n -C2 'CourseDescriptionRaw|PRIMARY KEY|UNIQUE|IDENTITY' src/tacos.sql src/tacos.coreRepository: ucdavis/tacos
Length of output: 41650
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show Up/Down for the migration and the ROW_NUMBER ORDER BY block
sed -n '1,220p' src/tacos.core/Migrations/20260526112751_DeduplicateCourseDescriptionsFromRawProcedure.cs | cat -n
echo "----- excerpt around Up/Down SQL strings -----"
rg -n "Up\\b|Down\\b|usp_ReplaceCourseDescriptionsFromRaw|ROW_NUMBER\\(\\) OVER|PARTITION BY" src/tacos.core/Migrations/20260526112751_DeduplicateCourseDescriptionsFromRawProcedure.cs
echo "----- show full embedded SQL (Up) where possible -----"
# Print only the portion containing the procedure SQL (avoid dumping whole file if large)
rg -n -C3 "CREATE PROCEDURE|ALTER PROCEDURE|usp_ReplaceCourseDescriptionsFromRaw" src/tacos.core/Migrations/20260526112751_DeduplicateCourseDescriptionsFromRawProcedure.cs
echo "----- show Down() block only -----"
# Extract lines from "protected override void Down" to end of method (best-effort)
rg -n "protected override void Down" src/tacos.core/Migrations/20260526112751_DeduplicateCourseDescriptionsFromRawProcedure.cs || true
start=$(rg -n "protected override void Down" src/tacos.core/Migrations/20260526112751_DeduplicateCourseDescriptionsFromRawProcedure.cs | cut -d: -f1 | head -n1 || true)
if [ -n "${start:-}" ]; then
# print 200 lines from start
sed -n "${start},$((start+220))p" src/tacos.core/Migrations/20260526112751_DeduplicateCourseDescriptionsFromRawProcedure.cs | cat -n
fiRepository: ucdavis/tacos
Length of output: 10444
Make the course description dedup deterministic (and ensure rollback works)
ROW_NUMBER()is ordered by[UpdatedOn],[CreatedOn],TRY_CONVERT(int, [EffectiveTerm]),[Course], butdbo.CourseDescriptionRawhas noPRIMARY KEY/IDENTITY/UNIQUEcolumn to serve as a guaranteed per-row tiebreaker—if multiple raw rows tie on those ORDER BY columns within the same[SubjectCode],[CourseNumber],[Status]partition, the chosenRowNumber = 1row is nondeterministic.- Add a real stable final tiebreaker (e.g., add an
IDENTITY/PK toCourseDescriptionRawand include it in the ORDER BY), or explicitly detect/throw on ambiguous duplicates before replacingdbo.CourseDescription. Down()is empty in20260526112751_DeduplicateCourseDescriptionsFromRawProcedure, so downgrading won’t restore the previous procedure definition (theUp()usesCREATE OR ALTER PROCEDURE).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/tacos.core/Migrations/20260526112751_DeduplicateCourseDescriptionsFromRawProcedure.cs`
around lines 88 - 92, The ROW_NUMBER() ordering in the migration's procedure is
nondeterministic because dbo.CourseDescriptionRaw lacks a stable per-row
tiebreaker; modify the migration so the raw table has a stable PK/IDENTITY
(e.g., add an identity column to CourseDescriptionRaw) and include that identity
column in the ORDER BY clause used to compute ROW_NUMBER() in the procedure (or
alternatively add explicit detection and THROW when ties remain), and also
implement the migration's Down() in
20260526112751_DeduplicateCourseDescriptionsFromRawProcedure to restore the
previous procedure definition (or DROP the altered procedure) so downgrades
revert the change.
Comment on lines
+190
to
+192
| protected override void Down(MigrationBuilder migrationBuilder) | ||
| { | ||
| } |
Contributor
There was a problem hiding this comment.
Restore the previous procedure in Down.
This migration changes the procedure's behavior, but the empty Down leaves the deduplicating version in place after a rollback to the prior migration. Please reapply the previous procedure body here, or fail fast if that downgrade is intentionally unsupported.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/tacos.core/Migrations/20260526112751_DeduplicateCourseDescriptionsFromRawProcedure.cs`
around lines 190 - 192, The Down method in the
DeduplicateCourseDescriptionsFromRawProcedure migration is empty so rolling back
will leave the deduplicating procedure in place; restore the previous
stored-procedure body (the SQL that was replaced in Up) inside protected
override void Down(MigrationBuilder migrationBuilder) by reapplying the original
CREATE/ALTER PROCEDURE or by executing the prior SQL via
migrationBuilder.Sql(...), or if downgrades are intentionally unsupported throw
a clear exception (e.g., throw new InvalidOperationException) from Down to fail
fast; update the Down method in the migration class accordingly.
Comment on lines
+100
to
101
| await _sqlGateway.ReplaceCourseDescriptionsFromRawAsync(); | ||
| await _sqlGateway.RebuildCoursesAsync(processingWindow.AcademicTermCodes); |
Contributor
There was a problem hiding this comment.
Keep the description refresh and course rebuild atomic.
ReplaceCourseDescriptionsFromRawAsync() commits before the main rebuild starts. If usp_RebuildCoursesFromProcessingWindow times out or fails, CourseDescription is left on the new snapshot while the course rebuild stays on the old one. Please wrap both steps in one transaction or move the replace into the rebuild procedure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tacos.mvc/Services/CourseRebuildService.cs` around lines 100 - 101,
Summary: ReplaceCourseDescriptionsFromRawAsync commits before
RebuildCoursesAsync, leaving snapshot inconsistent if
usp_RebuildCoursesFromProcessingWindow fails; make both steps atomic. Fix:
modify the SQL gateway to provide a single operation (e.g.,
ReplaceAndRebuildCoursesFromRawAsync or a transaction-aware RebuildCoursesAsync
overload) that opens one DB connection and begins a transaction, calls the
replacement logic (currently in ReplaceCourseDescriptionsFromRawAsync) and then
calls the rebuild stored proc (usp_RebuildCoursesFromProcessingWindow via
RebuildCoursesAsync) on the same connection/transaction, committing only after
both succeed (or alternatively move the replace logic into the stored proc
itself); update CourseRebuildService to call the new atomic gateway method
instead of calling ReplaceCourseDescriptionsFromRawAsync and RebuildCoursesAsync
separately.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fabric time
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation