mikebridge commented on PR #43634:
URL: https://github.com/apache/superset/pull/43634#issuecomment-5455651464
Ran a seven-lens review at `f15ef99` (python, sqlalchemy, preset, committer,
ddia, clean-code, react). Net: no blocker, and the mechanics are sound — four
lenses compiled the CASE and the production-shaped FAB query and confirmed the
client map reaches SQL only as bound parameters (a `x'; DROP TABLE slices; --`
key compiles to `WHEN %(param_3)s`), the CASE stays out of `query_count()`, the
`ContextVar` is greenlet-local with reset in `finally`, and the outer-join path
is handled. All four of @aminghadersohi's threads confirmed. What follows is
what they add.
**The one hard ask — the new parameter is public API by accident.**
`ChartRestApi` doesn't override `get_list`, so `viz_type_names` survives
`@rison(get_list_schema)` only because FAB's schema has no
`additionalProperties: false`; it then gets hand-validated with a `ValueError`
chain instead of a marshmallow/rison schema, is invisible in `/swagger/v1`, is
silently accepted-and-ignored by every other list endpoint (and by this one
when `order_column != viz_type` — a malformed map on a name sort returns 200),
and the PR ticks "Introduces new feature or API" while nothing lands in
schemas, docstrings or `docs/`. Two clean resolutions: a chart-specific rison
schema (`get_list_schema` + `viz_type_names: {type: object, maxProperties: 256,
additionalProperties: {type: string, maxLength: …}}`) with a `get_list`
override and an OpenAPI docstring in the shape of `dashboards/api.py:396` —
which also replaces the validator; or declare it an internal UI detail, untick
the box, and say
so. Either way the title should agree with the checkbox (`feat` vs `fix`).
**One design change that three lenses reached independently, fixing two
things at once.** The CASE yields mixed-case translated strings and the DB
collates them: SQLite `BINARY` / PG `C` put `Word Cloud` before `deck.gl Arc`
and `Big Number` before `apache_echarts`; MySQL `_ci` / `en_US.UTF-8`
interleave; none matches a JS `localeCompare` — so "sorted by Type" now varies
by metadata DB, and CI passed on all three only because the fixture is
lowercase ASCII. Separately, the whole registry rides in the GET request line:
measured with the stock `MainPreset`, 63 entries → `q=` 2,365 chars, ~40 per
extra plugin; 75 entries → ~4.7 KB — against gunicorn's default 4,094-byte
`--limit-request-line` (Docker sets 0; Helm leaves `SERVER_LIMIT_REQUEST_LINE`
commented out; bare gunicorn and small-header proxies fail at the gateway as
414, never reaching the tidy 400). The server cap of 256×512 chars (~256 KB) is
far beyond what any transport carries. Both go away if the client sorts th
e registry with `localeCompare` (locale-correct, matching what the column
renders) and sends an **ordered slug array**; the server builds `CASE viz_type
WHEN slug THEN <index>` and orders by the integer, unknown slugs to a sentinel
then slug. Collation-independent, roughly half the payload, same bounded
validation. If the string map stays: `func.lower()` on both CASE branches plus
a mixed-case/non-ASCII fixture, send only entries whose name differs from the
slug, and document the request-line requirement.
**Tests.** As Amin noted, `columns: [slice_name, viz_type]` keeps the test
on the inner-only branch; the UI never sends `columns`, falls back to
`list_columns` with `dashboards.*`/`tags.*`/`editors.*`/`viewers.*`, and takes
the outer-join branch where `apply_order_by` runs a second time without
`add_pk`. Compiled and correct, but untested — drop `columns` or add a to-many
column. Also missing: `desc`; the control (no map → slug order); map on a
non-Type sort; empty map; every validator branch (a dropped `isinstance` guard
would 500 from `case()` with no red test — these belong in
`tests/unit_tests/charts/` as pure-function tests). Frontend: nothing asserts
`refreshData()` re-sends the map (it does, via `lastFetchDataConfigRef` —
exactly the invariant a cache refactor would silently break) or that non-Type
sorts omit it.
**Smaller**
- Cap overflow is a hard 400 → toast + empty list, so a plugin-heavy
deployment loses Type sort outright; degrade to slug order server-side (fall
through to `super()` with a warning) or truncate client-side.
- `_MAX_VIZ_TYPE_NAME_LENGTH = 512` is applied to keys too, but
`Slice.viz_type` is `String(250)` — keys over 250 can never match and only
inflate the CASE.
- The full map is also written into `logs.json` on every Type-sorted page
fetch via `log_this_with_context` → `collect_request_payload` (~2–4 KB per
request, no retention) — drop it from the logged payload.
- `extraQueryParams` can still inject `filters`/`select_columns` when the
base config has none (those two are spread conditionally); the "cannot override
pagination" rule lives only in spread order with no test — one `{ page: 99 }`
test pins it.
- The PR body's "retaining the primary-key tie breaker" holds for the inner
row-selection query; FAB's outer ORDER BY has no pk (pre-existing, not a
regression) — worth a precise sentence.
- Five names for one concept (`viz_type_names` / friendly / display name /
client registry / client chart metadata names); pick one.
- Up to ~1,024 bound params in the production shape — fine on PG/MySQL, over
SQLite < 3.32's 999 (deprecated as a metadata DB; informational).
**On the bito threads:** three lenses independently found "`refreshData`
bypasses wrapper" to be a **false positive** — `refreshData()` replays
`lastFetchDataConfigRef.current`, which now includes `extraQueryParams`, and
`ListView` only ever calls the `fetchData` prop, so bulk delete, import,
favourites and page/sort/filter refetches all carry the map. And hoisting the
registry map to module scope (bito's other suggestion) would freeze it before
runtime-registered plugins land; per-call over ~63 entries is sub-millisecond
and correct.
Nice: overriding `apply_order_by` (ahead of `apply_pagination`) rather than
re-sorting the page is the only approach that works across page boundaries; the
test's inverted fixture (`slug_a → zulu`, `slug_z → alpha`, `page_size: 2`)
discriminates both "map ignored" and "sorted after pagination"; the
`ContextVar` over stashing state on the shared `datamodel` singleton avoided a
real cross-request race; rolling-deploy compatibility is correct in both
directions.
_Review produced by seven AI lenses, four of which compiled the SQL against
FAB 5.2.2 / SQLAlchemy 2.0.52 and one of which measured the encoded request
line; posted by @mikebridge after reading. Full report in the spec repo._
--
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]
