codeant-ai-for-open-source[bot] commented on code in PR #37395: URL: https://github.com/apache/superset/pull/37395#discussion_r2722629606
########## tests/unit_tests/models/test_double_rls_virtual_dataset.py: ########## @@ -0,0 +1,286 @@ +# 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 double RLS application in virtual datasets (Issue #37359). + +This module tests that guest user RLS filters are applied only once +when querying virtual datasets, not both in the underlying table SQL +and the outer query. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest +from flask import Flask +from sqlalchemy.sql.elements import TextClause + +from superset.connectors.sqla.models import BaseDatasource + + [email protected] +def mock_datasource() -> MagicMock: + """Create a mock datasource for testing.""" + datasource = MagicMock(spec=BaseDatasource) + datasource.get_template_processor.return_value = MagicMock() + datasource.get_template_processor.return_value.process_template = lambda x: x + datasource.text = lambda x: TextClause(x) + return datasource + + +def test_rls_filters_include_guest_when_enabled( + mock_datasource: MagicMock, + app: Flask, +) -> None: + """ + Test that RLS filters include guest filters when enabled. + + When include_guest_rls=True and EMBEDDED_SUPERSET is enabled, + both regular and guest RLS filters should be returned. + """ + regular_filter = MagicMock() + regular_filter.clause = "col1 = 'value1'" + regular_filter.group_key = None + + guest_rule = {"clause": "col2 = 'value2'"} + + with ( + patch( + "superset.connectors.sqla.models.security_manager.get_rls_filters", + return_value=[regular_filter], + ), + patch( + "superset.connectors.sqla.models.security_manager.get_guest_rls_filters", + return_value=[guest_rule], + ), + patch( + "superset.connectors.sqla.models.is_feature_enabled", + return_value=True, + ), Review Comment: **Suggestion:** The tests use "with (patch(...), patch(...), patch(...),):" which constructs a tuple of context managers rather than chaining them; a tuple is not a valid context manager and will raise a TypeError at runtime. Replace the grouped tuple form with chained context managers separated by commas (or use nested with/contextlib.ExitStack) so each patch is used as a context manager. [runtime error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Unit tests fail in CI for RLS virtual dataset tests. - ⚠️ Blocks PR merge until tests are fixed. ``` </details> ```suggestion with patch( "superset.connectors.sqla.models.security_manager.get_rls_filters", return_value=[regular_filter], ), patch( "superset.connectors.sqla.models.security_manager.get_guest_rls_filters", return_value=[guest_rule], ), patch( "superset.connectors.sqla.models.is_feature_enabled", return_value=True, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run pytest to execute unit tests in tests/unit_tests/models/test_double_rls_virtual_dataset.py. 2. Pytest imports the module and executes test_rls_filters_include_guest_when_enabled (defined at tests/unit_tests/models/test_double_rls_virtual_dataset.py:46). 3. During the test, execution reaches the with-statement at lines 62-75 which uses a tuple of patch() calls: the code executes the expression producing a tuple rather than chaining context managers (see file: tests/unit_tests/models/test_double_rls_virtual_dataset.py lines 62-75). 4. Python attempts to use the tuple as a context manager and raises TypeError (e.g., "TypeError: __enter__"), causing the test to error out immediately instead of applying patches and exercising BaseDatasource._get_sqla_row_level_filters_internal. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/models/test_double_rls_virtual_dataset.py **Line:** 62:74 **Comment:** *Runtime Error: The tests use "with (patch(...), patch(...), patch(...),):" which constructs a tuple of context managers rather than chaining them; a tuple is not a valid context manager and will raise a TypeError at runtime. Replace the grouped tuple form with chained context managers separated by commas (or use nested with/contextlib.ExitStack) so each patch is used as a context manager. 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> ########## tests/unit_tests/models/test_double_rls_virtual_dataset.py: ########## @@ -0,0 +1,286 @@ +# 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 double RLS application in virtual datasets (Issue #37359). + +This module tests that guest user RLS filters are applied only once +when querying virtual datasets, not both in the underlying table SQL +and the outer query. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest +from flask import Flask +from sqlalchemy.sql.elements import TextClause + +from superset.connectors.sqla.models import BaseDatasource + + [email protected] +def mock_datasource() -> MagicMock: + """Create a mock datasource for testing.""" + datasource = MagicMock(spec=BaseDatasource) + datasource.get_template_processor.return_value = MagicMock() + datasource.get_template_processor.return_value.process_template = lambda x: x + datasource.text = lambda x: TextClause(x) + return datasource + + +def test_rls_filters_include_guest_when_enabled( + mock_datasource: MagicMock, + app: Flask, +) -> None: + """ + Test that RLS filters include guest filters when enabled. + + When include_guest_rls=True and EMBEDDED_SUPERSET is enabled, + both regular and guest RLS filters should be returned. + """ + regular_filter = MagicMock() + regular_filter.clause = "col1 = 'value1'" + regular_filter.group_key = None + + guest_rule = {"clause": "col2 = 'value2'"} + + with ( + patch( + "superset.connectors.sqla.models.security_manager.get_rls_filters", + return_value=[regular_filter], + ), + patch( + "superset.connectors.sqla.models.security_manager.get_guest_rls_filters", + return_value=[guest_rule], + ), + patch( + "superset.connectors.sqla.models.is_feature_enabled", + return_value=True, + ), + ): + # Call with include_guest_rls=True + filters = BaseDatasource._get_sqla_row_level_filters_internal( + mock_datasource, include_guest_rls=True + ) + + # Should include both regular and guest RLS + assert len(filters) == 2 + filter_strs = [str(f) for f in filters] + assert any("col1" in s for s in filter_strs) + assert any("col2" in s for s in filter_strs) + + +def test_rls_filters_exclude_guest_when_requested( + mock_datasource: MagicMock, + app: Flask, +) -> None: + """ + Test that RLS filters exclude guest filters when requested. + + Issue #37359: When analyzing underlying tables in virtual datasets, + guest RLS should be excluded to prevent double application. + """ + regular_filter = MagicMock() + regular_filter.clause = "col1 = 'value1'" + regular_filter.group_key = None + + guest_rule = {"clause": "col2 = 'value2'"} + + with ( + patch( + "superset.connectors.sqla.models.security_manager.get_rls_filters", + return_value=[regular_filter], + ), + patch( + "superset.connectors.sqla.models.security_manager.get_guest_rls_filters", + return_value=[guest_rule], + ), + patch( + "superset.connectors.sqla.models.is_feature_enabled", + return_value=True, + ), Review Comment: **Suggestion:** The second test repeats the invalid "with (patch(...), ...):" pattern; that constructs a tuple and will raise TypeError. Chain the patch context managers with commas (or use ExitStack) so the patches are applied correctly. [runtime error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Unit tests fail in CI for RLS exclusion tests. - ⚠️ Prevents verification of double-RLS regression fix. ``` </details> ```suggestion with patch( "superset.connectors.sqla.models.security_manager.get_rls_filters", return_value=[regular_filter], ), patch( "superset.connectors.sqla.models.security_manager.get_guest_rls_filters", return_value=[guest_rule], ), patch( "superset.connectors.sqla.models.is_feature_enabled", return_value=True, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run pytest to execute tests in tests/unit_tests/models/test_double_rls_virtual_dataset.py. 2. Pytest runs test_rls_filters_exclude_guest_when_requested (defined at tests/unit_tests/models/test_double_rls_virtual_dataset.py:88). 3. The test reaches the with-statement that constructs a tuple of patch() context managers (patch calls listed in the file around lines 107-117). 4. Python raises TypeError when trying to use the tuple as a context manager, causing the test to error and preventing the intended assertions against BaseDatasource._get_sqla_row_level_filters_internal. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/models/test_double_rls_virtual_dataset.py **Line:** 107:116 **Comment:** *Runtime Error: The second test repeats the invalid "with (patch(...), ...):" pattern; that constructs a tuple and will raise TypeError. Chain the patch context managers with commas (or use ExitStack) so the patches are applied 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> ########## tests/unit_tests/models/test_double_rls_virtual_dataset.py: ########## @@ -0,0 +1,286 @@ +# 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 double RLS application in virtual datasets (Issue #37359). + +This module tests that guest user RLS filters are applied only once +when querying virtual datasets, not both in the underlying table SQL +and the outer query. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest +from flask import Flask +from sqlalchemy.sql.elements import TextClause + +from superset.connectors.sqla.models import BaseDatasource + + [email protected] +def mock_datasource() -> MagicMock: + """Create a mock datasource for testing.""" + datasource = MagicMock(spec=BaseDatasource) + datasource.get_template_processor.return_value = MagicMock() + datasource.get_template_processor.return_value.process_template = lambda x: x + datasource.text = lambda x: TextClause(x) + return datasource + + +def test_rls_filters_include_guest_when_enabled( + mock_datasource: MagicMock, + app: Flask, +) -> None: + """ + Test that RLS filters include guest filters when enabled. + + When include_guest_rls=True and EMBEDDED_SUPERSET is enabled, + both regular and guest RLS filters should be returned. + """ + regular_filter = MagicMock() + regular_filter.clause = "col1 = 'value1'" + regular_filter.group_key = None + + guest_rule = {"clause": "col2 = 'value2'"} + + with ( + patch( + "superset.connectors.sqla.models.security_manager.get_rls_filters", + return_value=[regular_filter], + ), + patch( + "superset.connectors.sqla.models.security_manager.get_guest_rls_filters", + return_value=[guest_rule], + ), + patch( + "superset.connectors.sqla.models.is_feature_enabled", + return_value=True, + ), + ): + # Call with include_guest_rls=True + filters = BaseDatasource._get_sqla_row_level_filters_internal( + mock_datasource, include_guest_rls=True + ) + + # Should include both regular and guest RLS + assert len(filters) == 2 + filter_strs = [str(f) for f in filters] + assert any("col1" in s for s in filter_strs) + assert any("col2" in s for s in filter_strs) + + +def test_rls_filters_exclude_guest_when_requested( + mock_datasource: MagicMock, + app: Flask, +) -> None: + """ + Test that RLS filters exclude guest filters when requested. + + Issue #37359: When analyzing underlying tables in virtual datasets, + guest RLS should be excluded to prevent double application. + """ + regular_filter = MagicMock() + regular_filter.clause = "col1 = 'value1'" + regular_filter.group_key = None + + guest_rule = {"clause": "col2 = 'value2'"} + + with ( + patch( + "superset.connectors.sqla.models.security_manager.get_rls_filters", + return_value=[regular_filter], + ), + patch( + "superset.connectors.sqla.models.security_manager.get_guest_rls_filters", + return_value=[guest_rule], + ), + patch( + "superset.connectors.sqla.models.is_feature_enabled", + return_value=True, + ), + ): + # Call internal API with include_guest_rls=False + filters = BaseDatasource._get_sqla_row_level_filters_internal( + mock_datasource, include_guest_rls=False + ) + + # Should include only regular RLS, not guest RLS + assert len(filters) == 1 + filter_strs = [str(f) for f in filters] + assert any("col1" in s for s in filter_strs) + assert not any("col2" in s for s in filter_strs) + + +def test_rls_filters_include_guest_by_default( + mock_datasource: MagicMock, + app: Flask, +) -> None: + """ + Test that RLS filters include guest filters by default. + + The default behavior (include_guest_rls=True) ensures backwards + compatibility with existing code. + """ + regular_filter = MagicMock() + regular_filter.clause = "col1 = 'value1'" + regular_filter.group_key = None + + guest_rule = {"clause": "col2 = 'value2'"} + + with ( + patch( + "superset.connectors.sqla.models.security_manager.get_rls_filters", + return_value=[regular_filter], + ), + patch( + "superset.connectors.sqla.models.security_manager.get_guest_rls_filters", + return_value=[guest_rule], + ), + patch( Review Comment: **Suggestion:** The third test also constructs a tuple of patches inside a with-statement ("with (patch(...), ...):"), which is invalid and will raise TypeError; change it to chain the patches with commas (or use contextlib.ExitStack) so the patches are properly entered and exited. [runtime error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Unit tests for default guest RLS behavior fail. - ⚠️ Hinders verification of backward-compatibility behavior. ``` </details> ```suggestion with patch( "superset.connectors.sqla.models.security_manager.get_rls_filters", return_value=[regular_filter], ), patch( "superset.connectors.sqla.models.security_manager.get_guest_rls_filters", return_value=[guest_rule], ), patch( "superset.connectors.sqla.models.is_feature_enabled", return_value=True, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Execute pytest which loads tests/unit_tests/models/test_double_rls_virtual_dataset.py. 2. Pytest runs test_rls_filters_include_guest_by_default (defined at tests/unit_tests/models/test_double_rls_virtual_dataset.py:130). 3. The test executes the with-statement that uses a tuple of patch() context managers (patch entries listed in the file in the with block near lines 150-156). 4. Python raises TypeError because a tuple is not a context manager; the test errors before exercising the internal RLS behavior, failing CI. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/models/test_double_rls_virtual_dataset.py **Line:** 150:155 **Comment:** *Runtime Error: The third test also constructs a tuple of patches inside a with-statement ("with (patch(...), ...):"), which is invalid and will raise TypeError; change it to chain the patches with commas (or use contextlib.ExitStack) so the patches are properly entered and exited. 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> -- 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]
