mikebridge commented on code in PR #44851: URL: https://github.com/apache/superset/pull/44851#discussion_r4212260329
########## superset-frontend/src/explore/actions/metadataSync.test.ts: ########## @@ -0,0 +1,370 @@ +/** + * 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 { AnyAction, applyMiddleware, createStore } from 'redux'; +import { + ControlPanelState, + ControlStateMapping, + Dataset, + dndAdhocFilterControl, + datePickerInAdhocFilterMixin, +} from '@superset-ui/chart-controls'; +import { + DatasourceType, + FeatureFlag, + getChartControlPanelRegistry, + QueryFormData, + TimeGranularity, + SupersetClient, +} from '@superset-ui/core'; +import exploreReducer, { + ExploreState, +} from 'src/explore/reducers/exploreReducer'; +import { ExplorePageState } from 'src/explore/types'; +import { getControlsState } from 'src/explore/store'; +import { + getControlConfig, + getControlStateFromControlConfig, + getFormDataFromControls, +} from 'src/explore/controlUtils'; +import tableControlPanel from '../../../plugins/plugin-chart-table/src/controlPanel'; +import { buildQuery } from '../../../plugins/plugin-chart-table/src/buildQuery'; +import { TableChartFormData } from '../../../plugins/plugin-chart-table/src/types'; +import versionHistoryReducer, { + CLEAR_VERSION_SESSION_LOG, +} from 'src/features/versionHistory/reducer'; +import { VersionHistoryState } from 'src/features/versionHistory/types'; +import { versionSessionLogMiddleware } from 'src/features/versionHistory/sessionLogMiddleware'; +import { + refreshSemanticMetadata, + setExploreControls, + syncSemanticMetadata, +} from './exploreActions'; + +const vizType = 'metadata-sync-regression'; + +beforeEach(() => { + getChartControlPanelRegistry().registerValue(vizType, { + controlPanelSections: [ + { + controlSetRows: [ + ['metrics'], + [ + { + name: 'adhoc_filters', + config: { + ...dndAdhocFilterControl, + ...datePickerInAdhocFilterMixin, + }, + }, + ], + [ + { + name: 'groupby', + config: { + type: 'SelectControl', + multi: true, + mapStateToProps: (state: ControlPanelState) => ({ + choices: (state.datasource as Dataset).columns.map(column => [ + column.column_name, + column.column_name, + ]), + }), + }, + }, + ], + ], + }, + ], + }); +}); + +afterEach(() => { + getChartControlPanelRegistry().remove(vizType); + jest.restoreAllMocks(); +}); + +test.each([false, true])( + 'metadata sync preserves a cleared time filter and logs only removed choices (removed choice: %s)', + async removed => { + const previousFlags = window.featureFlags; + window.featureFlags = { + ...previousFlags, + [FeatureFlag.VersionHistory]: true, + }; + const datasource: Dataset = { + id: 7, + type: DatasourceType.SemanticView, + columns: [ + { column_name: 'country', type: 'STRING', groupby: true }, + { column_name: 'created_at', type: 'TIMESTAMP', is_dttm: true }, + ], + metrics: [ + { uuid: 'orders-metric', metric_name: 'orders', expression: 'orders' }, + ], + column_formats: {}, + verbose_map: {}, + main_dttm_col: 'created_at', + datasource_name: 'orders', + description: null, + }; + const formData: QueryFormData = { + datasource: '7__semantic_view', + viz_type: vizType, + metrics: ['orders'], + groupby: ['country'], + adhoc_filters: [], + }; + const common = { + conf: { DEFAULT_VIZ_TYPE: vizType, DEFAULT_TIME_FILTER: 'Last year' }, + }; + const initialExplore: ExploreState = { + datasource, + common, + form_data: formData, + controls: { + datasource: { type: 'SelectControl', value: formData.datasource }, + viz_type: { type: 'SelectControl', value: vizType }, + metrics: { type: 'SelectControl', value: ['orders'] }, + groupby: { type: 'SelectControl', value: ['country'] }, + }, + }; + initialExplore.controls = getControlsState( + initialExplore as Parameters<typeof getControlsState>[0], + formData, + ) as ControlStateMapping; + expect(initialExplore.controls.adhoc_filters.value).toEqual([ + expect.objectContaining({ + operator: 'TEMPORAL_RANGE', + comparator: 'Last year', + }), + ]); + // The user removed the initialized time filter before syncing metadata. + initialExplore.controls.adhoc_filters = { + ...initialExplore.controls.adhoc_filters, + value: [], + }; + Object.freeze(initialExplore.controls.metrics); + Object.freeze(initialExplore.controls.groupby); + Object.freeze(initialExplore.controls); + Object.freeze(initialExplore); + const initialState = { + explore: initialExplore, + versionHistory: versionHistoryReducer(undefined, { + type: CLEAR_VERSION_SESSION_LOG, + }), + }; + const store = createStore( + ( + state: { + explore: ExploreState; + versionHistory: VersionHistoryState; + } = initialState, + action: AnyAction, + ) => ({ + explore: exploreReducer( + state.explore, + action as Parameters<typeof exploreReducer>[1], + ), + versionHistory: versionHistoryReducer( + state.versionHistory, + action as Parameters<typeof versionHistoryReducer>[1], + ), + }), + applyMiddleware(versionSessionLogMiddleware), + ); + const fresh: Dataset = { + ...datasource, + columns: removed ? [] : datasource.columns, + metrics: [ + ...datasource.metrics, + { + uuid: 'revenue-metric', + metric_name: 'revenue', + expression: 'revenue', + }, + ], + }; + const getSpy = jest + .spyOn(SupersetClient, 'get') + .mockResolvedValueOnce({ json: fresh } as never); + const postSpy = jest.spyOn(SupersetClient, 'post').mockResolvedValueOnce({ + json: { + result: { + compatible_metrics: ['orders', 'revenue'], + compatible_dimensions: ['country'], + }, + }, + } as never); + try { + await refreshSemanticMetadata(7, () => true)( + store.dispatch, + () => store.getState() as Pick<ExplorePageState, 'explore'>, + ); + expect(getSpy).toHaveBeenCalledTimes(1); + expect(postSpy).toHaveBeenCalledTimes(1); + expect(store.getState().explore.datasource).toEqual(fresh); Review Comment: Addressed in https://github.com/apache/superset/commit/e20fea019d, though the Big Number variant was not taken. The test now asserts that revenue appears in savedMetrics for both the plural metrics control and the singleton metric control, and that the existing orders selection is preserved on both. Those assertions pass without a reducer change. ########## docs/developer_docs/semantic-metadata-operations.md: ########## @@ -0,0 +1,126 @@ +--- +title: Semantic metadata operations +--- + +<!-- +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 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. +--> + +## Authority and scope + +Metadata maintenance resolves a stored semantic-view UUID and its owning +connection. It requires the existing SemanticView read permission, SemanticLayer +read/write permissions, view/layer data access, and permission to modify the +connection. Per FR-015, all three maintenance routes deliberately use a +SemanticView read gate, while their commands require write on the owning +SemanticLayer and connection-modify authority; mapping the routes to write would +add a SemanticView-write requirement outside that contract. View editorship +alone does not grant connection maintenance. +Anonymous and embedded guest principals cannot perform maintenance. Ordinary +chart access retains its canonical guest/dashboard/viewer/editor policy. + +All controls require `SEMANTIC_LAYERS`, the default-off +`SEMANTIC_LAYER_METADATA_REFRESH_ENABLED`, trusted tenant namespace, and provider +support. Provider construction happens after authorization. Fresh metadata DB +sessions recheck the persisted principal, view binding and connection configuration +before publication; request transactions are neither committed nor discarded. +Metadata DB connection/statement timeouts remain operator requirements. + +## Separate operations + +Use POST with `{}` and the stored view UUID for: + +- `/api/v1/semantic_view/<uuid>/refresh_metadata/`: acquire and publish the + connection catalog. Response contains `status` (`changed` or `unchanged`) and + `observed_at`. Even unchanged discovery gets a fresh cache token internally. +- `/api/v1/semantic_view/<uuid>/invalidate_catalog/`: retire the catalog and older + writer authority without fetching. Later authorized reads refill it. +- `/api/v1/semantic_view/<uuid>/invalidate_compatibility/`: retire compatibility + entries only. Catalog and query-result identities stay unchanged. + +No endpoint accepts raw keys, tenant/configuration overrides or unsaved edits. +No operation runs a chart or saves its settings. These commands are independent +of any particular UI. Existing query force-refresh remains separately authorized. + +POST `/api/v1/semantic_view/<uuid>/cache_metadata/` with `{"kind":"catalog"}` +or `{"kind":"compatibility","selected_metrics":[],"selected_dimensions":[]}` +returns the scoped `CacheEntryInfo`. Inspection never fills the catalog, creates +a generation or renews expiry. Backend limitations are reported explicitly. +Redis inspection requires the configured bounded reader for the same data cache; +unsupported custom URL/options configurations report unsupported expiry inspection. + +## Result diagnostics: captured identities only + +`InspectQueryResultCommand` accepts a host-prepared query context and query index. +The normal result-key path records the private identity in the HTTP request. +Inspection rechecks canonical context access and the subject/query/RLS fingerprint. +It never recomputes a provider UID or fetches metadata. A fresh request, changed +scope, annotation query or worker-only context returns `unsupported`. This is an +intentional limit; there is no standalone raw-key or reconstruction endpoint. +Internal callers can inspect during the same request after normal key construction. +Captured identities disappear with that request and are never returned to clients. + +## Failures and rollout + +Discovery and maintenance map typed service failures consistently: 409 for active +refresh/configuration changes, 502 for upstream/invalid catalogs, 503 for unavailable +storage/database or unconfirmed outcomes, 504 for deadline expiry, and 422 for +unsupported/incomplete configuration. Existing access/missing-resource errors stay +403/404. Errors never include provider payloads, credentials or database statements. + +The same typed error mapping applies to datasource metadata, query and column-value +requests, Explore context loading, and chart-data requests (including result +cache-key construction). Access checks still precede discovery. Unrelated database +and validation errors retain each endpoint's existing handling. + +Dashboard dataset loading retains its per-datasource failure isolation: if a +semantic provider fails during discovery, the response still contains the healthy +datasets with HTTP 200. It omits the failed semantic view and logs a safe warning +instead of failing metadata loading for every chart on the dashboard. + +The chart-context factory authorizes the full semantic context before column +discovery. Later query validation/access checks remain in place. The default-off +store alone did not provide this earlier boundary; enablement requires this command +slice plus compatible provider/fleet configuration. MCP/async/CLI adaptation, +operator timeouts, topology/load checks and UI/live-provider acceptance remain +separate rollout gates. No database migration or role grant is added. + +## Provisional editor interaction + +The semantic-view editor exposes **Sync metadata** next to its tabs when the +server reports maintenance capability and a stored UUID. This action refreshes +metadata without saving the description or cache-timeout draft. It preserves +the active tab. If publication succeeds but local reload fails, **Reload fields** +retries the read only. An unconfirmed sync also offers **Reload fields** and +disables another sync until the reload succeeds. This refreshes both the editor Review Comment: Fixed in https://github.com/apache/superset/commit/e20fea019d. An unconfirmed sync is now remembered per semantic view until it is confirmed or the page is reloaded, so reopening the editor keeps the warning and the Sync lock. Other views are unaffected, and a successful field reload clears it. A regression with the real store covers unmount and reopen, view isolation and no duplicate publication, and the operations guide documents the behavior. -- 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]
