codeant-ai-for-open-source[bot] commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3509988346
##########
tests/integration_tests/datasets/api_tests.py:
##########
@@ -191,19 +193,34 @@ def create_virtual_datasets(self):
@pytest.fixture
def create_datasets(self):
with self.create_app().app_context():
+ # Purge any soft-deleted rows that occupy the unique constraint
+ stale = self.get_fixture_datasets()
+ for ds in stale:
+ db.session.delete(ds)
+ if stale:
+ db.session.commit()
+
datasets = []
admin = self.get_user("admin")
main_db = get_main_database()
for tables_name in self.fixture_tables_names:
datasets.append(self.insert_dataset(tables_name, [admin.id],
main_db))
+ # Capture IDs eagerly — dataset objects may be detached after yield
+ dataset_ids = [ds.id for ds in datasets]
+
yield datasets
- # rollback changes
- for dataset in datasets:
- state = inspect(dataset)
- if not state.was_deleted:
- db.session.delete(dataset)
+ # rollback changes (including soft-deleted rows)
+ for dataset_id in dataset_ids:
+ row = (
+ db.session.query(SqlaTable)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES:
{SqlaTable}})
+ .filter(SqlaTable.id == dataset_id)
+ .one_or_none()
+ )
Review Comment:
**Suggestion:** Add a type annotation for this new query-result variable to
make the optional model type explicit. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This query result variable is newly added and left unannotated. Since it can
be annotated as an optional SqlaTable, the suggestion identifies a real
type-hint omission.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d4f267bae7de4570a3b793df92cb4fd1&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=d4f267bae7de4570a3b793df92cb4fd1&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/integration_tests/datasets/api_tests.py
**Line:** 216:221
**Comment:**
*Custom Rule: Add a type annotation for this new query-result variable
to make the optional model type explicit.
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%2F40130&comment_hash=e21edee2539058310422560962730383220c267a2dffe622dbe0ee387ef45967&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=e21edee2539058310422560962730383220c267a2dffe622dbe0ee387ef45967&reaction=dislike'>👎</a>
##########
tests/integration_tests/datasets/api_tests.py:
##########
@@ -191,19 +193,34 @@ def create_virtual_datasets(self):
@pytest.fixture
def create_datasets(self):
with self.create_app().app_context():
+ # Purge any soft-deleted rows that occupy the unique constraint
+ stale = self.get_fixture_datasets()
+ for ds in stale:
+ db.session.delete(ds)
+ if stale:
+ db.session.commit()
+
datasets = []
admin = self.get_user("admin")
main_db = get_main_database()
for tables_name in self.fixture_tables_names:
datasets.append(self.insert_dataset(tables_name, [admin.id],
main_db))
+ # Capture IDs eagerly — dataset objects may be detached after yield
+ dataset_ids = [ds.id for ds in datasets]
+
yield datasets
- # rollback changes
- for dataset in datasets:
- state = inspect(dataset)
- if not state.was_deleted:
- db.session.delete(dataset)
+ # rollback changes (including soft-deleted rows)
+ for dataset_id in dataset_ids:
Review Comment:
**Suggestion:** Annotate this new scalar identifier variable with its
expected primitive type. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a new scalar local variable introduced in modified code and it lacks
an explicit annotation. It falls under the type-hint requirement.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7105fb078baa485ba13ea639bbeddbe4&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=7105fb078baa485ba13ea639bbeddbe4&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/integration_tests/datasets/api_tests.py
**Line:** 215:215
**Comment:**
*Custom Rule: Annotate this new scalar identifier variable with its
expected primitive 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%2F40130&comment_hash=e4a068b9683006f7a710fcd1c601609c775ed497cce1f28b7dd7a2f1377e0243&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=e4a068b9683006f7a710fcd1c601609c775ed497cce1f28b7dd7a2f1377e0243&reaction=dislike'>👎</a>
##########
tests/unit_tests/commands/dataset/restore_test.py:
##########
@@ -0,0 +1,172 @@
+# 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.
+"""Unit tests for RestoreDatasetCommand."""
+
+from __future__ import annotations
+
+import uuid
+from datetime import datetime, timezone
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+# Stable UUIDs make the test boundary realistic: the production command
+# loads by UUID, not integer ID. The test only mocks the DAO lookup, but
+# the argument shape should still match.
+_DATASET_UUID = str(uuid.uuid4())
+_MISSING_UUID = str(uuid.uuid4())
+
+
+def test_restore_dataset_clears_deleted_at(app_context: None) -> None:
+ """RestoreDatasetCommand.run() restores a soft-deleted dataset."""
+ from superset.commands.dataset.restore import RestoreDatasetCommand
+
+ dataset = MagicMock()
Review Comment:
**Suggestion:** Add a type annotation to this local mock variable.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The variable is newly introduced in test code and is unannotated despite
being annotatable (for example, `dataset: MagicMock = MagicMock()`). This
matches the type-hints rule for relevant variables.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4bcff6d630f94c289a33af93789ba2fa&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=4bcff6d630f94c289a33af93789ba2fa&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/commands/dataset/restore_test.py
**Line:** 38:38
**Comment:**
*Custom Rule: Add a type annotation to this local mock variable.
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%2F40130&comment_hash=ace0c80fd7aeceb36eabeadd889f9d7de215b16eb68a393d02d1e2a6840dcac8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=ace0c80fd7aeceb36eabeadd889f9d7de215b16eb68a393d02d1e2a6840dcac8&reaction=dislike'>👎</a>
##########
tests/unit_tests/commands/databases/sync_permissions_test.py:
##########
@@ -409,3 +409,49 @@ def test_sync_permissions_command_rename_db_in_perms(
assert (
mock_chart.schema_perm ==
f"[{database_with_catalog.name}].[catalog1].[schema1]"
)
+
+
+def test_sync_permissions_command_rename_bypasses_visibility_filter(
+ mocker: MockerFixture, database_with_catalog: MagicMock
+):
Review Comment:
**Suggestion:** Add an explicit return type annotation to this new test
function signature. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python function and it omits an explicit return type
annotation. The custom rule requires type hints on new or modified Python
functions when they can be annotated, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=79b9c1daa1e749fcb7edc8f3754033ca&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=79b9c1daa1e749fcb7edc8f3754033ca&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/commands/databases/sync_permissions_test.py
**Line:** 414:416
**Comment:**
*Custom Rule: Add an explicit return type annotation to this new test
function signature.
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%2F40130&comment_hash=2b8a11165fe13518da2e8e8f7ac6cde4624ba001348ac2494f2e8c996b726d39&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=2b8a11165fe13518da2e8e8f7ac6cde4624ba001348ac2494f2e8c996b726d39&reaction=dislike'>👎</a>
##########
tests/unit_tests/commands/databases/sync_permissions_test.py:
##########
@@ -409,3 +409,49 @@ def test_sync_permissions_command_rename_db_in_perms(
assert (
mock_chart.schema_perm ==
f"[{database_with_catalog.name}].[catalog1].[schema1]"
)
+
+
+def test_sync_permissions_command_rename_bypasses_visibility_filter(
+ mocker: MockerFixture, database_with_catalog: MagicMock
+):
+ """The dataset/chart perm rewrite must run with the soft-delete
+ visibility filter bypassed for ``SqlaTable`` and ``Slice``.
+
+ A soft-deleted dataset's ``schema_perm``/``catalog_perm`` (and its
+ charts') must still be rewritten on a database rename; without the
+ bypass, the listener hides those rows from ``get_datasets`` and a
+ later restore resurrects stale perm strings referencing the old
+ database name — matchable by a ``schema_access`` grant on a new
+ database reusing that name.
+ """
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES
+ from superset.models.slice import Slice
+
+ mocker.patch(
+ "superset.commands.database.sync_permissions."
+ "security_manager.find_permission_view_menu",
+ return_value=None,
+ )
+ observed_bypass: list[set[type]] = []
+
+ def capture_bypass(*args, **kwargs):
+ observed_bypass.append(
+ set(db.session.info.get(SKIP_VISIBILITY_FILTER_CLASSES, set()))
+ )
+ return []
+
Review Comment:
**Suggestion:** Add type annotations for this nested helper function
parameters and return type. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This nested helper function is newly introduced and has neither parameter
type annotations nor a return type annotation. That violates the type-hint
requirement for new Python code that can be annotated.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2203c311d415410eaf6459674d107a43&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=2203c311d415410eaf6459674d107a43&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/commands/databases/sync_permissions_test.py
**Line:** 438:443
**Comment:**
*Custom Rule: Add type annotations for this nested helper function
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%2F40130&comment_hash=fb9fdf21c2865daccf12485d97627a44ce52f02ec346bdd88c46de4d0643e242&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=fb9fdf21c2865daccf12485d97627a44ce52f02ec346bdd88c46de4d0643e242&reaction=dislike'>👎</a>
##########
superset/daos/dataset.py:
##########
@@ -186,16 +236,111 @@ def validate_update_uniqueness(
) -> bool:
# The catalog might not be set even if the database supports catalogs,
in case
# multi-catalog is disabled.
- catalog = table.catalog or database.get_default_catalog()
-
- dataset_query = db.session.query(SqlaTable).filter(
- SqlaTable.table_name == table.table,
- SqlaTable.database_id == database.id,
- SqlaTable.schema == table.schema,
- SqlaTable.catalog == catalog,
- SqlaTable.id != dataset_id,
+ default_catalog = database.get_default_catalog()
+ catalog = table.catalog or default_catalog
+
+ # Same rationale as ``validate_uniqueness`` above: a soft-deleted
+ # twin must block the update rather than being silently ignored.
+ # The bypass is session-scoped, not per-query, because the EXISTS
+ # subquery pattern below leaves the inner Query's execution_options
+ # invisible to the outer execute (where the listener fires).
+ #
+ # avoid app-init regression: see ``validate_uniqueness`` above — a
+ # module-top import from ``superset.models.helpers`` raises
+ # "App not initialized yet" outside an app context (see PR #40573).
+ from superset.models.helpers import ( # pylint:
disable=import-outside-toplevel
+ skip_visibility_filter,
)
- return not db.session.query(dataset_query.exists()).scalar()
+
+ with skip_visibility_filter(db.session, SqlaTable):
+ dataset_query = db.session.query(SqlaTable).filter(
Review Comment:
**Suggestion:** Add an explicit type annotation for this query variable so
the modified method includes complete type hints. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is another newly added unannotated local query variable in a modified
Python method, so it matches the type-hint requirement for annotatable locals.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6fa5f4a2dd8a4dd88a68bb4fb8b83708&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=6fa5f4a2dd8a4dd88a68bb4fb8b83708&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/daos/dataset.py
**Line:** 256:256
**Comment:**
*Custom Rule: Add an explicit type annotation for this query variable
so the modified method includes complete type hints.
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%2F40130&comment_hash=309ab9998c7d93d087af3c27f870ebe78cbaa5e087a416b24bebddc2155f6409&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=309ab9998c7d93d087af3c27f870ebe78cbaa5e087a416b24bebddc2155f6409&reaction=dislike'>👎</a>
##########
superset/daos/dataset.py:
##########
@@ -163,20 +188,45 @@ def validate_uniqueness(
) -> bool:
# The catalog might not be set even if the database supports catalogs,
in case
# multi-catalog is disabled.
- catalog = table.catalog or database.get_default_catalog()
-
- dataset_query = db.session.query(SqlaTable).filter(
- SqlaTable.table_name == table.table,
- SqlaTable.schema == table.schema,
- SqlaTable.catalog == catalog,
- SqlaTable.database_id == database.id,
+ default_catalog = database.get_default_catalog()
+ catalog = table.catalog or default_catalog
+
+ # Bypass the soft-delete visibility filter so a soft-deleted row with
+ # the same physical identity still blocks the create. Otherwise a
+ # delete-then-create-then-restore sequence could produce two live
+ # datasets pointing at the same physical table.
+ #
+ # The bypass MUST be session-scoped, not per-query. The
+ # ``db.session.query(dataset_query.exists()).scalar()`` pattern below
+ # builds an OUTER query whose ``execution_options`` are independent
+ # of the inner ``dataset_query`` — the listener fires on the outer
+ # execute and would attach ``deleted_at IS NULL`` to all SqlaTable
+ # references in the statement (including inside the EXISTS
+ # subquery) via ``with_loader_criteria(include_aliases=True)``.
+ # A per-query option on the inner query never reaches that listener.
+ #
+ # avoid app-init regression: a module-top import from
+ # ``superset.models.helpers`` eagerly loads ``core.py``, whose model
+ # classes evaluate ``encrypted_field_factory.create(...)`` at
+ # class-definition time and raise "App not initialized yet" when no
+ # Flask app context exists (see PR #40573).
+ from superset.models.helpers import ( # pylint:
disable=import-outside-toplevel
+ skip_visibility_filter,
)
- if dataset_id:
- # make sure the dataset found is different from the target (if any)
- dataset_query = dataset_query.filter(SqlaTable.id != dataset_id)
+ with skip_visibility_filter(db.session, SqlaTable):
+ dataset_query = db.session.query(SqlaTable).filter(
Review Comment:
**Suggestion:** Add an explicit type annotation for this query variable to
satisfy the type-hint requirement for relevant local variables. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The modified method introduces a new local `dataset_query` without any type
annotation, and this variable is a relevant query local that could be annotated
under the Python type-hint rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5c94b294c3d341c5885acbcc1b174efb&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=5c94b294c3d341c5885acbcc1b174efb&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/daos/dataset.py
**Line:** 218:218
**Comment:**
*Custom Rule: Add an explicit type annotation for this query variable
to satisfy the type-hint requirement for relevant local variables.
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%2F40130&comment_hash=c04555b42ae7ecc1314c74f0152bdd042a130049cb23050b94b1e844fd8cb17b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=c04555b42ae7ecc1314c74f0152bdd042a130049cb23050b94b1e844fd8cb17b&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]
