codeant-ai-for-open-source[bot] commented on code in PR #38362: URL: https://github.com/apache/superset/pull/38362#discussion_r2879238107
########## tests/unit_tests/semantic_layers/api_test.py: ########## @@ -0,0 +1,244 @@ +# 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. + +from typing import Any +from unittest.mock import MagicMock + +from pytest_mock import MockerFixture + +from superset.commands.semantic_layer.exceptions import ( + SemanticViewForbiddenError, + SemanticViewInvalidError, + SemanticViewNotFoundError, + SemanticViewUpdateFailedError, +) + + +def test_put_semantic_view( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """Test successful PUT updates a semantic view.""" + changed_model = MagicMock() + changed_model.id = 1 + + mock_command = mocker.patch( + "superset.semantic_layers.api.UpdateSemanticViewCommand", + ) + mock_command.return_value.run.return_value = changed_model + + payload = {"description": "Updated description", "cache_timeout": 300} + response = client.put( + "/api/v1/semantic_view/1", + json=payload, + ) + + assert response.status_code == 200 + assert response.json["id"] == 1 + assert response.json["result"] == payload + mock_command.assert_called_once_with("1", payload) Review Comment: **Suggestion:** The test for the semantic view PUT endpoint asserts that the command is called with a string ID, but the API implementation defines the path parameter as an integer and passes an int to the command; this type mismatch will cause the assertion to fail even when the API behaves correctly. [type error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Semantic view PUT API unit test always fails in CI. - ⚠️ Mis-specified expectation may hide real regressions in ID handling. ``` </details> ```suggestion mock_command.assert_called_once_with(1, payload) ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Start the test suite which includes `tests/unit_tests/semantic_layers/api_test.py` (see file at `/workspace/superset/tests/unit_tests/semantic_layers/api_test.py`). 2. Test `test_put_semantic_view` (lines 31–54 in that file) issues a PUT request to `/api/v1/semantic_view/1` via the Flask test client, which is routed to `SemanticViewRestApi.put` in `/workspace/superset/superset/semantic_layers/api.py` line 65. 3. In `SemanticViewRestApi.put(self, pk: int)` (line 65), Flask/FAB provides `pk` as an `int` (per the type annotation and OpenAPI schema), and the implementation calls `UpdateSemanticViewCommand(pk, item)` at line 112, so the patched `UpdateSemanticViewCommand` mock is invoked with `1` (integer) as the first argument. 4. After the request, the test asserts `mock_command.assert_called_once_with("1", payload)` at line 54, expecting a string `"1"`, which does not match the actual integer `1` argument, causing the test to fail with an assertion error even though the API behaves correctly. ``` </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:** 54:54 **Comment:** *Type Error: The test for the semantic view PUT endpoint asserts that the command is called with a string ID, but the API implementation defines the path parameter as an integer and passes an int to the command; this type mismatch will cause the assertion to fail even when the API behaves correctly. 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%2F38362&comment_hash=64b75d6e331b161e25cecb0792ec228fdf41c2789880f1f2a6878a18c7d0e473&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38362&comment_hash=64b75d6e331b161e25cecb0792ec228fdf41c2789880f1f2a6878a18c7d0e473&reaction=dislike'>👎</a> ########## superset/initialization/__init__.py: ########## @@ -266,6 +267,7 @@ def init_views(self) -> None: appbuilder.add_api(ReportExecutionLogRestApi) appbuilder.add_api(RLSRestApi) appbuilder.add_api(SavedQueryRestApi) + appbuilder.add_api(SemanticViewRestApi) Review Comment: **Suggestion:** The new semantic view API is registered unconditionally, ignoring the `SEMANTIC_LAYERS` feature flag declared as required in the PR description; this means that even when the feature flag is disabled, clients can still access the editing endpoint, which is inconsistent with the intended gating of this feature and can lead to unexpected or partially enabled behavior. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Semantic view edit endpoint always enabled at runtime. - ⚠️ Backend feature gating diverges from configuration expectations. - ⚠️ Experimental semantic editing accessible via REST API directly. ``` </details> ```suggestion if feature_flag_manager.is_feature_enabled("SEMANTIC_LAYERS"): appbuilder.add_api(SemanticViewRestApi) ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Start Superset with this PR code so that `SupersetAppInitializer.init_app()` in `superset/initialization/__init__.py:726` runs during application startup. 2. During initialization, `init_app_in_ctx()` at `superset/initialization/__init__.py:600` is invoked, which in turn calls `init_views()` in the same file. 3. Inside `init_views()`, `SemanticViewRestApi` is imported unconditionally at `superset/initialization/__init__.py:186` (`from superset.semantic_layers.api import SemanticViewRestApi`) and then registered unconditionally with `appbuilder.add_api(SemanticViewRestApi)` at `superset/initialization/__init__.py:270`, with no surrounding check for any feature flag. 4. With the server running, an authenticated client can issue a `PUT` request to the semantic view editing endpoint (implemented by `SemanticViewRestApi.put` in `superset/semantic_layers/api.py:57-65`, with `resource_name = "semantic_view"` at line 49) — e.g. `PUT /api/v1/semantic_view/<pk>` — and the request will be accepted and processed by `UpdateSemanticViewCommand`, regardless of any `SEMANTIC_LAYERS` feature flag setting in configuration, because no feature-flag check protects the API registration path. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/initialization/__init__.py **Line:** 270:270 **Comment:** *Logic Error: The new semantic view API is registered unconditionally, ignoring the `SEMANTIC_LAYERS` feature flag declared as required in the PR description; this means that even when the feature flag is disabled, clients can still access the editing endpoint, which is inconsistent with the intended gating of this feature and can lead to unexpected or partially enabled behavior. 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%2F38362&comment_hash=abb4e94aa80d7d1461e11a6630421b7fac0819549007273975b8d7b74496f4b6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38362&comment_hash=abb4e94aa80d7d1461e11a6630421b7fac0819549007273975b8d7b74496f4b6&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]
