codeant-ai-for-open-source[bot] commented on code in PR #41909:
URL: https://github.com/apache/superset/pull/41909#discussion_r3626658819
##########
superset/charts/schemas.py:
##########
@@ -1380,6 +1380,22 @@ class Meta: # pylint: disable=too-few-public-methods
allow_none=True,
)
+ @post_load
+ def rename_deprecated_fields(
+ self, data: dict[str, Any], **kwargs: Any
+ ) -> dict[str, Any]:
+ _renames = (
+ ("groupby", "columns"),
+ ("granularity_sqla", "granularity"),
+ ("timeseries_limit", "series_limit"),
+ ("timeseries_limit_metric", "series_limit_metric"),
+ )
+ for old, new in _renames:
+ value = data.pop(old, None)
+ if value or value == 0:
+ data[new] = value
Review Comment:
**Suggestion:** The deprecated-to-canonical rename currently overwrites an
already provided canonical field, so a payload that includes both keys will
silently prefer the deprecated value. This conflicts with the factory behavior
(where canonical keys take precedence) and can produce different query
semantics depending on whether data passed through schema deserialization.
Preserve existing canonical values when both keys are present. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Chart data API misinterprets columns when both keys present.
- ⚠️ Behavior diverges between schema and factory normalization paths.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note that `ChartDataQueryObjectSchema` defines a `@post_load` hook
`rename_deprecated_fields` in `superset/charts/schemas.py` at lines
1383–1397, and that
this schema is used by `ChartDataQueryContextSchema`
(`superset/charts/schemas.py:1400-1404`) via `queries =
fields.List(fields.Nested(ChartDataQueryObjectSchema))`.
2. Start the Superset backend so the Flask view that handles chart data
requests uses
`ChartDataQueryContextSchema` (the repo code graph explicitly lists
`schemas.py` being
called by Flask).
3. Send a POST request to that Flask endpoint with a query payload where a
single query
object includes both the canonical `"columns"` key and the deprecated
`"groupby"` key,
e.g. `{"queries": [{"columns": ["col_a"], "groupby": ["col_b"], ...}]}` so
both keys are
simultaneously present.
4. During deserialization, Marshmallow invokes `rename_deprecated_fields`,
executing the
loop at `superset/charts/schemas.py:1393-1396`:
`for old, new in _renames: value = data.pop(old, None); if value or value
== 0:
data[new] = value`,
which pops `"groupby"` then unconditionally assigns `data["columns"] =
value`,
overwriting the already-present canonical `"columns"` value; the
resulting query
context (later consumed by `QueryContextFactory` in
`superset/common/query_context_factory.py:40`) now uses the deprecated
`"groupby"`
value instead of the canonical `"columns"` value, demonstrating that the
deprecated key
incorrectly takes precedence.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0de98e7f9630444b82561910b347e6d2&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=0de98e7f9630444b82561910b347e6d2&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/charts/schemas.py
**Line:** 1393:1396
**Comment:**
*Logic Error: The deprecated-to-canonical rename currently overwrites
an already provided canonical field, so a payload that includes both keys will
silently prefer the deprecated value. This conflicts with the factory behavior
(where canonical keys take precedence) and can produce different query
semantics depending on whether data passed through schema deserialization.
Preserve existing canonical values when both keys are present.
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%2F41909&comment_hash=2c78b26def729639fd86d1fc76f772e241916fce884288cedbcf33d021c9e495&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41909&comment_hash=2c78b26def729639fd86d1fc76f772e241916fce884288cedbcf33d021c9e495&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]
