Validate any SQL migration scripts are the most recent #6652
Conversation
|
Claude finished @withinfocus's task —— View job Review Update - Recent Changes
Summary of Changes Since Last ReviewThe author has made 4 commits addressing previous review feedback:
Status of Previous FindingsFinding 1 (Format enforcement) - ❌ PARTIALLY FIXED, NEW ISSUE INTRODUCED The author made the regex permissive ( Critical flaw: The migration This means:
Recommendation: Either:
Finding 2 (git fetch error handling) - Line git fetch origin main:mainIf this fails silently, validation proceeds with stale/missing data, producing incorrect results. Finding 3 (Temporary file cleanup) - ✅ FIXED The author intentionally removed cleanup per commit 1ab6ad3, noting the files are in Finding 4 (Multiple new migrations validation) - 💭 BEHAVIOR CONFIRMED The script validates each new migration against Finding 5 (Enhanced error messages) - 🎨 UNFIXED Error messages at New Critical FindingFinding 6 - ❌ Broken chronological ordering due to existing non-compliant migration The repository contains Problem: Lexicographic string comparison (used by the script) produces different ordering than chronological date ordering: # String comparison (what the script does):
"2025-10-3" > "2025-10-30" # TRUE - compares character by character
# At position 8: '3' (from "10-3") > '3' (from "10-30") is false
# At position 9: '_' (from "3_") > '0' (from "30") is TRUE
# Chronological comparison (what we want):
2025-10-03 < 2025-10-30 # TRUEImpact:
Evidence from codebase: Recommended fix: Rename the problematic file before merging this PR: git mv util/Migrator/DbScripts/2025-10-3_00_AddOrganizationIntegration_ReadByTeamsConfigurationTenantIdTeamIdStoredProcedure.sql \
util/Migrator/DbScripts/2025-10-03_00_AddOrganizationIntegration_ReadByTeamsConfigurationTenantIdTeamIdStoredProcedure.sqlGood Practices Observed
Action Items for Author
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6652 +/- ##
==========================================
+ Coverage 53.27% 57.12% +3.84%
==========================================
Files 1906 1906
Lines 84973 84973
Branches 7639 7639
==========================================
+ Hits 45268 48538 +3270
+ Misses 37952 34607 -3345
- Partials 1753 1828 +75 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great job! No new security vulnerabilities introduced in this pull request |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| export LC_ALL=C | ||
| # Get list of migrations from main branch | ||
| git fetch origin main:main |
There was a problem hiding this comment.
❌ Finding 1 continued: This fetch command should have error handling. If it fails, the script will continue with a stale or non-existent local main branch.
Suggestion:
if ! git fetch origin main:main; then
echo "ERROR: Failed to fetch main branch"
exit 1
fi
| MIGRATION_NAME=$(basename "$migration") | ||
| # Validate migration filename format (YYYY-MM-DD_NN_Description.sql with leading zeros) | ||
| if ! [[ "$MIGRATION_NAME" =~ ^[0-9]{4}-[0-9]{2}-[0-9]{2}_[0-9]{2}_.+\.sql$ ]]; then |
There was a problem hiding this comment.
❌ Finding 1 - Format enforcement issue: This regex requires leading zeros ([0-9]{2} for month and day), but the repository already contains at least one migration that violates this:
2025-10-3_00_AddOrganizationIntegration_ReadByTeamsConfigurationTenantIdTeamIdStoredProcedure.sql
This will cause validation failures on PRs that rebase against main if the problematic migration gets included.
Options:
- Rename existing non-compliant migrations before merging this PR (preferred for consistency)
- Make regex more permissive:
^[0-9]{4}-[0-9]{1,2}-[0-9]{1,2}_[0-9]{2}_.+\.sql$
| fi | ||
| # Compare migration name with last main migration | ||
| if [[ "$MIGRATION_NAME" < "$LAST_MAIN_MIGRATION" ]]; then |
There was a problem hiding this comment.
💭 Finding 5 - Multiple new migrations: When a PR adds multiple migrations (e.g., 2025-11-27_00_A.sql and 2025-11-26_00_B.sql), this only validates each against LAST_MAIN_MIGRATION, not against each other.
The out-of-order new migrations would be caught (B would fail since it's < A alphabetically), but the error message might be confusing since it references the main branch rather than the other new migration.
Is this the intended behavior, or should we also validate ordering within the set of new migrations?
| # Save to temporary files for comparison | ||
| echo "$MAIN_MIGRATIONS" > /tmp/main_migrations.txt | ||
| echo "$CURRENT_MIGRATIONS" > /tmp/current_migrations.txt |
There was a problem hiding this comment.
♻️ Finding 4 - Cleanup: Consider adding cleanup for temporary files at the end of the script:
rm -f /tmp/main_migrations.txt /tmp/current_migrations.txt
| echo "New migrations must have dates/names that come after existing migrations." | ||
| echo "Expected format: YYYY-MM-DD_NN_Description.sql (with leading zeros)" | ||
| echo "" | ||
| echo "To fix this issue:" |
There was a problem hiding this comment.
🎨 Finding 6 - Enhanced error message: Consider adding the specific git command to help developers fix the issue:
echo "To fix this issue:"
echo " 1. Rename using git:"
echo " git mv util/Migrator/DbScripts/$MIGRATION_NAME util/Migrator/DbScripts/YYYY-MM-DD_NN_NewName.sql"
echo " 2. Choose a date after $LAST_MAIN_MIGRATION"
echo " 3. Commit the change: git commit -m 'Rename migration to fix ordering'"
| export LC_ALL=C | ||
| # Get list of migrations from main branch | ||
| git fetch origin main:main |
There was a problem hiding this comment.
git fetch fails, the script continues with potentially stale or missing data, leading to incorrect validation results.
| git fetch origin main:main | |
| if ! git fetch origin main:main; then | |
| echo "ERROR: Failed to fetch main branch" | |
| exit 1 | |
| fi |
| # Validate NEW migration filename format - enforce strict format for new migrations | ||
| # Required format: YYYY-MM-DD_NN_Description.sql (with leading zeros in date and sequence) | ||
| if ! [[ "$MIGRATION_NAME" =~ ^[0-9]{4}-[0-9]{2}-[0-9]{2}_[0-9]{2}_.+\.sql$ ]]; then |
There was a problem hiding this comment.
❌ Critical: Regex allows non-compliant migration that breaks sorting
The repository contains 2025-10-3_00_AddOrganizationIntegration_ReadByTeamsConfigurationTenantIdTeamIdStoredProcedure.sql (note: 10-3 not 10-03).
Problem: Lexicographic string comparison produces incorrect ordering:
"2025-10-3"sorts AFTER"2025-10-30"in ASCII comparison- At character position 9:
'3'(from"10-3_") vs'0'(from"10-30") - Character
'3'has higher ASCII value than'0', so"10-3">"10-30"
Impact:
- New migrations dated 2025-10-30 through 2025-11-03 will incorrectly fail validation
- Chronological order is violated in the migration list
- Sorting doesn't reflect actual date order
Fix required before merging: Rename the non-compliant migration:
git mv util/Migrator/DbScripts/2025-10-3_00_AddOrganizationIntegration_ReadByTeamsConfigurationTenantIdTeamIdStoredProcedure.sql \
util/Migrator/DbScripts/2025-10-03_00_AddOrganizationIntegration_ReadByTeamsConfigurationTenantIdTeamIdStoredProcedure.sqlThen restore strict regex validation:
| if ! [[ "$MIGRATION_NAME" =~ ^[0-9]{4}-[0-9]{2}-[0-9]{2}_[0-9]{2}_.+\.sql$ ]]; then | |
| if ! [[ "$MIGRATION_NAME" =~ ^[0-9]{4}-[0-9]{2}-[0-9]{2}_[0-9]{2}_.+\.sql$ ]]; then |
| echo " 3. Include a 2-digit sequence number (e.g., _00, _01)" | ||
| echo " 4. Have a filename that sorts after the last migration in main" | ||
| echo "" | ||
| echo "To fix this issue:" |
There was a problem hiding this comment.
🎨 Enhancement: Add specific git commands to help developers fix issues:
| echo "To fix this issue:" | |
| echo "To fix this issue:" | |
| echo " 1. Rename using git:" | |
| echo " git mv util/Migrator/DbScripts/$MIGRATION_NAME util/Migrator/DbScripts/YYYY-MM-DD_NN_NewName.sql" | |
| echo " 2. Choose a date after $LAST_MAIN_MIGRATION" | |
| echo " 3. Commit: git commit -m 'Rename migration to fix chronological ordering'" | |
| echo "" |
|
Had some good back and forth from Claude that I wanted to commit explicitly, but I think this is at a good point. What it doesn't know is we will not ever introduce past migrations, so I don't think we should do anything further here. |
There was a problem hiding this comment.
Thanks for following this up! I think it'll be useful.
I'm not sure how to handle the incorrectly named migration, seems like a fair comment by Claude, but fixing the name will cause it to be re-run. I guess that's OK as long as we verify that it's properly idempotent (our scripts should be but this is not tested/enforced).
| echo "ERROR: Migration '$MIGRATION_NAME' does not match required format" | ||
| echo "Required format: YYYY-MM-DD_NN_Description.sql" |
There was a problem hiding this comment.
👏 Also a nice improvement here.
| echo "SUCCESS: All new migrations are correctly named and in chronological order" | ||
| shell: bash | ||
|
|
||
| - name: Validate new migrations for push |
There was a problem hiding this comment.
I assume these 2 jobs are identical except for the source of the current migrations? I suggest that the comparison logic could be moved into a script for reuse, e.g.
// job-specific logic to get the migrations and write them to temp files
// then run the comparison/validation
sh verify-migrations.sh /tmp/prev_migrations.txt /tmp/current_migrations.txt
🎟️ Tracking
https://bitwarden.slack.com/archives/C065AUDER62/p1764108324477319
📔 Objective
Adds automated validation to ensure database migrations are added in chronological order.
Changes
check-migration-orderjob to thetest-database.ymlworkflowWhy
Database migrations must execute in chronological order. This check prevents merge conflicts and ensures migrations run in the correct sequence by validating that new migration filenames (format:
YYYY-MM-DD_NN_Description.sql) sort alphabetically after existing ones.⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes