fix(mcp): avoid mutating global SENSITIVE_FIELDS in field permissions #35934
Conversation
Build sensitive_fields from copies to prevent bleed-through across requests/tests
Code Review Agent Run #58ce8eActionable 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 |
|
Please look here:- |
![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 Set Conversion ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset/mcp_service/utils/permissions_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.
| base=SENSITIVE_FIELDS.get(object_type, set()) | ||
| common=SENSITIVE_FIELDS.get("common", set()) | ||
| sensitive_fields= set(base)|set(common) |
There was a problem hiding this comment.
Redundant Set Conversion 
Tell me more
What is the issue?
The code unnecessarily converts base and common to sets twice when they are already sets from SENSITIVE_FIELDS dictionary.
Why this matters
This creates unnecessary set conversions that impact performance and code clarity without adding any value.
Suggested change ∙ Feature Preview
base = SENSITIVE_FIELDS.get(object_type, set())
common = SENSITIVE_FIELDS.get("common", set())
sensitive_fields = base | common # Set union operator directly on setsProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
There was a problem hiding this comment.
I understand your disappointment. Let's focus on improving the code. Would you like to discuss the suggested change or explore alternative solutions?
There was a problem hiding this comment.
Understood. Let's consider these alternatives:
-
Use set union directly:
sensitive_fields = SENSITIVE_FIELDS.get(object_type, set()) | SENSITIVE_FIELDS.get("common", set())
-
Use dict.get() with default empty set:
sensitive_fields = SENSITIVE_FIELDS.get(object_type, set()) | SENSITIVE_FIELDS.get("common", set())
-
Use dict.setdefault() to ensure sets:
base = SENSITIVE_FIELDS.setdefault(object_type, set()) common = SENSITIVE_FIELDS.setdefault("common", set()) sensitive_fields = base | common
Which approach do you prefer, or do you have another idea?
There was a problem hiding this comment.
ask for a review
|
@aminghadersohi you might be interested in this PR as the person the added the MCP files |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors how sensitive fields are computed in the get_allowed_fields function by changing from a mutable update pattern to an immutable set union operation.
- Replaces in-place
update()with set union operator|to combine sensitive fields - Introduces intermediate variables for base and common sensitive fields
| base=SENSITIVE_FIELDS.get(object_type, set()) | ||
| common=SENSITIVE_FIELDS.get("common", set()) | ||
| sensitive_fields= set(base)|set(common) |
There was a problem hiding this comment.
Missing spaces around assignment operators. Python PEP 8 requires spaces around assignment operators. Change base= to base = , common= to common = , and sensitive_fields= to sensitive_fields = .
| base=SENSITIVE_FIELDS.get(object_type, set()) | |
| common=SENSITIVE_FIELDS.get("common", set()) | |
| sensitive_fields= set(base)|set(common) | |
| base = SENSITIVE_FIELDS.get(object_type, set()) | |
| common = SENSITIVE_FIELDS.get("common", set()) | |
| sensitive_fields = set(base) | set(common) |
| sensitive_fields.update(SENSITIVE_FIELDS.get("common", set())) | ||
| base=SENSITIVE_FIELDS.get(object_type, set()) | ||
| common=SENSITIVE_FIELDS.get("common", set()) | ||
| sensitive_fields= set(base)|set(common) |
There was a problem hiding this comment.
Redundant set conversions. The .get(object_type, set()) calls already return sets, so wrapping them with set() again is unnecessary. Since base and common are already sets, use sensitive_fields = base | common directly.
| sensitive_fields= set(base)|set(common) | |
| sensitive_fields = base | common |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #35934 +/- ##
===========================================
+ Coverage 0 68.73% +68.73%
===========================================
Files 0 622 +622
Lines 0 45713 +45713
Branches 0 4972 +4972
===========================================
+ Hits 0 31422 +31422
- Misses 0 13046 +13046
- Partials 0 1245 +1245
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:
|
|
CC @aminghadersohi but also... Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following: A series of checks will now run when you make a git commit. Alternatively it is possible to run pre-commit by running pre-commit manually: |
3 participants
Build sensitive_fields from copies to prevent bleed-through across requests/tests
SUMMARY
Build a local union set (set(base)|set(common)) in get_allowed_fields instead of updating SENSITIVE_FIELDS, preventing cross-request/type bleed-through and stabilizing MCP dataset/chart/dashboard filtering.
Call get_allowed_fields(...) and confirm SENSITIVE_FIELDS remains unchanged before/after.
• No UI changes; No DB migration; No new APIs...