From 0f08f016d2e24dca0187156da5f38d6d50688ab1 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Thu, 23 Apr 2026 14:00:59 -0400 Subject: [PATCH] Address semantic layer review nits Improve semantic layer schema refresh error handling and connections endpoint behavior to reduce noisy failures while keeping this feature branch focused. Also restore frontend typing consistency and add debounce coverage for dynamic schema refresh. --- .../SemanticLayerModal.test.tsx | 127 ++++++++++++++++++ .../semanticLayers/SemanticLayerModal.tsx | 84 +++++------- .../src/pages/DatabaseList/index.tsx | 26 ++-- superset/semantic_layers/api.py | 43 +++++- tests/unit_tests/semantic_layers/api_test.py | 39 ++---- 5 files changed, 228 insertions(+), 91 deletions(-) create mode 100644 superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx diff --git a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx new file mode 100644 index 00000000000..3ac348fdc73 --- /dev/null +++ b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx @@ -0,0 +1,127 @@ +/** + * 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 { SupersetClient } from '@superset-ui/core'; +import { render, waitFor } from 'spec/helpers/testing-library'; + +import SemanticLayerModal from './SemanticLayerModal'; + +let mockJsonFormsChangeTriggered = false; + +jest.mock('@jsonforms/react', () => ({ + ...jest.requireActual('@jsonforms/react'), + JsonForms: ({ onChange }: { onChange: (value: unknown) => void }) => { + // eslint-disable-next-line react-hooks/rules-of-hooks + if (!mockJsonFormsChangeTriggered) { + mockJsonFormsChangeTriggered = true; + onChange({ + data: { warehouse: 'wh1' }, + errors: [], + }); + } + return null; + }, +})); + +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + SupersetClient: { + ...jest.requireActual('@superset-ui/core').SupersetClient, + get: jest.fn(), + post: jest.fn(), + put: jest.fn(), + }, + getClientErrorObject: jest.fn(() => Promise.resolve({ error: '' })), +})); + +const mockedGet = SupersetClient.get as jest.Mock; +const mockedPost = SupersetClient.post as jest.Mock; + +const props = { + show: true, + onHide: jest.fn(), + addDangerToast: jest.fn(), + addSuccessToast: jest.fn(), + semanticLayerUuid: '11111111-1111-1111-1111-111111111111', +}; + +beforeEach(() => { + mockJsonFormsChangeTriggered = false; + jest.useFakeTimers(); + mockedGet.mockReset(); + mockedPost.mockReset(); + + mockedGet + .mockResolvedValueOnce({ + json: { + result: [{ id: 'snowflake', name: 'Snowflake', description: '' }], + }, + }) + .mockResolvedValueOnce({ + json: { + result: { + name: 'Layer 1', + type: 'snowflake', + configuration: { warehouse: 'wh0' }, + }, + }, + }); + + mockedPost.mockResolvedValue({ + json: { + result: { + type: 'object', + properties: { + warehouse: { + type: 'string', + 'x-dynamic': true, + 'x-dependsOn': ['warehouse'], + }, + }, + }, + }, + }); +}); + +afterEach(() => { + jest.runOnlyPendingTimers(); + jest.useRealTimers(); +}); + +test('posts configuration schema refresh after debounce', async () => { + render(); + + await waitFor(() => { + expect(mockedPost).toHaveBeenCalledWith({ + endpoint: '/api/v1/semantic_layer/schema/configuration', + jsonPayload: { type: 'snowflake' }, + }); + }); + + jest.advanceTimersByTime(501); + + await waitFor(() => { + expect(mockedPost).toHaveBeenCalledWith({ + endpoint: '/api/v1/semantic_layer/schema/configuration', + jsonPayload: { + type: 'snowflake', + configuration: { warehouse: 'wh1' }, + }, + }); + }); +}); diff --git a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx index 7803138542f..e00eead4ba7 100644 --- a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx +++ b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx @@ -18,10 +18,8 @@ */ import { useState, useEffect, useCallback, useRef } from 'react'; import { t } from '@apache-superset/core/translation'; -import { styled } from '@apache-superset/core/theme'; -import { SupersetClient } from '@superset-ui/core'; -import { Input, Spin } from 'antd'; -import { Select } from '@superset-ui/core/components'; +import { SupersetClient, getClientErrorObject } from '@superset-ui/core'; +import { Input, Select, Button } from '@superset-ui/core/components'; import { Icons } from '@superset-ui/core/components/Icons'; import { JsonForms, withJsonFormsControlProps } from '@jsonforms/react'; import type { @@ -131,7 +129,7 @@ function DynamicFieldControl(props: ControlProps) { options: { ...props.uischema.options, placeholderText: t('Loading...'), - inputProps: { suffix: }, + inputProps: { suffix: }, }, }; return TextControl({ ...props, uischema, enabled: false }); @@ -271,27 +269,6 @@ function serializeDependencyValues( return JSON.stringify(snapshot); } -const ModalContent = styled.div` - padding: ${({ theme }) => theme.sizeUnit * 4}px; -`; - -const BackLink = styled.button` - background: none; - border: none; - color: ${({ theme }) => theme.colorPrimary}; - cursor: pointer; - padding: 0; - font-size: ${({ theme }) => theme.fontSize}px; - margin-bottom: ${({ theme }) => theme.sizeUnit * 2}px; - display: inline-flex; - align-items: center; - gap: ${({ theme }) => theme.sizeUnit}px; - - &:hover { - text-decoration: underline; - } -`; - interface SemanticLayerType { id: string; name: string; @@ -368,11 +345,21 @@ export default function SemanticLayerModal({ jsonPayload: { type, configuration }, }); applySchema(json.result); + if (json.warning) { + addDangerToast(String(json.warning)); + } if (isInitialFetch) setStep('config'); - } catch { + } catch (error) { + const clientError = await getClientErrorObject(error); if (isInitialFetch) { addDangerToast( - t('An error occurred while fetching the configuration schema'), + clientError.error || + t('An error occurred while fetching the configuration schema'), + ); + } else { + addDangerToast( + clientError.error || + t('An error occurred while refreshing the configuration schema'), ); } } finally { @@ -405,9 +392,11 @@ export default function SemanticLayerModal({ }); applySchema(schemaJson.result); setStep('config'); - } catch { + } catch (error) { + const clientError = await getClientErrorObject(error); addDangerToast( - t('An error occurred while fetching the semantic layer'), + clientError.error || + t('An error occurred while fetching the semantic layer'), ); } finally { setLoading(false); @@ -477,11 +466,13 @@ export default function SemanticLayerModal({ addSuccessToast(t('Semantic layer created')); } onHide(); - } catch { + } catch (error) { + const clientError = await getClientErrorObject(error); addDangerToast( - isEditMode - ? t('An error occurred while updating the semantic layer') - : t('An error occurred while creating the semantic layer'), + clientError.error || + (isEditMode + ? t('An error occurred while updating the semantic layer') + : t('An error occurred while creating the semantic layer')), ); } finally { setSaving(false); @@ -536,15 +527,9 @@ export default function SemanticLayerModal({ setFormData(data); errorsRef.current = errors ?? []; setHasErrors(errorsRef.current.length > 0); - if ( - validationMode === 'ValidateAndShow' && - errorsRef.current.length === 0 - ) { - handleCreate(); - } maybeRefreshSchema(data); }, - [validationMode, handleCreate, maybeRefreshSchema], + [maybeRefreshSchema], ); const selectedTypeName = @@ -574,7 +559,7 @@ export default function SemanticLayerModal({ contentLoading={loading} > {step === 'type' ? ( - + <> )} - + )} ); diff --git a/superset-frontend/src/pages/DatabaseList/index.tsx b/superset-frontend/src/pages/DatabaseList/index.tsx index a263fe2d2b4..2d7ee93b0ca 100644 --- a/superset-frontend/src/pages/DatabaseList/index.tsx +++ b/superset-frontend/src/pages/DatabaseList/index.tsx @@ -25,6 +25,7 @@ import { } from '@superset-ui/core'; import { css, styled, useTheme } from '@apache-superset/core/theme'; import { useState, useMemo, useEffect, useCallback } from 'react'; +import type { CellProps } from 'react-table'; import rison from 'rison'; import { useSelector } from 'react-redux'; import { useQueryParams, BooleanParam } from 'use-query-params'; @@ -68,6 +69,7 @@ import { DatabaseObject } from 'src/features/databases/types'; import { QueryObjectColumns } from 'src/views/CRUD/types'; import { WIDER_DROPDOWN_WIDTH } from 'src/components/ListView/utils'; import { ModalTitleWithIcon } from 'src/components/ModalTitleWithIcon'; +import type Owner from 'src/types/Owner'; const extensionsRegistry = getExtensionsRegistry(); const DatabaseDeleteRelatedExtension = extensionsRegistry.get( @@ -82,6 +84,8 @@ const PAGE_SIZE = 25; type ConnectionItem = DatabaseObject & { source_type?: 'database' | 'semantic_layer'; sl_type?: string; + changed_by?: Owner; + changed_on_delta_humanized?: string; }; interface DatabaseDeleteObject extends DatabaseObject { @@ -497,7 +501,7 @@ function DatabaseList({ await handleResourceExport('database', [database.id], () => { setPreparingExport(false); }); - } catch (error) { + } catch { setPreparingExport(false); addDangerToast(t('There was an issue exporting the database')); } @@ -596,11 +600,11 @@ function DatabaseList({ {t('AQE')} ), - Cell: ({ row: { original } }: any) => + Cell: ({ row: { original } }: CellProps) => original.source_type === 'semantic_layer' ? ( – ) : ( - + ), size: 'sm', id: 'allow_run_async', @@ -616,11 +620,11 @@ function DatabaseList({ {t('DML')} ), - Cell: ({ row: { original } }: any) => + Cell: ({ row: { original } }: CellProps) => original.source_type === 'semantic_layer' ? ( – ) : ( - + ), size: 'sm', id: 'allow_dml', @@ -628,11 +632,11 @@ function DatabaseList({ { accessor: 'allow_file_upload', Header: t('File upload'), - Cell: ({ row: { original } }: any) => + Cell: ({ row: { original } }: CellProps) => original.source_type === 'semantic_layer' ? ( – ) : ( - + ), size: 'md', id: 'allow_file_upload', @@ -640,11 +644,11 @@ function DatabaseList({ { accessor: 'expose_in_sqllab', Header: t('Expose in SQL Lab'), - Cell: ({ row: { original } }: any) => + Cell: ({ row: { original } }: CellProps) => original.source_type === 'semantic_layer' ? ( – ) : ( - + ), size: 'md', id: 'expose_in_sqllab', @@ -657,7 +661,9 @@ function DatabaseList({ changed_on_delta_humanized: changedOn, }, }, - }: any) => , + }: CellProps) => ( + + ), Header: t('Last modified'), accessor: 'changed_on_delta_humanized', size: 'xl', diff --git a/superset/semantic_layers/api.py b/superset/semantic_layers/api.py index 14527ffcf8a..d74decb1a82 100644 --- a/superset/semantic_layers/api.py +++ b/superset/semantic_layers/api.py @@ -25,6 +25,7 @@ from flask_appbuilder.api.schemas import get_list_schema from flask_appbuilder.models.sqla.interface import SQLAInterface from marshmallow import ValidationError from pydantic import ValidationError as PydanticValidationError +from sqlalchemy.orm import load_only from superset import db, event_logger, is_feature_enabled from superset.commands.semantic_layer.create import CreateSemanticLayerCommand @@ -314,14 +315,23 @@ class SemanticLayerRestApi(BaseSupersetApi): if config := body.get("configuration"): parsed_config = _parse_partial_config(cls, config) + warning: str | None = None try: schema = cls.get_configuration_schema(parsed_config) - except Exception: # pylint: disable=broad-except + except Exception as ex: # pylint: disable=broad-except + warning = str(ex) + logger.exception( + "Error enriching semantic layer configuration schema for type %s", + sl_type, + ) # Connection or query failures during schema enrichment should not # prevent the form from rendering — return the base schema instead. schema = cls.get_configuration_schema(None) - resp = make_response(json.dumps({"result": schema}, sort_keys=False), 200) + payload: dict[str, Any] = {"result": schema} + if warning: + payload["warning"] = warning + resp = make_response(json.dumps(payload, sort_keys=False), 200) resp.headers["Content-Type"] = "application/json; charset=utf-8" return resp @@ -569,7 +579,7 @@ class SemanticLayerRestApi(BaseSupersetApi): source_type, name_filter = self._parse_connection_filters(filters) if not is_feature_enabled("SEMANTIC_LAYERS"): - source_type = "database" + return self.response_404() all_items = self._fetch_connection_items(source_type, name_filter) @@ -611,18 +621,41 @@ class SemanticLayerRestApi(BaseSupersetApi): """Fetch database and semantic layer items based on filters.""" db_items: list[tuple[str, Database]] = [] if source_type in ("all", "database"): - db_q = db.session.query(Database) + db_q = db.session.query(Database).options( + load_only( + Database.id, + Database.uuid, + Database.database_name, + Database.backend, + Database.allow_run_async, + Database.allow_dml, + Database.allow_file_upload, + Database.expose_in_sqllab, + Database.changed_on, + Database.changed_by_fk, + ) + ) if name_filter: db_q = db_q.filter(Database.database_name.ilike(f"%{name_filter}%")) db_items = [("database", obj) for obj in db_q.all()] sl_items: list[tuple[str, SemanticLayer]] = [] if source_type in ("all", "semantic_layer"): - sl_q = db.session.query(SemanticLayer) + sl_q = db.session.query(SemanticLayer).options( + load_only( + SemanticLayer.uuid, + SemanticLayer.name, + SemanticLayer.type, + SemanticLayer.description, + SemanticLayer.changed_on, + SemanticLayer.changed_by_fk, + ) + ) if name_filter: sl_q = sl_q.filter(SemanticLayer.name.ilike(f"%{name_filter}%")) sl_items = [("semantic_layer", obj) for obj in sl_q.all()] + # TODO: move sort + pagination to SQL before GA. return db_items + sl_items # type: ignore @staticmethod diff --git a/tests/unit_tests/semantic_layers/api_test.py b/tests/unit_tests/semantic_layers/api_test.py index c6482c0f139..48a5ab64e57 100644 --- a/tests/unit_tests/semantic_layers/api_test.py +++ b/tests/unit_tests/semantic_layers/api_test.py @@ -1181,6 +1181,7 @@ def test_configuration_schema_enrichment_error_fallback( assert response.status_code == 200 assert response.json["result"] == {"type": "object"} + assert response.json["warning"] == "connection failed" assert mock_cls.get_configuration_schema.call_count == 2 @@ -1218,9 +1219,11 @@ def test_connections_list( mock_db_session = mocker.patch("superset.semantic_layers.api.db.session") db_query = MagicMock() + db_query.options.return_value = db_query db_query.all.return_value = [mock_db] db_query.filter.return_value = db_query sl_query = MagicMock() + sl_query.options.return_value = sl_query sl_query.all.return_value = [mock_layer] sl_query.filter.return_value = sl_query mock_db_session.query.side_effect = [db_query, sl_query] @@ -1252,28 +1255,7 @@ def test_connections_database_only( full_api_access: None, mocker: MockerFixture, ) -> None: - """Test GET /connections/ with semantic layers disabled.""" - from datetime import datetime - - mock_db = MagicMock() - mock_db.id = 1 - mock_db.uuid = uuid_lib.uuid4() - mock_db.database_name = "PostgreSQL" - mock_db.backend = "postgresql" - mock_db.allow_run_async = False - mock_db.allow_dml = False - mock_db.allow_file_upload = False - mock_db.expose_in_sqllab = True - mock_db.changed_on = datetime(2026, 1, 1) - mock_db.changed_on_delta_humanized.return_value = "1 month ago" - mock_db.changed_by = MagicMock() - mock_db.changed_by.first_name = "John" - mock_db.changed_by.last_name = "Doe" - - mock_db_session = mocker.patch("superset.semantic_layers.api.db.session") - db_query = MagicMock() - db_query.all.return_value = [mock_db] - mock_db_session.query.return_value = db_query + """Test GET /connections/ returns 404 when feature flag is disabled.""" mocker.patch( "superset.semantic_layers.api.is_feature_enabled", @@ -1282,12 +1264,7 @@ def test_connections_database_only( response = client.get("/api/v1/semantic_layer/connections/") - assert response.status_code == 200 - assert response.json["count"] == 1 - result = response.json["result"][0] - assert result["source_type"] == "database" - assert result["database_name"] == "PostgreSQL" - assert result["changed_by"]["first_name"] == "John" + assert response.status_code == 404 @SEMANTIC_LAYERS_APP @@ -1299,9 +1276,11 @@ def test_connections_name_filter( """Test GET /connections/ with name filter.""" mock_db_session = mocker.patch("superset.semantic_layers.api.db.session") db_query = MagicMock() + db_query.options.return_value = db_query db_query.all.return_value = [] db_query.filter.return_value = db_query sl_query = MagicMock() + sl_query.options.return_value = sl_query sl_query.all.return_value = [] sl_query.filter.return_value = sl_query mock_db_session.query.side_effect = [db_query, sl_query] @@ -1359,8 +1338,10 @@ def test_connections_sort_by_name( mock_db_session = mocker.patch("superset.semantic_layers.api.db.session") db_query = MagicMock() + db_query.options.return_value = db_query db_query.all.return_value = [mock_db] sl_query = MagicMock() + sl_query.options.return_value = sl_query sl_query.all.return_value = [mock_layer] mock_db_session.query.side_effect = [db_query, sl_query] @@ -1412,6 +1393,7 @@ def test_connections_source_type_filter( mock_db_session = mocker.patch("superset.semantic_layers.api.db.session") db_query = MagicMock() + db_query.options.return_value = db_query db_query.all.return_value = [mock_db] mock_db_session.query.return_value = db_query @@ -1454,6 +1436,7 @@ def test_connections_source_type_semantic_layer_only( mock_db_session = mocker.patch("superset.semantic_layers.api.db.session") sl_query = MagicMock() + sl_query.options.return_value = sl_query sl_query.all.return_value = [mock_layer] mock_db_session.query.return_value = sl_query