codeant-ai-for-open-source[bot] commented on code in PR #36076:
URL: https://github.com/apache/superset/pull/36076#discussion_r3524946128
##########
superset/utils/schema.py:
##########
@@ -83,3 +84,35 @@ def validate_external_url(value: Optional[str]) -> None:
)
if not parsed.netloc:
raise ValidationError("URL must be absolute and include a host.")
+
+
+def validate_query_context_metadata(value: Union[bytes, bytearray, str, None])
-> None:
+ """
+ Validator for query_context field to ensure it contains required metadata.
+
+ Validates that the query_context JSON contains the required 'datasource'
and
+ 'queries' fields needed for chart data retrieval.
+
+ :raises ValidationError: if value is not valid JSON or missing required
fields
+ :param value: a JSON string that should contain datasource and queries
metadata
+ """
+ if value is None or value == "":
+ return # Allow None values and empty strings
+
+ # Reuse existing JSON validation logic
+ validate_json(value)
+
+ # Parse and validate the structure
+ parsed_data = json.loads(value)
+
+ # Validate required fields exist in the query_context
+ if not isinstance(parsed_data, dict):
+ error_msg = "Query context must be a valid JSON object"
+ raise ValidationError(error_msg)
+
+ # When query_context is provided (not None), validate it has required
fields
+ required_fields = {"datasource", "queries"}
+ missing_fields: set[str] = required_fields - parsed_data.keys()
+ if missing_fields:
+ fields_str = ", ".join(sorted(missing_fields))
+ raise ValidationError(f"Query context is missing required fields:
{fields_str}")
Review Comment:
**Suggestion:** The new validator only checks that `datasource` and
`queries` keys exist, but it does not validate their types/shape. This allows
invalid payloads such as `{"datasource": null, "queries": null}` or
`{"datasource": {}, "queries": "bad"}` to pass schema validation and later
crash when chart code builds a query context (for example in
`Slice.get_query_context()` / `QueryContextFactory.create()`). Add structural
checks that `datasource` is a dict with required datasource attributes and
`queries` is a list of query objects (and reject null/invalid types).
[incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Chart cache warm-up fails for malformed query_context metadata.
- ❌ Annotation layers using chart query_context crash during execution.
- ⚠️ Dataset column discovery breaks when query_context.queries is invalid.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or update a chart via Chart REST API (`ChartRestApi` in
`superset/charts/api.py:31-59`) using `POST /api/v1/chart/` or `PUT
/api/v1/chart/<id>`,
including a `query_context` payload like `{"datasource": null, "queries":
null}`
(syntactically valid JSON with required keys but null values).
2. The incoming payload is validated by `ChartPostSchema` or
`ChartPutSchema` in
`superset/charts/schemas.py:225-285`, where the `query_context` field at
lines 251-255 and
85-89 uses `validate=utils.validate_query_context_metadata`, which only
checks that
`"datasource"` and `"queries"` keys exist and explicitly allows null values
as shown by
tests `test_validate_query_context_metadata_null_datasource` and
`test_validate_query_context_metadata_null_queries` in
`superset/tests/unit_tests/utils/test_schema.py:179-199`.
3. Because `validate_query_context_metadata` in
`superset/utils/schema.py:50-79` only
enforces key presence (lines 69-77 in the file, corresponding to PR lines
109-118) and
does not validate that `datasource` is a dict or `queries` is an iterable of
dicts, the
malformed `query_context` string is accepted, and `CreateChartCommand` /
`UpdateChartCommand` in `superset/commands/chart/create.py:45-62` and
`superset/commands/chart/update.py:55-75` persist it into
`Slice.query_context`
(SQLAlchemy column defined in `superset/models/slice.py:85-93`).
4. Later, when Superset executes features that rely on the saved
`query_context`, such as
cache warm-up (`_warm_up_non_legacy_cache` in
`superset/commands/chart/warm_up_cache.py:11-27`), annotation query
processing
(`superset/common/query_context_processor.py:14-37`), or column discovery in
`superset/connectors/sqla/models.py:15-36`, they all call
`chart.get_query_context()` in
`superset/models/slice.py:19-28`, which does
`json.loads(self.query_context)` and then
`self.get_query_context_factory().create(**{...})`. Inside
`QueryContextFactory.create` in
`superset/common/query_context_factory.py:45-89`, the comprehension `for
query_obj in
queries` at lines 77-88 attempts to iterate `queries=None` and raises a
`TypeError:
'NoneType' object is not iterable`, causing these operations to fail at
runtime for any
chart whose `query_context` has the malformed structure that current
validation permits.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=858383055a114931b8a64029301ffa90&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=858383055a114931b8a64029301ffa90&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/schema.py
**Line:** 109:118
**Comment:**
*Incomplete Implementation: The new validator only checks that
`datasource` and `queries` keys exist, but it does not validate their
types/shape. This allows invalid payloads such as `{"datasource": null,
"queries": null}` or `{"datasource": {}, "queries": "bad"}` to pass schema
validation and later crash when chart code builds a query context (for example
in `Slice.get_query_context()` / `QueryContextFactory.create()`). Add
structural checks that `datasource` is a dict with required datasource
attributes and `queries` is a list of query objects (and reject null/invalid
types).
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%2F36076&comment_hash=3cbf399cb58f3ef5940b987ce367ff74ae9de60ed74109df6ef4e0286f444952&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F36076&comment_hash=3cbf399cb58f3ef5940b987ce367ff74ae9de60ed74109df6ef4e0286f444952&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: [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]
