sadpandajoe commented on code in PR #44851:
URL: https://github.com/apache/superset/pull/44851#discussion_r4200965818
##########
superset-frontend/src/features/semanticViews/SemanticViewEditModal.tsx:
##########
@@ -118,65 +129,69 @@ export default function SemanticViewEditModal({
null,
);
const [structureLoading, setStructureLoading] = useState(false);
+ const [syncState, setSyncState] = useState<SyncState>({ status: 'idle' });
+ const [activeTab, setActiveTab] = useState('details');
+ const generation = useRef(0);
+ const busy = useRef(false);
+ const syncing =
+ syncState.status === 'syncing' || syncState.status === 'reloading';
- // Seeds the form from the caller's copy — what the form falls back to if
- // /structure fails (a spinner covers the form while that fetch is in
- // flight). Keyed to open/identity rather than the semanticView object so a
- // parent re-render cannot clobber in-progress edits.
useEffect(() => {
- if (semanticView) {
- setDescription(semanticView.description || '');
- setCacheTimeout(semanticView.cache_timeout ?? null);
- }
- }, [show, semanticView?.id]); // eslint-disable-line
react-hooks/exhaustive-deps
+ generation.current += 1;
+ const requestGeneration = generation.current;
+ const isCurrent = () => requestGeneration === generation.current;
+ busy.current = false;
+ setSaving(false);
+ setSyncState({ status: 'idle' });
+ setActiveTab('details');
+ setStructure(null);
+ setStructureLoading(false);
+ if (!show || !semanticView) return undefined;
- useEffect(() => {
- if (!show || !semanticView) {
- setStructure(null);
- setStructureLoading(false);
- return undefined;
- }
-
- let cancelled = false;
+ // Only initial hydration touches the draft. Sync reloads structure alone.
+ setDescription(semanticView.description || '');
+ setCacheTimeout(semanticView.cache_timeout ?? null);
setStructureLoading(true);
SupersetClient.get({
endpoint: `/api/v1/semantic_view/${semanticView.id}/structure`,
})
.then(({ json }) => {
- if (cancelled) return;
+ if (!isCurrent()) return;
setStructure(json.result);
- // The caller's copy of these fields goes stale the moment this modal
- // saves, so re-open hydrates from the server rather than the prop.
- // Only overwrite when the response carries the field: an older
- // backend that omits it (deploy skew) must not blank the form — while
- // an explicit null still means "cleared on the server".
- if ('description' in json.result) {
+ if ('description' in json.result)
setDescription(json.result.description || '');
- }
- if ('cache_timeout' in json.result) {
+ if ('cache_timeout' in json.result)
setCacheTimeout(json.result.cache_timeout ?? null);
- }
})
.catch(async error => {
- if (cancelled) return;
+ if (!isCurrent()) return;
const clientError = await getClientErrorObject(error);
- if (cancelled) return;
+ if (!isCurrent()) return;
addDangerToast?.(
clientError.error ||
Review Comment:
The new structure-endpoint decorators return `{error: <category>, message:
<localized copy>}`, and `getClientErrorObject` keeps `error` as the category
(the sync path in `metadataSyncError.ts` relies on that too). So when metadata
storage is unavailable, opening the editor toasts just `unavailable`, which is
untranslated and has no recovery guidance. Should this map the category to the
same local copy used for sync errors, or fall back to the response `message`?
##########
superset/initialization/__init__.py:
##########
@@ -163,10 +163,23 @@ class AppContextTask(task_base): # type: ignore
# nested context here would silently hand the task a second,
# blind session unable to see the caller's uncommitted work.
def __call__(self, *args: Any, **kwargs: Any) -> Any:
- if has_app_context():
- return task_base.__call__(self, *args, **kwargs)
- with superset_app.app_context():
- return task_base.__call__(self, *args, **kwargs)
+ # Avoid circular import through superset.app during
initialization.
+ from superset.semantic_layers.metadata_binding import
metadata_operation
+
+ with (
+ contextlib.nullcontext()
+ if has_app_context()
+ else superset_app.app_context()
+ ):
+ with (
+ metadata_operation()
Review Comment:
This opens one 30-second metadata operation for the entire Celery task, and
nested calls never replenish it. For a dashboard Excel export where an earlier
SQL chart takes ~45s, a later semantic chart reaches its metadata access after
the shared deadline has expired. It then raises a deadline error even with a
healthy Redis and warm catalog, and the export loop omits that chart even
though the task has a 600s limit. Should batch tasks like the export get a
per-chart operation budget instead of one budget spanning the whole task?
##########
superset-frontend/src/features/semanticViews/SemanticViewEditModal.tsx:
##########
@@ -188,15 +203,87 @@ export default function SemanticViewEditModal({
});
addSuccessToast?.(t('Semantic view updated'));
onSave();
- onHide();
+ if (isCurrent()) handleHide();
} catch (error) {
const clientError = await getClientErrorObject(error);
addDangerToast?.(
clientError.error ||
t('An error occurred while saving the semantic view'),
);
} finally {
- setSaving(false);
+ if (isCurrent()) {
+ busy.current = false;
+ setSaving(false);
+ }
+ }
+ };
+
+ const reloadFields = async (
+ viewId: number,
+ changed: boolean,
+ isCurrent: () => boolean,
+ ) => {
+ setSyncState({ status: 'reloading', changed });
+ try {
+ const { json } = await SupersetClient.get({
+ endpoint: `/api/v1/semantic_view/${viewId}/structure`,
+ });
+ if (!isCurrent()) return;
+ setStructure(json.result);
+ await onMetadataSync?.(isCurrent);
+ if (!isCurrent()) return;
+ setSyncState({ status: 'done', changed });
+ } catch {
+ if (isCurrent()) setSyncState({ status: 'reload-error', changed });
+ }
+ };
+
+ const handleSync = async () => {
+ if (
+ !semanticView ||
+ !structure?.uuid ||
+ structure.can_refresh_metadata !== true ||
+ busy.current ||
+ syncState.status === 'reload-error'
+ )
+ return;
+ busy.current = true;
+ generation.current += 1;
+ const requestGeneration = generation.current;
+ const isCurrent = () => requestGeneration === generation.current;
+ setSyncState({ status: 'syncing' });
+ try {
+ const { json } = await SupersetClient.post({
+ endpoint: `/api/v1/semantic_view/${structure.uuid}/refresh_metadata/`,
+ jsonPayload: {},
+ });
+ if (!isCurrent()) return;
+ await reloadFields(
+ semanticView.id,
+ json.result.status === 'changed',
+ isCurrent,
+ );
+ } catch (error) {
+ if (!isCurrent()) return;
+ const message = await metadataSyncError(error);
+ if (!isCurrent()) return;
+ setSyncState({ status: 'error', message });
Review Comment:
An `indeterminate` response lands in the generic error state with no reload
action, and its copy tells the user to reopen the editor. Reopening only
re-hydrates the modal, not Explore's datasource or compatibility state, so if
the catalog did publish, Explore keeps the old metrics/dimensions. Retrying
Sync just issues another publication POST. Should the indeterminate case offer
the same reload-only recovery as `reload-error`?
##########
superset-frontend/src/features/semanticViews/metadataSyncError.ts:
##########
@@ -0,0 +1,65 @@
+/**
+ * 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 { t } from '@apache-superset/core/translation';
+import { getClientErrorObject } from '@superset-ui/core';
+
+/** Only locally defined copy crosses this presentation boundary. */
+export async function metadataSyncError(
+ error: Parameters<typeof getClientErrorObject>[0],
+): Promise<string> {
+ const fallback = t('Unable to sync metadata. Try again later.');
+ try {
+ const parsed = await getClientErrorObject(error);
+ const messages: Record<string, string> = {
+ unsupported: t('This semantic layer does not support metadata sync.'),
+ configuration: t(
+ 'Complete the semantic layer configuration before syncing metadata.',
+ ),
+ in_progress: t(
+ 'A metadata sync is already in progress. Try again shortly.',
+ ),
+ configuration_changed: t(
+ 'The semantic view or connection changed. Reopen the editor and try
again.',
+ ),
+ upstream: t(
+ 'The semantic layer could not return its catalog. Try again later.',
+ ),
+ invalid_payload: t(
+ 'The semantic layer returned an invalid or oversized catalog.',
+ ),
+ deadline: t('Metadata sync timed out. Try again later.'),
+ unavailable: t('Metadata storage is unavailable. Try again later.'),
+ indeterminate: t(
+ 'Metadata sync could not be confirmed. Reopen the editor to reload
fields before trying again.',
+ ),
+ };
+ if (parsed.status === 401 || parsed.status === 403) {
Review Comment:
The 401/403 and 404 branches have no coverage: the modal tests mock
`getClientErrorObject` without a `status`, so deleting these branches would
leave every test green. If the user's permission is revoked, the session
expires, or the view is deleted after the editor opens, they would silently get
the generic retry text. Could you add a modal test that rejects realistic
403/404 responses through the real parser and asserts the
permission/missing-view message, preserved draft fields, and no success state
or structure reload?
--
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]
