You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/08/25 04:42:19 UTC
[PR] fix(charts): reject non-table datasource_type instead of crashing [superset]
rusackas opened a new pull request, #43500:
URL: https://github.com/apache/superset/pull/43500
### SUMMARY
`POST /api/v1/chart/` (and `PUT /api/v1/chart/<pk>`) with `datasource_type: "saved_query"` crashes with an unhandled `AttributeError: 'SavedQuery' object has no attribute 'name'`, surfaced to API clients as an opaque 500 `{"message": "Fatal error"}`.
The deeper issue: `Slice.datasource` only ever resolves the `table` relationship (`superset/models/slice.py`), so a chart pointed at any other `datasource_type` could never actually render even if creation succeeded. That's the case for `datasource_type: "query"` too — `Query` happens to have a synthetic `.name` property (used for CTAS table naming, not a real display name), so it doesn't crash, it just silently produces a permanently broken chart instead.
`CreateChartCommand` and `UpdateChartCommand` now reject both up front with the existing `DatasourceTypeInvalidError` (422), matching the pattern already used for this same class of problem elsewhere in the codebase (`explore/utils.py`, `dataset/duplicate.py`), instead of adding a new one-off error type.
Root-caused with help from the discussion on the issue — thanks @endimonan for the minimal repro that ruled out pivot tables as a factor and narrowed this down.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable (API error-handling behavior).
### TESTING INSTRUCTIONS
```
pytest tests/unit_tests/commands/chart/create_test.py tests/unit_tests/commands/chart/update_test.py tests/unit_tests/commands/chart/ tests/unit_tests/charts/
```
Written test-first: each new unit test was confirmed to fail against unfixed code (reproducing the exact `AttributeError` for `saved_query`, and the "silently succeeds with a broken chart" behavior for `query`) before the fix was applied, then confirmed to pass after.
Also added an integration test (`tests/integration_tests/charts/api_tests.py::test_create_chart_from_saved_query_rejected_cleanly`) that reproduces the original bug report's exact API call shape end-to-end — creates a real `SavedQuery` row, then asserts `POST /api/v1/chart/` against it returns a clean 422 instead of a 500. Couldn't run the integration suite locally (this environment's SQLite test DB isn't migrated), so CI is the authority on that one.
Manual repro before the fix, matching the original report:
```bash
curl -X POST "$SUPERSET_API_URL/api/v1/saved_query/" ... --data '{"db_id": 1, "label": "repro", "sql": "SELECT 1"}'
# => 201, id: N
curl -X POST "$SUPERSET_API_URL/api/v1/chart/" ... --data '{"datasource_id": N, "datasource_type": "saved_query", "slice_name": "x", "viz_type": "table"}'
# before: 500 {"message": "Fatal error"}
# after: 422 {"message": {"datasource_type": ["Datasource type is invalid"]}}
```
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #29697
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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(charts): reject non-table datasource_type instead of crashing [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43500:
URL: https://github.com/apache/superset/pull/43500#issuecomment-5408181460
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(charts): reject non-table datasource_type instead of crashing [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43500:
URL: https://github.com/apache/superset/pull/43500#issuecomment-5405471176
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43500?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 `75.00000%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 66.08%. Comparing base ([`6c2fef2`](https://app.codecov.io/gh/apache/superset/commit/6c2fef29cb7a781b2e451b2731cfd8a889dc6299?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f750321`](https://app.codecov.io/gh/apache/superset/commit/f7503213e29caf31f2ec9c1bd74eb288dc24509c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/43500?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/update.py](https://app.codecov.io/gh/apache/superset/pull/43500?src=pr&el=tree&filepath=superset%2Fcommands%2Fchart%2Fupdate.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvY2hhcnQvdXBkYXRlLnB5) | 50.00% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/superset/pull/43500?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 (6c2fef2) and HEAD (f750321). Click for more details.
>
> <details><summary>HEAD has 5 uploads less than BASE</summary>
>
>| Flag | BASE (6c2fef2) | HEAD (f750321) |
>|------|------|------|
>|python|8|3|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #43500 +/- ##
===========================================
- Coverage 78.93% 66.08% -12.85%
===========================================
Files 2877 2877
Lines 165232 165240 +8
Branches 38184 38186 +2
===========================================
- Hits 130423 109207 -21216
- Misses 32347 53811 +21464
+ Partials 2462 2222 -240
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43500/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/43500/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.01% <50.00%> (+<0.01%)` | :arrow_up: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/43500/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/43500/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/43500/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.94% <50.00%> (+<0.01%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/43500/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.61% <75.00%> (-25.95%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/43500/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.48% <75.00%> (+<0.01%)` | :arrow_up: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/43500/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/43500?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(charts): reject non-table datasource_type instead of crashing [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe merged PR #43500:
URL: https://github.com/apache/superset/pull/43500
--
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(charts): reject non-table datasource_type instead of crashing [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43500:
URL: https://github.com/apache/superset/pull/43500#discussion_r3850044779
##########
superset/commands/chart/update.py:
##########
@@ -223,6 +225,17 @@ def validate(self) -> None: # noqa: C901
# Validate/Populate datasource
if datasource_id is not None:
try:
+ # Slice.datasource only ever resolves the ``table``
+ # relationship (see Slice.datasource in
+ # superset/models/slice.py), so repointing a chart at any
+ # other datasource_type would "succeed" but leave the chart
+ # permanently unable to render. Reject those up front
+ # instead of failing later -- either at this lookup
+ # (SavedQuery/Query have no ``.name`` attribute, so
+ # accessing it below raises an unhandled AttributeError) or
+ # silently.
+ if datasource_type != DatasourceType.TABLE:
+ raise DatasourceTypeInvalidError()
Review Comment:
Good catch — fixed. The guard now only fires when `datasource_type` is actually non-empty, so the missing-type case still surfaces `DatasourceTypeUpdateRequiredValidationError` instead of getting overwritten.
--
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(charts): reject non-table datasource_type instead of crashing [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43500:
URL: https://github.com/apache/superset/pull/43500#discussion_r3869478693
##########
superset/commands/chart/update.py:
##########
@@ -221,8 +223,22 @@ def validate(self) -> None: # noqa: C901
exceptions.append(ex)
# Validate/Populate datasource
- if datasource_id is not None:
+ # An empty datasource_type was already flagged above via
+ # DatasourceTypeUpdateRequiredValidationError; skip this block so
+ # we don't clobber that message with DatasourceTypeInvalidError.
+ if datasource_id is not None and datasource_type:
Review Comment:
Good catch, fixed! The check now fires whenever `datasource_type` is present, not just alongside `datasource_id`, so a type-only update cant slip a saved_query/query type past it anymore.
##########
superset/commands/chart/update.py:
##########
@@ -221,8 +223,22 @@ def validate(self) -> None: # noqa: C901
exceptions.append(ex)
# Validate/Populate datasource
- if datasource_id is not None:
+ # An empty datasource_type was already flagged above via
+ # DatasourceTypeUpdateRequiredValidationError; skip this block so
+ # we don't clobber that message with DatasourceTypeInvalidError.
+ if datasource_id is not None and datasource_type:
Review Comment:
Good catch, fixed! The check now fires whenever `datasource_type` is present, not just alongside `datasource_id`, so a type-only update can't slip a saved_query/query type past it anymore.
--
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(charts): reject non-table datasource_type instead of crashing [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43500:
URL: https://github.com/apache/superset/pull/43500#issuecomment-5405367692
<!-- Bito Reply -->
The flagged issue is correct. The current implementation in `superset/commands/chart/update.py` raises `DatasourceTypeInvalidError` inside the `try` block, which causes it to be caught and appended to the validation errors. If `datasource_type` is missing, a `DatasourceTypeUpdateRequiredValidationError` is already present, and appending the invalid-type error for an empty string overwrites the more relevant required-field message.
To resolve this, you should only perform the `DatasourceType` check when `datasource_type` is provided. You can update the guard in `superset/commands/chart/update.py` as follows:
```python
if datasource_type and datasource_type != DatasourceType.TABLE:
raise DatasourceTypeInvalidError()
```
This ensures the check only runs when a type is supplied, preserving the existing required-field validation when it is missing. Would you like me to check the rest of the comments on this PR to see if there are other issues to address?
**superset/commands/chart/update.py**
```
if datasource_type and datasource_type != DatasourceType.TABLE:
raise DatasourceTypeInvalidError()
```
--
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(charts): reject non-table datasource_type instead of crashing [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43500:
URL: https://github.com/apache/superset/pull/43500#discussion_r3858441555
##########
tests/integration_tests/charts/api_tests.py:
##########
@@ -660,6 +662,46 @@ def test_create_chart_validate_datasource(self):
response = json.loads(rv.data.decode("utf-8"))
assert response == {"message": {"datasource_id": ["Datasource does not exist"]}}
+ def test_create_chart_from_saved_query_rejected_cleanly(self):
+ """
+ Chart API: creating a chart with datasource_type="saved_query" must
+ fail with a clean validation error, not the unhandled 500 "Fatal
+ error" reported in apache/superset#29697. Slice.datasource only
+ ever resolves the "table" relationship, so even a chart that
+ "created" successfully with this datasource_type could never
+ actually render -- "saved_query" is a real, existing row here
+ (not a bad ID), reproducing the original report exactly rather
+ than a not-found case.
+ """
+ self.login(ADMIN_USERNAME)
+ example_db = get_example_database()
+ saved_query = SavedQuery(
+ db_id=example_db.id,
+ label="issue-29697-repro",
+ schema=get_example_default_schema(),
+ sql="SELECT 1 AS value",
+ )
+ db.session.add(saved_query)
+ db.session.commit()
+ saved_query_id = saved_query.id
+
+ chart_data = {
+ "slice_name": "issue-29697-repro-chart",
+ "datasource_id": saved_query_id,
+ "datasource_type": "saved_query",
+ "viz_type": "table",
Review Comment:
If `post_assert_metric` raises, this committed `SavedQuery` is never removed, so the shared integration database retains it and later tests can observe the leaked row. Could this setup/cleanup use a fixture or `try`/`finally` so the row is removed on failure too?
--
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(charts): reject non-table datasource_type instead of crashing [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43500:
URL: https://github.com/apache/superset/pull/43500#issuecomment-5419811926
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | e82adfbf5d1d8966238fe51715da8850fcb45a5f |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a8e50c692edc700089cc766 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-43500--superset-docs-preview.netlify.app](https://deploy-preview-43500--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#DctLDkAwEADQu8xao5Gozw0cY8yMkKA1SjTi7rp7m_dCUL-FCD1AAaPiTnP2tDwlzajRnHgLm-MSTYYx4ukvJTExBcmBJaw-DZyLw1ZqS66rhKmx1rYdUeMcfD8) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43500--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(charts): reject non-table datasource_type instead of crashing [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 #43500:
URL: https://github.com/apache/superset/pull/43500#discussion_r3849646226
##########
superset/commands/chart/update.py:
##########
@@ -223,6 +225,17 @@ def validate(self) -> None: # noqa: C901
# Validate/Populate datasource
if datasource_id is not None:
try:
+ # Slice.datasource only ever resolves the ``table``
+ # relationship (see Slice.datasource in
+ # superset/models/slice.py), so repointing a chart at any
+ # other datasource_type would "succeed" but leave the chart
+ # permanently unable to render. Reject those up front
+ # instead of failing later -- either at this lookup
+ # (SavedQuery/Query have no ``.name`` attribute, so
+ # accessing it below raises an unhandled AttributeError) or
+ # silently.
+ if datasource_type != DatasourceType.TABLE:
+ raise DatasourceTypeInvalidError()
Review Comment:
**Suggestion:** The new guard raises `DatasourceTypeInvalidError` inside a `try` block that catches `ValidationError`, so it is appended rather than propagated. When `datasource_id` is supplied without `datasource_type`, the existing `DatasourceTypeUpdateRequiredValidationError` is already appended, then this guard appends the invalid-type error for the empty string. `normalized_messages()` updates the same `datasource_type` key with the later error, changing the established response from “Datasource type is required when datasource_id is given” to the incorrect “Datasource type is invalid.” Only perform this check when a non-empty datasource type was supplied, or preserve the required-field error. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Chart PUT requests omitting datasource type return the wrong validation message.
- ⚠️ Clients lose the required-field guidance for datasource updates.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=74294e23a9b34d18ab56e39074c67f88&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=74294e23a9b34d18ab56e39074c67f88&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:** 237:238
**Comment:**
*Api Mismatch: The new guard raises `DatasourceTypeInvalidError` inside a `try` block that catches `ValidationError`, so it is appended rather than propagated. When `datasource_id` is supplied without `datasource_type`, the existing `DatasourceTypeUpdateRequiredValidationError` is already appended, then this guard appends the invalid-type error for the empty string. `normalized_messages()` updates the same `datasource_type` key with the later error, changing the established response from “Datasource type is required when datasource_id is given” to the incorrect “Datasource type is invalid.” Only perform this check when a non-empty datasource type was supplied, or preserve the required-field error.
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%2F43500&comment_hash=7796b213412594992a8f4d3b1054f1d0868c685508d7e49a677b2a046ff557b6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43500&comment_hash=7796b213412594992a8f4d3b1054f1d0868c685508d7e49a677b2a046ff557b6&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(charts): reject non-table datasource_type instead of crashing [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43500:
URL: https://github.com/apache/superset/pull/43500#issuecomment-5405328432
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(charts): reject non-table datasource_type instead of crashing [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43500:
URL: https://github.com/apache/superset/pull/43500#discussion_r3868477844
##########
superset/commands/chart/update.py:
##########
@@ -221,8 +223,22 @@ def validate(self) -> None: # noqa: C901
exceptions.append(ex)
# Validate/Populate datasource
- if datasource_id is not None:
+ # An empty datasource_type was already flagged above via
+ # DatasourceTypeUpdateRequiredValidationError; skip this block so
+ # we don't clobber that message with DatasourceTypeInvalidError.
+ if datasource_id is not None and datasource_type:
Review Comment:
A type-only chart update can still set `datasource_type` to `saved_query` while retaining the existing datasource ID, leaving `Slice.datasource` unable to resolve the chart. Could this reject a non-table type whenever that field is present, and cover that partial-update request?
--
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(charts): reject non-table datasource_type instead of crashing [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43500:
URL: https://github.com/apache/superset/pull/43500#issuecomment-5437628778
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(charts): reject non-table datasource_type instead of crashing [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43500:
URL: https://github.com/apache/superset/pull/43500#issuecomment-5421073811
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(charts): reject non-table datasource_type instead of crashing [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43500:
URL: https://github.com/apache/superset/pull/43500#discussion_r3859192554
##########
tests/integration_tests/charts/api_tests.py:
##########
@@ -660,6 +662,46 @@ def test_create_chart_validate_datasource(self):
response = json.loads(rv.data.decode("utf-8"))
assert response == {"message": {"datasource_id": ["Datasource does not exist"]}}
+ def test_create_chart_from_saved_query_rejected_cleanly(self):
+ """
+ Chart API: creating a chart with datasource_type="saved_query" must
+ fail with a clean validation error, not the unhandled 500 "Fatal
+ error" reported in apache/superset#29697. Slice.datasource only
+ ever resolves the "table" relationship, so even a chart that
+ "created" successfully with this datasource_type could never
+ actually render -- "saved_query" is a real, existing row here
+ (not a bad ID), reproducing the original report exactly rather
+ than a not-found case.
+ """
+ self.login(ADMIN_USERNAME)
+ example_db = get_example_database()
+ saved_query = SavedQuery(
+ db_id=example_db.id,
+ label="issue-29697-repro",
+ schema=get_example_default_schema(),
+ sql="SELECT 1 AS value",
+ )
+ db.session.add(saved_query)
+ db.session.commit()
+ saved_query_id = saved_query.id
+
+ chart_data = {
+ "slice_name": "issue-29697-repro-chart",
+ "datasource_id": saved_query_id,
+ "datasource_type": "saved_query",
+ "viz_type": "table",
Review Comment:
Good call — wrapped it in try/finally so the row gets cleaned up even if the assertion fails.
--
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