aminghadersohi commented on code in PR #43634:
URL: https://github.com/apache/superset/pull/43634#discussion_r3877781296
##########
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:
`viz_type_names` is a new public query parameter but it is not in the
OpenAPI spec. FAB's `get_list_schema` does not set `additionalProperties:
false`, so this passes rison validation silently and never shows up in
`/swagger/v1` — and every *other* list endpoint will also accept and silently
ignore it, which is a confusing contract for API consumers.
Worth documenting it on the `get_list` docstring override (or via
`openapi_spec_component_schemas`) so the parameter is discoverable and its 400
conditions are specified.
##########
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:
This pins `columns` to two scalar columns, which keeps FAB on the
inner-query-only branch of `apply_all`. The Chart list page does **not** send
`columns`, so `_handle_columns_args` falls back to `list_select_columns`, which
contains to-many columns (`dashboards.id`, `tags.name`, `editors.id`,
`viewers.id`). That makes `exists_col_to_many()` true and routes production
through the outer-query branch, where `apply_order_by` is called a *second*
time on the outer query (this time without `add_pk`). The path the UI actually
exercises is therefore untested.
Dropping the `"columns"` key here would exercise the outer branch and still
assert the same page sequence.
##########
superset/charts/api.py:
##########
@@ -142,9 +146,74 @@
delete_failed=ChartDeleteFailedError,
)
+_MAX_VIZ_TYPE_NAMES = 256
Review Comment:
nit: exceeding this cap returns a 400, which the client surfaces as an error
toast and an empty list — a deployment with many custom viz plugins would lose
Type sorting outright rather than degrading to slug order. Truncating (or
ignoring the map and falling back to `super()`) fails softer for input the
client generates automatically.
##########
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:
Ordering the `CASE` result delegates string comparison to the metadata DB's
collation, so the visible order varies by backend for the same registry map.
Concretely, an unregistered slug (lowercase, e.g. `apache_echarts`) vs a
registered display name (`Big Number`): SQLite `BINARY` sorts `Big Number`
first, Postgres `en_US.UTF-8` sorts `apache_echarts` first, MySQL
`utf8mb4_general_ci` matches Postgres. Accented names diverge similarly.
Not a regression (the Type filter dropdown at `ChartList/index.tsx` already
uses a naive codepoint `>` comparator), but it means the column order won't
always match what a locale-aware client would produce. If you want determinism,
normalizing to a sort key on the client (e.g. lowercased,
`localeCompare`-derived rank as the CASE value) would remove the DB dependency
entirely.
--
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]
