Repository navigation
Conversation
Code Review Agent Run #b8bc04Actionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. The Here is the corrected implementation for @property
def name(self) -> str:
"""
Expose ``label`` as ``name`` so callers that treat a ``SavedQuery`` as
a generic datasource (e.g. chart create/update commands) can rely on
a uniform ``name`` attribute across all datasource types.
"""
return self.label or ""There are no other comments on this PR to address. Would you like me to check for any other potential issues in the codebase? superset/models/sql_lab.py |
There was a problem hiding this comment.
Pull request overview
Fixes saved-query datasource support for chart creation and updates by exposing SavedQuery.label as name.
Changes:
- Adds
SavedQuery.name. - Adds chart validation regression coverage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Review summary |
|---|---|
tests/unit_tests/commands/chart/test_access_checks.py |
Adds validation coverage, but not full access or persistence coverage. |
superset/models/sql_lab.py |
Adds name, but saved-query access checks and slice persistence still require missing permission/data attributes. |
Suppressed comments (1)
tests/unit_tests/commands/chart/test_access_checks.py:122
- This regression test patches out
security_manager.raise_for_access, so it only proves thatnameexists and cannot detect the subsequent access/persistence failures described above. It also does not exercise the advertisedUpdateChartCommandpath. Add coverage for both command paths that validates the selected saved-query access contract and the chart persistence hook.
with patch(
"superset.commands.chart.create.get_datasource_by_id",
return_value=saved_query,
):
with patch("superset.commands.chart.create.security_manager.raise_for_access"):
with patch(
"superset.commands.chart.create.populate_subjects",
return_value=[],
):
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #43499 +/- ##
===========================================
+ Coverage 56.59% 84.76% +28.16%
===========================================
Files 3071 3071
Lines 206037 206141 +104
Branches 48506 48523 +17
===========================================
+ Hits 116607 174730 +58123
+ Misses 88317 28108 -60209
- Partials 1113 3303 +2190
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:
|
Code Review Agent Run #100eb3Actionable Suggestions - 0Additional Suggestions - 1
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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #d62867Actionable Suggestions - 0Additional Suggestions - 2
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 |
b20876c to
decf0eb
Compare
|
Looks like #43500 solved the underlying #29697 crash the other way, rejecting |
![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 #77baf9
Actionable Suggestions - 1
-
superset/models/sql_lab.py - 1
- schema_perm format mismatch · Line 601-601
Additional Suggestions - 1
-
superset/models/sql_lab.py - 1
-
unguarded database deref · Line 613-613`perm`/`schema_perm`/`catalog_perm` dereference `self.database.database_name` unguarded, while the sibling `data` property guards `if self.database:`. The DAO guard (datasource.py:92) checks `db_id is None`, not the relationship being loaded, so a saved query with an orphaned `db_id` (FK set, `database` None) passes the guard and crashes here. Consider guarding like `data`.
-
Review Details
-
Files reviewed - 4 · Commit Range:
f6e8aef..decf0eb- superset/daos/datasource.py
- superset/models/sql_lab.py
- tests/unit_tests/commands/chart/test_access_checks.py
- tests/unit_tests/datasource/dao_tests.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- 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
Code Review Agent Run #03285aActionable 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 |
There was a problem hiding this comment.
Code Review Agent Run #b02e31
Actionable Suggestions - 1
-
superset/commands/chart/utils.py - 1
- saved_query chart cannot execute · Line 38-42
Review Details
-
Files reviewed - 1 · Commit Range:
7188d5f..3ea862d- superset/commands/chart/utils.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- 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
Code Review Agent Run #7973e4Actionable 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 |
There was a problem hiding this comment.
Code Review Agent Run #4e1e9f
Actionable Suggestions - 1
-
tests/integration_tests/charts/semantic_save_tests.py - 1
- Test expects 422 but gets 400 · Line 124-124
Review Details
-
Files reviewed - 3 · Commit Range:
761be17..760c2a6- superset/migrations/versions/2026-09-29_00-00_dfa683969f48_allow_saved_query_chart_datasource.py
- tests/integration_tests/charts/api_tests.py
- tests/integration_tests/charts/semantic_save_tests.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- 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
Code Review Agent Run #50bb06Actionable 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 |
github-actions
Bot
added
the
requires:rebase
24e192d to
9443417
Compare
|
25b57dd to
d7b0dfe
Compare
|
…rent migration, cover catalog_perm Address review on #43499: - `database_after_update` only walked `SqlaTable` rows, so charts backed by a saved query kept `[old].[...]` perm/schema_perm/catalog_perm after the database was renamed and would match a grant on an unrelated connection that later took the old name. Rewrite those chart rows on rename too, sharing the perm-string helper with `saved_query_before_update`. - Re-parent the `allow_saved_query_chart_datasource` migration onto the current head (`d623a0cb6bb0`) and make the docstring agree with `down_revision`. - Test the catalog half of the sync for real: move a saved query between catalogs and delete one with a non-null catalog, asserting the replaced and cleared `catalog_perm`; add a database-rename regression test. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
d7b0dfe to
c14bd6d
Compare
|
|
||
| with ( | ||
| patch( | ||
| "superset.commands.chart.utils.security_manager.can_access_all_queries", |
There was a problem hiding this comment.
This test forces can_access_all_queries to True and mocks raise_for_access, and the API test runs as Admin, so the ownership rejection is only exercised by calling the helper directly with mocked ownership. If CreateChartCommand stopped calling raise_for_saved_query_ownership, every test here would still pass. Could an API test cover a non-admin caller with chart-create and schema access but no all_query_access, expecting 403 and no persisted Slice for another user's saved query, and 201 for their own?
github-actions
Bot
added
the
requires:rebase
21d7c9f to
3a4b717
Compare
github-actions
Bot
removed
the
requires:rebase
| database, so editorship of one (being its creator) must not stand in for a | ||
| database, catalog, or schema grant: require the real grant. | ||
| """ | ||
| security_manager.raise_for_access( |
There was a problem hiding this comment.
With editorship now off for saved queries, an owner whose only grant is catalog_access on the database's default catalog gets a 403 binding a chart to their own saved query that has catalog unset. can_access_schema checks the raw datasource.catalog, which is None here, so the catalog grant is skipped, even though catalog_perm and schema_perm on the same saved query now resolve against the default catalog. Should the access check resolve the effective catalog the same way, so the grant that the chart's own perm strings match is honored?
|
|
||
| # The bulk update above bypasses ``saved_query_before_update``, so | ||
| # re-copy the saved queries' perm strings onto their dependent charts. | ||
| if database := db.session.query(Database).get(database_id): |
There was a problem hiding this comment.
Nothing exercises this resync: test_update_with_catalog_change mocks _update_catalog_attribute entirely, and the model tests drive the ORM listeners, so deleting this call leaves the suite green. Could a test persist a saved-query chart, run the real _update_catalog_attribute from catalog A to B, and assert after expiring the session that the chart's catalog_perm and schema_perm both carry B?
|
1 similar comment
|
cb86146 to
478b840
Compare
github-actions
Bot
added
the
requires:rebase
Rebased squash of the saved_query chart datasource work onto current master. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Require a real database/schema grant (not saved-query editorship) when binding a chart to a saved query, fall back to the default catalog for catalog_perm, resync dependent charts on catalog changes, and limit database-rename resync to saved queries that have charts, resolving the default catalog once. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…asources Only pass allow_editorship=False when binding a chart to a saved query; tables and semantic views go through the same raise_for_access(datasource=...) call as before. The extra kwarg broke semantic_save_tests' call assertion, and because that test aborted before cleanup, its leftover semantic_view chart cascaded into the reports, dashboards export, and change_records failures later in the integration run. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…head Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
478b840 to
0053e08
Compare
github-actions
Bot
removed
the
requires:rebase
SUMMARY
Creating (or updating) a chart with
datasource_type: saved_queryraised a fatal'SavedQuery' object has no attribute 'name'error.CreateChartCommand/UpdateChartCommand.validate()readdatasource.nameto populatedatasource_name, and every other datasource type (SqlaTable,Query,SemanticView) exposes aname, butSavedQueryonly had alabelcolumn. This adds anameproperty toSavedQuerythat returnslabel, so it behaves like the rest of theDatasourceunion instead of special-casing the command layer.saved_queryis now a fully persistable chart datasource type: theck_chart_datasourceCHECK constraint onslices.datasource_typeis extended (migrationdfa683969f48) to allow it alongsidetable/semantic_view, matching whatvalidate_chart_datasource_typealready accepted.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend-only fix, no UI change)
TESTING INSTRUCTIONS
tests/unit_tests/commands/chart/test_access_checks.py::test_create_chart_command_supports_saved_query_datasourcebuilds a realSavedQuery(not a mock) and assertsCreateChartCommand.validate()no longer raisesAttributeErrorand correctly derivesdatasource_namefromlabel.tests/integration_tests/charts/api_tests.py::TestChartApi::test_create_chart_from_saved_queryposts a real chart-create request end to end and asserts a 201.POST /api/v1/chart/withdatasource_type: "saved_query"and a validdatasource_idshould now create the chart instead of returning a 500.pytest tests/unit_tests/commands/chart/test_access_checks.py tests/unit_tests/models/sql_lab_test.pyADDITIONAL INFORMATION