You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/08/30 18:31:42 UTC
(superset) branch master updated: fix(semantic-layer): hydrate the semantic view edit modal from /structure (#42839)
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 87fb7d18e16 fix(semantic-layer): hydrate the semantic view edit modal from /structure (#42839)
87fb7d18e16 is described below
commit 87fb7d18e1621fe1a634ca21fb7fbc642ef31443
Author: Mike Bridge <mi...@preset.io>
AuthorDate: Sun Aug 30 12:31:35 2026 -0600
fix(semantic-layer): hydrate the semantic view edit modal from /structure (#42839)
Co-authored-by: Mike Bridge <mi...@ext.preset.io>
---
.../semanticViews/SemanticViewEditModal.test.tsx | 178 ++++++++++++++++++++-
.../semanticViews/SemanticViewEditModal.tsx | 71 +++++---
superset/semantic_layers/api.py | 8 +-
tests/unit_tests/semantic_layers/api_test.py | 12 ++
4 files changed, 246 insertions(+), 23 deletions(-)
diff --git a/superset-frontend/src/features/semanticViews/SemanticViewEditModal.test.tsx b/superset-frontend/src/features/semanticViews/SemanticViewEditModal.test.tsx
index 7229ccd635e..d1a71d2b3c8 100644
--- a/superset-frontend/src/features/semanticViews/SemanticViewEditModal.test.tsx
+++ b/superset-frontend/src/features/semanticViews/SemanticViewEditModal.test.tsx
@@ -17,7 +17,7 @@
* under the License.
*/
import userEvent from '@testing-library/user-event';
-import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import { act, render, screen, waitFor } from 'spec/helpers/testing-library';
import { SupersetClient, getClientErrorObject } from '@superset-ui/core';
import SemanticViewEditModal from './SemanticViewEditModal';
@@ -38,6 +38,10 @@ const mockedGetClientErrorObject = getClientErrorObject as jest.Mock;
const MOCK_STRUCTURE = {
result: {
+ // Matches createProps() so tests that are not about hydration keep
+ // asserting the same values whichever source the form reads from.
+ description: 'old description',
+ cache_timeout: 60,
dimensions: [
{
name: 'order_date',
@@ -158,6 +162,137 @@ test('fetches structure on mount', async () => {
});
});
+// sc-107904: the caller passes its own copy of description/cache_timeout, and
+// that copy goes stale as soon as this modal saves — Explore feeds the
+// pre-edit datasource straight back into the store. The modal must therefore
+// hydrate the Details tab from /structure, not from the prop.
+test('hydrates Details from the server, not the stale caller prop', async () => {
+ mockedGet.mockResolvedValue({
+ json: {
+ result: {
+ ...MOCK_STRUCTURE.result,
+ description: 'saved on the server',
+ cache_timeout: 900,
+ },
+ },
+ });
+ const props = createProps();
+
+ render(<SemanticViewEditModal {...props} />);
+
+ await waitFor(() => {
+ expect(screen.getByDisplayValue('saved on the server')).toBeInTheDocument();
+ });
+ expect(screen.queryByDisplayValue('old description')).not.toBeInTheDocument();
+
+ mockedPut.mockResolvedValue({});
+ await userEvent.click(screen.getByRole('button', { name: /save/i }));
+
+ await waitFor(() => {
+ expect(mockedPut).toHaveBeenCalledWith({
+ endpoint: '/api/v1/semantic_view/7',
+ jsonPayload: {
+ description: 'saved on the server',
+ cache_timeout: 900,
+ },
+ });
+ });
+});
+
+test('preserves unsaved edits when the parent recreates its props', async () => {
+ const props = createProps();
+ const { rerender } = render(<SemanticViewEditModal {...props} />);
+
+ const description = await screen.findByDisplayValue('old description');
+ await userEvent.clear(description);
+ await userEvent.type(description, 'unsaved edit');
+
+ rerender(
+ <SemanticViewEditModal
+ {...props}
+ semanticView={{
+ ...props.semanticView,
+ description: 'changed parent value',
+ cache_timeout: 120,
+ }}
+ />,
+ );
+
+ expect(screen.getByDisplayValue('unsaved edit')).toBeInTheDocument();
+});
+
+test('hydrates null description and cache timeout from the server', async () => {
+ mockedGet.mockResolvedValue({
+ json: {
+ result: {
+ ...MOCK_STRUCTURE.result,
+ description: null,
+ cache_timeout: null,
+ },
+ },
+ });
+ const props = createProps();
+
+ render(<SemanticViewEditModal {...props} />);
+
+ mockedPut.mockResolvedValue({});
+ await waitFor(() => {
+ expect(screen.getByRole('button', { name: /save/i })).toBeEnabled();
+ });
+ await userEvent.click(screen.getByRole('button', { name: /save/i }));
+
+ await waitFor(() => {
+ expect(mockedPut).toHaveBeenCalledWith({
+ endpoint: '/api/v1/semantic_view/7',
+ jsonPayload: {
+ description: null,
+ cache_timeout: null,
+ },
+ });
+ });
+});
+
+test('keeps prop-seeded values when the server omits the editable fields', async () => {
+ // An older backend (deploy skew) returns structure without description /
+ // cache_timeout. Hydration must not blank the form — otherwise a save
+ // right after would silently null the user's persisted description.
+ const { dimensions, metrics } = MOCK_STRUCTURE.result;
+ mockedGet.mockResolvedValue({ json: { result: { dimensions, metrics } } });
+ const props = createProps();
+
+ render(<SemanticViewEditModal {...props} />);
+
+ mockedPut.mockResolvedValue({});
+ await waitFor(() => {
+ expect(screen.getByRole('button', { name: /save/i })).toBeEnabled();
+ });
+ expect(screen.getByDisplayValue('old description')).toBeInTheDocument();
+ await userEvent.click(screen.getByRole('button', { name: /save/i }));
+
+ await waitFor(() => {
+ expect(mockedPut).toHaveBeenCalledWith({
+ endpoint: '/api/v1/semantic_view/7',
+ jsonPayload: {
+ description: 'old description',
+ cache_timeout: 60,
+ },
+ });
+ });
+});
+
+test('falls back to the caller prop when the structure fetch fails', async () => {
+ mockedGet.mockRejectedValue(new Error('structure failed'));
+ mockedGetClientErrorObject.mockResolvedValue({ error: 'boom' });
+ const props = createProps();
+
+ render(<SemanticViewEditModal {...props} />);
+
+ await waitFor(() => {
+ expect(props.addDangerToast).toHaveBeenCalledWith('boom');
+ });
+ expect(screen.getByDisplayValue('old description')).toBeInTheDocument();
+});
+
test('fetches and displays dimensions tab', async () => {
const props = createProps();
@@ -238,6 +373,45 @@ test('handles structure fetch error', async () => {
});
});
+test('does not toast if the fetch is cancelled while formatting its error', async () => {
+ let resolveClientError: (value: { error: string }) => void = () => {};
+ mockedGet.mockRejectedValue(new Error('fetch failed'));
+ mockedGetClientErrorObject.mockImplementation(
+ () =>
+ new Promise(resolve => {
+ resolveClientError = resolve;
+ }),
+ );
+ const props = createProps();
+ const { rerender } = render(<SemanticViewEditModal {...props} />);
+
+ await waitFor(() => {
+ expect(mockedGetClientErrorObject).toHaveBeenCalled();
+ });
+ rerender(<SemanticViewEditModal {...props} show={false} />);
+ await act(async () => {
+ resolveClientError({ error: 'Failed to load structure' });
+ });
+
+ expect(props.addDangerToast).not.toHaveBeenCalled();
+});
+
+test('clears structure loading when the semantic view is removed', async () => {
+ mockedGet.mockReturnValue(new Promise(() => {}));
+ const props = createProps();
+ const { rerender } = render(<SemanticViewEditModal {...props} />);
+
+ await waitFor(() => {
+ expect(screen.getByRole('button', { name: /save/i })).toBeDisabled();
+ });
+
+ rerender(<SemanticViewEditModal {...props} semanticView={null} />);
+
+ await waitFor(() => {
+ expect(screen.getByRole('button', { name: /save/i })).toBeEnabled();
+ });
+});
+
test('details tab save still works after viewing structure tabs', async () => {
mockedPut.mockResolvedValue({});
const props = createProps();
@@ -272,6 +446,8 @@ test('details tab save still works after viewing structure tabs', async () => {
const LARGE_STRUCTURE = {
result: {
+ description: 'old description',
+ cache_timeout: 60,
dimensions: Array.from({ length: 500 }, (_, i) => ({
name: `dim_${String(i).padStart(3, '0')}`,
type: 'string',
diff --git a/superset-frontend/src/features/semanticViews/SemanticViewEditModal.tsx b/superset-frontend/src/features/semanticViews/SemanticViewEditModal.tsx
index 3466585d198..1eb40ba2712 100644
--- a/superset-frontend/src/features/semanticViews/SemanticViewEditModal.tsx
+++ b/superset-frontend/src/features/semanticViews/SemanticViewEditModal.tsx
@@ -57,6 +57,10 @@ interface SemanticMetric {
}
interface SemanticViewStructure {
+ // Optional because an older backend may not emit them (deploy skew); the
+ // hydration effect only overwrites the form when they are present.
+ description?: string | null;
+ cache_timeout?: number | null;
dimensions: SemanticDimension[];
metrics: SemanticMetric[];
}
@@ -115,36 +119,61 @@ export default function SemanticViewEditModal({
);
const [structureLoading, setStructureLoading] = useState(false);
+ // 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);
}
- }, [semanticView]);
+ }, [show, semanticView?.id]); // eslint-disable-line react-hooks/exhaustive-deps
useEffect(() => {
- if (show && semanticView) {
- setStructureLoading(true);
- SupersetClient.get({
- endpoint: `/api/v1/semantic_view/${semanticView.id}/structure`,
- })
- .then(({ json }) => {
- setStructure(json.result);
- })
- .catch(async error => {
- const clientError = await getClientErrorObject(error);
- addDangerToast?.(
- clientError.error ||
- t('An error occurred while fetching the semantic view structure'),
- );
- })
- .finally(() => {
- setStructureLoading(false);
- });
- } else {
+ if (!show || !semanticView) {
setStructure(null);
+ setStructureLoading(false);
+ return undefined;
}
- }, [show, semanticView]); // eslint-disable-line react-hooks/exhaustive-deps
+
+ let cancelled = false;
+ setStructureLoading(true);
+ SupersetClient.get({
+ endpoint: `/api/v1/semantic_view/${semanticView.id}/structure`,
+ })
+ .then(({ json }) => {
+ if (cancelled) 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) {
+ setDescription(json.result.description || '');
+ }
+ if ('cache_timeout' in json.result) {
+ setCacheTimeout(json.result.cache_timeout ?? null);
+ }
+ })
+ .catch(async error => {
+ if (cancelled) return;
+ const clientError = await getClientErrorObject(error);
+ if (cancelled) return;
+ addDangerToast?.(
+ clientError.error ||
+ t('An error occurred while fetching the semantic view structure'),
+ );
+ })
+ .finally(() => {
+ if (!cancelled) setStructureLoading(false);
+ });
+
+ return () => {
+ cancelled = true;
+ };
+ }, [show, semanticView?.id]); // eslint-disable-line react-hooks/exhaustive-deps
const handleSave = async () => {
if (!semanticView) return;
diff --git a/superset/semantic_layers/api.py b/superset/semantic_layers/api.py
index f418e98eefe..07f7ef51be5 100644
--- a/superset/semantic_layers/api.py
+++ b/superset/semantic_layers/api.py
@@ -197,7 +197,11 @@ class SemanticViewRestApi(BaseSupersetModelRestApi):
log_to_statsd=False,
)
def structure(self, pk: int) -> Response:
- """Get the structure (dimensions and metrics) of a semantic view.
+ """Get a semantic view's editable fields and its structure.
+
+ The editable fields (``description``, ``cache_timeout``) are served here
+ because ``SemanticViewRestApi`` exposes no detail route: this is the only
+ read endpoint an editor can hydrate from after a write.
---
get:
summary: Get semantic view structure
@@ -267,6 +271,8 @@ class SemanticViewRestApi(BaseSupersetModelRestApi):
200,
result={
"name": view.name,
+ "description": view.description,
+ "cache_timeout": view.cache_timeout,
"dimensions": dimensions,
"metrics": metrics,
},
diff --git a/tests/unit_tests/semantic_layers/api_test.py b/tests/unit_tests/semantic_layers/api_test.py
index f2dea7cce30..385dcdbc6c8 100644
--- a/tests/unit_tests/semantic_layers/api_test.py
+++ b/tests/unit_tests/semantic_layers/api_test.py
@@ -2275,6 +2275,8 @@ def test_get_semantic_view_structure(
mock_view = MagicMock()
mock_view.name = "orders"
+ mock_view.description = "All orders"
+ mock_view.cache_timeout = 600
mock_view.implementation.get_dimensions.return_value = {mock_dim}
mock_view.implementation.get_metrics.return_value = {mock_metric}
@@ -2288,6 +2290,10 @@ def test_get_semantic_view_structure(
assert response.status_code == 200
result = response.json["result"]
assert result["name"] == "orders"
+ # The edit modal has no detail route to hydrate from, so these must ride
+ # along with the structure or a saved description cannot be read back.
+ assert result["description"] == "All orders"
+ assert result["cache_timeout"] == 600
assert len(result["dimensions"]) == 1
assert result["dimensions"][0]["name"] == "order_date"
assert result["dimensions"][0]["type"] == "timestamp[us]"
@@ -2371,6 +2377,8 @@ def test_get_semantic_view_structure_no_grain(
mock_view = MagicMock()
mock_view.name = "customers"
+ mock_view.description = None
+ mock_view.cache_timeout = None
mock_view.implementation.get_dimensions.return_value = {mock_dim}
mock_view.implementation.get_metrics.return_value = set()
@@ -2385,6 +2393,10 @@ def test_get_semantic_view_structure_no_grain(
result = response.json["result"]
assert result["dimensions"][0]["grain"] is None
assert result["metrics"] == []
+ # An unset description must come back as null, not be omitted: the modal
+ # distinguishes "cleared" from "absent" when hydrating.
+ assert result["description"] is None
+ assert result["cache_timeout"] is None
mock_view.raise_for_access.assert_called_once()