chore: bump FAB to 5.0.2 #36086
chore: bump FAB to 5.0.2 #36086
Conversation
|
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #36086 +/- ##
===========================================
+ Coverage 0 68.53% +68.53%
===========================================
Files 0 634 +634
Lines 0 46302 +46302
Branches 0 4991 +4991
===========================================
+ Hits 0 31731 +31731
- Misses 0 13325 +13325
- Partials 0 1246 +1246
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dpgaspar
requested review from
betodealmeida,
eschutho,
geido,
kgabryje,
michael-s-molina,
mistercrunch,
rusackas and
sadpandajoe
as code owners
dosubot
bot
added
change:backend
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Duplicate Column Check Logic ▹ view | |
 |
Incomplete upgrade skip condition ▹ view | |
|
Incorrect downgrade skip condition ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset/migrations/versions/2025-11-12_12-54_x2s8ocx6rto6_expand_username_field_to_128_chars.py | ✅ |
| superset/migrations/shared/utils.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| def upgrade(): | ||
| """Expand ab_user.username field from 64 to 128 characters.""" | ||
| # Check current column length to avoid errors if already upgraded | ||
| column_info = get_table_column("ab_user", "username") |
There was a problem hiding this comment.
Duplicate Column Check Logic 
Tell me more
What is the issue?
The code for checking column information and handling migration logic is duplicated between upgrade() and downgrade() functions.
Why this matters
Code duplication increases maintenance burden and risk of inconsistencies when changes are needed. If the column checking logic needs to be modified, it would require changes in multiple places.
Suggested change ∙ Feature Preview
Extract the common column checking logic into a helper function:
def get_username_column_length():
column_info = get_table_column("ab_user", "username")
if column_info is None:
return None
current_type = column_info.get("type")
return getattr(current_type, "length", None)Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| current_type = column_info.get("type") | ||
| current_length = getattr(current_type, "length", None) | ||
|
|
||
| if current_length is not None and current_length <= 64: |
There was a problem hiding this comment.
Incorrect downgrade skip condition 
Tell me more
What is the issue?
The downgrade function uses '<= 64' condition which will skip the downgrade when the column length is exactly 64, but it should only skip when the length is already less than 64.
Why this matters
This prevents the downgrade from running when it should, making the migration non-reversible in cases where the column is exactly 64 characters.
Suggested change ∙ Feature Preview
Change the condition to check only for lengths less than 64:
if current_length is not None and current_length < 64:Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| current_type = column_info.get("type") | ||
| current_length = getattr(current_type, "length", None) | ||
|
|
||
| if current_length is not None and current_length == 128: |
There was a problem hiding this comment.
Incomplete upgrade skip condition
Tell me more
What is the issue?
The upgrade function uses '== 128' condition which will not skip the migration if the column length is greater than 128, potentially causing issues with larger existing column sizes.
Why this matters
This could lead to unnecessary schema changes or errors when the column is already larger than the target size of 128 characters.
Suggested change ∙ Feature Preview
Change the condition to check for lengths greater than or equal to 128:
if current_length is not None and current_length >= 128:Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| ) | ||
| return | ||
|
|
||
| with op.batch_alter_table("ab_user") as batch_op: |
There was a problem hiding this comment.
Just checking that we don't have a utils to do batch alters. If not then this looks good to me, else we should switch to the util
There was a problem hiding this comment.
Good call, just checked and there isn't one
| # under the License. | ||
| """Expand username field to 128 chars | ||
|
|
||
| Revision ID: x2s8ocx6rto6 |
There was a problem hiding this comment.
#TIL Alembic revisions don't need to hexadecimal...
(cherry picked from commit b051f77)
(cherry picked from commit b051f77)
(cherry picked from commit b051f77)
(cherry picked from commit b051f77)
4 participants
SUMMARY
Upgrades flask-appbuilder to 5.0.2
Release notes: https://github.com/dpgaspar/Flask-AppBuilder/releases/tag/v5.0.2
This release will increase the
user.usernamelength from 64 chars to 128 charsBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION