eschutho opened a new pull request, #41263:
URL: https://github.com/apache/superset/pull/41263
### What's the change?
`ChartDataQueryObjectSchema.rename_deprecated_fields()` is a Marshmallow
`@post_load` hook that renames legacy query field names to their modern
equivalents:
| Old (deprecated) | New |
|---|---|
| `timeseries_limit` | `series_limit` |
| `timeseries_limit_metric` | `series_limit_metric` |
| `granularity_sqla` | `granularity` |
| `groupby` | `columns` |
The original implementation used the walrus operator with a truthiness check:
```python
if value := data.pop(old, None):
data[new] = value
```
**Bug:** `data.pop(old, None)` always removes the old key, but when the
value is falsy, the new key is never set. Real-world cases silently affected:
- `timeseries_limit=0` — the default for most charts meaning "no series
limit"
- `groupby=[]` — explicit empty groupby (`fields.List(...)`, empty list is
falsy)
- `timeseries_limit_metric=None` or `granularity_sqla=None` (all four
deprecated fields carry `allow_none=True`)
Result: the renamed field (`series_limit`) is silently absent from the
`data` dict passed to `QueryObject`. `QueryObject.__init__` falls back to its
`series_limit: int = 0` default — numerically the same but semantically wrong;
the caller's explicit `0` or `None` is discarded.
**Fix:**
```python
for old, new in _renames:
if old in data:
data.setdefault(new, data.pop(old))
```
- `old in data` — checks key presence, not value truthiness
- `data.pop(old)` — always removes the deprecated key
- `data.setdefault(new, ...)` — sets the new key only if it isn't already
present (an explicit `series_limit` from the caller is never silently clobbered
by a coexisting deprecated `timeseries_limit`)
### No behavior change for truthy values
For any truthy value the behavior is identical to the old code.
### Test plan
- Unit tests for `ChartDataQueryObjectSchema`: verify that
`timeseries_limit=0` is renamed to `series_limit=0` after deserialization (was
silently discarded before).
- Integration test: run the existing `query_context_tests.py` tests that set
deprecated fields to ensure they still pass.
### Follow-up (non-blocking)
`QueryObject._rename_deprecated_fields` (`query_object.py:232`) has the same
`if value:` guard for the `setattr` path. The deprecation *warning* still fires
there regardless (the log line precedes the `if`), and the functional outcome
is identical since `series_limit` defaults to `0`. A follow-up fix there would
make both rename paths consistent.
--
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]
