You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "sadpandajoe (via GitHub)" <gi...@apache.org> on 2025/11/04 05:54:52 UTC
[PR] fix(explore): show validation errors in View Query modal [superset]
sadpandajoe opened a new pull request, #35969:
URL: https://github.com/apache/superset/pull/35969
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
Fixes a regression where the "View Query" modal displays a blank panel instead of helpful error messages when chart queries fail due to validation errors (e.g., missing required fields, invalid metrics).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
#### Automated Tests
```bash
# Backend unit tests
pytest tests/unit_tests/charts/commands/data/test_get_data_command.py -v
# Frontend component test
npm run test -- ViewQueryModal.test.tsx
```
#### Manual Testing
**Test Case 1: Validation Error (The Fix)**
1. Create a chart in Explore view
2. Add duplicate metric labels (e.g., add "sum__num" twice)
3. Click "View Query" button
4. **Expected**: Red Alert shows "Duplicate column/metric labels: sum__num"
5. **Before fix**: Blank panel or generic error
**Test Case 2: Valid Chart (No Regression)**
1. Create a valid chart
2. Click "View Query"
3. **Expected**: SQL query displayed normally
4. **Verify**: No regression on happy path
**Test Case 3: Database Error (No Regression)**
1. Create a chart with invalid column in database
2. Click "View Query"
3. **Expected**: Shows SQL query (generated before execution error)
4. **Verify**: Database errors still work
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue: Fixes #35492
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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(explore): show validation errors in View Query modal [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35969:
URL: https://github.com/apache/superset/pull/35969#issuecomment-3503860500
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/19176342710) for [7620bc8](https://github.com/apache/superset/commit/7620bc8)
--
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(explore): show validation errors in View Query modal [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #35969:
URL: https://github.com/apache/superset/pull/35969#discussion_r2501595432
##########
superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:
##########
@@ -87,16 +89,26 @@ const ViewQueryModal: FC<Props> = ({ latestQueryFormData }) => {
return (
<ViewQueryModalContainer>
- {result.map((item, index) =>
- item.query ? (
- <ViewQuery
- key={`query-${index}`}
- datasource={latestQueryFormData.datasource}
- sql={item.query}
- language="sql"
- />
- ) : null,
- )}
+ {result.map((item, index) => (
+ <Fragment key={index}>
+ {item.error && (
+ <Alert
+ key={`error-${index}`}
+ type="error"
+ message={item.error}
+ closable={false}
+ />
+ )}
+ {item.query && (
+ <ViewQuery
+ key={`query-${index}`}
+ datasource={latestQueryFormData.datasource}
+ sql={item.query}
+ language={item.language}
+ />
+ )}
+ </Fragment>
+ ))}
Review Comment:
The result array is static (loaded once from API, never modified)
--
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(explore): show validation errors in View Query modal [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35969:
URL: https://github.com/apache/superset/pull/35969#issuecomment-3504438924
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/19178838846) for [5030db3](https://github.com/apache/superset/commit/5030db3)
--
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(explore): show validation errors in View Query modal [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35969:
URL: https://github.com/apache/superset/pull/35969#issuecomment-3505113427
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/19181947656) for [add009b](https://github.com/apache/superset/commit/add009b)
• **Environment:** http://35.166.36.14:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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(explore): show validation errors in View Query modal [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #35969:
URL: https://github.com/apache/superset/pull/35969#discussion_r2501602724
##########
superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:
##########
@@ -87,16 +89,26 @@ const ViewQueryModal: FC<Props> = ({ latestQueryFormData }) => {
return (
<ViewQueryModalContainer>
- {result.map((item, index) =>
- item.query ? (
- <ViewQuery
- key={`query-${index}`}
- datasource={latestQueryFormData.datasource}
- sql={item.query}
- language="sql"
- />
- ) : null,
- )}
+ {result.map((item, index) => (
+ <Fragment key={index}>
+ {item.error && (
+ <Alert
+ key={`error-${index}`}
+ type="error"
+ message={item.error}
+ closable={false}
+ />
+ )}
+ {item.query && (
+ <ViewQuery
+ key={`query-${index}`}
+ datasource={latestQueryFormData.datasource}
+ sql={item.query}
+ language={item.language}
+ />
+ )}
+ </Fragment>
+ ))}
Review Comment:
Duplicate keys are being set. The Fragment has `key={index}`, but the child Alert and ViewQuery components also have keys (e.g., `key={`error-${index}`}`). Since the Fragment already has a unique key, the child component keys are unnecessary. Remove the keys from the Alert and ViewQuery components to avoid redundancy.
--
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(explore): show validation errors in View Query modal [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35969:
URL: https://github.com/apache/superset/pull/35969#issuecomment-3503929507
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/19176342710) for [7620bc8](https://github.com/apache/superset/commit/7620bc8)
• **Environment:** http://52.24.34.130:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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(explore): show validation errors in View Query modal [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35969:
URL: https://github.com/apache/superset/pull/35969#issuecomment-3500054758
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/19155054264) for [565fcfb](https://github.com/apache/superset/commit/565fcfb)
--
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(explore): show validation errors in View Query modal [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35969:
URL: https://github.com/apache/superset/pull/35969#issuecomment-3494217559
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/19120500756) for [345a93e](https://github.com/apache/superset/commit/345a93e)
--
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(explore): show validation errors in View Query modal [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #35969:
URL: https://github.com/apache/superset/pull/35969#issuecomment-3494303994
@copilot review
--
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(explore): show validation errors in View Query modal [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #35969:
URL: https://github.com/apache/superset/pull/35969#discussion_r2504766333
##########
superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:
##########
@@ -87,16 +89,24 @@ const ViewQueryModal: FC<Props> = ({ latestQueryFormData }) => {
return (
<ViewQueryModalContainer>
- {result.map((item, index) =>
- item.query ? (
- <ViewQuery
- key={`query-${index}`}
- datasource={latestQueryFormData.datasource}
- sql={item.query}
- language="sql"
- />
- ) : null,
- )}
+ {result.map((item, index) => {
+ // Use content-based key when available, fall back to index
+ const key = item.query || item.error || `result-${index}`;
+ return (
+ <Fragment key={key}>
Review Comment:
Using query or error text as a React key could cause issues if the same error/query appears multiple times, or if very long SQL queries are used as keys. Consider using a more stable identifier or always use the index-based key pattern.
```suggestion
// Use index as key to avoid issues with duplicate or long query/error strings
return (
<Fragment key={index}>
```
--
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(explore): show validation errors in View Query modal [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe merged PR #35969:
URL: https://github.com/apache/superset/pull/35969
--
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(explore): show validation errors in View Query modal [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35969:
URL: https://github.com/apache/superset/pull/35969#issuecomment-3500080072
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/19155054264) for [565fcfb](https://github.com/apache/superset/commit/565fcfb)
• **Environment:** http://18.236.166.70:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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(explore): show validation errors in View Query modal [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35969:
URL: https://github.com/apache/superset/pull/35969#discussion_r2488820199
##########
superset/charts/schemas.py:
##########
@@ -1482,9 +1482,12 @@ class ChartDataResponseResult(Schema):
allow_none=None,
)
query = fields.String(
- metadata={"description": "The executed query statement"},
- required=True,
- allow_none=False,
+ metadata={
+ "description": "The executed query statement. May be absent when "
+ "validation errors occur."
+ },
+ required=False,
+ allow_none=True,
)
Review Comment:
### Missing validation for query field on successful status <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The query field is now optional and nullable, but there's no validation to ensure it's present when status indicates success, potentially allowing successful responses without query information.
###### Why this matters
This could lead to inconsistent API responses where successful query executions don't include the executed query statement, making debugging and auditing difficult for consumers who expect query information on successful operations.
###### Suggested change ∙ *Feature Preview*
Add conditional validation to ensure the query field is present when status indicates success. This can be implemented using Marshmallow's `validates_schema` decorator:
```python
from marshmallow import validates_schema, ValidationError
@validates_schema
def validate_query_presence(self, data, **kwargs):
status = data.get('status')
query = data.get('query')
if status == 'success' and not query:
raise ValidationError('Query field is required when status is success')
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2bb21247-9ec3-4805-8d62-7ac3ab17cbd2/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2bb21247-9ec3-4805-8d62-7ac3ab17cbd2?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2bb21247-9ec3-4805-8d62-7ac3ab17cbd2?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2bb21247-9ec3-4805-8d62-7ac3ab17cbd2?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2bb21247-9ec3-4805-8d62-7ac3ab17cbd2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:13601073-42ad-4f12-a44f-fc494175c45b -->
[](13601073-42ad-4f12-a44f-fc494175c45b)
##########
superset/commands/chart/data/get_data_command.py:
##########
@@ -48,8 +49,13 @@ def run(self, **kwargs: Any) -> dict[str, Any]:
except CacheLoadError as ex:
raise ChartDataCacheLoadError(ex.message) from ex
+ # Skip error check for query-only requests - errors are returned in payload
+ # This allows View Query modal to display validation errors
for query in payload["queries"]:
- if query.get("error"):
+ if (
+ query.get("error")
+ and self._query_context.result_type != ChartDataResultType.QUERY
+ ):
Review Comment:
### Negative logic in result type check may bypass error handling <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The condition uses inequality (!=) to check result_type, which will skip error raising for QUERY type but may also skip it for other unexpected result types due to the negative logic.
###### Why this matters
If new ChartDataResultType values are added in the future or if result_type is None/undefined, the error checking will be unexpectedly bypassed, potentially allowing invalid queries to proceed without proper error handling.
###### Suggested change ∙ *Feature Preview*
Use positive logic to explicitly check for the QUERY type:
```python
if (
query.get("error")
and self._query_context.result_type == ChartDataResultType.QUERY
):
# Skip raising error for query-only requests
continue
# Always raise error for non-query types
if query.get("error"):
raise ChartDataQueryFailedError(
_("Error: %(error)s", error=query["error"])
)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d9b475ce-3dad-4ea4-b5b9-c2afcda55396/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d9b475ce-3dad-4ea4-b5b9-c2afcda55396?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d9b475ce-3dad-4ea4-b5b9-c2afcda55396?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d9b475ce-3dad-4ea4-b5b9-c2afcda55396?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d9b475ce-3dad-4ea4-b5b9-c2afcda55396)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8a13a5b5-1e6a-4903-a23f-22374da429bc -->
[](8a13a5b5-1e6a-4903-a23f-22374da429bc)
##########
superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:
##########
@@ -88,12 +90,19 @@ const ViewQueryModal: FC<Props> = ({ latestQueryFormData }) => {
return (
<ViewQueryModalContainer>
{result.map((item, index) =>
- item.query ? (
+ item.error ? (
+ <Alert
+ key={`error-${index}`}
+ type="error"
+ message={item.error}
+ closable={false}
+ />
+ ) : item.query ? (
<ViewQuery
key={`query-${index}`}
datasource={latestQueryFormData.datasource}
sql={item.query}
- language="sql"
+ language={item.language}
Review Comment:
### Potential undefined language prop <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The ViewQuery component receives item.language which could be undefined when item.query exists but item.language is not properly set.
###### Why this matters
This could cause the ViewQuery component to receive an undefined language prop, potentially breaking syntax highlighting or causing runtime errors in the code display functionality.
###### Suggested change ∙ *Feature Preview*
Add a fallback language value or ensure language is always defined:
```typescript
language={item.language || 'sql'}
```
Or add a type guard to ensure both query and language are defined before rendering ViewQuery.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fac6bab7-e3dd-4b66-992b-237eb71c47bd/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fac6bab7-e3dd-4b66-992b-237eb71c47bd?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fac6bab7-e3dd-4b66-992b-237eb71c47bd?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fac6bab7-e3dd-4b66-992b-237eb71c47bd?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fac6bab7-e3dd-4b66-992b-237eb71c47bd)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:18c3e6e9-cfd3-472c-b9f1-69274b2f4e7a -->
[](18c3e6e9-cfd3-472c-b9f1-69274b2f4e7a)
--
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(explore): show validation errors in View Query modal [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35969:
URL: https://github.com/apache/superset/pull/35969#issuecomment-3484050087
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/19059429438) for [bf4a66e](https://github.com/apache/superset/commit/bf4a66e)
--
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(explore): show validation errors in View Query modal [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35969:
URL: https://github.com/apache/superset/pull/35969#discussion_r2501597666
##########
superset/commands/chart/data/get_data_command.py:
##########
@@ -48,8 +49,13 @@ def run(self, **kwargs: Any) -> dict[str, Any]:
except CacheLoadError as ex:
raise ChartDataCacheLoadError(ex.message) from ex
+ # Skip error check for query-only requests - errors are returned in payload
+ # This allows View Query modal to display validation errors
for query in payload["queries"]:
- if query.get("error"):
+ if (
+ query.get("error")
+ and self._query_context.result_type != ChartDataResultType.QUERY
+ ):
Review Comment:
Thank you for the clarification. Your explanation makes sense, and I agree that the current implementation is correct and more maintainable. The comprehensive test coverage further supports this approach.
--
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(explore): show validation errors in View Query modal [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #35969:
URL: https://github.com/apache/superset/pull/35969#discussion_r2501594454
##########
superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:
##########
@@ -25,7 +25,8 @@ import {
getClientErrorObject,
QueryFormData,
} from '@superset-ui/core';
-import { Loading } from '@superset-ui/core/components';
+import { Loading, Alert } from '@superset-ui/core/components';
Review Comment:
nope we should not be using this
--
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(explore): show validation errors in View Query modal [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35969:
URL: https://github.com/apache/superset/pull/35969#issuecomment-3500273717
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/19156355278) for [2f41fce](https://github.com/apache/superset/commit/2f41fce)
• **Environment:** http://52.40.60.56:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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(explore): show validation errors in View Query modal [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #35969:
URL: https://github.com/apache/superset/pull/35969#discussion_r2501595979
##########
superset/charts/schemas.py:
##########
@@ -1482,9 +1482,12 @@ class ChartDataResponseResult(Schema):
allow_none=None,
)
query = fields.String(
- metadata={"description": "The executed query statement"},
- required=True,
- allow_none=False,
+ metadata={
+ "description": "The executed query statement. May be absent when "
+ "validation errors occur."
+ },
+ required=False,
+ allow_none=True,
)
Review Comment:
The schema is used for multiple result types with different field requirements
--
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(explore): show validation errors in View Query modal [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35969:
URL: https://github.com/apache/superset/pull/35969#issuecomment-3504504371
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/19178838846) for [5030db3](https://github.com/apache/superset/commit/5030db3)
• **Environment:** http://35.95.73.104:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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(explore): show validation errors in View Query modal [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35969:
URL: https://github.com/apache/superset/pull/35969#issuecomment-3505090739
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/19181947656) for [add009b](https://github.com/apache/superset/commit/add009b)
--
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(explore): show validation errors in View Query modal [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35969:
URL: https://github.com/apache/superset/pull/35969#issuecomment-3484029474
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(explore): show validation errors in View Query modal [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #35969:
URL: https://github.com/apache/superset/pull/35969#discussion_r2501596866
##########
superset/commands/chart/data/get_data_command.py:
##########
@@ -48,8 +49,13 @@ def run(self, **kwargs: Any) -> dict[str, Any]:
except CacheLoadError as ex:
raise ChartDataCacheLoadError(ex.message) from ex
+ # Skip error check for query-only requests - errors are returned in payload
+ # This allows View Query modal to display validation errors
for query in payload["queries"]:
- if query.get("error"):
+ if (
+ query.get("error")
+ and self._query_context.result_type != ChartDataResultType.QUERY
+ ):
Review Comment:
1. The logic is correct: QUERY is the special case, all others should fail fast
2. More maintainable than positive logic - new result types will automatically get safe default behavior
3. Comprehensive test coverage confirms it works for QUERY, FULL, and RESULTS types
--
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(explore): show validation errors in View Query modal [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #35969:
URL: https://github.com/apache/superset/pull/35969#discussion_r2504638134
##########
superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:
##########
@@ -87,16 +89,26 @@ const ViewQueryModal: FC<Props> = ({ latestQueryFormData }) => {
return (
<ViewQueryModalContainer>
- {result.map((item, index) =>
- item.query ? (
- <ViewQuery
- key={`query-${index}`}
- datasource={latestQueryFormData.datasource}
- sql={item.query}
- language="sql"
- />
- ) : null,
- )}
+ {result.map((item, index) => (
+ <Fragment key={index}>
+ {item.error && (
+ <Alert
+ key={`error-${index}`}
+ type="error"
+ message={item.error}
+ closable={false}
+ />
+ )}
+ {item.query && (
+ <ViewQuery
+ key={`query-${index}`}
+ datasource={latestQueryFormData.datasource}
+ sql={item.query}
+ language={item.language}
+ />
+ )}
+ </Fragment>
+ ))}
Review Comment:
nice catch
--
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(explore): show validation errors in View Query modal [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35969:
URL: https://github.com/apache/superset/pull/35969#issuecomment-3503672886
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/19175353956) for [3f34a81](https://github.com/apache/superset/commit/3f34a81)
--
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(explore): show validation errors in View Query modal [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35969:
URL: https://github.com/apache/superset/pull/35969#issuecomment-3484080841
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/19059429438) for [bf4a66e](https://github.com/apache/superset/commit/bf4a66e)
• **Environment:** http://35.92.249.128:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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(explore): show validation errors in View Query modal [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35969:
URL: https://github.com/apache/superset/pull/35969#issuecomment-3494265220
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/19120500756) for [345a93e](https://github.com/apache/superset/commit/345a93e)
• **Environment:** http://35.90.244.30:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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(explore): show validation errors in View Query modal [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #35969:
URL: https://github.com/apache/superset/pull/35969#discussion_r2496670649
##########
superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:
##########
@@ -25,7 +25,8 @@ import {
getClientErrorObject,
QueryFormData,
} from '@superset-ui/core';
-import { Loading } from '@superset-ui/core/components';
+import { Loading, Alert } from '@superset-ui/core/components';
Review Comment:
The `Alert` import should be verified. Based on the typical structure of @superset-ui/core, Alert components are usually imported from a different path (e.g., from the package that provides Ant Design components). Please verify that this import path is correct for the Alert component.
```suggestion
import { Loading } from '@superset-ui/core/components';
import { Alert } from 'antd';
```
##########
superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:
##########
@@ -87,16 +89,26 @@ const ViewQueryModal: FC<Props> = ({ latestQueryFormData }) => {
return (
<ViewQueryModalContainer>
- {result.map((item, index) =>
- item.query ? (
- <ViewQuery
- key={`query-${index}`}
- datasource={latestQueryFormData.datasource}
- sql={item.query}
- language="sql"
- />
- ) : null,
- )}
+ {result.map((item, index) => (
+ <Fragment key={index}>
+ {item.error && (
+ <Alert
+ key={`error-${index}`}
+ type="error"
+ message={item.error}
+ closable={false}
+ />
+ )}
+ {item.query && (
+ <ViewQuery
+ key={`query-${index}`}
+ datasource={latestQueryFormData.datasource}
+ sql={item.query}
+ language={item.language}
+ />
+ )}
+ </Fragment>
+ ))}
Review Comment:
Using `index` as the React key is an anti-pattern when the list order can change or items can be added/removed. Consider using a more stable identifier, such as combining it with a unique property from the item (e.g., `${item.query?.substring(0, 20) || item.error?.substring(0, 20)}-${index}`).
```suggestion
{result.map((item, index) => {
const stableKey =
(item.query?.substring(0, 20) || item.error?.substring(0, 20) || '') +
'-' +
index;
return (
<Fragment key={stableKey}>
{item.error && (
<Alert
key={`error-${stableKey}`}
type="error"
message={item.error}
closable={false}
/>
)}
{item.query && (
<ViewQuery
key={`query-${stableKey}`}
datasource={latestQueryFormData.datasource}
sql={item.query}
language={item.language}
/>
)}
</Fragment>
);
})}
```
##########
superset/common/query_actions.py:
##########
@@ -86,7 +86,15 @@ def _get_query(
try:
result["query"] = datasource.get_query_str(query_obj.to_dict())
except QueryObjectValidationError as err:
+ # Validation errors (missing required fields, invalid config)
+ # No SQL was generated
result["error"] = err.message
+ except SupersetParseError as err:
+ # Parsing errors (SQL optimization/parsing failed)
+ # SQL was generated but couldn't be optimized - show both
+ if err.error.extra:
+ result["query"] = err.error.extra.get("sql")
Review Comment:
The conditional check `if err.error.extra:` should also verify that the 'sql' key exists before attempting to get it. Consider using `if err.error.extra and 'sql' in err.error.extra:` or check if the value is not None after getting it to avoid setting `result['query']` to `None`.
```suggestion
if err.error.extra and "sql" in err.error.extra and err.error.extra["sql"] is not None:
result["query"] = err.error.extra["sql"]
```
--
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(explore): show validation errors in View Query modal [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35969:
URL: https://github.com/apache/superset/pull/35969#issuecomment-3500250640
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/19156355278) for [2f41fce](https://github.com/apache/superset/commit/2f41fce)
--
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(explore): show validation errors in View Query modal [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35969:
URL: https://github.com/apache/superset/pull/35969#issuecomment-3503745788
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/19175353956) for [3f34a81](https://github.com/apache/superset/commit/3f34a81)
• **Environment:** http://44.244.205.125:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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