codeant-ai-for-open-source[bot] commented on code in PR #34565:
URL: https://github.com/apache/superset/pull/34565#discussion_r2621613929
##########
superset/views/base.py:
##########
@@ -523,7 +522,10 @@ def cached_common_bootstrap_data( # pylint:
disable=unused-argument
def common_bootstrap_payload() -> dict[str, Any]:
- return cached_common_bootstrap_data(utils.get_user_id(), get_locale())
+ locale = get_locale()
+ # Convert locale to string for proper cache key hashing
+ locale_str = str(locale) if locale else None
+ return cached_common_bootstrap_data(utils.get_user_id(), locale_str)
Review Comment:
**Suggestion:** The updated `common_bootstrap_payload` now returns only the
cached bootstrap data and no longer includes Flask flash messages, and the
module no longer exposes a `get_flashed_messages` symbol; this breaks the
existing contract relied on by tests (and likely the frontend) that expect
`flash_messages` in the payload and a patchable
`superset.views.base.get_flashed_messages`, causing tests to fail and flash
notifications to disappear from the client. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
def get_flashed_messages() -> list[Any]:
"""
Wrapper around Flask's get_flashed_messages so that this module
exposes a patchable symbol and can be used without importing it
at the top level.
"""
from flask import get_flashed_messages as _get_flashed_messages
return _get_flashed_messages()
def common_bootstrap_payload() -> dict[str, Any]:
locale = get_locale()
# Convert locale to string for proper cache key hashing
locale_str = str(locale) if locale else None
data = cached_common_bootstrap_data(utils.get_user_id(), locale_str)
# Include flash messages in the payload for the frontend
return {**data, "flash_messages": get_flashed_messages()}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The tests (tests/unit_tests/views/test_base.py) patch
superset.views.base.get_flashed_messages and expect the payload to include a
"flash_messages" key. A repository-wide search shows usages in the tests that
mock get_flashed_messages and assert the flash_messages field is present.
Removing that symbol and the flash messages from the payload will break those
tests and likely remove flash notifications from the frontend. The proposed
improved_code restores a module-level get_flashed_messages wrapper and
re-inserts flash messages into the returned payload, directly addressing the
broken contract.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/views/base.py
**Line:** 524:528
**Comment:**
*Logic Error: The updated `common_bootstrap_payload` now returns only
the cached bootstrap data and no longer includes Flask flash messages, and the
module no longer exposes a `get_flashed_messages` symbol; this breaks the
existing contract relied on by tests (and likely the frontend) that expect
`flash_messages` in the payload and a patchable
`superset.views.base.get_flashed_messages`, causing tests to fail and flash
notifications to disappear from the client.
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/views/test_base.py:
##########
@@ -0,0 +1,110 @@
+# 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 superset.views.base module"""
+
+from unittest.mock import MagicMock, patch
+
+
+@patch("superset.views.base.utils.get_user_id", return_value=1)
+@patch("superset.views.base.get_flashed_messages", return_value=[])
Review Comment:
**Suggestion:** The patch decorator targets
`superset.views.base.get_flashed_messages`, but this attribute does not exist
on the `superset.views.base` module, so test collection will raise an
AttributeError; using `create=True` allows patch to create the attribute
without failing even though the module does not define it. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
@patch("superset.views.base.get_flashed_messages", return_value=[],
create=True)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
If superset.views.base does not define get_flashed_messages at
import/test-collection time, patch(... ) without create=True will raise an
AttributeError during test collection. A quick repo search did not find any
definition of get_flashed_messages, so allowing patch to create the attribute
is defensive and prevents spurious test failures. This fixes a real
test-collection problem, not a cosmetic change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/views/test_base.py
**Line:** 23:23
**Comment:**
*Logic Error: The patch decorator targets
`superset.views.base.get_flashed_messages`, but this attribute does not exist
on the `superset.views.base` module, so test collection will raise an
AttributeError; using `create=True` allows patch to create the attribute
without failing even though the module does not define it.
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/integration_tests/security_tests.py:
##########
@@ -136,31 +168,47 @@ def setUp(self):
s.schema_perm = ds.schema_perm
create_schema_perm("[examples].[temp_schema]")
gamma_user = security_manager.find_user(username="gamma")
- gamma_user.roles.append(security_manager.find_role(SCHEMA_ACCESS_ROLE))
+ if gamma_user:
+
gamma_user.roles.append(security_manager.find_role(SCHEMA_ACCESS_ROLE))
db.session.commit()
def tearDown(self):
- ds = (
- db.session.query(SqlaTable)
- .filter_by(table_name="wb_health_population", schema="temp_schema")
- .first()
- )
- schema_perm = ds.schema_perm
- ds.schema = get_example_default_schema()
- ds.schema_perm = None
- ds_slices = (
- db.session.query(Slice)
- .filter_by(datasource_type=DatasourceType.TABLE)
- .filter_by(datasource_id=ds.id)
- .all()
- )
- for s in ds_slices:
- s.schema_perm = None
+ # Ensure the session is in a good state before teardown
+ try:
+ db.session.rollback()
+ except Exception: # noqa: S110
+ pass # Ignore rollback errors in teardown
+
+ try:
+ ds = (
+ db.session.query(SqlaTable)
+ .filter_by(table_name="wb_health_population",
schema="temp_schema")
+ .first()
+ )
+ if ds:
+ schema_perm = ds.schema_perm
+ ds.schema = get_example_default_schema()
+ ds.schema_perm = None
+ ds_slices = (
+ db.session.query(Slice)
+ .filter_by(datasource_type=DatasourceType.TABLE)
+ .filter_by(datasource_id=ds.id)
+ .all()
+ )
+ for s in ds_slices:
+ s.schema_perm = None
- delete_schema_perm(schema_perm)
- db.session.delete(security_manager.find_role(SCHEMA_ACCESS_ROLE))
- db.session.commit()
- super().tearDown()
+ delete_schema_perm(schema_perm)
Review Comment:
**Suggestion:** The new `tearDown` logic assumes `delete_schema_perm` will
delete the schema permission corresponding to `schema_perm`, but
`delete_schema_perm` ignores its argument and always operates on a hard-coded
value, so the permission created in `setUp` is never removed and the wrong
permission may be deleted, causing cross-test leakage of schema permissions.
[logic error]
**Severity Level:** Minor ⚠️
```suggestion
if schema_perm:
pv = security_manager.find_permission_view_menu(
"schema_access", schema_perm
)
if pv is not None:
security_manager.del_permission_role(
security_manager.find_role(SCHEMA_ACCESS_ROLE),
pv
)
security_manager.del_permission_view_menu(
"schema_access", schema_perm
)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The file's delete_schema_perm implementation (present in the current file)
ignores its argument and always looks up/deletes "[examples].[2]". That means
delete_schema_perm(schema_perm) in tearDown won't remove the permission created
with "[examples].[temp_schema]" in setUp, causing permission leakage across
tests. The improved code explicitly finds the permission view menu for the
actual schema_perm and removes it — this fixes a real cleanup bug.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/security_tests.py
**Line:** 201:201
**Comment:**
*Logic Error: The new `tearDown` logic assumes `delete_schema_perm`
will delete the schema permission corresponding to `schema_perm`, but
`delete_schema_perm` ignores its argument and always operates on a hard-coded
value, so the permission created in `setUp` is never removed and the wrong
permission may be deleted, causing cross-test leakage of schema permissions.
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/integration_tests/security_tests.py:
##########
@@ -123,6 +149,12 @@ def setUp(self):
.filter_by(table_name="wb_health_population", schema=schema)
.first()
)
+
+ if not ds:
+ # If the table doesn't exist, skip this test setup
+ # This can happen if the example data isn't loaded
+ return
Review Comment:
**Suggestion:** In `setUp`, returning early when the `wb_health_population`
table is missing skips the setup but still runs the tests, so tests that assume
the schema/permissions are configured will fail with misleading assertions
instead of being explicitly marked as skipped when example data is absent.
[logic error]
**Severity Level:** Minor ⚠️
```suggestion
self.skipTest(
"Example data not loaded; skipping schema-access tests in
TestRolePermission.setUp"
)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Silently returning from setUp leaves the TestCase instance in a partially
initialized state while tests still execute; this can produce confusing
failures rather than marking tests as skipped. Calling self.skipTest(...) in
setUp is the right unittest pattern to explicitly skip the test when required
test data is absent.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/security_tests.py
**Line:** 156:156
**Comment:**
*Logic Error: In `setUp`, returning early when the
`wb_health_population` table is missing skips the setup but still runs the
tests, so tests that assume the schema/permissions are configured will fail
with misleading assertions instead of being explicitly marked as skipped when
example data is absent.
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/views/test_base.py:
##########
@@ -0,0 +1,110 @@
+# 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 superset.views.base module"""
+
+from unittest.mock import MagicMock, patch
+
+
+@patch("superset.views.base.utils.get_user_id", return_value=1)
+@patch("superset.views.base.get_flashed_messages", return_value=[])
+@patch(
+ "superset.views.base.cached_common_bootstrap_data", return_value={"test":
"data"}
+)
+@patch("superset.views.base.get_locale")
+def test_common_bootstrap_payload_passes_locale_string(
+ mock_get_locale, mock_cached, mock_flash, mock_user_id
+):
+ """Test that common_bootstrap_payload converts locale to string"""
+ # Mock get_locale to return a Locale-like object with __str__
+ mock_locale = MagicMock()
+ mock_locale.__str__ = lambda self: "de_DE"
Review Comment:
**Suggestion:** The test intends to simulate a Locale-like object whose
string representation is `"de_DE"`, but assigning `mock_locale.__str__ = lambda
self: "de_DE"` does not affect how `str(mock_locale)` is computed, so
`common_bootstrap_payload` will receive the wrong locale string and the
assertion on the cached call will fail; configuring the mock's `__str__` return
value correctly makes `str(mock_locale)` return `"de_DE"`. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
mock_locale.__str__.return_value = "de_DE"
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Assigning a lambda to mock_locale.__str__ does not set up MagicMock's
descriptor behavior so that str(mock_locale) returns the expected value.
Setting mock_locale.__str__.return_value = "de_DE" or using type('X', (),
{'__str__': lambda s: 'de_DE'}) would reliably make str(...) produce "de_DE".
This is a real correctness fix for the test's intent.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/views/test_base.py
**Line:** 34:34
**Comment:**
*Logic Error: The test intends to simulate a Locale-like object whose
string representation is `"de_DE"`, but assigning `mock_locale.__str__ = lambda
self: "de_DE"` does not affect how `str(mock_locale)` is computed, so
`common_bootstrap_payload` will receive the wrong locale string and the
assertion on the cached call will fail; configuring the mock's `__str__` return
value correctly makes `str(mock_locale)` return `"de_DE"`.
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]
