gabotorresruiz commented on code in PR #42539: URL: https://github.com/apache/superset/pull/42539#discussion_r3825285822
########## superset-frontend/src/dashboard/components/nativeFilters/useDisplayControlDatasource.ts: ########## @@ -0,0 +1,133 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { useEffect, useRef, useState } from 'react'; +import { DatasourceType } from '@superset-ui/core'; +import { cachedSupersetGet } from 'src/utils/cachedSupersetGet'; +import { + fetchSemanticViewStructure, + semanticViewDimensionsToColumns, +} from './FiltersConfigModal/FiltersConfigForm/utils'; + +/** The column fields display controls read to build their options. */ +export interface DisplayControlColumn { + column_name?: string; + name?: string; + verbose_name?: string | null; + filterable?: boolean; +} + +export interface DisplayControlDatasource { + /** The datasource's display name (dataset table_name / semantic view name). */ + name: string | undefined; + columns: DisplayControlColumn[]; + loading: boolean; + error: Error | undefined; +} + +/** + * The single, type-aware datasource resolution point for dashboard display + * controls (Dynamic group by today; siblings adopt this hook as they gain + * loaders). + * + * Semantic views and regular datasets have independent numeric-id + * sequences, so a binding must be resolved by BOTH id and datasourceType — + * an id-only lookup silently reads whatever regular dataset shares the + * view's number (sc-111089). The default branch preserves the legacy + * display-control request (`GET /api/v1/dataset/<id>`, full resource, no + * projection) byte-for-byte; changing that shape is a deliberate, + * separately-tested decision, not a drive-by. + * + * Responses are guarded by a monotonic request id: on a same-id type flip + * a slow response for the previous binding is discarded rather than + * overwriting the newer one — reintroducing the wrong-object bug through + * a race would otherwise be possible. + */ +export function useDisplayControlDatasource( + datasetId: number | string | undefined | null, + datasourceType?: DatasourceType, +): DisplayControlDatasource { + const [name, setName] = useState<string | undefined>(); + const [columns, setColumns] = useState<DisplayControlColumn[]>([]); + const [loading, setLoading] = useState(false); + const [error, setError] = useState<Error | undefined>(); + const requestIdRef = useRef(0); + + useEffect(() => { + // Invalidate any in-flight request on every binding change (and on + // unmount, via the cleanup below). + requestIdRef.current += 1; + const requestId = requestIdRef.current; + + if (datasetId === undefined || datasetId === null || datasetId === '') { Review Comment: Elizabeth flagged the id 0 deviation from the legacy guard as almost certainly moot; it turns out to be reachable, so I would restore the falsy guard. migrateChartCustomization uses 0 as its null sentinel: extractDatasetId returns 0 for a null or unparsable legacy dataset (src/dashboard/util/migrateChartCustomization.ts:43-58), and that migration runs on dashboard hydrate (src/dashboard/actions/hydrate.ts:301). I verified it live: a dashboard carrying a legacy-format customization with dataset: null issues GET /api/v1/dataset/0 on this branch, gets a 404, and fires the danger toast "Failed to load columns for datasource 0"; the same dashboard on the pre-fix build makes no request and shows no toast. `if (!datasetId)` restores the byte-identical legacy behaviour the docstring promises and covers 0 and the empty string in one line. A renderCard([{ datasetId: 0 }]) case asserting no fetch and no toast would pin it. Small severity, but it is a real regression for dashboards that still carry a legacy control without a resolvable dataset. ########## superset-frontend/src/utils/semanticViewStructure.ts: ########## @@ -0,0 +1,117 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { Column } from '@superset-ui/core'; +import { GenericDataType } from '@apache-superset/core/common'; +import { cachedSupersetGet } from 'src/utils/cachedSupersetGet'; + +/** + * Shared semantic-view structure helpers. This module is deliberately + * layer-neutral (`src/utils`) because its consumers span layers: the + * native-filter configuration form, its column select, and the generic + * dataset drill-info hook all resolve semantic views through it. + */ + +export const mapSemanticTypeToGenericDataType = ( + semanticType?: string | null, +): GenericDataType | undefined => { + if (!semanticType) { + return undefined; + } + + const normalized = semanticType.toLowerCase(); + + if ( + /^(struct|list|map|array|fixed_size_list|large_list|union|dictionary)\b/.test( + normalized, + ) + ) { + return undefined; + } + + if (normalized.includes('bool')) { + return GenericDataType.Boolean; + } + + if (/(date|time|timestamp|datetime)/.test(normalized)) { + return GenericDataType.Temporal; + } + + if ( + /(\b(u?int\d*)\b|\bfloat\d*\b|\bdouble\b|\bdecimal\d*\b|\bnumber\b)/.test( + normalized, + ) + ) { + return GenericDataType.Numeric; + } + + if ( + /(\bstr(ing)?\b|\butf8\b|\blarge_string\b|\bbinary\b|\bjson\b|\buuid\b)/.test( + normalized, + ) + ) { + return GenericDataType.String; + } + + return undefined; +}; + +/** The slice of `GET /api/v1/semantic_view/<id>/structure` consumers rely on. */ +export interface SemanticViewStructure { + name?: string; + dimensions: { name: string; type: string }[]; + metrics: { name: string; definition: string }[]; +} + +/** + * Fetch a semantic view's structure — the single, shared entry point for + * every type-aware datasource consumer (ColumnSelect, FiltersConfigForm, + * display controls, drill metadata). Semantic views and regular datasets + * have independent numeric-id sequences, so callers must route here (and + * never to `/api/v1/dataset/<id>`) whenever the binding's datasourceType + * is SemanticView. + */ +export const fetchSemanticViewStructure = async ( + semanticViewId: number | string, +): Promise<SemanticViewStructure> => { + const response = await cachedSupersetGet({ + endpoint: `/api/v1/semantic_view/${semanticViewId}/structure`, Review Comment: This block worries me a bit: cachedSupersetGet caches the promise itself and cacheWrapper never evicts on rejection (src/utils/cacheWrapper.ts:25-33), so a single failed /structure response is cached for the rest of the page session. The regular-dataset drill branch compensates for exactly this with supersetGetCache.delete(endpoint) on error (src/hooks/apiResources/datasets.ts:155), but this shared helper does not, so the two drill branches recover differently after a transient failure. I verified it live on this branch: with a display control and a semantic-view chart sharing this endpoint, one injected 422 produced the failure toast and an errored drill state; after the endpoint was healthy again (confirmed 200 via curl), force-refreshing the chart re-queried /api/v1/chart/data but nothing re-requested /structure for the rest of the session; only a full page reload recovered. Since every semantic consumer now routes through this helper, one fix covers them all: ```ts export const fetchSemanticViewStructure = async ( semanticViewId: number | string, ): Promise<SemanticViewStructure> => { const endpoint = `/api/v1/semantic_view/${semanticViewId}/structure`; try { const response = await cachedSupersetGet({ endpoint }); const { name, dimensions = [], metrics = [] } = response.json?.result ?? {}; return { name, dimensions, metrics }; } catch (error) { supersetGetCache.delete(endpoint); throw error; } }; ``` plus a test that mocks a 500 then a 200 and asserts the second call refetches. Not a blocker, but it is cheap here and the asymmetry with the dataset branch will surprise someone later. -- 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]
