sadpandajoe commented on code in PR #41714:
URL: https://github.com/apache/superset/pull/41714#discussion_r3685942153
##########
superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.tsx:
##########
@@ -336,35 +363,114 @@ const DeckMulti = (props: DeckMultiProps) => {
},
} as any as JsonObject & { slice_id: number };
- const url = getExploreLongUrl(subsliceCopy.form_data, 'json');
-
- if (url) {
- SupersetClient.get({ endpoint: url })
- .then(({ json }) => {
- const layer = createLayerFromData(subsliceCopy, json);
+ const vizType = subsliceCopy.form_data.viz_type as string;
+ Promise.all([
+ getChartBuildQueryRegistry().get(vizType),
+ getChartTransformPropsRegistry().get(vizType),
+ ])
+ .then(([layerBuildQuery, layerTransformProps]) => {
+ if (
+ typeof layerBuildQuery !== 'function' ||
+ typeof layerTransformProps !== 'function'
+ ) {
+ throw new Error(`Unknown deck.gl layer type: ${vizType}`);
+ }
+ const queryContext = layerBuildQuery(
+ subsliceCopy.form_data as QueryFormData,
+ );
+ return SupersetClient.post({
+ endpoint: '/api/v1/chart/data',
+ jsonPayload: {
+ ...queryContext,
+ result_format: 'json',
+ result_type: 'full',
+ },
+ }).then(({ json }) => {
+ // A newer loadLayers call (deck_slices or the visibility filter
+ // changed again) has already reset state; this response belongs
+ // to an abandoned generation and must not write into it.
+ if (loadGenerationRef.current !== generation) {
+ return;
+ }
+ const chartProps = new ChartProps({
+ width: props.width,
+ height: props.height,
+ datasource: props.datasource as unknown as JsonObject,
+ formData: subsliceCopy.form_data,
+ queriesData: (json as JsonObject).result,
+ theme,
+ hooks: {},
+ initialValues: {},
+ });
+ const layerProps = layerTransformProps(chartProps) as JsonObject;
+ const layer = createLayerFromData(subsliceCopy,
layerProps.payload);
setSubSlicesLayers(subSlicesLayers => ({
...subSlicesLayers,
[subsliceCopy.slice_id]: layer,
}));
- })
- .catch(error => {
- throw new Error(
- `Error loading layer for slice ${subsliceCopy.slice_id}:
${error}`,
- );
+
+ // Refit the viewport to the data now that this layer's features
are
+ // known (unless autozoom is off). The initial getAdjustedViewport
+ // could not do this because the v1 payload carries no features.
+ const layerFeatures = (layerProps.payload as JsonObject |
undefined)
+ ?.data?.features;
+ if (formData.autozoom !== false && Array.isArray(layerFeatures)) {
+ layerFeaturesRef.current = {
+ ...layerFeaturesRef.current,
+ [vizType]: layerFeatures,
Review Comment:
Keying accumulated features by `vizType` replaces the first layer whenever a
multi chart contains two layers of the same type, so autozoom fits only the
last response and can move the other rendered layer out of view. Could this
accumulate by slice ID (or concatenate same-type features) instead?
##########
superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.tsx:
##########
@@ -409,8 +515,50 @@ const DeckMulti = (props: DeckMultiProps) => {
const prevDeckSlices = usePrevious(props.formData.deck_slices);
const prevVisibleLayersRedux = usePrevious(visibleDeckLayersFromRedux);
+ const fetchSubslices = useCallback(
+ (sliceIds: number[]) =>
+ Promise.all<({ slice_id: number } & JsonObject) | null>(
+ sliceIds.map(sliceId =>
Review Comment:
This fans out one metadata request and then one data request for every
persisted layer ID, while removing the old `DECK_MULTI_MAX_SLICES` guard
(default 50), so an imported or crafted chart can now trigger an unbounded
request burst and stall the dashboard/server. Should the configured cap be
enforced before starting this fanout?
##########
superset-frontend/packages/superset-ui-core/src/chart/clients/ChartClient.ts:
##########
@@ -104,24 +104,15 @@ export default class ChartClient {
const buildQueryRegistry = getChartBuildQueryRegistry();
if (metaDataRegistry.has(visType)) {
- const { useLegacyApi } = metaDataRegistry.get(visType)!;
const buildQuery =
(await buildQueryRegistry.get(visType)) ?? (() => formData);
- const requestConfig: RequestConfig = useLegacyApi
- ? {
- endpoint: '/explore_json/',
- postPayload: {
- form_data: buildQuery(formData),
- },
- ...options,
- }
- : {
- endpoint: '/api/v1/chart/data',
- jsonPayload: {
- query_context: buildQuery(formData),
- },
- ...options,
- };
+ const requestConfig: RequestConfig = {
+ endpoint: '/api/v1/chart/data',
+ jsonPayload: {
+ query_context: buildQuery(formData),
Review Comment:
The v1 endpoint expects the built query context fields (`datasource`,
`queries`, etc.) at the top level, but this wraps them under `query_context`;
it also returns `{result: [...]}` rather than a bare array. Now that
formerly-legacy chart types are routed here, `ChartDataProvider` consumers of
those types will receive a schema error (or the wrapper as query data); should
this post `buildQuery(formData)` directly and unwrap `result`?
##########
superset/migrations/shared/migrate_viz/base.py:
##########
@@ -167,7 +167,13 @@ def upgrade_slice(cls, slc: Slice) -> None:
if "form_data" in query_context:
query_context["form_data"] = clz.data
- queries_bak = copy.deepcopy(query_context["queries"])
+ # 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).
+ queries_bak = copy.deepcopy(query_context.get("queries"))
Review Comment:
This newly accepts a nonempty stored `query_context` with no `queries`, but
records `None`; the downgrade treats that as no prior context and sets
`slc.query_context = None`, losing the original hand-edited context instead of
reversing the migration. Could the backup distinguish an absent context from
one whose `queries` key was absent?
--
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]
