Skip to content

Conversation

@galvana
Copy link
Contributor

@galvana galvana commented Oct 29, 2025

Description Of Changes

Added a test to validate migration downgrades. A diff is generated against the target branch to determine the new migrations. The downgrade and upgrade steps are then tested for each migration.

Code Changes

  • Added nox -s check_migration_downgrade command
  • Added new Migration-Checks step to .github/workflows/backend_checks.yml
  • The result of Migration-Checks is included in the backend checks summary

Steps to verify

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

@galvana galvana requested a review from a team as a code owner October 29, 2025 17:10
@galvana galvana requested review from vcruces and removed request for a team October 29, 2025 17:10
@vercel
Copy link

vercel bot commented Oct 29, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
fides-plus-nightly Ready Ready Preview Comment Oct 30, 2025 11:39pm
1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
fides-privacy-center Ignored Ignored Oct 30, 2025 11:39pm

@galvana galvana requested review from erosselli and removed request for vcruces October 29, 2025 17:11
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Greptile Summary

Adds automated testing for migration downgrades to ensure migrations can be safely reversed.

Key Changes:

  • Implements check_new_migrations_upgrade_downgrade() function that detects new migration files via git diff and tests their upgrade/downgrade functionality
  • Adds new CI job Migration-Checks that runs both check_migrations and check_migration_downgrade in parallel
  • Separates migration checks from Misc-Tests into dedicated job for better organization

Critical Issue Found:
The logic in _test_migration_upgrade_downgrade() has a serious bug when testing multiple new migrations in sequence. After testing migration A, the database is left at revision A, but testing migration B expects the database to be at HEAD (revision B). This will cause the downgrade test for migration B to fail.

Confidence Score: 1/5

  • This PR contains a critical logic error that will cause test failures when multiple migrations are added in sequence
  • The migration downgrade testing logic has a critical bug that breaks when testing multiple sequential migrations. The test leaves the database in an incorrect state between iterations, causing subsequent tests to fail. This needs to be fixed before merging.
  • src/fides/api/db/database.py requires immediate attention - the loop testing multiple migrations needs to restore database to HEAD after each iteration

Important Files Changed

File Analysis

Filename Score Overview
.github/workflows/backend_checks.yml 5/5 Adds new Migration-Checks job to test migration downgrades separately, moves check_migrations from Misc-Tests to dedicated job for better organization
noxfiles/ci_nox.py 5/5 Adds check_migration_downgrade nox session, minor formatting improvement to tuple unpacking for consistency
src/fides/api/db/database.py 1/5 Adds migration downgrade testing logic with git-based detection; contains critical bug when testing multiple sequential migrations

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@galvana
Copy link
Contributor Author

galvana commented Oct 29, 2025

@greptileai

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Greptile Summary

This PR adds automated testing for migration downgrades by introducing a new check_migration_downgrade nox command that:

  • Detects new migrations via git diff against the target/base branch
  • Tests each migration's upgrade and downgrade in sequence
  • Integrates into CI via a new Migration-Checks job in the GitHub workflow

The implementation uses a two-phase approach: first downgrading through all new migrations (newest to oldest), then upgrading back (oldest to newest). This validates that migrations can be safely reversed and tests interactions between sequential migrations.

Key changes:

  • Added check_new_migrations_upgrade_downgrade() function and supporting helpers in database.py
  • Created check_migration_downgrade nox session in ci_nox.py
  • Split migration checks into dedicated Migration-Checks workflow job (previously in Misc-Tests)
  • Matrix strategy runs both check_migrations and check_migration_downgrade tests

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The code adds comprehensive testing for migration downgrades without modifying any production code paths. The implementation is well-structured with proper error handling, clear documentation, and logical flow. The sequential testing approach correctly validates migrations by downgrading through all new migrations and then upgrading back. All changes are additive (new test infrastructure) with no modifications to existing functionality.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
src/fides/api/db/database.py 5/5 Added comprehensive migration upgrade/downgrade testing that detects new migrations via git diff and validates them in sequence

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@codecov
Copy link

codecov bot commented Oct 29, 2025

Codecov Report

❌ Patch coverage is 9.87654% with 73 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.29%. Comparing base (44024ab) to head (498052d).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/fides/api/db/database.py 9.87% 73 Missing ⚠️

❌ Your patch status has failed because the patch coverage (9.87%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6873      +/-   ##
==========================================
- Coverage   87.47%   87.29%   -0.19%     
==========================================
  Files         523      523              
  Lines       34088    34169      +81     
  Branches     3916     3931      +15     
==========================================
+ Hits        29820    29827       +7     
- Misses       3412     3485      +73     
- Partials      856      857       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Base automatically changed from ENG-1704-identity-definition-models to main October 29, 2025 19:00
Copy link
Contributor

@erosselli erosselli left a comment

Choose a reason for hiding this comment

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

This is cool, let's send out a message in the eng channel about it so people know what the new CI check is :)

print("No migrations need to be generated.")


def _get_base_branch_for_comparison() -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add # pragma: no cover to this function and the one below so they don't get counted for coverage reports?

@vercel
Copy link

vercel bot commented Oct 31, 2025

Deployment failed with the following error:

You must set up Two-Factor Authentication before accessing this team.

View Documentation: https://vercel.com/docs/two-factor-authentication

@galvana galvana added this pull request to the merge queue Nov 3, 2025
Merged via the queue into main with commit 4cb7648 Nov 3, 2025
65 of 68 checks passed
@galvana galvana deleted the migration-downgrade-test branch November 3, 2025 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants