Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Validate any SQL migration scripts are the most recent#6652

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
withinfocus wants to merge5 commits intomain
base:main
Choose a base branch
Loading
fromdb-validation
Open
Changes from1 commit
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
PrevPrevious commit
NextNext commit
Make string checks more robust
  • Loading branch information
@withinfocus
withinfocus committedNov 26, 2025
commit2f1fe507229d0921b09f77202ed7ae83cc9a40ab

Some comments aren't visible on the classic Files Changed page.

35 changes: 30 additions & 5 deletions.github/workflows/test-database.yml
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -280,6 +280,9 @@ jobs:
- name: Validate migration order for pull request
if: github.event_name == 'pull_request'
run: |
# Use C locale for consistent, locale-independent string comparison
export LC_ALL=C

# Get list of migrations from main branch
git fetch origin main:main
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

❌Finding 1 continued: This fetch command should have error handling. If it fails, the script will continue with a stale or non-existent localmain branch.

Suggestion:

if! git fetch origin main:main;thenecho"ERROR: Failed to fetch main branch"exit 1fi

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

⚠️Missing error handling: Ifgit fetch fails, the script continues with potentially stale or missing data, leading to incorrect validation results.

Suggested change
git fetch origin main:main
if !git fetch origin main:main; then
echo "ERROR:Failed to fetch main branch"
exit 1
fi

MAIN_MIGRATIONS=$(git ls-tree -r --name-only main -- util/Migrator/DbScripts/*.sql | sort)
Expand DownExpand Up@@ -312,6 +315,15 @@ jobs:
while IFS= read -r migration; do
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

❌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:

  1. Rename existing non-compliant migrations before merging this PR (preferred for consistency)
  2. Make regex more permissive:^[0-9]{4}-[0-9]{1,2}-[0-9]{1,2}_[0-9]{2}_.+\.sql$

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

❌Critical: Regex allows non-compliant migration that breaks sorting

The repository contains2025-10-3_00_AddOrganizationIntegration_ReadByTeamsConfigurationTenantIdTeamIdStoredProcedure.sql (note:10-3 not10-03).

Problem: Lexicographic string comparison produces incorrect ordering:

  • "2025-10-3" sortsAFTER"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.sql

Then restore strict regex validation:

Suggested change
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 "ERROR: Migration '$MIGRATION_NAME' does not match expected format"
echo "Expected format: YYYY-MM-DD_NN_Description.sql (with leading zeros)"
echo "Example: 2025-01-15_00_MyMigration.sql"
VALIDATION_FAILED=1
continue
fi

# Compare migration name with last main migration
if [[ "$MIGRATION_NAME" < "$LAST_MAIN_MIGRATION" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

💭Finding 5 - Multiple new migrations: When a PR adds multiple migrations (e.g.,2025-11-27_00_A.sql and2025-11-26_00_B.sql), this only validates each againstLAST_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?

echo "ERROR: New migration '$MIGRATION_NAME' is not chronologically after '$LAST_MAIN_MIGRATION'"
Expand All@@ -323,16 +335,16 @@ jobs:

echo ""
if [ $VALIDATION_FAILED -eq 1 ]; then
echo "FAILED: One or more migrations are not in chronological order"
echo "FAILED: One or more migrations are not in chronological order or incorrectly formatted"
echo ""
echo "Migration files must be named to execute in chronological order."
echo "New migrations must have dates/names that come after existing migrations."
echo "Expected format: YYYY-MM-DD_NN_Description.sql"
echo "Expected format: YYYY-MM-DD_NN_Description.sql (with leading zeros)"
echo ""
echo "To fix this issue:"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

🎨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'"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

🎨Enhancement: Add specific git commands to help developers fix issues:

Suggested change
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 ""

echo " 1. Rename your migration file(s) to use a date after $LAST_MAIN_MIGRATION"
echo " 2. Ensure the date format is YYYY-MM-DD(with leading zeros)"
echo " 3. Use _NN suffix if multiple migrations exist for the same date"
echo " 2. Ensure the date format is YYYY-MM-DD with leading zeros (e.g., 2025-01-05, not 2025-1-5)"
echo " 3. Use _NN suffix(e.g., _00, _01)if multiple migrations exist for the same date"
exit 1
fi

Expand All@@ -342,6 +354,9 @@ jobs:
- name: Validate migration order for push
if: github.event_name == 'push' || github.event_name == 'workflow_dispatch'
run: |
# Use C locale for consistent, locale-independent string comparison
export LC_ALL=C

# Get list of migrations from previous commit
PREV_MIGRATIONS=$(git ls-tree -r --name-only HEAD~1 -- util/Migrator/DbScripts/*.sql 2>/dev/null | sort || echo "")

Expand DownExpand Up@@ -374,6 +389,15 @@ jobs:
while IFS= read -r migration; do
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
echo "ERROR: Migration '$MIGRATION_NAME' does not match expected format"
echo "Expected format: YYYY-MM-DD_NN_Description.sql (with leading zeros)"
echo "Example: 2025-01-15_00_MyMigration.sql"
VALIDATION_FAILED=1
continue
fi

# Compare migration name with last previous migration
if [[ "$MIGRATION_NAME" < "$LAST_PREV_MIGRATION" ]]; then
echo "ERROR: New migration '$MIGRATION_NAME' is not chronologically after '$LAST_PREV_MIGRATION'"
Expand All@@ -385,10 +409,11 @@ jobs:

echo ""
if [ $VALIDATION_FAILED -eq 1 ]; then
echo "FAILED: One or more migrations are not in chronological order"
echo "FAILED: One or more migrations are not in chronological order or incorrectly formatted"
echo ""
echo "Migration files must be named to execute in chronological order."
echo "New migrations must have dates/names that come after existing migrations."
echo "Expected format: YYYY-MM-DD_NN_Description.sql (with leading zeros)"
exit 1
fi
else
Expand Down
[8]ページ先頭
©2009-2025 Movatter.jp