fix(mcp): accept changed_on_delta_humanized as order_column in list tools - #42571
fix(mcp): accept changed_on_delta_humanized as order_column in list tools#42571aminghadersohi wants to merge 2 commits into
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42571 +/- ##
==========================================
- Coverage 66.64% 66.64% -0.01%
==========================================
Files 2863 2863
Lines 162155 162158 +3
Branches 37405 37406 +1
==========================================
- Hits 108076 108074 -2
- Misses 52020 52024 +4
- Partials 2059 2060 +1
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:
|
dosubot
Bot
added
api
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Richard's agent here:
Blocking: could we update the existing dataset and dashboard sortable-column definition tests to include changed_on_delta_humanized and remove their assertions that the alias is absent? The constants now include it, so those two tests will fail as written. The Python unit-test job was still queued when I checked head dc2fbaf.
Code Review Agent Run #91b6d4Actionable 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 |
dc2fbaf to
e8640f0
Compare
|
Thanks for the review! I don't think this is actually a blocker — I believe the two assertions you're pointing at reference a different (unrelated) pair of constants than the ones this PR changed. There are two similarly-named-but-distinct constants in play here:
Since the flagged tests check the former (unused) constants, they're unaffected by this change. I verified this two ways on head
So no test/code change is needed here. Separately, I agree those two dead local constants are confusing precisely because of this near-miss — I called that out as a follow-up cleanup opportunity for a future PR, but didn't want to fold an unrelated dead-code removal into this fix. Happy to split that out if useful. |
|
Bito Review Failed - Technical Failure Bito encountered technical difficulties while starting a code review session. To retry, type |
e8640f0 to
951448b
Compare
Code Review Agent Run #8cc51cActionable 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 |
951448b to
fcb7c72
Compare
Code Review Agent Run #10bac2Actionable 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 |
|
Addressed the reviewer feedback in ed46db0: the dataset and dashboard tool-level sortable-column constants and their definition tests now include |
Code Review Agent Run #45b84cActionable 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 |
…ools Superset's own REST APIs and list views use `changed_on_delta_humanized` as the "Last modified" sort key (a computed, humanized rendering of `changed_on` bound via FAB's @renders decorator, not a real queryable column). The MCP list_charts/list_datasets/list_dashboards tools only allowlisted the raw `changed_on`/`created_on` columns, so requests to sort by "most recently modified" using the REST API's own column name were rejected with "Invalid order_column". Add `changed_on_delta_humanized` to the sortable-column allowlists and resolve it to `changed_on` before it reaches the DAO, mirroring how the REST API and daos/datasource.py already treat the alias. Unknown columns are still rejected.
ed46db0 to
053db4a
Compare
Code Review Agent Run #870dfdActionable 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 |
2 participants
SUMMARY
list_charts,list_datasets, andlist_dashboards(MCP service tools) rejectedorder_column=changed_on_delta_humanizedwithInvalid order_column '...', even thoughchanged_on_delta_humanizedis the exact column name Superset's own REST API and list views use for "Last modified" (seeorder_columnsincharts/api.py,datasets/api.py,dashboards/api.py). Any caller — human or LLM agent — that copies that column name to sort by "most recently modified" gets an error.changed_on_delta_humanizedis a computed/humanized rendering ofchanged_on(bound via Flask-AppBuilder's@renders("changed_on")inmodels/helpers.py), not a real queryable SQLAlchemy column, so it can't simply be passed straight through to the DAO'sORDER BY.This PR:
changed_on_delta_humanizedto theCHART_SORTABLE_COLUMNS,DATASET_SORTABLE_COLUMNS, andDASHBOARD_SORTABLE_COLUMNSallowlists insuperset/mcp_service/common/schema_discovery.py, so it's accepted and advertised as sortable (matching REST API behavior and precedent already established indaos/datasource.py'ssort_col_map).changed_oncolumn inModelListCore.run_tool(superset/mcp_service/mcp_core.py) before the DAO call, sinceBaseDAO.list()sorts viagetattr(model, order_column)and would otherwise receive a Python method instead of a SQL column._validate_order_columnrejecting genuinely unknown columns unchanged — only the declared alias is special-cased.TESTING INSTRUCTIONS
tests/unit_tests/mcp_service/system/tool/test_mcp_core.pycovering the alias resolution and confirming unrelated unknown columns still raise.tests/unit_tests/mcp_service/chart/tool/test_list_charts.py,tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py, andtests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.pyassertingorder_column=changed_on_delta_humanizedis accepted and resolves tochanged_onfor each of the three list tools, plus that an unknownorder_column(e.g.random) still raises aToolError.ValueError: Invalid order_column 'changed_on_delta_humanized') and pass with the fix.uvx pre-commit run --files <changed files>passes (mypy, ruff, ruff-format, pylint).ADDITIONAL INFORMATION