bito-code-review[bot] commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4174207599
##########
superset/charts/data/form_data.py:
##########
@@ -31,33 +31,69 @@ def set_form_data(form_data: dict[str, Any]) -> None:
g.form_data = form_data
+def _as_form_data_dict(value: Any) -> dict[str, Any]:
+ return value if isinstance(value, dict) else {}
+
+
+def _as_query_list(value: Any) -> list[Any]:
+ if isinstance(value, (list, tuple)):
+ return list(value)
+ return []
+
+
def _serialize_query(
query: QueryObject,
form_data: dict[str, Any],
-) -> dict[str, Any]:
- """Serialize query fields consumed by the Jinja form-data fallback."""
- query_data = dict(query.to_dict())
- query_data["filters"] = query.filter
- if query.time_range is not None:
- query_data["time_range"] = query.time_range
+ time_range: str | None = None,
+) -> dict[str, Any] | None:
+ """Serialize query fields consumed by the Jinja form-data fallback.
+
+ Incomplete stubs (unit-test doubles without ``to_dict``) are skipped so
+ callers can still publish datasource context for Jinja without requiring a
+ full ``QueryObject``.
+
+ ``time_range`` is an optional overlay for callers that deliberately leave
+ ``QueryObject.time_range`` unset (tabular queries, so relative ranges keep
+ ``from_dttm``/``to_dttm`` in the cache key). Chart and async callers omit
+ it so ``get_time_filter()`` matches the chart-data API: a TEMPORAL_RANGE
+ filter alone is not a published time range.
+ """
+ to_dict = getattr(query, "to_dict", None)
+ if not callable(to_dict):
+ return None
+
+ query_data = dict(to_dict())
+ filters = getattr(query, "filter", None)
+ query_data["filters"] = filters
+ resolved = time_range
+ if resolved is None:
+ obj_range = getattr(query, "time_range", None)
+ if isinstance(obj_range, str):
+ resolved = obj_range
+ if resolved is not None:
+ query_data["time_range"] = resolved
if url_params := form_data.get("url_params"):
query_data["url_params"] = url_params
return query_data
def set_query_context_form_data(
query_context: QueryContext,
- datasource_id: int,
+ datasource_id: int | str,
datasource_type: str,
+ time_range: str | None = None,
) -> None:
"""Expose a programmatically-created query like a chart data API
request."""
- form_data = query_context.form_data or {}
+ form_data = _as_form_data_dict(getattr(query_context, "form_data", None))
+ queries = _as_query_list(getattr(query_context, "queries", None))
Review Comment:
<!-- Bito Reply -->
The changes you have pushed correctly address the issue of relying on
`getattr` with defaults, which previously caused soft-failures for incomplete
stubs. By introducing `_as_form_data_dict` and `_as_query_list` to read
`form_data` and `queries` directly from `QueryContext`, you have improved the
robustness of the Jinja context population. This approach ensures that the
helper functions explicitly handle the expected attributes, avoiding the silent
empty-context behavior that was previously flagged.
**superset/charts/data/form_data.py**
```
def _as_form_data_dict(value: Any) -> dict[str, Any]:
return value if isinstance(value, dict) else {}
def _as_query_list(value: Any) -> list[Any]:
if isinstance(value, (list, tuple)):
return list(value)
return []
```
--
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]
