Repository navigation
fix(marshmallow): add compatibility layer for Flask-AppBuilder with marshmallow 4.x - #35920
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
|
|
/korbit-review |
![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 |
|---|---|---|
 |
Redundant JSON serialization without caching ▹ view | |
 |
Sanitized data stored but not consumed ▹ view | |
|
Missing import for Length validator ▹ view | |
|
Module-level patch execution adds import overhead ▹ view | |
|
Marshmallow patch applied at module level ▹ view | |
|
Inefficient retry loop with redundant field processing ▹ view | |
 |
Unmanaged Monkey-Patching of External Library ▹ view | |
|
Improper Logging Mechanism ▹ view | |
|
Unreliable KeyError message parsing ▹ view | |
|
Unsafe declared_fields modification ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset/advanced_data_type/schemas.py | ✅ |
| superset/marshmallow_compatibility.py | ✅ |
| superset/themes/schemas.py | ✅ |
| superset/app.py | ✅ |
| superset/db_engine_specs/databend.py | ✅ |
| superset/reports/schemas.py | ✅ |
| superset/charts/schemas.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.
|
@mistercrunch Hi, could you re-run workflows on this PR? Also lmk if you have any concerns/suggestions about the compatibility changes here. Much appreciated 🙏 |
|
re-running workflows 🤞 |
There was a problem hiding this comment.
Pull Request Overview
This PR upgrades marshmallow from version 3.x to 4.x to resolve compatibility issues with Flask-AppBuilder 5.0.0. The upgrade addresses a critical initialization error where Flask-AppBuilder auto-generates schema fields for SQLAlchemy relationships that don't pass marshmallow 4.x's stricter field validation.
Key Changes:
- Introduces a compatibility layer that patches marshmallow's
Schema._init_fieldsto dynamically add missing auto-generated fields as Raw fields - Updates all marshmallow schemas to follow 4.x conventions (validator signatures, field parameters, metadata usage)
- Upgrades marshmallow to >=4.0.0 and marshmallow-sqlalchemy to 1.4.2
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| superset/marshmallow_compatibility.py | New module that patches marshmallow to handle Flask-AppBuilder compatibility by dynamically adding missing fields |
| superset/app.py | Applies the marshmallow compatibility patch during application initialization |
| tests/unit_tests/test_marshmallow_compatibility.py | Comprehensive unit tests for the compatibility module |
| superset/themes/schemas.py | Migrates validators to accept **kwargs parameter and replaces self.context with ContextVar |
| superset/reports/schemas.py | Updates validator signatures and replaces default with load_default |
| superset/models/helpers.py | Updates validator signature to include **kwargs |
| superset/databases/schemas.py | Updates validator signature to include **kwargs |
| superset/charts/schemas.py | Migrates field parameters: description to metadata, minLength to validate=Length(), min to validate=Range() |
| superset/db_engine_specs/databend.py | Migrates description to metadata and default to dump_default |
| superset/advanced_data_type/schemas.py | Updates JSON schema with dump_default (needs correction) |
| pyproject.toml | Updates marshmallow dependency from >=3.0, <4 to >=4 |
| requirements/base.in | Updates marshmallow-sqlalchemy from <1.4.1 to >=1.4.2 |
| requirements/base.txt | Locks marshmallow at 4.0.0 and marshmallow-sqlalchemy at 1.4.2 |
| requirements/development.txt | Locks marshmallow at 4.0.0 and marshmallow-sqlalchemy at 1.4.2 |
|
@rusackas The tests/checks should be all fixed now (hopefully). Could you rerun once again? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #35920 +/- ##
=======================================
Coverage 64.48% 64.48%
=======================================
Files 2661 2661
Lines 145642 145665 +23
Branches 33613 33613
=======================================
+ Hits 93912 93930 +18
- Misses 50029 50032 +3
- Partials 1701 1703 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
The above commit should fix the remaining checks 33c0814 
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #c3fd87
Actionable Suggestions - 3
-
tests/unit_tests/test_marshmallow_compatibility.py - 3
- Incomplete Test Coverage · Line 1-197
- Duplicated Test Setup Pattern · Line 1-197
- Inconsistent Error Handling in Tests · Line 1-197
Additional Suggestions - 2
-
superset/marshmallow_compatibility.py - 2
-
Private member access violation · Line 49-49Accessing private member `_init_fields` (lines 49 and 92) violates encapsulation. Consider if this is necessary or if a public API alternative exists. Note: Similar issue exists at line 92.
Code suggestion
@@ -48,6 +48,7 @@ import marshmallow # Store the original method + # Note: Accessing private member is necessary for Flask-AppBuilder 5.0.0 compatibility original_init_fields = marshmallow.Schema._init_fields -
Try-except block in loop · Line 56-56The `try`-`except` block inside the `while` loop (lines 56-85) incurs performance overhead. Consider restructuring to avoid exception handling in loops.
-
Review Details
-
Files reviewed - 12 · Commit Range:
f220397..e2e5d09- pyproject.toml
- requirements/base.in
- requirements/base.txt
- requirements/development.txt
- superset/app.py
- superset/charts/schemas.py
- superset/databases/schemas.py
- superset/marshmallow_compatibility.py
- superset/models/helpers.py
- superset/reports/schemas.py
- superset/themes/schemas.py
- tests/unit_tests/test_marshmallow_compatibility.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
|
@kevin-lann running CI 🤞 Still some (reasonable sounding) bot feedback if you're able to address or respond to it. |
|
@kevin-lann let us know if you're able to assess and apply/debunk the bot comments, give this a rebase to resolve conflicts, and get pre-commit to pass. We'd love to merge it. |
|
@rusackas hey, sounds great, will look at shortly. Been away from this for a bit and missed your previous message, my bad! |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@rusackas Cleaned up all the leftover bits and pieces.
The previous CI run also had unit tests failing but seems like it was due to it being cancelled so I assume this is not an issue. Let me know if there's anything else with this you'd like addressed |
Code Review Agent Run #dd1af0Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@kevin-lann this looks in good shape after your cleanup — just conflicting now. I'll rebase your branch to clear it, then ping @dpgaspar and @Antonio-RiveroMartnez to review the marshmallow 4.x compat layer. |
…le to be logged in
…ask-AppBuilder 5.0.0 and marshmallow 4.x
7e20aa4 to
5258a00
Compare
|
@rusackas hey - appreciate it. Just rebased with master actually, conflicts are gone. |
Code Review Agent Run #710057Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Thanks @kevin-lann, LGTM. Swapping the mm4-removed Schema.context write-back for a ContextVar + @post_load is the right shape, and since field validators run before post_load the sanitized value lands correctly. Nice that it also clears the latent AttributeError the old self.context[...] path would hit under mm4.
Mind updating the PR description though? It still points at a marshmallow_fix.py/app.py change that isn't in this diff.
|
Of course, updated. Appreciate the look over @rusackas 🙏 |
SUMMARY
Adds the Marshmallow 4 compatibility work needed by Superset's backend schema layer.
Adds a small compatibility shim for Flask-AppBuilder-generated Marshmallow schemas.
Marshmallow 4 is stricter about
declared_fields, and FAB can reference auto-generated relationship field names that are not explicitly declared. The new compatibility layer pre-stubs those FAB-style missing fields asRaw(allow_none=True, load_default=None)before_init_fieldsruns, which avoids schema initialization failures while still letting genuine non-FAB schema errors surface.Updates theme schema sanitization to work correctly with Marshmallow 4.
The theme POST/PUT schemas now persist sanitized
json_datathroughpost_loadusing a context variable, so sanitized theme payloads are what ultimately get loaded and stored.Applies small Marshmallow-4-compatible signature cleanup in model helpers and adds unit coverage for the compatibility layer and theme schema behavior.
TESTING INSTRUCTIONS
pytest tests/unit_tests/test_marshmallow_compatibility.pypytest tests/unit_tests/themes/api_test.pyjson_datawhen unsafe theme token values are provided.ADDITIONAL INFORMATION
marshmallow>=4.0.0#33162Credit to
@Austin-X @Eyang0612 @PDOracle @MasahisaSekita