sancho11 commented on PR #41714:
URL: https://github.com/apache/superset/pull/41714#issuecomment-5189827707
I've been running an adaptation of #41730 (the `deck_multi` migration merged
into this branch) in production, on an instance with `GLOBAL_ASYNC_QUERIES`
enabled and an embedded dashboard. It works, but I hit four issues that I think
will show up for everyone once this lands. All four are in `Multi.tsx`, its
metadata, none require changes to the removal itself.
Sharing them here with the fixes I ended up with, in case they're useful.
Happy to open a PR against this branch for any or all of them.
Confidence, so you can weigh them: 2, 3 and 4 I watched fail in production
and then traced to the code below. 1 I derived from the code and fixed before
it could bite, so I have not actually witnessed it; treat it as a code reading
rather than a report.
### 1. Autozoom silently stops working
`getAdjustedViewport` still reads the container's pre-merged features:
```ts
const features = props.payload?.data?.features || {};
```
But the new `Multi/buildQuery.ts` returns `buildQueryContext(formData, () =>
[])`, so the container issues no query and that field is never populated.
`points` ends up empty, `fitViewport` is never called, and the chart opens at
the saved viewport instead of fitting the data. The `// there may be none here`
comment in the diff acknowledges the empty case, but nothing replaces the
points.
Fix: collect the points from the features each layer's own `transformProps`
returned, aggregated by `viz_type` after the layers resolve. This is arguably
better than the old behavior, it fits the data actually being rendered rather
than a separately-computed merge.
### 2. The container renders the "No results" empty state
With `queries: []` the endpoint answers `result: []`, and in `SuperChart`:
```ts
const noResultQueries =
enableNoResults &&
(!queriesData ||
queriesData
.slice(0, getQueryCount())
.every(({ data }) => !data || (Array.isArray(data) && data.length ===
0)));
```
`[].every(...)` is vacuously `true`, so the chart renders "No results were
returned for this query" and the map never appears at all.
`ChartMetadata` already carries `enableNoResults` for exactly this
situation, but only the native-filter path reads it; `ChartRenderer` derives
the flag solely from server pagination:
```ts
const bypassNoResult = !(
currentFormDataExtended?.server_pagination &&
(hasSearchText || hasAgGridFilters)
);
```
Fix: honor `getChartMetadataRegistry().get(vizType)?.enableNoResults` there
as well, and set `enableNoResults: false` on the Multiple Layers metadata. Also
worth having `transformProps` not hand the container an `undefined` payload
when `queriesData` is empty.
### 3. Layer lookup 404s for any principal whose chart access is scoped
`fetchSubslices` reads each layer with `GET /api/v1/chart/<id>`. Layer
charts are saved charts that normally sit on no dashboard of their own, so
`ChartFilter` denies them:
```python
if (guest_dashboards := guest_embedded_dashboard_filter()) is not None:
return query.filter(self.model.dashboards.any(guest_dashboards))
if security_manager.can_access_all_datasources():
return query
return self._apply_viewers(query)
```
An embedded guest gets denied by the first branch (the layers are not on the
token's dashboards) and a role without an explicit grant on the layer gets
denied by `_apply_viewers`; in both cases even though the principal is entitled
to the container. In our embedded dashboard every layer 404s, consistently.
Confirming it was the filter and not a stale id or a missing session, for
one of our layers:
- `GET /api/v1/chart/369` as an admin returns `200`, so the chart exists and
is readable.
- Its payload has `"dashboards": []`; it is a layer of a `deck_multi` chart,
so it sits on no dashboard of its own. `Slice.dashboards.any(<token
dashboards>)` can therefore never match it.
- The same request with no session returns `{"msg": "Missing Authorization
Header"}`, not a `404`. So the `404` we saw came from a request that *was*
authenticated and scoped, which is the guest branch above.
Note this failure is deterministic rather than intermittent, so for embedded
deployments it is the one that fully breaks the chart.
The legacy pipeline never hit this because it resolved the layers
server-side, under the container's access:
```python
slices = db.session.query(Slice).filter(Slice.id.in_(slice_ids)).all()
```
Fix: a `GET /chart/<pk>/deck_layers/` endpoint that gates on the container
(the normal base filter applies to *that* lookup) and then resolves the
declared layers with `skip_base_filter=True`, exposing only their `slice_id`,
`viz_type` and `params`. That reproduces the legacy rule, access to the
container governs access to its layers, without widening the general chart
filter. `Multi.tsx` uses it when the container is saved and falls back to the
per-chart reads otherwise, so an unsaved chart in Explore still works.
Worth noting this also collapses N requests into one.
### 4. Sub-layer requests still take the async handoff
This is the one that cost me the most time, so it may be the most valuable.
`Multi.tsx` posts with `result_type: 'full'`, and in `charts/data/api.py`:
```python
use_async = (
is_feature_enabled("GLOBAL_ASYNC_QUERIES")
and query_context.result_format == ChartDataResultFormat.JSON
and query_context.result_type == ChartDataResultType.FULL
and cache_timeout != CACHE_DISABLED_TIMEOUT
)
if use_async:
return self._run_async(json_body, command, add_extra_log_payload)
```
So with `GLOBAL_ASYNC_QUERIES` enabled and a cold cache, each sub-layer
request answers `202` with job metadata and no data. `Multi.tsx` posts directly
with `SupersetClient.post` rather than through the chart pipeline, so it never
registers a listener and cannot follow the job. The layer simply never renders,
and the async middleware logs one of these per layer:
```
listener not found for job_id 9301681d-69e7-40c6-837a-83f1dcc9e7ce
```
It presents as intermittent and "fixed by reloading", because by the time
you reload the job has completed and warmed the cache. That made it quite hard
to chase.
Note this is the *same* failure the legacy path had on `/explore_json/` ->
moving to `/api/v1/chart/data` carries it along rather than resolving it.
Fix: request `result_type: 'results'` instead of `'full'`. Both share the
same preparer (`None` in `_data_result_type_preparers`), so it is the same
query against the same cache; only the payload envelope is trimmed, and it
still returns `data` / `colnames` / `coltypes`, which is all the layer
transforms read (`getRecordsFromQuery` uses `queriesData[0].data`).
---
If it helps in prioritizing: #2 and #3 break the chart completely and
deterministically; #2 for everyone, #3 for embedded deployments; while #4
presents as intermittent and self-healing, which made it by far the most
expensive to diagnose.
--
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]
