Skip to content

refactor(mcp): use serialize_user_object in get_instance_info#38613

Merged
Antonio-RiveroMartnez merged 1 commit intoapache:masterfrom
aminghadersohi:amin/refactor-get-instance-info-use-serialize-user-object
Mar 13, 2026
Merged

refactor(mcp): use serialize_user_object in get_instance_info#38613
Antonio-RiveroMartnez merged 1 commit intoapache:masterfrom
aminghadersohi:amin/refactor-get-instance-info-use-serialize-user-object

Conversation

@aminghadersohi
Copy link
Contributor

SUMMARY

Follow-up to #38612 per review comment from @rebenitez1802.

Replaces the inline role-extraction logic in get_instance_info with the shared serialize_user_object() helper introduced in #38612, reducing code duplication.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A - refactor only, no behavior change

TESTING INSTRUCTIONS

pytest tests/unit_tests/mcp_service/system/ -x -q
# 67 passed

pytest tests/unit_tests/mcp_service/ -x -q --ignore=tests/unit_tests/mcp_service/test_mcp_tool_registration.py
# 781 passed

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

Replace inline role-extraction logic with the shared
serialize_user_object helper, reducing duplication.
@bito-code-review
Copy link
Contributor

bito-code-review bot commented Mar 12, 2026

Code Review Agent Run #9959e4

Actionable Suggestions - 0
Additional Suggestions - 1
  • tests/unit_tests/mcp_service/system/tool/test_get_current_user.py - 1
    • Incomplete test assertion · Line 224-224
      The test mocks `mock_g_user.active = True` but does not assert that `active` appears in the `current_user` response. Since the `UserInfo` schema includes `active` and `serialize_user_object` extracts it, the test should verify this behavior to ensure complete coverage of the serialization logic.
      Code suggestion
       @@ -248,1 +248,2 @@
      -        assert cu["roles"] == ["Alpha"]
      +        assert cu["roles"] == ["Alpha"]
      +        assert cu["active"] == True
Review Details
  • Files reviewed - 2 · Commit Range: 17c64d6..17c64d6
    • superset/mcp_service/system/tool/get_instance_info.py
    • tests/unit_tests/mcp_service/system/tool/test_get_current_user.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

@dosubot dosubot bot added the change:backend Requires changing the backend label Mar 12, 2026
@codecov
Copy link

codecov bot commented Mar 12, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 64.40%. Comparing base (d4f1f8d) to head (17c64d6).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
...erset/mcp_service/system/tool/get_instance_info.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #38613      +/-   ##
==========================================
- Coverage   65.01%   64.40%   -0.62%     
==========================================
  Files        1817     2529     +712     
  Lines       72318   128945   +56627     
  Branches    23032    29718    +6686     
==========================================
+ Hits        47016    83042   +36026     
- Misses      25302    44458   +19156     
- Partials        0     1445    +1445     
Flag Coverage Δ
hive 40.76% <0.00%> (?)
mysql 61.90% <0.00%> (?)
postgres 61.97% <0.00%> (?)
presto 40.78% <0.00%> (?)
python 63.59% <0.00%> (?)
sqlite 61.60% <0.00%> (?)
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.

Copy link
Contributor

@rebenitez1802 rebenitez1802 left a comment

Choose a reason for hiding this comment

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

Thanks for the refactor LGTM

@Antonio-RiveroMartnez Antonio-RiveroMartnez merged commit 83955e8 into apache:master Mar 13, 2026
74 of 75 checks passed
aminghadersohi added a commit to aminghadersohi/superset that referenced this pull request Mar 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:backend Requires changing the backend size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants