codeant-ai-for-open-source[bot] commented on code in PR #38396: URL: https://github.com/apache/superset/pull/38396#discussion_r3016435593
########## superset-frontend/src/features/semanticLayers/jsonFormsHelpers.tsx: ########## @@ -0,0 +1,301 @@ +/** + * 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 } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { Spin } from 'antd'; +import { withJsonFormsControlProps } from '@jsonforms/react'; +import type { + JsonSchema, + UISchemaElement, + ControlProps, +} from '@jsonforms/core'; +import { + rankWith, + and, + isStringControl, + formatIs, + schemaMatches, +} from '@jsonforms/core'; +import { + rendererRegistryEntries, + TextControl, +} from '@great-expectations/jsonforms-antd-renderers'; + +export const SCHEMA_REFRESH_DEBOUNCE_MS = 500; + +/** + * Custom renderer that renders `Input.Password` for fields with + * `format: "password"` in the JSON Schema (e.g. Pydantic `SecretStr`). + */ +function PasswordControl(props: ControlProps) { + const uischema = { + ...props.uischema, + options: { ...props.uischema.options, type: 'password' }, + }; + return TextControl({ ...props, uischema }); +} +const PasswordRenderer = withJsonFormsControlProps(PasswordControl); +const passwordEntry = { + tester: rankWith(3, and(isStringControl, formatIs('password'))), + renderer: PasswordRenderer, +}; + +/** + * Renderer for `const` properties (e.g. Pydantic discriminator fields). + * Renders nothing visually but ensures the const value is set in form data, + * so discriminated unions resolve correctly on the backend. + */ +function ConstControl({ data, handleChange, path, schema }: ControlProps) { + const constValue = (schema as Record<string, unknown>).const; + useEffect(() => { + if (constValue !== undefined && data !== constValue) { + handleChange(path, constValue); + } + }, [constValue, data, handleChange, path]); + return null; +} +const ConstRenderer = withJsonFormsControlProps(ConstControl); +const constEntry = { + tester: rankWith( + 10, + schemaMatches( + s => + s !== undefined && Review Comment: **Suggestion:** The `const` renderer tester uses the `in` operator on `s` without ensuring `s` is an object. JSON Schema allows boolean schemas, and evaluating `'const' in s` on a boolean throws a runtime `TypeError`, which can break form rendering. Guard the type before using `in`. [type error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Semantic layer configuration modal crashes rendering configuration form. - ❌ Add semantic view modal crashes when configuration schema boolean. - ⚠️ Any plugin returning boolean schemas can break forms. ``` </details> ```suggestion typeof s === 'object' && s !== null && ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Implement or configure a semantic layer type whose configuration schema is returned by `get_configuration_schema` in `superset/semantic_layers/api.py:503` with at least one property using a boolean JSON Schema, e.g. `schema["properties"]["foo"] = True` (boolean), which is valid per JSON Schema. 2. In the Superset UI, open the "New Semantic Layer" modal implemented in `superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx` by navigating to the Semantic Layers feature and clicking the button that renders `SemanticLayerModal` (see import of `renderers` from `./jsonFormsHelpers` at lines 37–44). 3. Select the custom semantic layer type so `fetchConfigSchema` in `SemanticLayerModal.tsx:135–159` POSTs to `/api/v1/semantic_layer/schema/configuration` and then calls `applySchema`, which sets `configSchema` and `uiSchema` and hands them to `<JsonForms>` along with the custom `renderers` array from `jsonFormsHelpers.tsx:180–186`. 4. When JsonForms evaluates the `constEntry` tester defined at `jsonFormsHelpers.tsx:75–85`, it calls the predicate `s => s !== undefined && 'const' in s && …` with `s === true` for the boolean subschema; the expression `'const' in s` at line 80 attempts to use the `in` operator on a boolean, throwing `TypeError: right-hand side of 'in' should be an object`, which causes the React render of the configuration form to fail and the modal to error. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/features/semanticLayers/jsonFormsHelpers.tsx **Line:** 79:79 **Comment:** *Type Error: The `const` renderer tester uses the `in` operator on `s` without ensuring `s` is an object. JSON Schema allows boolean schemas, and evaluating `'const' in s` on a boolean throws a runtime `TypeError`, which can break form rendering. Guard the type before using `in`. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38396&comment_hash=3331e1598d898a382a63a1e51005d78272af4a0b6646d537e29883519156c465&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38396&comment_hash=3331e1598d898a382a63a1e51005d78272af4a0b6646d537e29883519156c465&reaction=dislike'>👎</a> ########## superset/commands/semantic_layer/delete.py: ########## @@ -54,3 +56,48 @@ def validate(self) -> None: self._model = SemanticLayerDAO.find_by_uuid(self._uuid) if not self._model: raise SemanticLayerNotFoundError() + + +class DeleteSemanticViewCommand(BaseCommand): + def __init__(self, pk: int): + self._pk = pk + self._model: SemanticView | None = None + + @transaction( + on_error=partial( + on_error, + catches=(SQLAlchemyError,), + reraise=SemanticViewDeleteFailedError, + ) + ) + def run(self) -> None: + self.validate() + assert self._model + SemanticViewDAO.delete([self._model]) + + def validate(self) -> None: + self._model = SemanticViewDAO.find_by_id(self._pk, id_column="id") + if not self._model: + raise SemanticViewNotFoundError() + + +class BulkDeleteSemanticViewCommand(BaseCommand): + def __init__(self, model_ids: list[int]): + self._model_ids = model_ids + self._models: list[SemanticView] = [] + + @transaction( + on_error=partial( + on_error, + catches=(SQLAlchemyError,), + reraise=SemanticViewDeleteFailedError, + ) + ) + def run(self) -> None: + self.validate() Review Comment: **Suggestion:** Bulk delete removes all fetched semantic views without per-object ownership checks, enabling unauthorized mass deletion of other users' resources. Validate ownership for each model before deleting and fail safely if authorization fails. [security] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Non-owners can bulk delete others' semantic views. - ❌ Mass deletion possible with a single bulk_delete request. - ⚠️ Bulk delete diverges from dataset/chart ownership enforcement. ``` </details> ```suggestion from superset import security_manager from superset.exceptions import SupersetSecurityException self.validate() try: for model in self._models: security_manager.raise_for_ownership(model) except SupersetSecurityException as ex: raise SemanticViewDeleteFailedError() from ex ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Ensure `SEMANTIC_LAYERS` feature flag is enabled so `SemanticViewRestApi.bulk_delete` is registered; `superset/initialization/__init__.py:271–278` (per Grep) adds `SemanticViewRestApi` to `appbuilder`, and the class enables bulk delete via `include_route_methods = {"put", "post", "delete", "bulk_delete"}` (`superset/semantic_layers/api.py:176`). 2. Have User A create several semantic views (via `SemanticViewRestApi.post`), making them the owner. Have User B configured with `can delete` permission on the `SemanticView` resource (so `@protect()` on `SemanticViewRestApi.bulk_delete` at `superset/semantic_layers/api.py:44–52` allows access) but not listed as an owner of User A's views. 3. While logged in as User B, issue an HTTP DELETE to `/api/v1/semantic_view/?q=<rison>` where `<rison>` encodes a list of database IDs belonging entirely to User A's semantic views, for example `q=(1,2)`; the `@rison(get_delete_ids_schema)` decorator parses this into `item_ids: list[int]` passed to `SemanticViewRestApi.bulk_delete` (`superset/semantic_layers/api.py:83–85`). 4. `SemanticViewRestApi.bulk_delete` calls `BulkDeleteSemanticViewCommand(item_ids).run()`. In `BulkDeleteSemanticViewCommand.validate` (`superset/commands/semantic_layer/delete.py:100–103`), `SemanticViewDAO.find_by_ids(self._model_ids, id_column="id")` uses `BaseDAO.find_by_ids` (`superset/daos/base.py:311–62`) without any ownership filter, populating `self._models` with all requested views. No call to `security_manager.raise_for_ownership` is made in either `validate` or `run` (`lines 96–98`), so `BulkDeleteSemanticViewCommand.run` deletes all models with `SemanticViewDAO.delete(self._models)`. The API then returns HTTP 200 with a success message, even though User B did not own any of the deleted semantic views. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/commands/semantic_layer/delete.py **Line:** 97:97 **Comment:** *Security: Bulk delete removes all fetched semantic views without per-object ownership checks, enabling unauthorized mass deletion of other users' resources. Validate ownership for each model before deleting and fail safely if authorization fails. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38396&comment_hash=0eee7350258e3d101c239c35c8e27d456ca49b3fcdf018abef7511f5e91daab2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38396&comment_hash=0eee7350258e3d101c239c35c8e27d456ca49b3fcdf018abef7511f5e91daab2&reaction=dislike'>👎</a> ########## superset/commands/semantic_layer/delete.py: ########## @@ -54,3 +56,48 @@ def validate(self) -> None: self._model = SemanticLayerDAO.find_by_uuid(self._uuid) if not self._model: raise SemanticLayerNotFoundError() + + +class DeleteSemanticViewCommand(BaseCommand): + def __init__(self, pk: int): + self._pk = pk + self._model: SemanticView | None = None + + @transaction( + on_error=partial( + on_error, + catches=(SQLAlchemyError,), + reraise=SemanticViewDeleteFailedError, + ) + ) + def run(self) -> None: + self.validate() + assert self._model Review Comment: **Suggestion:** Deleting a single semantic view does not enforce ownership checks, allowing users with generic write permission to delete views they do not own. Add an ownership validation step before deletion and convert security exceptions into a handled delete failure. [security] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Non-owners can delete others' semantic views via API. - ⚠️ Single delete lacks per-object ownership enforcement. - ⚠️ Inconsistent with update command using ownership checks. ``` </details> ```suggestion from superset import security_manager from superset.exceptions import SupersetSecurityException self.validate() assert self._model try: security_manager.raise_for_ownership(self._model) except SupersetSecurityException as ex: raise SemanticViewDeleteFailedError() from ex ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. With `SEMANTIC_LAYERS` feature flag enabled, Superset registers `SemanticViewRestApi` in `superset/initialization/__init__.py` (Grep shows `SemanticViewRestApi` added around lines 274–278). Permissions for this API are controlled via `class_permission_name = "SemanticView"` and `method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP` in `SemanticViewRestApi` (`superset/semantic_layers/api.py:169–177` via Grep). 2. Log in as User A and create a semantic view using `POST /api/v1/semantic_view/` (handled by `SemanticViewRestApi.post` at `superset/semantic_layers/api.py:31–109`), so the new `SemanticView` row is owned by User A (standard Superset ownership model). 3. Log in as User B who has API access and the `can delete` permission for the `SemanticView` resource (enough to pass the `@protect()` decorator on `SemanticViewRestApi.delete` at `superset/semantic_layers/api.py:331–42`), but who is not in the `owners` of User A's view. 4. User B sends `DELETE /api/v1/semantic_view/<pk>` targeting User A's view. The request reaches `SemanticViewRestApi.delete`, which calls `DeleteSemanticViewCommand(pk).run()` (`superset/semantic_layers/api.py:361`). Inside `DeleteSemanticViewCommand.run` (`superset/commands/semantic_layer/delete.py:73–76`), `self.validate()` calls `SemanticViewDAO.find_by_id(self._pk, id_column="id")` in `validate` (`lines 78–81`), which uses `BaseDAO.find_by_id` with no `base_filter` (see `superset/daos/base.py:171–195`), so it loads the view regardless of ownership. Because there is no call to `security_manager.raise_for_ownership` anywhere in `DeleteSemanticViewCommand`, `run()` proceeds to `SemanticViewDAO.delete([self._model])`, permanently deleting a semantic view that User B does not own, and `SemanticViewRestApi.delete` returns HTTP 200. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/commands/semantic_layer/delete.py **Line:** 74:75 **Comment:** *Security: Deleting a single semantic view does not enforce ownership checks, allowing users with generic write permission to delete views they do not own. Add an ownership validation step before deletion and convert security exceptions into a handled delete failure. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38396&comment_hash=473bd1417d04c2960419abc6274129ae2147a06c6b46e10b33dec8bc737c16a0&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38396&comment_hash=473bd1417d04c2960419abc6274129ae2147a06c6b46e10b33dec8bc737c16a0&reaction=dislike'>👎</a> ########## superset-frontend/src/pages/DatasetList/index.tsx: ########## @@ -894,14 +987,39 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({ }; const handleBulkDatasetDelete = (datasetsToDelete: Dataset[]) => { - SupersetClient.delete({ - endpoint: `/api/v1/dataset/?q=${rison.encode( - datasetsToDelete.map(({ id }) => id), - )}`, - }).then( - ({ json = {} }) => { + const datasets = datasetsToDelete.filter( + d => d.source_type !== 'semantic_layer', + ); + const semanticViews = datasetsToDelete.filter( + d => d.source_type === 'semantic_layer', + ); + + const promises: Promise<unknown>[] = []; + + if (datasets.length) { + promises.push( + SupersetClient.delete({ + endpoint: `/api/v1/dataset/?q=${rison.encode( + datasets.map(({ id }) => id), + )}`, + }), + ); + } + + if (semanticViews.length) { + promises.push( + SupersetClient.delete({ + endpoint: `/api/v1/semantic_view/?q=${rison.encode( + semanticViews.map(({ id }) => id), + )}`, + }), + ); + } + + Promise.all(promises).then( Review Comment: **Suggestion:** Using `Promise.all` for mixed dataset/semantic-view bulk deletion causes a partial-success bug: if one delete request succeeds and the other fails, the promise rejects, `refreshData` is skipped, and the UI can remain stale even though some items were actually deleted. Refresh the list regardless of partial failures and only show full success when all requests succeed. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Datasets list can show stale rows after mixed deletion. - ⚠️ Users misled on which items actually deleted. ``` </details> ########## superset-frontend/src/pages/DatasetList/index.tsx: ########## @@ -527,25 +558,43 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({ Cell: ({ row: { original } }: any) => { const isSemanticView = original.source_type === 'semantic_layer'; - // Semantic view: only show edit button + // Semantic view: show edit and delete buttons if (isSemanticView) { - if (!canEdit) return null; + if (!canEdit && !canDelete) return null; return ( <Actions className="actions"> - <Tooltip - id="edit-action-tooltip" - title={t('Edit')} - placement="bottom" - > - <span - role="button" - tabIndex={0} - className="action-button" - onClick={() => setSvCurrentlyEditing(original)} + {canDelete && ( + <Tooltip + id="delete-action-tooltip" + title={t('Delete')} + placement="bottom" > - <Icons.EditOutlined iconSize="l" /> - </span> - </Tooltip> + <span + role="button" + tabIndex={0} + className="action-button" + onClick={() => handleSemanticViewDelete(original)} + > + <Icons.DeleteOutlined iconSize="l" /> + </span> + </Tooltip> + )} + {canEdit && ( + <Tooltip + id="edit-action-tooltip" + title={t('Edit')} + placement="bottom" + > + <span + role="button" + tabIndex={0} + className="action-button" + onClick={() => setSvCurrentlyEditing(original)} Review Comment: **Suggestion:** Semantic view edit actions are shown to any user with write permission, but dataset edits are owner/admin-gated; this breaks existing authorization behavior and can expose edit operations to unintended users. Apply the same owner/admin check before enabling semantic view edits. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Non-owners can open semantic view edit modal. - ⚠️ Authz inconsistent between datasets and semantic views. ``` </details> ```suggestion const allowSemanticViewEdit = original.owners.map((o: Owner) => o.id).includes(user.userId) || isUserAdmin(user); if (!canDelete && !allowSemanticViewEdit) return null; return ( <Actions className="actions"> {canDelete && ( <Tooltip id="delete-action-tooltip" title={t('Delete')} placement="bottom" > <span role="button" tabIndex={0} className="action-button" onClick={() => handleSemanticViewDelete(original)} > <Icons.DeleteOutlined iconSize="l" /> </span> </Tooltip> )} {canEdit && ( <Tooltip id="edit-action-tooltip" title={ allowSemanticViewEdit ? t('Edit') : t( 'You must be a dataset owner in order to edit. Please reach out to a dataset owner to request modifications or edit access.', ) } placement="bottom" > <span role="button" tabIndex={0} className={`action-button ${allowSemanticViewEdit ? '' : 'disabled'}`} onClick={ allowSemanticViewEdit ? () => setSvCurrentlyEditing(original) : undefined } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In `superset-frontend/src/pages/DatasetList/index.tsx`, note that `canEdit` is derived solely from `hasPerm('can_write')` at `index.tsx:309-313`, while dataset editability is further gated by ownership/admin via `allowEdit` at `index.tsx:603-605`. 2. Observe the Actions column cell implementation at `index.tsx:558-688`: for semantic views, `const isSemanticView = original.source_type === 'semantic_layer';` at `index.tsx:559` and the semantic-view branch starting at `index.tsx:562-600` only checks `canEdit`/`canDelete`, not ownership. 3. Run the UI as a user who has `can_write` permission (so `canEdit` is true) but is not in `original.owners` and is not an admin (so the dataset `allowEdit` check at `index.tsx:603-605` would be false for normal datasets). 4. With a semantic view row (where `original.source_type === 'semantic_layer'`), the code at `index.tsx:562-599` renders an enabled Edit icon whenever `canEdit` is true and wires `onClick={() => setSvCurrentlyEditing(original)}` at `index.tsx:588-593`, allowing this non-owner user to open `SemanticViewEditModal` (`index.tsx:1198-1205`) for a semantic view, whereas the equivalent dataset edit path is correctly disabled for non-owners/admins, leading to inconsistent and potentially over-permissive edit behavior for semantic views. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/pages/DatasetList/index.tsx **Line:** 563:592 **Comment:** *Logic Error: Semantic view edit actions are shown to any user with write permission, but dataset edits are owner/admin-gated; this breaks existing authorization behavior and can expose edit operations to unintended users. Apply the same owner/admin check before enabling semantic view edits. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38396&comment_hash=52ca27d43f4e5c47ea8656a92b69e499a831714c4193bb8ef656357629135c7c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38396&comment_hash=52ca27d43f4e5c47ea8656a92b69e499a831714c4193bb8ef656357629135c7c&reaction=dislike'>👎</a> ########## tests/unit_tests/semantic_layers/api_test.py: ########## @@ -1488,3 +1490,456 @@ def test_connections_source_type_semantic_layer_only( assert result["source_type"] == "semantic_layer" # Only one query (SemanticLayer), no Database query mock_db_session.query.assert_called_once() + + +# ============================================================================= +# SemanticViewRestApi.post (bulk create) tests +# ============================================================================= + + +@SEMANTIC_LAYERS_APP +def test_post_semantic_view_bulk_create( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """Test POST / bulk creates semantic views.""" + new_model = MagicMock() + new_model.uuid = uuid_lib.uuid4() + new_model.name = "View 1" + + mock_command = mocker.patch( + "superset.semantic_layers.api.CreateSemanticViewCommand", + ) + mock_command.return_value.run.return_value = new_model + + payload = { + "views": [ + { + "name": "View 1", + "semantic_layer_uuid": str(uuid_lib.uuid4()), + "configuration": {"database": "db1"}, + }, + ], + } + response = client.post("/api/v1/semantic_view/", json=payload) + + assert response.status_code == 201 + result = response.json["result"] + assert len(result["created"]) == 1 + assert result["created"][0]["name"] == "View 1" + + +@SEMANTIC_LAYERS_APP +def test_post_semantic_view_empty_views( + client: Any, + full_api_access: None, +) -> None: + """Test POST / returns 400 when no views provided.""" + response = client.post("/api/v1/semantic_view/", json={"views": []}) + + assert response.status_code == 400 + + +@SEMANTIC_LAYERS_APP +def test_post_semantic_view_validation_error( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """Test POST / collects validation errors for individual views.""" + # Missing required field "semantic_layer_uuid" + payload = { + "views": [ + {"name": "Bad View"}, + ], + } + response = client.post("/api/v1/semantic_view/", json=payload) + + assert response.status_code == 422 + result = response.json["result"] + assert len(result["errors"]) == 1 + assert result["errors"][0]["name"] == "Bad View" + + +@SEMANTIC_LAYERS_APP +def test_post_semantic_view_layer_not_found( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """Test POST / collects layer-not-found errors.""" + mock_command = mocker.patch( + "superset.semantic_layers.api.CreateSemanticViewCommand", + ) + mock_command.return_value.run.side_effect = SemanticLayerNotFoundError() + + payload = { + "views": [ + { + "name": "View 1", + "semantic_layer_uuid": str(uuid_lib.uuid4()), + "configuration": {}, + }, + ], + } + response = client.post("/api/v1/semantic_view/", json=payload) + + assert response.status_code == 422 + result = response.json["result"] + assert len(result["errors"]) == 1 + assert result["errors"][0]["error"] == "Semantic layer not found" + + +@SEMANTIC_LAYERS_APP +def test_post_semantic_view_create_failed( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """Test POST / collects create-failed errors.""" + mock_command = mocker.patch( + "superset.semantic_layers.api.CreateSemanticViewCommand", + ) + mock_command.return_value.run.side_effect = SemanticViewCreateFailedError() + + payload = { + "views": [ + { + "name": "View 1", + "semantic_layer_uuid": str(uuid_lib.uuid4()), + "configuration": {}, + }, + ], + } + response = client.post("/api/v1/semantic_view/", json=payload) + + assert response.status_code == 422 + result = response.json["result"] + assert len(result["errors"]) == 1 + + +@SEMANTIC_LAYERS_APP +def test_post_semantic_view_partial_success( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """Test POST / returns 201 with partial success (some created, some errors).""" + new_model = MagicMock() + new_model.uuid = uuid_lib.uuid4() + new_model.name = "Good View" + + mock_command = mocker.patch( + "superset.semantic_layers.api.CreateSemanticViewCommand", + ) + mock_command.return_value.run.side_effect = [ + new_model, + SemanticLayerNotFoundError(), + ] + + layer_uuid = str(uuid_lib.uuid4()) + payload = { + "views": [ + { + "name": "Good View", + "semantic_layer_uuid": layer_uuid, + "configuration": {}, + }, + { + "name": "Bad View", + "semantic_layer_uuid": layer_uuid, + "configuration": {}, + }, + ], + } + response = client.post("/api/v1/semantic_view/", json=payload) + + assert response.status_code == 201 + result = response.json["result"] + assert len(result["created"]) == 1 + assert len(result["errors"]) == 1 + + +# ============================================================================= +# SemanticViewRestApi.delete tests +# ============================================================================= + + +@SEMANTIC_LAYERS_APP +def test_delete_semantic_view( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """Test DELETE /<pk> deletes a semantic view.""" + mock_command = mocker.patch( + "superset.semantic_layers.api.DeleteSemanticViewCommand", + ) + mock_command.return_value.run.return_value = None + + response = client.delete("/api/v1/semantic_view/1") + + assert response.status_code == 200 + mock_command.assert_called_once_with("1") + + +@SEMANTIC_LAYERS_APP +def test_delete_semantic_view_not_found( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """Test DELETE /<pk> returns 404 when view not found.""" + mock_command = mocker.patch( + "superset.semantic_layers.api.DeleteSemanticViewCommand", + ) + mock_command.return_value.run.side_effect = SemanticViewNotFoundError() + + response = client.delete("/api/v1/semantic_view/999") + + assert response.status_code == 404 + + +@SEMANTIC_LAYERS_APP +def test_delete_semantic_view_failed( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """Test DELETE /<pk> returns 422 when deletion fails.""" + mock_command = mocker.patch( + "superset.semantic_layers.api.DeleteSemanticViewCommand", + ) + mock_command.return_value.run.side_effect = SemanticViewDeleteFailedError() + + response = client.delete("/api/v1/semantic_view/1") + + assert response.status_code == 422 + + +# ============================================================================= +# SemanticViewRestApi.bulk_delete tests +# ============================================================================= + + +@SEMANTIC_LAYERS_APP +def test_bulk_delete_semantic_view( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """Test DELETE / deletes multiple semantic views and returns a count message.""" + import prison as rison_lib + + mock_command = mocker.patch( + "superset.semantic_layers.api.BulkDeleteSemanticViewCommand", + ) + mock_command.return_value.run.return_value = None + + q = rison_lib.dumps([1, 2, 3]) + response = client.delete(f"/api/v1/semantic_view/?q={q}") + + assert response.status_code == 200 + assert "3" in response.json["message"] + mock_command.assert_called_once_with([1, 2, 3]) + + +@SEMANTIC_LAYERS_APP +def test_bulk_delete_semantic_view_not_found( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """Test DELETE / returns 404 when any id is missing.""" + import prison as rison_lib + + mock_command = mocker.patch( + "superset.semantic_layers.api.BulkDeleteSemanticViewCommand", + ) + mock_command.return_value.run.side_effect = SemanticViewNotFoundError() + + q = rison_lib.dumps([1, 999]) + response = client.delete(f"/api/v1/semantic_view/?q={q}") + + assert response.status_code == 404 + + +@SEMANTIC_LAYERS_APP +def test_bulk_delete_semantic_view_failed( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """Test DELETE / returns 422 when deletion fails.""" + import prison as rison_lib + + mock_command = mocker.patch( + "superset.semantic_layers.api.BulkDeleteSemanticViewCommand", + ) + mock_command.return_value.run.side_effect = SemanticViewDeleteFailedError() + + q = rison_lib.dumps([1, 2]) + response = client.delete(f"/api/v1/semantic_view/?q={q}") + + assert response.status_code == 422 + + +# ============================================================================= +# SemanticLayerRestApi.views tests +# ============================================================================= + + +@SEMANTIC_LAYERS_APP +def test_get_views( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """Test POST /<uuid>/views returns available views.""" + test_uuid = str(uuid_lib.uuid4()) + mock_layer = MagicMock() + mock_layer.uuid = uuid_lib.uuid4() + + mock_view1 = MagicMock() + mock_view1.name = "View A" + mock_view2 = MagicMock() + mock_view2.name = "View B" + mock_layer.implementation.get_semantic_views.return_value = [ + mock_view1, + mock_view2, + ] + + mock_dao = mocker.patch("superset.semantic_layers.api.SemanticLayerDAO") + mock_dao.find_by_uuid.return_value = mock_layer + + mock_sv_dao = mocker.patch("superset.semantic_layers.api.SemanticViewDAO") + mock_sv_dao.find_by_semantic_layer.return_value = [] + + response = client.post( + f"/api/v1/semantic_layer/{test_uuid}/views", + json={"runtime_data": {"database": "mydb"}}, + ) + + assert response.status_code == 200 + result = response.json["result"] + assert len(result) == 2 + assert result[0]["name"] == "View A" + assert result[0]["already_added"] is False + assert result[1]["name"] == "View B" + + +@SEMANTIC_LAYERS_APP +def test_get_views_with_existing( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """Test POST /<uuid>/views marks already-added views.""" + test_uuid = str(uuid_lib.uuid4()) + mock_layer = MagicMock() + mock_layer.uuid = uuid_lib.uuid4() + + mock_view = MagicMock() + mock_view.name = "Existing View" + mock_layer.implementation.get_semantic_views.return_value = [mock_view] + + mock_dao = mocker.patch("superset.semantic_layers.api.SemanticLayerDAO") + mock_dao.find_by_uuid.return_value = mock_layer + + existing_view = MagicMock() + existing_view.name = "Existing View" + existing_view.configuration = '{"database": "mydb"}' + + mock_sv_dao = mocker.patch("superset.semantic_layers.api.SemanticViewDAO") + mock_sv_dao.find_by_semantic_layer.return_value = [existing_view] + + response = client.post( + f"/api/v1/semantic_layer/{test_uuid}/views", + json={"runtime_data": {"database": "mydb"}}, + ) + + assert response.status_code == 200 + result = response.json["result"] + assert len(result) == 1 + assert result[0]["name"] == "Existing View" + assert result[0]["already_added"] is True + + +@SEMANTIC_LAYERS_APP +def test_get_views_not_found( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """Test POST /<uuid>/views returns 404 when layer not found.""" + mock_dao = mocker.patch("superset.semantic_layers.api.SemanticLayerDAO") + mock_dao.find_by_uuid.return_value = None + + response = client.post( + f"/api/v1/semantic_layer/{uuid_lib.uuid4()}/views", + json={}, + ) + + assert response.status_code == 404 + + +@SEMANTIC_LAYERS_APP +def test_get_views_exception( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """Test POST /<uuid>/views returns 400 when implementation raises.""" + test_uuid = str(uuid_lib.uuid4()) + mock_layer = MagicMock() + mock_layer.uuid = uuid_lib.uuid4() + mock_layer.implementation.get_semantic_views.side_effect = ValueError( + "Connection failed" + ) + + mock_dao = mocker.patch("superset.semantic_layers.api.SemanticLayerDAO") + mock_dao.find_by_uuid.return_value = mock_layer + + response = client.post( + f"/api/v1/semantic_layer/{test_uuid}/views", + json={"runtime_data": {}}, + ) Review Comment: **Suggestion:** This assertion expects the raw exception text to be returned, but the endpoint intentionally hides internal errors and returns a generic user-facing message. The current check will fail even though the API behavior is correct. Assert the documented generic message instead. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Unit test for `/semantic_layer/<uuid>/views` fails consistently. - ⚠️ CI for semantic_layers API changes can be blocked by this test. - ⚠️ Test contradicts documented generic error handling in `views` endpoint. ``` </details> ```suggestion assert "Unable to fetch semantic views" in response.json["message"] ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the unit test `test_get_views_exception` in `tests/unit_tests/semantic_layers/api_test.py` (around lines 1887–1909), which calls `client.post(f"/api/v1/semantic_layer/{test_uuid}/views", json={"runtime_data": {}})`. 2. Inside this test, `SemanticLayerDAO.find_by_uuid` is patched to return a `MagicMock` layer whose `implementation.get_semantic_views` is configured with `side_effect = ValueError("Connection failed")`, forcing an exception when the view list is fetched. 3. The POST request is served by `SemanticLayerRestApi.views` in `superset/semantic_layers/api.py` (lines 165–215): in the `except Exception as ex` block at lines 202–215, the code logs the internal error but returns `self.response_400(message=t("Unable to fetch semantic views. Check the layer configuration."))`, i.e. a generic, user-facing message, not the raw `"Connection failed"` text. 4. Back in the test (`tests/unit_tests/semantic_layers/api_test.py`, line 1907), the assertion `assert "Connection failed" in response.json["message"]` executes; since `response.json["message"]` contains `"Unable to fetch semantic views. Check the layer configuration."`, the substring check fails and the test raises `AssertionError` even though the API behavior matches the implementation in `superset/semantic_layers/api.py`. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/semantic_layers/api_test.py **Line:** 1907:1907 **Comment:** *Logic Error: This assertion expects the raw exception text to be returned, but the endpoint intentionally hides internal errors and returns a generic user-facing message. The current check will fail even though the API behavior is correct. Assert the documented generic message instead. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38396&comment_hash=d9e47aaaa7c04b2bed49851e14d129778fd4a4f202bcca638206afe4675231dc&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38396&comment_hash=d9e47aaaa7c04b2bed49851e14d129778fd4a4f202bcca638206afe4675231dc&reaction=dislike'>👎</a> ########## superset-frontend/src/features/semanticViews/AddSemanticViewModal.tsx: ########## @@ -0,0 +1,512 @@ +/** + * 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 { 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 { Spin } from 'antd'; +import { Select } from '@superset-ui/core/components'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { JsonForms } from '@jsonforms/react'; +import type { JsonSchema, UISchemaElement } from '@jsonforms/core'; +import { cellRegistryEntries } from '@great-expectations/jsonforms-antd-renderers'; +import type { ErrorObject } from 'ajv'; +import { + StandardModal, + ModalFormField, + MODAL_STANDARD_WIDTH, +} from 'src/components/Modal'; +import { + renderers, + sanitizeSchema, + buildUiSchema, + getDynamicDependencies, + areDependenciesSatisfied, + serializeDependencyValues, + SCHEMA_REFRESH_DEBOUNCE_MS, +} from 'src/features/semanticLayers/jsonFormsHelpers'; + +interface SemanticLayerOption { + uuid: string; + name: string; +} + +interface AvailableView { + name: string; + already_added: boolean; +} + +const ModalContent = styled.div` + padding: ${({ theme }) => theme.sizeUnit * 4}px; +`; + +const LoadingContainer = styled.div` + display: flex; + justify-content: center; + padding: ${({ theme }) => theme.sizeUnit * 4}px; +`; + +const SectionLabel = styled.div` + color: ${({ theme }) => theme.colorText}; + font-size: ${({ theme }) => theme.fontSize}px; + margin-top: ${({ theme }) => theme.sizeUnit}px; + margin-bottom: ${({ theme }) => theme.sizeUnit * 2}px; +`; + +const VerticalFormFields = styled.div` + margin-bottom: ${({ theme }) => theme.sizeUnit * 4}px; + + /* The antd renderer's VerticalLayout creates its own <Form> — + force flex-column so gap controls spacing between fields */ + && form { + display: flex; + flex-direction: column; + gap: ${({ theme }) => theme.sizeUnit * 4}px; + } + + /* Reset antd default margins so gap controls all spacing */ + && .ant-form-item { + margin-bottom: 0; + } + + /* Override ant-form-item-horizontal: stack label above control */ + && .ant-form-item-row { + flex-direction: column; + align-items: stretch; + } + + && .ant-form-item-label { + text-align: left; + max-width: 100%; + flex: none; + padding-bottom: ${({ theme }) => theme.sizeUnit}px; + } + + && .ant-form-item-control { + max-width: 100%; + flex: auto; + } + + && .ant-form-item-label > label { + color: ${({ theme }) => theme.colorText}; + font-size: ${({ theme }) => theme.fontSize}px; + } +`; + +interface AddSemanticViewModalProps { + show: boolean; + onHide: () => void; + onSuccess: () => void; + addDangerToast: (msg: string) => void; + addSuccessToast: (msg: string) => void; +} + +export default function AddSemanticViewModal({ + show, + onHide, + onSuccess, + addDangerToast, + addSuccessToast, +}: AddSemanticViewModalProps) { + // --- Layer --- + const [layers, setLayers] = useState<SemanticLayerOption[]>([]); + const [selectedLayerUuid, setSelectedLayerUuid] = useState<string | null>( + null, + ); + const [loadingLayers, setLoadingLayers] = useState(false); + + // --- Runtime config --- + const [runtimeSchema, setRuntimeSchema] = useState<JsonSchema | null>(null); + const [runtimeUiSchema, setRuntimeUiSchema] = useState< + UISchemaElement | undefined + >(); + const [runtimeData, setRuntimeData] = useState<Record<string, unknown>>({}); + const [loadingRuntime, setLoadingRuntime] = useState(false); + const [refreshingSchema, setRefreshingSchema] = useState(false); + const errorsRef = useRef<ErrorObject[]>([]); + const dynamicDepsRef = useRef<Record<string, string[]>>({}); + const lastDepSnapshotRef = useRef(''); + const schemaTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); + + // --- Views --- + const [availableViews, setAvailableViews] = useState<AvailableView[]>([]); + const [selectedViewNames, setSelectedViewNames] = useState<string[]>([]); + const [loadingViews, setLoadingViews] = useState(false); + const viewsTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); + const lastViewsKeyRef = useRef(''); + + // --- Misc --- + const [saving, setSaving] = useState(false); + const fetchGenRef = useRef(0); + + // ========================================================================= + // Fetch helpers + // ========================================================================= + + const fetchLayers = async () => { + setLoadingLayers(true); + try { + const { json } = await SupersetClient.get({ + endpoint: '/api/v1/semantic_layer/', + }); + setLayers( + (json.result ?? []).map((l: { uuid: string; name: string }) => ({ + uuid: l.uuid, + name: l.name, + })), + ); + } catch { + addDangerToast(t('An error occurred while fetching semantic layers')); + } finally { + setLoadingLayers(false); + } + }; + + const fetchViews = useCallback( + async (uuid: string, rData: Record<string, unknown>, gen: number) => { + setLoadingViews(true); + setAvailableViews([]); + setSelectedViewNames([]); + try { + const { json } = await SupersetClient.post({ + endpoint: `/api/v1/semantic_layer/${uuid}/views`, + jsonPayload: { runtime_data: rData }, + }); + if (gen !== fetchGenRef.current) return; + setAvailableViews(json.result ?? []); + } catch { + if (gen !== fetchGenRef.current) return; + addDangerToast(t('An error occurred while fetching available views')); + } finally { + if (gen === fetchGenRef.current) setLoadingViews(false); + } + }, + [addDangerToast], + ); + + const applyRuntimeSchema = useCallback((rawSchema: JsonSchema) => { + const schema = sanitizeSchema(rawSchema); + setRuntimeSchema(schema); + setRuntimeUiSchema(buildUiSchema(schema)); + dynamicDepsRef.current = getDynamicDependencies(rawSchema); + }, []); + + const scheduleFetchViews = useCallback( + (uuid: string, data: Record<string, unknown>) => { + const key = JSON.stringify(data); + if (key === lastViewsKeyRef.current) return; + lastViewsKeyRef.current = key; + if (viewsTimerRef.current) clearTimeout(viewsTimerRef.current); + viewsTimerRef.current = setTimeout(() => { + fetchViews(uuid, data, fetchGenRef.current); + }, SCHEMA_REFRESH_DEBOUNCE_MS); + }, + [fetchViews], + ); + + // ========================================================================= + // Layer change — fetch runtime schema, clear downstream state + // ========================================================================= + + const handleLayerChange = useCallback( + async (uuid: string) => { + fetchGenRef.current += 1; + const gen = fetchGenRef.current; + + setSelectedLayerUuid(uuid); + if (schemaTimerRef.current) clearTimeout(schemaTimerRef.current); + if (viewsTimerRef.current) clearTimeout(viewsTimerRef.current); + setRuntimeSchema(null); + setRuntimeUiSchema(undefined); + setRuntimeData({}); + errorsRef.current = []; + dynamicDepsRef.current = {}; + lastDepSnapshotRef.current = ''; + setAvailableViews([]); + setSelectedViewNames([]); + lastViewsKeyRef.current = ''; + + setLoadingRuntime(true); + try { + const { json } = await SupersetClient.post({ + endpoint: `/api/v1/semantic_layer/${uuid}/schema/runtime`, + jsonPayload: {}, + }); + if (gen !== fetchGenRef.current) return; + const schema = json.result; + if ( + !schema?.properties || + Object.keys(schema.properties).length === 0 + ) { + // No runtime config needed — fetch views right away + fetchViews(uuid, {}, gen); + } else { + applyRuntimeSchema(schema); + } + } catch { + if (gen !== fetchGenRef.current) return; + addDangerToast( + t('An error occurred while fetching the runtime schema'), + ); + } finally { + if (gen === fetchGenRef.current) setLoadingRuntime(false); + } + }, + [applyRuntimeSchema, fetchViews, addDangerToast], + ); + + // ========================================================================= + // Runtime form change — refresh dynamic fields or auto-fetch views + // ========================================================================= + + const handleRuntimeFormChange = useCallback( + ({ + data, + errors, + }: { + data: Record<string, unknown>; + errors?: ErrorObject[]; + }) => { + setRuntimeData(data); + errorsRef.current = errors ?? []; + + if (!selectedLayerUuid) return; + const gen = fetchGenRef.current; + + // Dynamic deps changed → refresh schema (e.g. database → schema) + const dynamicDeps = dynamicDepsRef.current; + if (Object.keys(dynamicDeps).length > 0) { + const hasSatisfiedDeps = Object.values(dynamicDeps).some(deps => + areDependenciesSatisfied(deps, data), + ); + if (hasSatisfiedDeps) { + const snapshot = serializeDependencyValues(dynamicDeps, data); + if (snapshot !== lastDepSnapshotRef.current) { + lastDepSnapshotRef.current = snapshot; + // Config is changing — clear views + setAvailableViews([]); + setSelectedViewNames([]); + lastViewsKeyRef.current = ''; + if (schemaTimerRef.current) clearTimeout(schemaTimerRef.current); + const uuid = selectedLayerUuid; + schemaTimerRef.current = setTimeout(async () => { + setRefreshingSchema(true); + try { + const { json } = await SupersetClient.post({ + endpoint: `/api/v1/semantic_layer/${uuid}/schema/runtime`, + jsonPayload: { runtime_data: data }, + }); + if (gen !== fetchGenRef.current) return; + applyRuntimeSchema(json.result); + } catch { + // Silent fail on refresh — form still works + } finally { + if (gen === fetchGenRef.current) setRefreshingSchema(false); + } + }, SCHEMA_REFRESH_DEBOUNCE_MS); + return; + } + } + } + + // No schema refresh needed — fetch views if form is valid + if (errorsRef.current.length === 0) { + scheduleFetchViews(selectedLayerUuid, data); + } + }, + [selectedLayerUuid, applyRuntimeSchema, scheduleFetchViews], + ); + + // After a schema refresh settles, JSON Forms re-validates and fires + // onChange → handleRuntimeFormChange handles view fetching. As a fallback + // (in case onChange doesn't fire), try once refreshingSchema flips false. + const prevRefreshingRef = useRef(false); + useEffect(() => { + if (prevRefreshingRef.current && !refreshingSchema && selectedLayerUuid) { + const timer = setTimeout(() => { + if (errorsRef.current.length === 0) { + scheduleFetchViews(selectedLayerUuid, runtimeData); + } + }, 100); + prevRefreshingRef.current = false; + return () => clearTimeout(timer); + } + prevRefreshingRef.current = refreshingSchema; + return undefined; + }, [refreshingSchema, selectedLayerUuid, runtimeData, scheduleFetchViews]); + + // ========================================================================= + // Modal open / close + // ========================================================================= + + useEffect(() => { + if (show) { + fetchLayers(); + } else { + fetchGenRef.current += 1; + if (schemaTimerRef.current) clearTimeout(schemaTimerRef.current); + if (viewsTimerRef.current) clearTimeout(viewsTimerRef.current); + setLayers([]); + setSelectedLayerUuid(null); + setLoadingLayers(false); + setRuntimeSchema(null); + setRuntimeUiSchema(undefined); + setRuntimeData({}); + setLoadingRuntime(false); + setRefreshingSchema(false); + errorsRef.current = []; + dynamicDepsRef.current = {}; + lastDepSnapshotRef.current = ''; + setAvailableViews([]); + setSelectedViewNames([]); + setLoadingViews(false); + setSaving(false); + lastViewsKeyRef.current = ''; + } + }, [show]); // eslint-disable-line react-hooks/exhaustive-deps + + // ========================================================================= + // Save + // ========================================================================= + + const newViewCount = availableViews.filter( + v => selectedViewNames.includes(v.name) && !v.already_added, + ).length; + + const handleSave = async () => { + if (!selectedLayerUuid || newViewCount === 0) return; + setSaving(true); + try { + const viewsToCreate = availableViews + .filter(v => selectedViewNames.includes(v.name) && !v.already_added) + .map(v => ({ + name: v.name, + semantic_layer_uuid: selectedLayerUuid, + configuration: runtimeData, + })); + + await SupersetClient.post({ + endpoint: '/api/v1/semantic_view/', + jsonPayload: { views: viewsToCreate }, + }); + + addSuccessToast(t('%s semantic view(s) added', viewsToCreate.length)); + onSuccess(); + onHide(); + } catch { + addDangerToast(t('An error occurred while adding semantic views')); + } finally { + setSaving(false); + } + }; + + // ========================================================================= + // Render + // ========================================================================= + + const hasRuntimeFields = + runtimeSchema?.properties && + Object.keys(runtimeSchema.properties).length > 0; + + const viewsDisabled = + loadingViews || (!loadingViews && availableViews.length === 0); + + return ( + <StandardModal + show={show} + onHide={onHide} + onSave={handleSave} + title={t('Add Semantic View')} + icon={<Icons.PlusOutlined />} + width={MODAL_STANDARD_WIDTH} + saveDisabled={newViewCount === 0 || saving} Review Comment: **Suggestion:** Save can remain enabled while runtime form validation has errors, allowing submission with stale/invalid runtime configuration and previously selected views. Disable saving when runtime validation errors exist (and while schema is refreshing) so invalid configs cannot be submitted. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Add Semantic View modal saves with invalid runtime configuration. - ⚠️ Created semantic views may error when used in queries. ``` </details> ```suggestion saveDisabled={ newViewCount === 0 || saving || errorsRef.current.length > 0 || refreshingSchema } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the datasets list page and click **New → Semantic View** (dropdown configured in `superset-frontend/src/pages/DatasetList/index.tsx:900-957`), which renders `AddSemanticViewModal` with `show` set to `true` (`index.tsx:1196-1212`). 2. In `AddSemanticViewModal`, choose a semantic layer and fill the runtime form until it is valid; `JsonForms` (`AddSemanticViewModal.tsx:471-479`) calls `onChange`, and `handleRuntimeFormChange` (`AddSemanticViewModal.tsx:285-332`) sets `runtimeData`, clears `errorsRef.current`, and because `errorsRef.current.length === 0` triggers `scheduleFetchViews`, which populates `availableViews` via `fetchViews` (`AddSemanticViewModal.tsx:180-201`). 3. Select one or more semantic views from the multi-select; `selectedViewNames` is updated and `newViewCount` at `AddSemanticViewModal.tsx:388-390` becomes > 0, so `StandardModal` is rendered with `saveDisabled={newViewCount === 0 || saving}` at line 438, enabling the **Add** button while `errorsRef.current` is not consulted anywhere in the save logic. 4. Now edit the runtime form to an invalid state (e.g., clear a required field that does not change the dynamic dependencies); `handleRuntimeFormChange` updates `runtimeData` and sets `errorsRef.current` to a non-empty list but, because no dependency snapshot change is detected, does not clear `availableViews` or `selectedViewNames`, so `newViewCount` remains > 0 and the **Add** button stays enabled (line 438), allowing `handleSave` (`AddSemanticViewModal.tsx:392-407`) to POST the invalid `runtimeData` as `configuration` to `/api/v1/semantic_view/`, where `CreateSemanticViewCommand.validate` in `superset/commands/semantic_layer/create.py:93-104` performs no configuration validation, resulting in a saved semantic view with a runtime configuration that the client-side schema flags as invalid. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/features/semanticViews/AddSemanticViewModal.tsx **Line:** 438:438 **Comment:** *Possible Bug: Save can remain enabled while runtime form validation has errors, allowing submission with stale/invalid runtime configuration and previously selected views. Disable saving when runtime validation errors exist (and while schema is refreshing) so invalid configs cannot be submitted. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38396&comment_hash=5d562cdaf5bdb4b0425b769f48d1318ffe18f3df3efd37771644ebd1fcc6f305&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38396&comment_hash=5d562cdaf5bdb4b0425b769f48d1318ffe18f3df3efd37771644ebd1fcc6f305&reaction=dislike'>👎</a> -- 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]
