codeant-ai-for-open-source[bot] commented on code in PR #41753: URL: https://github.com/apache/superset/pull/41753#discussion_r3546616317
########## tests/unit_tests/charts/test_filters.py: ########## @@ -0,0 +1,119 @@ +# 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. + +"""Tests for the ``ChartFilter`` embedded-guest scoping branch.""" + +from typing import Any +from unittest.mock import MagicMock + +import pytest +from pytest_mock import MockerFixture +from sqlalchemy import create_engine + + +def test_chart_filter_scopes_guest_to_token_dashboards(mocker: MockerFixture) -> None: + """A guest's chart list is scoped to the dashboards in its token.""" + from superset.charts.filters import ChartFilter + from superset.extensions import security_manager + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice + + mocker.patch.object( + security_manager, "can_access_all_datasources", return_value=False + ) + mocker.patch( + "superset.charts.filters.guest_embedded_dashboard_filter", + return_value=Dashboard.id.in_([1, 2]), + ) + + captured: dict[str, Any] = {} + query: MagicMock = MagicMock() + + def _capture_filter(clause: object) -> MagicMock: + captured["clause"] = clause + return query + + query.filter.side_effect = _capture_filter + + filt: ChartFilter = ChartFilter.__new__(ChartFilter) + filt.model = Slice + result = filt.apply(query, None) Review Comment: **Suggestion:** Add an explicit type annotation for this local variable to keep new test code fully type-hinted. [custom_rule] **Severity Level:** Minor β οΈ <details> <summary><b>Why it matters? π€ </b></summary> This new local variable is assigned without a type annotation even though its type is clear from the call result, so it matches the rule requiring type hints on relevant variables in new Python code. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=965aa4240a0a428aab5d0ac7d1e73c7a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=965aa4240a0a428aab5d0ac7d1e73c7a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/charts/test_filters.py **Line:** 54:54 **Comment:** *Custom Rule: Add an explicit type annotation for this local variable to keep new test code fully type-hinted. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=0e87cac2acbc57aabbcf0e6842374a86f018498f42ab72587e6995dad0345a58&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=0e87cac2acbc57aabbcf0e6842374a86f018498f42ab72587e6995dad0345a58&reaction=dislike'>π</a> ########## superset/utils/filters.py: ########## @@ -41,3 +41,43 @@ def get_dataset_access_filters( base_model.schema_perm.in_(schema_perms), *args, ) + + +def guest_embedded_dashboard_filter() -> Optional[BooleanClauseList]: + """SQLAlchemy condition matching dashboards embedded via the current guest + token's resources, or None when the user is not an embedded guest. + + Used by the chart access filter to scope an embedded guest to its token's + dashboards (and their charts), mirroring how DashboardAccessFilter already + scopes dashboards. + """ + # pylint: disable=import-outside-toplevel + from superset import is_feature_enabled, security_manager + from superset.models.dashboard import Dashboard, is_uuid + from superset.models.embedded_dashboard import EmbeddedDashboard + from superset.security.guest_token import GuestTokenResourceType + + if not is_feature_enabled("EMBEDDED_SUPERSET"): + return None + guest = security_manager.get_current_guest_user_if_guest() + if guest is None: + return None + ids: list[Any] = [ + r["id"] + for r in guest.resources + if r["type"] == GuestTokenResourceType.DASHBOARD.value + ] + if not ids: + return None + # TODO (embedded): only use the uuid filter once uuids are rolled out + # A guest token may mix uuid and int dashboard ids during the uuid rollout. + # Route each id kind to its own column and OR them β a plain int sent to the + # uuid-typed column would raise a bind/type error. + uuid_ids = [id_ for id_ in ids if is_uuid(id_)] + int_ids = [id_ for id_ in ids if not is_uuid(id_)] Review Comment: **Suggestion:** Add explicit type annotations for the newly introduced local lists that separate UUID and integer dashboard identifiers. [custom_rule] **Severity Level:** Minor β οΈ <details> <summary><b>Why it matters? π€ </b></summary> The new local variables `uuid_ids` and `int_ids` are list-valued and can be explicitly annotated (for example, as `list[Any]` or a more specific list type). Since the custom rule requires type hints for new or modified Python code where relevant variables can be annotated, omitting annotations here is a real violation. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d50a25f5ce27490e9a551461de3fd391&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d50a25f5ce27490e9a551461de3fd391&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/utils/filters.py **Line:** 76:77 **Comment:** *Custom Rule: Add explicit type annotations for the newly introduced local lists that separate UUID and integer dashboard identifiers. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=d2cef9ab17f7e277fb41cd595db93a126a6781ce05eb45e761d376574c9f364b&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=d2cef9ab17f7e277fb41cd595db93a126a6781ce05eb45e761d376574c9f364b&reaction=dislike'>π</a> ########## tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py: ########## @@ -1032,6 +1032,24 @@ async def test_chart_not_found(self, mock_find, mock_validate, mcp_server): assert data["error_type"] == "NotFound" assert "999" in data["error"] + @pytest.mark.asyncio + async def test_guest_denied(self, mcp_server): Review Comment: **Suggestion:** Add explicit type hints to this async test methodβs parameters and return type. [custom_rule] **Severity Level:** Minor β οΈ <details> <summary><b>Why it matters? π€ </b></summary> This is a newly added Python test method with no type hints on its parameters or return type. The rule requires new or modified Python code to include type hints where applicable, so this is a real violation. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=68a8827ea43c4a8aa75bdd900750012b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=68a8827ea43c4a8aa75bdd900750012b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py **Line:** 1036:1036 **Comment:** *Custom Rule: Add explicit type hints to this async test methodβs parameters and return type. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=1792f9ab7559d2fe3cbbdbebc711311d236d4a86d489979da3ca2d584feebcb0&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=1792f9ab7559d2fe3cbbdbebc711311d236d4a86d489979da3ca2d584feebcb0&reaction=dislike'>π</a> ########## superset/charts/filters.py: ########## @@ -111,6 +114,10 @@ def apply(self, query: Query, value: Any) -> Query: if security_manager.can_access_all_datasources(): return query + # Embedded guests: scope to charts on the dashboards in their token. + if (guest_dashboards := guest_embedded_dashboard_filter()) is not None: Review Comment: **Suggestion:** Introduce an explicitly typed local variable for the embedded guest dashboard predicate instead of using an unannotated walrus assignment in the condition. [custom_rule] **Severity Level:** Minor β οΈ <details> <summary><b>Why it matters? π€ </b></summary> The new local variable `guest_dashboards` is introduced without an explicit type hint, and it is a candidate for annotation under the Python type-hint rule. This is a real omission in newly added code, so the suggestion matches the custom rule. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f34794f71bdf4d0a8c513f140e34315c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f34794f71bdf4d0a8c513f140e34315c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/charts/filters.py **Line:** 118:118 **Comment:** *Custom Rule: Introduce an explicitly typed local variable for the embedded guest dashboard predicate instead of using an unannotated walrus assignment in the condition. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=0f584e84d4210bc057b11f15075d27fb5324a80c7eee51828e0436b3b314f816&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=0f584e84d4210bc057b11f15075d27fb5324a80c7eee51828e0436b3b314f816&reaction=dislike'>π</a> ########## superset/mcp_service/chart/tool/get_chart_data.py: ########## @@ -508,10 +526,11 @@ async def get_chart_data( # noqa: C901 from superset.common.query_context_factory import QueryContextFactory factory = QueryContextFactory() - row_limit = ( - request.limit - or form_data.get("row_limit") - or current_app.config["ROW_LIMIT"] + # row_limit from chart.params may be a str; coerce for + # apply_max_row_limit's int comparison. + row_limit = _coerce_row_limit( + request.limit or form_data.get("row_limit"), + current_app.config["ROW_LIMIT"], ) Review Comment: **Suggestion:** Add an explicit `int` type annotation to this newly introduced `row_limit` local variable assignment. [custom_rule] **Severity Level:** Minor β οΈ <details> <summary><b>Why it matters? π€ </b></summary> The new local variable is assigned without an explicit type annotation even though its type is known and can be annotated. This is a real instance of the Python type-hinting rule violation for newly modified code. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8e9a7cc575154fe69f1f9c370ba8d693&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8e9a7cc575154fe69f1f9c370ba8d693&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/chart/tool/get_chart_data.py **Line:** 531:534 **Comment:** *Custom Rule: Add an explicit `int` type annotation to this newly introduced `row_limit` local variable assignment. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=4e4ea0b4444741d96832f4f494c18e4d6a67ad7dc3afba8763dbc39ec855dba9&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=4e4ea0b4444741d96832f4f494c18e4d6a67ad7dc3afba8763dbc39ec855dba9&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]
