fitzee commented on code in PR #43634:
URL: https://github.com/apache/superset/pull/43634#discussion_r3902022342
##########
tests/integration_tests/charts/api_tests.py:
##########
@@ -1502,6 +1502,51 @@ def test_get_charts_filter(self):
data = json.loads(rv.data.decode("utf-8"))
assert data["count"] == 5
+ @pytest.mark.usefixtures("load_energy_table_with_slice")
+ def test_get_charts_orders_friendly_viz_types_before_pagination(self):
+ """Chart API: friendly chart type ordering happens before
pagination."""
+ admin = self.get_user("admin")
+ charts = [
+ self.insert_chart("friendly_type_sort_a", [admin.id], 1,
viz_type="slug_a"),
+ self.insert_chart(
+ "friendly_type_sort_middle", [admin.id], 1, viz_type="middle"
+ ),
+ self.insert_chart("friendly_type_sort_z", [admin.id], 1,
viz_type="slug_z"),
+ ]
+ self.login(ADMIN_USERNAME)
+
+ arguments = {
+ "filters": [
+ {
+ "col": "slice_name",
+ "opr": "sw",
+ "value": "friendly_type_sort_",
+ }
+ ],
+ "columns": ["slice_name", "viz_type"],
Review Comment:
Addressed in `8f9ce164d6`. I removed the scalar-only `columns` override, so
the regression uses the Chart UI's default `list_select_columns` and exercises
FAB's outer to-many query branch. It now verifies ascending and descending
order across both pages, including an unknown slug fallback.
##########
superset/charts/api.py:
##########
@@ -422,6 +491,23 @@ def pre_get_list(self, data: dict[str, Any]) -> None:
if row_id in extra_editors_by_id:
row["extra_editors"] = extra_editors_by_id[row_id]
+ def get_list_headless(self, **kwargs: Any) -> Response:
+ """Use client chart metadata names for SQL ordering before
pagination."""
+ args = kwargs.get("rison", {})
+ if args.get("order_column") != "viz_type" or "viz_type_names" not in
args:
Review Comment:
Addressed in `8f9ce164d6`. The implementation now uses a chart-specific
`chart_get_list_schema`, a decorated `ChartRestApi.get_list` override, and an
OpenAPI docstring for the bounded `viz_type_order` array. An integration test
asserts `/api/v1/_openapi` publishes the parameter and schema, and schema unit
tests cover its validation boundaries.
##########
superset/charts/api.py:
##########
@@ -142,9 +146,74 @@
delete_failed=ChartDeleteFailedError,
)
+_MAX_VIZ_TYPE_NAMES = 256
+_MAX_VIZ_TYPE_NAME_LENGTH = 512
+_viz_type_names: ContextVar[dict[str, str] | None] = ContextVar(
+ "chart_viz_type_names", default=None
+)
+
+
+def _validate_viz_type_names(value: Any) -> dict[str, str]:
+ """Validate the client registry used to order chart visualization types."""
+ if not isinstance(value, dict):
+ raise ValueError("viz_type_names must be an object")
+ if len(value) > _MAX_VIZ_TYPE_NAMES:
+ raise ValueError(
+ f"viz_type_names cannot contain more than {_MAX_VIZ_TYPE_NAMES}
entries"
+ )
+
+ for viz_type, name in value.items():
+ if not isinstance(viz_type, str) or not isinstance(name, str):
+ raise ValueError("viz_type_names keys and values must be strings")
+ if (
+ len(viz_type) > _MAX_VIZ_TYPE_NAME_LENGTH
+ or len(name) > _MAX_VIZ_TYPE_NAME_LENGTH
+ ):
+ raise ValueError(
+ "viz_type_names keys and values cannot exceed "
+ f"{_MAX_VIZ_TYPE_NAME_LENGTH} characters"
+ )
+ return value.copy()
+
+
+class ChartSQLAInterface(SQLAInterface):
+ """Chart model interface with request-scoped friendly viz type ordering."""
+
+ def apply_order_by(
+ self,
+ query: Query,
+ order_column: str,
+ order_direction: str,
+ aliases_mapping: dict[str, AliasedClass] | None = None,
+ bypass_many_to_many: bool = False,
+ add_pk: bool = False,
+ ) -> Query:
+ viz_type_names = _viz_type_names.get()
+ if order_column != "viz_type" or not viz_type_names:
+ return super().apply_order_by(
+ query,
+ order_column,
+ order_direction,
+ aliases_mapping=aliases_mapping,
+ bypass_many_to_many=bypass_many_to_many,
+ add_pk=add_pk,
+ )
+
+ order_expression = case(
Review Comment:
Addressed in `8f9ce164d6`. The client locale-sorts the runtime registry by
rendered display name and sends an ordered slug array. SQL now orders on
integer `CASE` ranks, with unknown slugs assigned a sentinel and then ordered
by slug, so registered display-name ordering no longer depends on metadata DB
string collation.
##########
superset/charts/api.py:
##########
@@ -142,9 +146,74 @@
delete_failed=ChartDeleteFailedError,
)
+_MAX_VIZ_TYPE_NAMES = 256
Review Comment:
Addressed in `8f9ce164d6`. The client truncates `viz_type_order` to the
schema's 256-item maximum, so UI-generated requests cannot hit the cap and fail
with an empty list. Uncaptured custom types degrade to the unknown-slug
fallback. The item limit is also aligned to `Slice.viz_type`'s 250-character
column length, with boundary coverage.
##########
superset-frontend/src/pages/ChartList/index.tsx:
##########
@@ -260,11 +261,33 @@ function ChartList(props: ChartListProps) {
},
setResourceCollection: setCharts,
hasPerm,
- fetchData,
+ fetchData: fetchChartData,
toggleBulkSelect,
refreshData,
} = useListViewResource<Chart>('chart', t('chart'), addDangerToast);
+ const fetchData = useCallback(
+ (config: ListViewFetchDataConfig) =>
+ fetchChartData({
+ ...config,
+ ...(config.sortBy[0]?.id === 'viz_type'
+ ? {
+ extraQueryParams: {
+ viz_type_names: Object.fromEntries(
+ registry
+ .keys()
+ .map(vizType => [
+ vizType,
+ registry.get(vizType)?.name || vizType,
+ ]),
+ ),
+ },
+ }
+ : {}),
+ }),
+ [fetchChartData],
+ );
Review Comment:
Declined as a false positive after tracing the hook. `refreshData()` calls
the hook's internal `fetchData(lastFetchDataConfigRef.current)`, and that
cached config includes `extraQueryParams`; it does not call or bypass the
ChartList wrapper. I added a regression test in `8f9ce164d6` that performs a
Type fetch, calls `refreshData()`, and asserts both requests contain the same
`viz_type_order`.
--
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]
