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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
