You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Kunal8954 (via GitHub)" <gi...@apache.org> on 2026/09/11 11:57:53 UTC
[PR] fix(chart): allow semantic view datasource when saving charts [superset]
Kunal8954 opened a new pull request, #44169:
URL: https://github.com/apache/superset/pull/44169
### SUMMARY
Fixes #44167 — charts backed by a SIP-182 **semantic view** render successfully in Explore but fail to save with a 422 `{"message":{"datasource_type":["Datasource type is invalid"]}}`.
**Root cause.** The datasource-type validation in `CreateChartCommand`/`UpdateChartCommand` introduced by #43500 (to reject `saved_query`/`query`, which `Slice.datasource` can never resolve) was written as a TABLE-only check. That unintentionally also rejects `semantic_view`, even though semantic views are a first-class, resolvable datasource (`Slice.semantic_view`, `DatasourceType.SEMANTIC_VIEW`).
**Fix.** Extend the guard to an explicit allowlist of exactly the datasource types `Slice` can resolve:
- `table` — unchanged, still accepted
- `semantic_view` — newly accepted
- `saved_query`, `query`, and unknown types — still rejected with the existing `DatasourceTypeInvalidError` (422)
The fix is deliberately narrow: it does not accept arbitrary non-table datasource types, does not normalize datasource types, and does not change any datasource representation.
**Tests.**
- Unit: `CreateChartCommand` and `UpdateChartCommand` accept a `semantic_view` datasource (validation reaches `get_datasource_by_id`, `datasource_name` is populated); `saved_query`/`query`/`bogus` remain rejected in both create and update paths.
- Integration API: `POST /api/v1/chart/` with `datasource_type="semantic_view"` against a real semantic view row creates the chart (201), mirroring the existing `saved_query` rejection test's API shape.
Honest caveat: the integration API test could **not** be run locally because this environment lacks a running Redis cache and the seeded integration test setup; it is left to CI. The relevant unit tests (29 passed) and all `pre-commit run` checks (mypy, ruff, ruff-format, pylint, etc.) passed locally.
### BEFORE/AFTER
Before: `POST /api/v1/chart/` with `datasource_type: "semantic_view"` → 422 `Datasource type is invalid`.
After: the chart saves successfully (201).
### TESTING INSTRUCTIONS
```
pytest tests/unit_tests/commands/chart/create_test.py tests/unit_tests/commands/chart/update_test.py
```
Integration (CI): `tests/integration_tests/charts/api_tests.py::TestChartApi::test_create_chart_from_semantic_view`
### ADDITIONAL INFORMATION
- Has associated issue: Fixes #44167
- Backward compatible: only widens acceptance by the one explicitly-supported `semantic_view` type; all previously invalid types remain invalid.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5711228166
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "Kunal8954 (via GitHub)" <gi...@apache.org>.
Kunal8954 commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5770188518
Heads up @rusackas — the PR is now merge-clean and the failing checks are addressed.
- **Conflicts resolved**: re-merged the latest `master` into the branch (`dashboard.py`'s `datasets_trimmed_for_slices` now uses master's `resolved_datasource` + typed `(datasource_type, datasource_id)` grouping, keeping the id-collision keying intact; the collision test was updated to mock the resolver). `mergeStateStatus` is now `MERGEABLE`, all affected unit tests (228) pass locally, ruff + mypy clean.
- **babel-extract check fixed**: the `messages.pot` drift (report-retry + screenshot strings) that was failing CI — it's also failing on plain `master`, so I regenerated via `babel_update.sh` (0 fuzzy regressions) and committed it.
The items you flagged last round are still open on my list: the bare-id comparison gap on `slice.py`, the sibling filter in `dashboards/filters.py`, and the purge-policy suite. Want me to land those in this PR or a follow-up?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "Kunal8954 (via GitHub)" <gi...@apache.org>.
Kunal8954 commented on code in PR #44169:
URL: https://github.com/apache/superset/pull/44169#discussion_r4016947609
##########
superset/commands/chart/create.py:
##########
@@ -81,7 +81,10 @@ def validate(self) -> None:
# (SavedQuery/Query have no ``.name`` attribute, so accessing it
# below raises an unhandled AttributeError) or silently, by
# producing a permanently broken chart.
- if datasource_type != DatasourceType.TABLE:
+ if datasource_type not in {
+ DatasourceType.TABLE,
+ DatasourceType.SEMANTIC_VIEW,
Review Comment:
I’ll check the chart export/import format and add semantic-view datasource references before keeping this type enabled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44169:
URL: https://github.com/apache/superset/pull/44169#discussion_r4015850650
##########
superset/models/slice.py:
##########
@@ -168,6 +169,15 @@ class Slice( # pylint: disable=too-many-public-methods
remote_side="SqlaTable.id",
lazy="subquery",
)
+ semantic_view = relationship(
Review Comment:
Adding this non-FK relationship makes deletion-retention policy validation fail for every `Slice`: dependency discovery includes relationships without a physical FK, but `purge_policy_registry` only declares `Slice.table`, so `get_purge_policy(Slice)` raises an incomplete-policy error before purging even a table-backed chart. Should this declare `semantic_view` as `PRESERVE` and add purge coverage?
##########
superset/commands/chart/create.py:
##########
@@ -81,7 +81,10 @@ def validate(self) -> None:
# (SavedQuery/Query have no ``.name`` attribute, so accessing it
# below raises an unhandled AttributeError) or silently, by
# producing a permanently broken chart.
- if datasource_type != DatasourceType.TABLE:
+ if datasource_type not in {
+ DatasourceType.TABLE,
+ DatasourceType.SEMANTIC_VIEW,
Review Comment:
Charts saved through this new type cannot survive their own export/import flow: the exporter removes `datasource_type` and only emits `dataset_uuid` when `model.table` is set, while chart import requires `dataset_uuid`. A semantic-view chart therefore exports YAML without any datasource pointer and immediately fails re-import; can the portable format carry a semantic-view reference before this type is accepted?
##########
superset/models/dashboard.py:
##########
@@ -358,7 +358,7 @@ def datasets_trimmed_for_slices(
# Use the eagerly-loaded datasource from any slice in the group
datasource = next(iter(slices)).datasource
- if datasource:
+ if isinstance(datasource, BaseDatasource):
Review Comment:
Grouping these slices by bare `datasource_id` merges the independent `SqlaTable` and `SemanticView` ID spaces. If both have ID 5, `next(iter(slices))` can select the semantic-view chart and this guard drops the entire group, including the table metadata, or it can pass the semantic slice into the table's `data_for_slices`; can this key by `(datasource_type, datasource_id)` and cover a mixed dashboard collision?
##########
superset/models/slice.py:
##########
@@ -190,8 +200,10 @@ def __repr__(self) -> str:
return self.slice_name or str(self.id)
@property
- def datasource(self) -> SqlaTable | None:
- return self.table
+ def datasource(self) -> SqlaTable | SemanticView | None:
+ if table := self.table:
+ return table
+ return self.semantic_view
Review Comment:
SECURITY FIX REQUIRED: Returning a `SemanticView` here feeds its integer ID into `Dashboard.datasources`, while `can_drill_dataset_via_dashboard_access` compares only bare IDs. A guest for a dashboard containing semantic view 7 can request `/datasource/samples` with table ID 7 and receive raw rows from the unrelated `SqlaTable` 7; can this keep table-only authorization callers type-pinned or compare `(datasource_type, id)`?
##########
superset/charts/filters.py:
##########
@@ -163,7 +175,21 @@ def _apply_viewers(self, query: Query) -> Query:
)
)
)
- filters.append(Slice.id.in_(no_viewer_query))
+ filters.append(Slice.id.in_(no_viewer_table_query))
+
+ # SEMANTIC_VIEW charts have no SqlaTable/Database row to join; access is
+ # evaluated against the chart's own perm, which set_related_perm keeps in
+ # sync with the view's ``datasource_access`` perm (no numeric-id join).
+ no_viewer_semantic_view_query = db.session.query(Slice.id).filter(
+ and_(
+ Slice.datasource_type == DatasourceType.SEMANTIC_VIEW,
+ ~chart_has_viewers,
+ Slice.perm.in_(
Review Comment:
Chart visibility now depends on the stored `Slice.perm`, but the semantic-view and semantic-layer rename hooks update the view menu and `SemanticView.perm` without updating existing slices, unlike the dataset rename path. After either rename, users retain current `datasource_access` yet their saved charts disappear from the list; can this join the current permission or cascade the rename and cover both cases?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5740235544
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5768824469
Aside from addressing @gabotorresruiz 's comments, would you mind tackling the rebase to resolve conflicts, @Kunal8954? Thanks in advance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on code in PR #44169:
URL: https://github.com/apache/superset/pull/44169#discussion_r4020486844
##########
superset/models/slice.py:
##########
@@ -168,6 +169,15 @@ class Slice( # pylint: disable=too-many-public-methods
remote_side="SqlaTable.id",
lazy="subquery",
)
+ semantic_view = relationship(
Review Comment:
Confirmed empirically: on this branch `tests/unit_tests/commands/deletion_retention/` has 14 failures, all `RuntimeError: Incomplete purge policy for Slice: missing=[relationship:slices->semantic_views:->:manytoone:semantic_view]`, and the same suite passes on the parent commit. Since `get_purge_policy(Slice)` raises, chart purging itself breaks, not just the validation tests.
##########
tests/integration_tests/charts/api_tests.py:
##########
@@ -703,6 +704,109 @@ def test_create_chart_from_saved_query_rejected_cleanly(self):
db.session.delete(db.session.query(SavedQuery).get(saved_query_id))
db.session.commit()
+ def test_create_chart_from_semantic_view(self):
+ """
+ Chart API: creating a chart with datasource_type="semantic_view" must
+ succeed (apache/superset#44167). Semantic views are first-class
+ resolvable datasources (Slice resolves them through the type-guarded
+ ``semantic_view`` relationship), so the non-table datasource_type
+ guard must explicitly allow them rather than rejecting them the way
+ it rejects saved_query. This reproduces the exact API call shape from
+ the bug report: a real semantic view row, then POST /api/v1/chart/
+ with datasource_type="semantic_view".
+ """
+ self.login(ADMIN_USERNAME)
+ suffix = uuid.uuid4().hex
+ layer = SemanticLayer(
+ uuid=uuid.uuid4(),
+ name=f"issue-44167-layer-{suffix}",
+ type="test",
+ configuration="{}",
+ )
+ view = SemanticView(
+ uuid=uuid.uuid4(),
+ name=f"issue-44167-view-{suffix}",
+ semantic_layer_uuid=layer.uuid,
+ configuration="{}",
+ )
+ db.session.add_all([layer, view])
+ db.session.commit()
+ view_id = view.id
+
+ chart_data = {
+ "slice_name": "issue-44167-repro-chart",
+ "datasource_id": view_id,
+ "datasource_type": "semantic_view",
+ "viz_type": "table",
+ }
+ chart_id = None
+ try:
+ rv = self.post_assert_metric("/api/v1/chart/", chart_data, "post")
+
+ assert rv.status_code == 201
+ data = json.loads(rv.data.decode("utf-8"))
+ chart_id = data.get("id")
+ model = db.session.query(Slice).get(chart_id)
+ assert model.datasource_type == "semantic_view"
+ assert model.datasource_id == view_id
+
+ # The saved chart is now resolvable: its owner (admin) can
+ # retrieve it, and the chart's perm carries the view perm.
+ rv = self.get_assert_metric(f"/api/v1/chart/{chart_id}", "get")
+ assert rv.status_code == 200
+ assert model.perm == view.perm
+
+ gamma = self.get_user("gamma")
+ uri = "api/v1/chart/?q=" + rison.dumps(
+ {
+ "filters": [
+ {
+ "col": "slice_name",
+ "opr": "ct",
+ "value": "issue-44167-repro-chart",
+ }
+ ]
+ }
+ )
+
+ # Without the view's datasource_access perm, a non-owner cannot
+ # list/open the chart.
+ with self.temporary_user(gamma, login=True):
Review Comment:
This test cannot pass as written. I ran it in a real environment (sqlite metadata DB plus Redis) and it fails here with `1 == 0`, for two independent reasons:
- `self.login(ADMIN_USERNAME)` above leaves an authenticated session, and FAB's `AuthDBView.login` redirects an already authenticated session without re-authenticating (the `g.user.is_authenticated` early return in `flask_appbuilder/security/views.py`). So every `temporary_user(..., login=True)` block here still runs as admin, and admin sees the chart. Adding `self.logout()` before the first block fixes this; I verified it on this branch.
- The `all_database_access` block then still fails, because `ChartFilter.apply` returns the unfiltered query when `security_manager.can_access_all_datasources()` is true, and that helper includes `all_database_access`. The short circuit happens before any join, so this assertion does not exercise the id ride-along protection the comment describes, and the expectation contradicts `SemanticView.raise_for_access`, which also grants on `can_access_all_datasources()`. To pin the ride-along protection, grant access to one specific database and create a table whose id collides with the view id, then assert the chart stays hidden.
With `self.logout()` added and that block corrected, the rest passes for me on this branch: 201 on create, perm synced, hidden without the view perm, visible with it.
##########
superset/charts/filters.py:
##########
@@ -151,10 +151,22 @@ def _apply_viewers(self, query: Query) -> Query:
# (C) No-viewer fallback: charts with no viewers → dataset-based access
chart_has_viewers = Slice.viewers.any()
+
+ # TABLE charts keep the table/database join for dataset-based access.
+ # The datasource_type guard keeps the join unambiguous: a semantic-view
+ # chart's datasource_id (shared auto-increment id space) must never be
+ # matched against a coincidental SqlaTable.id. TABLE rows are otherwise
+ # matched exactly as before.
table_alias = aliased(SqlaTable)
- no_viewer_query = (
+ no_viewer_table_query = (
db.session.query(Slice.id)
- .join(table_alias, Slice.datasource_id == table_alias.id)
+ .join(
+ table_alias,
+ and_(
+ Slice.datasource_id == table_alias.id,
+ Slice.datasource_type == DatasourceType.TABLE,
Review Comment:
The guard here is right, but its sibling is still unguarded: `DashboardAccessFilter._apply_viewers` in `superset/dashboards/filters.py` has the identical join, `.join(SqlaTable, Slice.datasource_id == SqlaTable.id)`, with no `datasource_type` condition. Once semantic view charts land on dashboards, a published no-viewer dashboard whose semantic view chart collides with an accessible `SqlaTable.id` becomes listable through that join, and conversely a dashboard containing only semantic view charts can never match the fallback even for users who hold the view perm, because the joined rows do not exist and `get_dataset_access_filters(Slice)` never gets to check `Slice.perm`. I would apply the same `datasource_type == DatasourceType.TABLE` guard there and add a semantic view arm mirroring this one, in this PR, since this PR is what makes such charts reachable.
##########
superset/charts/filters.py:
##########
@@ -163,7 +175,21 @@ def _apply_viewers(self, query: Query) -> Query:
)
)
)
- filters.append(Slice.id.in_(no_viewer_query))
+ filters.append(Slice.id.in_(no_viewer_table_query))
+
+ # SEMANTIC_VIEW charts have no SqlaTable/Database row to join; access is
+ # evaluated against the chart's own perm, which set_related_perm keeps in
+ # sync with the view's ``datasource_access`` perm (no numeric-id join).
+ no_viewer_semantic_view_query = db.session.query(Slice.id).filter(
+ and_(
+ Slice.datasource_type == DatasourceType.SEMANTIC_VIEW,
+ ~chart_has_viewers,
+ Slice.perm.in_(
Review Comment:
Not a blocker, a consistency question: `SemanticView.raise_for_access` also grants when the user holds `datasource_access` on the parent layer's perm, but both this branch and the chart access path (`can_access_datasource`, which checks only `datasource.perm`) look at the view perm alone. A user granted at the layer level can explore the view but cannot open or list a chart saved on it. Everything fails closed, so no security concern, but is the asymmetry intended?
##########
superset/models/slice.py:
##########
@@ -190,8 +200,10 @@ def __repr__(self) -> str:
return self.slice_name or str(self.id)
@property
- def datasource(self) -> SqlaTable | None:
- return self.table
+ def datasource(self) -> SqlaTable | SemanticView | None:
+ if table := self.table:
+ return table
+ return self.semantic_view
Review Comment:
While tracing this I found two more bare id comparisons the type pinning should cover in the same sweep: `_guest_token_allows_dataset(chart.datasource.id ...)` in the chart branch of `raise_for_access`, where a guest token's dataset allowlist of table ids is compared against a semantic view id, and the native filter arm of the datasource branch, which compares `target.get("datasetId")` from dashboard metadata against `datasource.data["id"]`, where `SemanticView.data["id"]` is the bare integer id.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "Kunal8954 (via GitHub)" <gi...@apache.org>.
Kunal8954 commented on code in PR #44169:
URL: https://github.com/apache/superset/pull/44169#discussion_r4016937253
##########
superset/models/slice.py:
##########
@@ -168,6 +169,15 @@ class Slice( # pylint: disable=too-many-public-methods
remote_side="SqlaTable.id",
lazy="subquery",
)
+ semantic_view = relationship(
Review Comment:
Thanks, I’ll update the purge policy coverage for the new relationship and verify deletion-retention validation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "Kunal8954 (via GitHub)" <gi...@apache.org>.
Kunal8954 commented on code in PR #44169:
URL: https://github.com/apache/superset/pull/44169#discussion_r4036308514
##########
superset/commands/chart/create.py:
##########
@@ -81,7 +81,10 @@ def validate(self) -> None:
# (SavedQuery/Query have no ``.name`` attribute, so accessing it
# below raises an unhandled AttributeError) or silently, by
# producing a permanently broken chart.
- if datasource_type != DatasourceType.TABLE:
+ if datasource_type not in {
+ DatasourceType.TABLE,
+ DatasourceType.SEMANTIC_VIEW,
Review Comment:
Following up on this thread: chart export/import for semantic-view charts (emitting a semantic-view datasource reference from the exported chart config and resolving it on import) is tracked as a follow-up rather than landing with this fix, so this PR is not blocked on it here. Current changes keep chart-list access, rename cascades, and trimmed-datasource validation on the new type consistent in the meantime.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5716145027
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5771280624
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5634103076
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | d15920743ec32d05d1fb39ee16c4f67408a38ac1 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aa3ecc22694ea0008d75f06 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-44169--superset-docs-preview.netlify.app](https://deploy-preview-44169--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctBDoMgEADAv-y5m1KlWP2Bz1iXJZCgECRq0_j3cpvL_CCXtOYKE8ADlkIb-2YXrqfWLzMgxZhO3GWlrQbGI8iJ7KlU3OmQdqzkmL6zbcsQ9cLcdWbUQkqpjx3eThm4_w) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44169--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5801909891
Follow-up, please. Looking again I was behind on that last pass, sorry: gabotorresruiz had already confirmed the purge policy entry and the `dashboards/filters.py` guard are on `master`, and `slice.py` isn't in this diff anymore, so the leftover bare-id comparisons belong in their own PR. Thanks for sticking with this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "Kunal8954 (via GitHub)" <gi...@apache.org>.
Kunal8954 commented on code in PR #44169:
URL: https://github.com/apache/superset/pull/44169#discussion_r4016953019
##########
superset/models/dashboard.py:
##########
@@ -358,7 +358,7 @@ def datasets_trimmed_for_slices(
# Use the eagerly-loaded datasource from any slice in the group
datasource = next(iter(slices)).datasource
- if datasource:
+ if isinstance(datasource, BaseDatasource):
Review Comment:
Good catch. I’ll change the grouping to include the datasource type and add coverage for table/semantic-view ID collisions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5708817469
@Kunal8954 — routing a concrete dashboard regression from SC-120295 / preset-io/superset-shell#5140 to the existing SC-120289 host work. This extends the dashboard metadata concern in https://github.com/apache/superset/pull/44169#discussion_r4016953019; it is separate from MCP dashboard serialization #44269.
On core `2db9d6d51c8623810a2dba5b796c5154147d7d76`, an isolated dashboard with semantic-view Table, BigNumber and monthly charts renders valid numeric results. With malformed, NaN or overflowing numeric input, all three chart requests correctly return HTTP 400 with a bounded metric/category message, and frontend Redux reaches `failed` with that message. The visible dashboard nevertheless shows loading indicators: `/api/v1/dashboard/1/datasets` returns `{"result":[]}`, so it retains `PLACEHOLDER_DATASOURCE`, and `components/Chart/Chart.tsx` suppresses message-only errors in that state.
Source inspection at your current `ff4829b8308c49e92942df3f53c510238c87525c` still shows the relevant path: `Dashboard.datasets_trimmed_for_slices` reads `next(iter(slices)).datasource`, while `Slice.datasource` returns only `self.table`; the placeholder guard is also present. This is source confirmation at your head, not a browser rerun of your branch. No changes made to your branch.
Suggested regression coverage: a semantic-only dashboard must supply its authorized datasource metadata; a mixed table/semantic dashboard with colliding integer IDs must keep both identities distinct; a semantic query HTTP400 must produce a visible terminal error rather than a spinner. Preserve the existing access checks/type boundaries. Numeric's valid rendering is verified with mocked remote GraphQL/Arrow and real provider/host/frontend; its visible-error gate remains open pending the host fix. Local reproducible scripts, screenshots, response bodies and Redux evidence are saved under `superset-spec/specs/sc-120295-metricflow-numeric-contract/validation/browser-foundation-20260916/` (no live-provider or production claim).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "Kunal8954 (via GitHub)" <gi...@apache.org>.
Kunal8954 commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5779497681
Thanks for the LGTM! Took `master`'s `messages.pot` and re-ran `babel_update.sh` after the merge — check is green now. Regeneration only re-adds the `valuename` msgid from the rename cascade.
On the `.po` files: they rode along with the script, but each diff is just `valuename` + a timestamp. Happy to drop them if it keeps the next merge cleaner — they'll catch up next extraction anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5652554525
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5634132897
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44169?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.85%. Comparing base ([`e8540b7`](https://app.codecov.io/gh/apache/superset/commit/e8540b7c49583a8ca89cfbcfd95a6a6e6a66a6ed?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`d159207`](https://app.codecov.io/gh/apache/superset/commit/d15920743ec32d05d1fb39ee16c4f67408a38ac1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 281 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44169?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/commands/chart/create.py](https://app.codecov.io/gh/apache/superset/pull/44169?src=pr&el=tree&filepath=superset%2Fcommands%2Fchart%2Fcreate.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvY2hhcnQvY3JlYXRlLnB5) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (e8540b7) and HEAD (d159207). Click for more details.
>
> <details><summary>HEAD has 41 uploads less than BASE</summary>
>
>| Flag | BASE (e8540b7) | HEAD (d159207) |
>|------|------|------|
>|python|37|1|
>|presto|6|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44169 +/- ##
===========================================
- Coverage 79.35% 56.85% -22.50%
===========================================
Files 2893 2910 +17
Lines 167502 170907 +3405
Branches 38801 39406 +605
===========================================
- Hits 132920 97173 -35747
- Misses 32086 72802 +40716
+ Partials 2496 932 -1564
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44169/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44169/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44169/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44169/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44169/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.22% <0.00%> (-0.50%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/44169/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.22% <0.00%> (-44.59%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44169/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/44169/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44169?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5795650310
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5769627378
Heya @Kunal8954, good progress here. The perm-cascade fix and the `datasource_type`-aware grouping in `dashboard.py` look right, and the api test's admin-session leak got sorted too.
But the id-collision gap sadpandajoe flagged on `slice.py` is still open, security wise, and gabotorresruiz turned up a couple more spots with the same bare-id comparison (including the sibling filter in `dashboards/filters.py`). That needs an actual fix, not just an ack, before this merges. The purge-policy failure gabotorresruiz reproduced (14 failing tests) is also still outstanding.
Rest looks close. Holler once those land (or if you want some help with any of it) and I'll take another pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5691854657
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5647620561
@Kunal8954 codeant's thread on this is still open: `Slice.datasource` only resolves `self.table`, so a semantic-view chart's `.datasource` comes back `None`. That breaks the security manager's access check for anyone who isn't chart owner/editor/admin, so the chart saves but non-privileged viewers can't open it. Needs a fix before this lands.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #44169:
URL: https://github.com/apache/superset/pull/44169
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "Kunal8954 (via GitHub)" <gi...@apache.org>.
Kunal8954 commented on code in PR #44169:
URL: https://github.com/apache/superset/pull/44169#discussion_r4016931151
##########
superset/models/slice.py:
##########
@@ -190,8 +200,10 @@ def __repr__(self) -> str:
return self.slice_name or str(self.id)
@property
- def datasource(self) -> SqlaTable | None:
- return self.table
+ def datasource(self) -> SqlaTable | SemanticView | None:
+ if table := self.table:
+ return table
+ return self.semantic_view
Review Comment:
I’ll keep the table-only authorization path type-pinned so a semantic-view ID cannot be treated as a table ID.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "Kunal8954 (via GitHub)" <gi...@apache.org>.
Kunal8954 commented on code in PR #44169:
URL: https://github.com/apache/superset/pull/44169#discussion_r4016950067
##########
superset/charts/filters.py:
##########
@@ -163,7 +175,21 @@ def _apply_viewers(self, query: Query) -> Query:
)
)
)
- filters.append(Slice.id.in_(no_viewer_query))
+ filters.append(Slice.id.in_(no_viewer_table_query))
+
+ # SEMANTIC_VIEW charts have no SqlaTable/Database row to join; access is
+ # evaluated against the chart's own perm, which set_related_perm keeps in
+ # sync with the view's ``datasource_access`` perm (no numeric-id join).
+ no_viewer_semantic_view_query = db.session.query(Slice.id).filter(
+ and_(
+ Slice.datasource_type == DatasourceType.SEMANTIC_VIEW,
+ ~chart_has_viewers,
+ Slice.perm.in_(
Review Comment:
I’ll trace the rename hooks and make sure existing slices keep their permissions in sync after a semantic-view rename.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5634084125
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44169:
URL: https://github.com/apache/superset/pull/44169#discussion_r3988927154
##########
superset/commands/chart/create.py:
##########
@@ -81,7 +81,10 @@ def validate(self) -> None:
# (SavedQuery/Query have no ``.name`` attribute, so accessing it
# below raises an unhandled AttributeError) or silently, by
# producing a permanently broken chart.
- if datasource_type != DatasourceType.TABLE:
+ if datasource_type not in {
+ DatasourceType.TABLE,
+ DatasourceType.SEMANTIC_VIEW,
+ }:
raise DatasourceTypeInvalidError()
datasource = get_datasource_by_id(datasource_id, datasource_type)
Review Comment:
**Suggestion:** `Slice.datasource` returns only `self.table`; allowing `semantic_view` stores charts whose datasource is `None`, breaking consumers that use the chart relationship. [api mismatch]
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3a1aeaf927334e6ba9a2dbb3bd1ed5d5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3a1aeaf927334e6ba9a2dbb3bd1ed5d5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/chart/create.py
**Line:** 84:89
**Comment:**
*Api Mismatch: `Slice.datasource` returns only `self.table`; allowing `semantic_view` stores charts whose datasource is `None`, breaking consumers that use the chart relationship.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44169&comment_hash=a2ada88978000ca13c530724ea5bc9b2c17f620939bd74fe2a81aa6bfdd617de&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44169&comment_hash=a2ada88978000ca13c530724ea5bc9b2c17f620939bd74fe2a81aa6bfdd617de&reaction=dislike'>👎</a>
##########
superset/commands/chart/update.py:
##########
@@ -236,7 +236,10 @@ def validate(self) -> None: # noqa: C901
# below (SavedQuery/Query have no ``.name`` attribute, so
# accessing it raises an unhandled AttributeError) or
# silently.
- if datasource_type != DatasourceType.TABLE:
+ if datasource_type not in {
+ DatasourceType.TABLE,
+ DatasourceType.SEMANTIC_VIEW,
+ }:
raise DatasourceTypeInvalidError()
if datasource_id is not None:
datasource = get_datasource_by_id(datasource_id, datasource_type)
Review Comment:
**Suggestion:** Updates can repoint a chart to `semantic_view`, but `Slice` has no semantic-view relationship, so the saved chart becomes unresolved through `Slice.datasource`. [api mismatch]
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=46547f9ad41c48d1bbef3100a8c8e4cc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=46547f9ad41c48d1bbef3100a8c8e4cc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/chart/update.py
**Line:** 239:245
**Comment:**
*Api Mismatch: Updates can repoint a chart to `semantic_view`, but `Slice` has no semantic-view relationship, so the saved chart becomes unresolved through `Slice.datasource`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44169&comment_hash=02e31780531340f33bede03947905ce73955b608f8fcc625c8839711e71ceda9&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44169&comment_hash=02e31780531340f33bede03947905ce73955b608f8fcc625c8839711e71ceda9&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "Kunal8954 (via GitHub)" <gi...@apache.org>.
Kunal8954 commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5733929387
@gabotorresruiz Thanks for the review buddy❤️
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "Kunal8954 (via GitHub)" <gi...@apache.org>.
Kunal8954 commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5634922143
The Python-Unit workflow was cancelled after all tests completed successfully (15131 passed, 5 skipped, 2 xfailed). The required check is failing only because the upstream job was cancelled. I’m unable to rerun the workflow with my contributor permissions. Could someone with maintainer access please rerun Python-Unit for this commit?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "Kunal8954 (via GitHub)" <gi...@apache.org>.
Kunal8954 commented on PR #44169:
URL: https://github.com/apache/superset/pull/44169#issuecomment-5780410862
Hey — could you re-run the failed `cypress-matrix` job when you get a chance? It died on a `fetch failed` in the setup-uv step before any tests ran, so it looks like a transient infra hiccup rather than something from this PR. Everything else on this head is green. 🙏
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on code in PR #44169:
URL: https://github.com/apache/superset/pull/44169#discussion_r4030072061
##########
tests/unit_tests/commands/chart/update_test.py:
##########
@@ -280,6 +280,42 @@ def test_update_chart_rejects_repointing_to_non_table_datasource(
get_datasource_by_id.assert_not_called()
+def test_update_chart_accepts_semantic_view_datasource(
+ mocker: MockerFixture,
+) -> None:
+ """Repointing a chart at a SIP-182 semantic view must be accepted: the
+ view is a first-class resolvable datasource (Slice resolves it through
+ the type-guarded ``semantic_view`` relationship), so the non-table guard
+ must explicitly allow it (apache/superset#44167)."""
+ from superset.semantic_layers.models import SemanticView
+
+ find_by_id = mocker.patch("superset.commands.chart.update.ChartDAO.find_by_id")
+ find_by_id.return_value = mocker.MagicMock(id=1, tags=[], dashboards=[])
Review Comment:
Blocker, but a small one and not your fault: this is the only `find_by_id` mock in the file that does not set `is_managed_externally`. The `master` merge added `raise_if_managed_externally(self._model, ChartForbiddenError)` right after the editorship check in `UpdateChartCommand.validate`, and a bare `MagicMock` attribute is truthy, so `validate()` raises `ChartForbiddenError` before it ever reaches the datasource guard this test is about.
I ran `tests/unit_tests/commands/chart/` against `refs/pull/44169/merge`: `1 failed, 69 passed`, then `70 passed` with this applied.
```suggestion
find_by_id.return_value = mocker.MagicMock(
is_managed_externally=False, id=1, tags=[], dashboards=[]
)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44169:
URL: https://github.com/apache/superset/pull/44169#discussion_r4018364951
##########
superset/commands/chart/create.py:
##########
@@ -81,7 +81,10 @@ def validate(self) -> None:
# (SavedQuery/Query have no ``.name`` attribute, so accessing it
# below raises an unhandled AttributeError) or silently, by
# producing a permanently broken chart.
- if datasource_type != DatasourceType.TABLE:
+ if datasource_type not in {
+ DatasourceType.TABLE,
+ DatasourceType.SEMANTIC_VIEW,
+ }:
raise DatasourceTypeInvalidError()
datasource = get_datasource_by_id(datasource_id, datasource_type)
Review Comment:
`Slice.datasource` now falls back to `self.semantic_view` when there is no table, so it does not come back `None` for a semantic-view chart anymore. That landed in a5d5b0b, resolving this.
##########
superset/commands/chart/update.py:
##########
@@ -236,7 +236,10 @@ def validate(self) -> None: # noqa: C901
# below (SavedQuery/Query have no ``.name`` attribute, so
# accessing it raises an unhandled AttributeError) or
# silently.
- if datasource_type != DatasourceType.TABLE:
+ if datasource_type not in {
+ DatasourceType.TABLE,
+ DatasourceType.SEMANTIC_VIEW,
+ }:
raise DatasourceTypeInvalidError()
if datasource_id is not None:
datasource = get_datasource_by_id(datasource_id, datasource_type)
Review Comment:
Same fix covers this one. `Slice.datasource` falls back to `self.semantic_view` now, so it no longer returns `None` for a semantic-view chart (landed in a5d5b0b).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): allow semantic view datasource when saving charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44169:
URL: https://github.com/apache/superset/pull/44169#discussion_r4038121734
##########
tests/unit_tests/models/dashboard_test.py:
##########
@@ -698,3 +699,40 @@ def test_tabs_places_a_node_the_layout_reaches_twice_only_once(
assert "skipping layout node TAB-1, the layout reaches it more than once" in (
caplog.text
)
+
+
+def test_datasets_trimmed_for_slices_keeps_colliding_ids_separate() -> None:
+ """Keys slices by (datasource_type, datasource_id) to avoid id collisions.
+
+ SqlaTable and SemanticView use independent auto-increment id spaces, so a
+ table chart and a semantic-view chart can carry the same datasource_id.
+ Grouping by the bare id would merge the table chart into the semantic-view
+ group (whose non-table datasource is dropped), losing the table chart's
+ metadata from the dashboard payload.
+ """
+ table_datasource = Mock(spec=BaseDatasource)
+ table_datasource.table_name = "orders"
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test AttributeError on spec mock</b></div>
<div id="fix">
`Mock(spec=BaseDatasource)` only permits attributes defined on `BaseDatasource`, but `table_name` lives on `SqlaTable` (connectors/sqla/models.py:1566), not the base class. Setting it raises `AttributeError`, so this new test fails at line 714. The attribute is never read afterward — drop the line or use `Mock(spec=SqlaTable)`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
````
</div>
</details>
</div>
<small><i>Code Review Run #3980ed</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org