Skip to content

fix(migrations): replace placeholder revision ID in granular export migration#40134

Closed
sadpandajoe wants to merge 1 commit into
masterfrom
fix/migration-revision-id
Closed

fix(migrations): replace placeholder revision ID in granular export migration#40134
sadpandajoe wants to merge 1 commit into
masterfrom
fix/migration-revision-id

Conversation

@sadpandajoe
Copy link
Copy Markdown
Member

Summary

The migration added in #38361 (add_granular_export_permissions) used a placeholder Alembic revision ID (a1b2c3d4e5f6) instead of a properly generated one. This replaces it with a real random hex ID (41292324bf4e) and updates the downstream migration's down_revision to match.

Changes:

Migration chain (after fix):

4b2a8c9d3e1f (create_tasks_table)
  → 41292324bf4e (add_granular_export_permissions)  ← was a1b2c3d4e5f6
    → ce6bd21901ab (migrate_deckgl_and_mapbox)
      → 33d7e0e21daa (add_semantic_layers_and_views)

Note for deployments already past this migration: The alembic_version table only stores the current head, so databases that have already run all migrations will not be affected. Any database stopped exactly at the old revision would need alembic stamp 41292324bf4e.

@bito-code-review
Copy link
Copy Markdown
Contributor

bito-code-review Bot commented May 15, 2026

Code Review Agent Run #2076

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: 6b912ba..6b912ba
    • superset/migrations/versions/2026-03-02_00-00_ce6bd21901ab_migrate_deckgl_and_mapbox.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@codecov
Copy link
Copy Markdown

codecov Bot commented May 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.17%. Comparing base (2b71d96) to head (594a9e9).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #40134   +/-   ##
=======================================
  Coverage   64.17%   64.17%           
=======================================
  Files        2590     2590           
  Lines      138087   138087           
  Branches    32039    32039           
=======================================
  Hits        88615    88615           
  Misses      47947    47947           
  Partials     1525     1525           
Flag Coverage Δ
hive 39.47% <ø> (ø)
mysql 59.17% <ø> (ø)
postgres 59.25% <ø> (ø)
presto 41.16% <ø> (ø)
python 60.69% <ø> (ø)
sqlite 58.89% <ø> (ø)
unit 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

…igration

The migration added in #38361 used a placeholder Alembic revision ID
(a1b2c3d4e5f6) instead of a properly generated one. Replace it with a
real random hex ID and update the downstream migration's down_revision.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@sadpandajoe sadpandajoe force-pushed the fix/migration-revision-id branch from 6b912ba to 594a9e9 Compare May 15, 2026 00:06
# revision identifiers, used by Alembic.
revision = "ce6bd21901ab"
down_revision = "a1b2c3d4e5f6"
down_revision = "41292324bf4e"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: Changing the parent revision to a brand-new ID breaks upgrade continuity for any environment currently stamped at the old placeholder revision (a1b2c3d4e5f6): Alembic will fail because the current DB revision no longer exists in the script directory. Preserve compatibility by keeping a legacy revision node (or adding a bridge migration) so both already-migrated and fresh environments can upgrade without manual stamping. [api mismatch]

Severity Level: Major ⚠️
- ❌ Alembic upgrade fails from databases stamped old placeholder revision.
- ⚠️ Deployment pipeline requires manual alembic stamp to recover upgrades.
Steps of Reproduction ✅
1. Note the new parent revision chain in the PR code:

   -
   `superset/migrations/versions/2026-03-02_12-00_41292324bf4e_add_granular_export_permissions.py`
   defines `Revision ID: 41292324bf4e` and `down_revision = "4b2a8c9d3e1f"` (verified via
   Grep: lines 19–21 and 26–27).

   -
   `superset/migrations/versions/2026-03-02_00-00_ce6bd21901ab_migrate_deckgl_and_mapbox.py`
   defines `revision = "ce6bd21901ab"` and at line 47 (in this PR hunk) sets
   `down_revision = "41292324bf4e"`.

2. Confirm that the previous placeholder revision ID no longer exists in the migration
scripts:

   - A repo-wide Grep for `a1b2c3d4e5f6` under `/workspace/superset` returns no matches,
   so there is no migration file whose `revision` equals `a1b2c3d4e5f6`.

3. Consider an environment that previously ran the original migration from PR #38361
before this fix:

   - In that environment, the Alembic `alembic_version` table was stamped to the
   placeholder revision ID `a1b2c3d4e5f6` (the old revision value that this PR replaces
   with `41292324bf4e`).

   - After deploying this PR's code (which only knows `41292324bf4e` and has
   `down_revision = "41292324bf4e"` at line 47), the database still reports `a1b2c3d4e5f6`
   as its current revision.

4. Run the standard migration upgrade command (e.g. `superset db upgrade`, which drives
Alembic using the script directory `superset/migrations/versions`):

   - Alembic reads the current DB revision `a1b2c3d4e5f6` from `alembic_version`, then
   looks for a migration script with `revision = "a1b2c3d4e5f6"`.

   - Because that revision no longer exists in the script directory (step 2), Alembic
   raises a `RevisionError` like "Can't locate revision identified by 'a1b2c3d4e5f6'",
   blocking upgrades until an operator manually runs `alembic stamp 41292324bf4e` or
   equivalent.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset/migrations/versions/2026-03-02_00-00_ce6bd21901ab_migrate_deckgl_and_mapbox.py
**Line:** 47:47
**Comment:**
	*Api Mismatch: Changing the parent revision to a brand-new ID breaks upgrade continuity for any environment currently stamped at the old placeholder revision (`a1b2c3d4e5f6`): Alembic will fail because the current DB revision no longer exists in the script directory. Preserve compatibility by keeping a legacy revision node (or adding a bridge migration) so both already-migrated and fresh environments can upgrade without manual stamping.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Doesn't alembic work by figuring out what the current version is and either

  1. if it is ahead do nothing on upgrades, can trace for down grades
  2. if behind, it'll just upgrade the current version
  3. if it is the exact, is where we might have a problem, but should be pretty easy to get around that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not quite — Alembic does not treat this as “if it’s ahead, do nothing.”

It first reads the DB’s current revision from alembic_version and then tries to find that exact revision node in the migration graph. So if an environment is stamped with a1b2c3d4e5f6 and that revision no longer exists in the scripts, Alembic can’t build the path and will fail before it can decide what to upgrade.

Why this PR’s change is risky

Changing:

down_revision = "a1b2c3d4e5f6"

to:

down_revision = "41292324bf4e"

fixes the chain for new installs, but it breaks existing databases that already recorded the old placeholder revision.

Safer fix

Keep a legacy node for a1b2c3d4e5f6 in the migration history, or add a bridge migration that preserves the old revision ID and points forward to the new one. That way both cases work:

  • old deployments can still resolve their current revision
  • fresh deployments follow the corrected chain

Conclusion

So the issue raised in the review is valid: Alembic needs the old revision to remain reachable. If you want, I can help suggest the smallest bridge-migration approach to preserve compatibility without manual stamping.

@bito-code-review
Copy link
Copy Markdown
Contributor

The flagged issue is correct: replacing the placeholder revision ID 'a1b2c3d4e5f6' with '41292324bf4e' breaks upgrade continuity for databases already stamped with the old ID, as Alembic won't find the old revision.

To resolve, create a no-op legacy migration with revision 'a1b2c3d4e5f6' and update the down_revision in the '41292324bf4e' file to reference it, maintaining the chain.

There are no other comments on this PR.

superset/migrations/versions/2026-03-02_12-00_a1b2c3d4e5f6_add_granular_export_permissions.py

# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements.  See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership.  The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License.  You may obtain a copy of the License at
#
#   http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied.  See the License for the
# specific language governing permissions and limitations
# under the License.
"""add granular export permissions

Revision ID: a1b2c3d4e5f6
Revises: 4b2a8c9d3e1f
Create Date: 2026-03-02 12:00:00.000000

"""

from alembic import op  # noqa: E402

# revision identifiers, used by Alembic.
revision = "a1b2c3d4e5f6"
down_revision = "4b2a8c9d3e1f"


def upgrade():
    pass


def downgrade():
    pass

superset/migrations/versions/2026-03-02_12-00_41292324bf4e_add_granular_export_permissions.py

down_revision = "a1b2c3d4e5f6"

@bito-code-review
Copy link
Copy Markdown
Contributor

bito-code-review Bot commented May 15, 2026

Code Review Agent Run #80dd08

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: 594a9e9..594a9e9
    • superset/migrations/versions/2026-03-02_00-00_ce6bd21901ab_migrate_deckgl_and_mapbox.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@betodealmeida
Copy link
Copy Markdown
Member

This looks good, but I wouldn't merge it — there's nothing wrong with using a1b2c3d4e5f6, it's a valid revision (in fact, any string is a valid revision), and I don't see any benefit in changing — on the contrary, it might break things for deployments currently in that revision when they try to upgrade.

@sadpandajoe
Copy link
Copy Markdown
Member Author

closing

@sadpandajoe sadpandajoe deleted the fix/migration-revision-id branch May 18, 2026 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:db-migration PRs that require a DB migration size/XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants