You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "ysinghc (via GitHub)" <gi...@apache.org> on 2025/10/22 14:31:37 UTC
[PR] fix(charts): add query_context validation to prevent incomplete metadata [superset]
ysinghc opened a new pull request, #35798:
URL: https://github.com/apache/superset/pull/35798
fix(charts): validate query_context metadata to prevent incomplete chart data
### SUMMARY
This PR addresses a bug where charts could be saved with incomplete
`query_context` metadata (missing `datasource` and `queries` fields),
causing 422 errors when attempting to load chart data.
**Problem:**
- Charts were being saved with `query_context` missing required fields
- This caused runtime errors: `"QueryContextFactory.create() missing 2 required keyword-only arguments: 'datasource' and 'queries'"`
- Errors only surfaced when loading chart data, not at save time
- Led to broken dashboards and poor user experience
**Solution:**
- Added `validate_query_context_metadata()` validator to ensure structural integrity
- Validates that when `query_context` is provided (not `None`), it must contain:
- `datasource` field with data source information
- `queries` field with query objects array
- Applied validator to both `ChartPostSchema` and `ChartPutSchema`
- Maintains full backwards compatibility: `None` values are still allowed
**Changes:**
1. `superset/utils/schema.py`: New `validate_query_context_metadata()` function
2. `superset/charts/schemas.py`: Updated validation in `ChartPostSchema` and `ChartPutSchema`
3. `tests/unit_tests/charts/test_schemas.py`: Added comprehensive test coverage
**Validation Behavior:**
- Valid `query_context` with `datasource` and `queries`: Pass
- `None`/`null` query_context: Pass (backwards compatible)
- Empty object `{}`: Rejected
- Missing `datasource` field: Rejected
- Missing `queries` field: Rejected
- Invalid JSON: Rejected
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
test_chart_post_schema_query_context_validation() - Validates POST endpoint
test_chart_put_schema_query_context_validation() - Validates PUT endpoint
Tests cover: valid data, None values, missing fields, empty objects, invalid JSON
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: #35774
- [ ] 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(charts): add query_context validation to prevent incomplete metadata [superset]
Posted by "ysinghc (via GitHub)" <gi...@apache.org>.
ysinghc closed pull request #35798: fix(charts): add query_context validation to prevent incomplete metadata
URL: https://github.com/apache/superset/pull/35798
--
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): add query_context validation to prevent incomplete metadata [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35798:
URL: https://github.com/apache/superset/pull/35798#issuecomment-3432731641
<details open>
<summary><strong>Interaction Diagram by <a href="https://bito.ai#sequence_diagram">Bito</a></strong></summary>
```mermaid
sequenceDiagram
participant API as ChartRestApi<br/>π Updated | βββ High
participant POST as ChartPostSchema<br/>π Updated | βββ High
participant PUT as ChartPutSchema<br/>π Updated | βββ High
participant UTIL as validate_query_context_metadata<br/>π© Added | βββ High
participant JSON as JSON Parser
participant CMD as CreateChartCommand
participant DB as Database
Note over POST, PUT: Enhanced validation for<br/>query_context metadata
API->>POST: POST /api/v1/chart (create)
POST->>UTIL: Validate query_context field
UTIL->>JSON: Parse and validate JSON
JSON-->>UTIL: Return parsed data
UTIL-->>POST: Validation result
POST->>CMD: Execute chart creation
CMD->>DB: Store validated chart
API->>PUT: PUT /api/v1/chart/{id} (update)
PUT->>UTIL: Validate query_context field
UTIL-->>PUT: Validation result
```
Critical path: ChartRestApi->ChartPostSchema->validate_query_context_metadata->JSON Parser->CreateChartCommand->Database
> **Note:** The chart creation and update flows now enforce stricter validation on query_context fields, requiring 'datasource' and 'queries' metadata. This ensures chart data retrieval has necessary context information before persistence.
</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): add query_context validation to prevent incomplete metadata [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35798:
URL: https://github.com/apache/superset/pull/35798#discussion_r2452320833
##########
superset/utils/schema.py:
##########
@@ -51,3 +51,41 @@ def validate_json(value: Union[bytes, bytearray, str]) -> None:
json.validate_json(value)
except json.JSONDecodeError as ex:
raise ValidationError("JSON not valid") from ex
+
+
+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:
+ return # Allow None values
+
+ try:
+ json.validate_json(value)
+ parsed_data = json.loads(value)
Review Comment:
### Redundant JSON validation and parsing <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code calls json.validate_json() followed by json.loads() on the same value, which is redundant and inefficient since json.loads() will also validate the JSON.
###### Why this matters
This creates unnecessary overhead by parsing and validating the JSON twice, and json.validate_json() may not be a standard function that exists in the json module.
###### Suggested change β *Feature Preview*
Remove the redundant json.validate_json() call and handle JSON parsing directly:
```python
try:
parsed_data = json.loads(value)
except json.JSONDecodeError as ex:
raise ValidationError("JSON not valid") from ex
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/07906f0f-4dea-462c-82df-48768a89b029/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/07906f0f-4dea-462c-82df-48768a89b029?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/07906f0f-4dea-462c-82df-48768a89b029?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/07906f0f-4dea-462c-82df-48768a89b029?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/07906f0f-4dea-462c-82df-48768a89b029)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b129c44e-85d9-40df-8198-b9b822651c38 -->
[](b129c44e-85d9-40df-8198-b9b822651c38)
--
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): add query_context validation to prevent incomplete metadata [superset]
Posted by "ysinghc (via GitHub)" <gi...@apache.org>.
ysinghc commented on PR #35798:
URL: https://github.com/apache/superset/pull/35798#issuecomment-3435435697
/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(charts): add query_context validation to prevent incomplete metadata [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #35798:
URL: https://github.com/apache/superset/pull/35798#issuecomment-3432754334
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/35798?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 `6.25000%` with `15 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 50.71%. Comparing base ([`76d897e`](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`afb5928`](https://app.codecov.io/gh/apache/superset/commit/afb5928bf9b23ab52f452d0f3aeb765ddafc8125?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2780 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/35798?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/utils/schema.py](https://app.codecov.io/gh/apache/superset/pull/35798?src=pr&el=tree&filepath=superset%2Futils%2Fschema.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvc2NoZW1hLnB5) | 6.25% | [15 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/35798?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #35798 +/- ##
==========================================
- Coverage 60.48% 50.71% -9.78%
==========================================
Files 1931 589 -1342
Lines 76236 43660 -32576
Branches 8568 4731 -3837
==========================================
- Hits 46114 22143 -23971
+ Misses 28017 20893 -7124
+ Partials 2105 624 -1481
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/35798/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/35798/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.26% <6.25%> (-2.90%)` | :arrow_down: |
| [javascript](https://app.codecov.io/gh/apache/superset/pull/35798/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/35798/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `49.96% <6.25%> (-3.84%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/35798/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `50.71% <6.25%> (-12.80%)` | :arrow_down: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/35798/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ΓΈ> (+42.36%)` | :arrow_up: |
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 Sentry](https://app.codecov.io/gh/apache/superset/pull/35798?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): add query_context validation to prevent incomplete metadata [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35798:
URL: https://github.com/apache/superset/pull/35798#issuecomment-3435436026
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): add query_context validation to prevent incomplete metadata [superset]
Posted by "ysinghc (via GitHub)" <gi...@apache.org>.
ysinghc commented on PR #35798:
URL: https://github.com/apache/superset/pull/35798#issuecomment-3433032642
I am adjusting the tests to increase coverage and also ensure coverage for the new commit i made
--
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): add query_context validation to prevent incomplete metadata [superset]
Posted by "ysinghc (via GitHub)" <gi...@apache.org>.
ysinghc commented on PR #35798:
URL: https://github.com/apache/superset/pull/35798#issuecomment-3436567673
/resolve
--
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): add query_context validation to prevent incomplete metadata [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35798:
URL: https://github.com/apache/superset/pull/35798#issuecomment-3432675853
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): add query_context validation to prevent incomplete metadata [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #35798:
URL: https://github.com/apache/superset/pull/35798#discussion_r2452353100
##########
superset/charts/schemas.py:
##########
@@ -213,7 +213,7 @@ class ChartPostSchema(Schema):
query_context = fields.String(
metadata={"description": query_context_description},
allow_none=True,
- validate=utils.validate_json,
+ validate=utils.validate_query_context_metadata,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Breaking validation change for query_context</b></div>
<div id="fix">
Changing the validation for `query_context` from `validate_json` to `validate_query_context_metadata` introduces stricter requirements that mandate 'datasource' and 'queries' fields in the JSON. This could break existing chart creation operations where `query_context` is provided as valid JSON but lacks these specific fields, potentially causing validation failures in production. The `ChartCreateSchema` and `ChartUpdateSchema` classes are impacted, affecting both new chart creation and updates through functions like chart API endpoints that deserialize input using these schemas.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- validate=utils.validate_query_context_metadata,
+ validate=utils.validate_json,
)
@@ -276,4 +276,4 @@
query_context = fields.String(
metadata={"description": query_context_description},
allow_none=True,
- validate=utils.validate_query_context_metadata,
+ validate=utils.validate_json,
)
```
</div>
</details>
</div>
<small><i>Code Review Run <a href=https://github.com/apache/superset/pull/35798#issuecomment-3432675853>#804563</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org