sadpandajoe commented on code in PR #41714:
URL: https://github.com/apache/superset/pull/41714#discussion_r3725904906
##########
superset/charts/api.py:
##########
@@ -404,6 +410,104 @@ def get(self, id_or_uuid: str) -> Response:
except ChartNotFoundError:
return self.response_404()
+ @expose("/<pk>/deck_layers/", methods=("GET",))
+ @protect()
+ @safe
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
(f"{self.__class__.__name__}.deck_layers"),
+ log_to_statsd=False,
+ )
+ def deck_layers(self, pk: int) -> Response:
+ """Gets the sub-layer charts declared by a deck.gl Multiple Layers
chart
+ ---
+ get:
+ summary: >-
+ Get the sub-layer charts declared by a deck.gl Multiple Layers
chart
+ description: >-
+ Multiple Layers charts (viz_type "deck_multi") reference other
+ saved charts as layers via their `deck_slices` config, but those
+ layer charts typically sit on no dashboard of their own, so a
+ per-layer `GET /api/v1/chart/<id>` can 404 for a principal
+ (e.g. an embedded guest) who is only entitled to the container.
+ This endpoint gates on the container chart and resolves the
+ layers it declares, mirroring the access the legacy explore_json
+ pipeline granted server-side.
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ description: The id of the Multiple Layers container chart
+ responses:
+ 200:
+ description: The container's declared layer charts
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ type: array
+ items:
+ type: object
+ properties:
+ slice_id:
+ type: integer
+ viz_type:
+ type: string
+ params:
+ type: string
+ datasource_id:
+ type: integer
+ datasource_type:
+ type: string
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ try:
+ container = ChartDAO.get_by_id_or_uuid(str(pk))
+ except ChartNotFoundError:
+ return self.response_404()
+
+ try:
+ container_params = json.loads(container.params or "{}")
+ except (TypeError, ValueError):
+ container_params = {}
+
+ deck_slice_ids = [
+ slice_id
+ for slice_id in container_params.get("deck_slices", [])
+ if isinstance(slice_id, int)
+ ]
+ if not deck_slice_ids:
+ return self.response(200, result=[])
+
+ # The container's own access has already been checked above; the
+ # layers it declares are resolved without the base filter (they
+ # sit on no dashboard of their own), same as the legacy explore_json
+ # pipeline resolved them server-side under the container's access.
+ layers = ChartDAO.find_by_ids(deck_slice_ids, skip_base_filter=True)
Review Comment:
An editor can save an accessible chart whose `deck_slices` names a chart
they cannot read directly, and this unfiltered lookup then returns that chart's
params and datasource metadata. Since the endpoint does not verify each child
(or even require the parent to be `deck_multi`), should ordinary users retain
the chart base filter and reserve the container-based bypass for the intended
embedded-guest case?
##########
superset/migrations/shared/migrate_viz/base.py:
##########
@@ -164,11 +172,22 @@ def upgrade_slice(cls, slc: Slice) -> None:
queries_bak = None
if query_context:
+ # A stored query_context is expected to carry "queries", but
+ # an atypical/malformed one (e.g. hand-edited via the API)
+ # missing it must not raise here: viz_type was already
+ # flipped above, so an uncaught exception at this point
+ # would leave the slice half-migrated (new viz_type, but
+ # stale params/query_context in the old shape). Back up the
+ # whole context in that case so downgrade can restore it
+ # verbatim instead of losing it (see FULL_CONTEXT_BAK_KEY).
+ if "queries" in query_context:
Review Comment:
A stored context with `"queries": null` is backed up as `None`, which
downgrade interprets as no original context and replaces with SQL `NULL`,
losing the original datasource and form data. Could the backup distinguish an
explicit JSON null from an absent context and add a round-trip case for it?
##########
superset/migrations/shared/migrate_viz/base.py:
##########
@@ -164,11 +172,22 @@ def upgrade_slice(cls, slc: Slice) -> None:
queries_bak = None
if query_context:
+ # A stored query_context is expected to carry "queries", but
+ # an atypical/malformed one (e.g. hand-edited via the API)
+ # missing it must not raise here: viz_type was already
+ # flipped above, so an uncaught exception at this point
+ # would leave the slice half-migrated (new viz_type, but
+ # stale params/query_context in the old shape). Back up the
+ # whole context in that case so downgrade can restore it
+ # verbatim instead of losing it (see FULL_CONTEXT_BAK_KEY).
+ if "queries" in query_context:
Review Comment:
A parseable non-object query context (for example `1` or a JSON list, both
accepted by the current schema validator) throws only after `viz_type` has
changed; the broad catch suppresses that error and the pagination loop commits
the partially mutated row. Could the parsed type be validated before mutating
the slice, with a regression covering these persisted values?
##########
superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.tsx:
##########
@@ -409,8 +537,89 @@ const DeckMulti = (props: DeckMultiProps) => {
const prevDeckSlices = usePrevious(props.formData.deck_slices);
const prevVisibleLayersRedux = usePrevious(visibleDeckLayersFromRedux);
+ const toLayerFormData = useCallback(
+ (
+ sliceId: number,
+ result: JsonObject,
+ ): ({ slice_id: number } & JsonObject) | null => {
+ let params: JsonObject = {};
+ try {
+ params = JSON.parse(result.params || '{}');
+ } catch {
+ params = {};
+ }
+ // The saved params carry a `datasource` string, but it can be
+ // stale (e.g. example charts hardcode an id that differs from the
+ // imported dataset's real id). Prefer the chart's authoritative
+ // datasource_id/datasource_type so the layer queries the dataset
+ // it is actually bound to, the same one it uses standalone.
+ const datasource =
+ result.datasource_id != null && result.datasource_type
+ ? `${result.datasource_id}__${result.datasource_type}`
+ : params.datasource;
+ return {
+ slice_id: sliceId,
+ form_data: {
+ ...params,
+ datasource,
+ slice_id: sliceId,
+ viz_type: result.viz_type ?? params.viz_type,
+ },
+ };
+ },
+ [],
+ );
+
+ const fetchSubslicesPerChart = useCallback(
+ (sliceIds: number[]) =>
+ Promise.all<({ slice_id: number } & JsonObject) | null>(
+ sliceIds.map(sliceId =>
+ SupersetClient.get({ endpoint: `/api/v1/chart/${sliceId}` })
+ .then(({ json }) =>
+ toLayerFormData(sliceId, (json as JsonObject).result || {}),
+ )
+ .catch(() => null),
+ ),
+ ).then(slices =>
+ slices.filter(
+ (slice): slice is { slice_id: number } & JsonObject => slice !==
null,
+ ),
+ ),
+ [toLayerFormData],
+ );
+
+ const fetchSubslices = useCallback(
+ (sliceIds: number[]) => {
+ const containerId = props.formData.slice_id;
Review Comment:
A saved chart in Explore still has `slice_id`, so changing its layer
selection fetches the container's persisted `deck_slices` rather than the
current form state; newly selected layers cannot preview until the chart is
saved. Could the edit path use the current per-chart reads (or otherwise send
the current selection) instead of treating every nonzero `slice_id` as
unchanged saved state?
--
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]
