codeant-ai-for-open-source[bot] commented on code in PR #43206:
URL: https://github.com/apache/superset/pull/43206#discussion_r3788660542
##########
superset/charts/schemas.py:
##########
@@ -677,15 +677,28 @@ class
ChartDataSortOptionsSchema(ChartDataPostProcessingOperationOptionsSchema):
Sort operation config.
"""
- columns = fields.Dict(
+ is_sort_index = fields.Boolean(
metadata={
- "description": "columns by by which to sort. The key specifies the
column "
- "name, value specifies if sorting in ascending order.",
- "example": {"country": True, "gender": False},
+ "description": "Whether to sort by the index rather than by column
values.",
+ "example": True,
+ },
+ )
+ by = fields.Raw(
+ # TODO: add correct union type once supported by Marshmallow
+ metadata={
+ "description": "Name, or list of names, of the columns to sort by.
"
+ "Ignored when `is_sort_index` is set.",
+ "example": "country",
+ },
+ )
+ ascending = fields.Raw(
+ # TODO: add correct union type once supported by Marshmallow
+ metadata={
+ "description": "Sort ascending (the default) or descending. A list
of "
+ "booleans may be given to set the direction per entry in `by`.",
+ "example": True,
},
Review Comment:
**Suggestion:** The `Raw` fields accept arbitrary JSON values, including
dictionaries, integers, and nested objects, although `sort()` only accepts a
string or list of strings for `by` and a boolean or list of booleans for
`ascending`. Because post-processing options are forwarded directly to pandas,
malformed values can bypass the documented API boundary and raise an unhandled
runtime exception instead of producing a validation error. Define typed union
fields or add equivalent validation before dispatch. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Malformed sort requests can reach pandas unchecked.
- ⚠️ Chart-data requests may return runtime errors instead of validation
errors.
- ⚠️ Operation-specific schemas currently document rather than enforce types.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=aa6ab8be43c14b15bd9a090ecc83691d&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=aa6ab8be43c14b15bd9a090ecc83691d&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/charts/schemas.py
**Line:** 686:700
**Comment:**
*Type Error: The `Raw` fields accept arbitrary JSON values, including
dictionaries, integers, and nested objects, although `sort()` only accepts a
string or list of strings for `by` and a boolean or list of booleans for
`ascending`. Because post-processing options are forwarded directly to pandas,
malformed values can bypass the documented API boundary and raise an unhandled
runtime exception instead of producing a validation error. Define typed union
fields or add equivalent validation before dispatch.
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%2F43206&comment_hash=0e91afeaaadeefca87ca5283811bc8cfc544268bb9123ca550ffb35e677db927&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43206&comment_hash=0e91afeaaadeefca87ca5283811bc8cfc544268bb9123ca550ffb35e677db927&reaction=dislike'>👎</a>
##########
tests/unit_tests/charts/test_schemas.py:
##########
@@ -478,3 +478,57 @@ def
test_chart_data_extras_rejects_system_sampling(app_context: None) -> None:
with pytest.raises(ValidationError) as exc_info:
ChartDataExtrasSchema().load({"system_sampling": True})
assert "system_sampling" in exc_info.value.messages
+
+
+def test_post_processing_option_schemas_match_their_functions(
+ app_context: None,
+) -> None:
+ """Every documented post-processing option must be a real parameter.
+
+ `QueryObject.exec_post_processing` dispatches with
+ `getattr(pandas_postprocessing, operation)(df, **options)`, and the
+ per-operation `options` dict is passed through unvalidated. So a field
+ that appears in one of these schemas but not in the corresponding
+ function signature is published in the OpenAPI spec as a valid option
+ while raising `TypeError: <op>() got an unexpected keyword argument` --
+ an HTTP 500 -- for any client that sends it.
+
+ `ChartDataSortOptionsSchema` documented a required `columns` dict and an
+ `aggregates` field, neither of which `sort()` accepts, and
+ `ChartDataProphetOptionsSchema` documented `monthly_seasonality` where
+ `prophet()` takes `daily_seasonality`.
+ """
+ import inspect
+
+ from marshmallow import Schema
+
+ from superset.charts import schemas as chart_schemas
+ from superset.utils import pandas_postprocessing
+
+ mismatches = {}
+ for name, schema_cls in vars(chart_schemas).items():
+ if not (
+ inspect.isclass(schema_cls)
+ and issubclass(schema_cls, Schema)
+ and name.startswith("ChartData")
+ and name.endswith("OptionsSchema")
+ ):
+ continue
+ operation = name[len("ChartData") : -len("OptionsSchema")].lower()
+ function = getattr(pandas_postprocessing, operation, None)
+ if function is None:
+ continue
Review Comment:
**Suggestion:** The test derives operation names by lowercasing the class
name, so `ChartDataGeohashDecodeOptionsSchema` becomes `geohashdecode` while
the exported operation is `geohash_decode` (and similarly for other underscored
operations). Those schemas are silently skipped when `getattr` returns `None`,
so the test does not actually cover every post-processing schema. It also
checks only schema fields absent from the function signature, allowing required
function parameters missing from a schema to regress unnoticed. Derive the
operation name from the actual schema registry or an explicit mapping, and
assert both directions of the parameter set. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Geohash schema/function mismatches are not tested.
- ⚠️ Missing documented operation parameters can regress silently.
- ⚠️ The test's “every” claim is false for underscored operations.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=11bd486e330943e289198d42b76627c8&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=11bd486e330943e289198d42b76627c8&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:** tests/unit_tests/charts/test_schemas.py
**Line:** 517:520
**Comment:**
*Possible Bug: The test derives operation names by lowercasing the
class name, so `ChartDataGeohashDecodeOptionsSchema` becomes `geohashdecode`
while the exported operation is `geohash_decode` (and similarly for other
underscored operations). Those schemas are silently skipped when `getattr`
returns `None`, so the test does not actually cover every post-processing
schema. It also checks only schema fields absent from the function signature,
allowing required function parameters missing from a schema to regress
unnoticed. Derive the operation name from the actual schema registry or an
explicit mapping, and assert both directions of the parameter set.
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%2F43206&comment_hash=fdfeca44011a3ea231c42c215e354c874a678d825d0ffa33eacc2ca0b3ad4963&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43206&comment_hash=fdfeca44011a3ea231c42c215e354c874a678d825d0ffa33eacc2ca0b3ad4963&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]
