codeant-ai-for-open-source[bot] commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3454235108
##########
superset/commands/dataset/exceptions.py:
##########
@@ -170,6 +170,26 @@ class DatasetUpdateFailedError(UpdateFailedError):
message = _("Dataset could not be updated.")
+class DatasetRestoreFailedError(UpdateFailedError):
+ # Restore semantically clears ``deleted_at``; it is an UPDATE, not a new
+ # row. ``UpdateFailedError`` is the nearest typed middle-tier base in the
+ # codebase. A dedicated ``RestoreFailedError`` in
+ # ``superset/commands/exceptions.py`` would be more precise across the
+ # entity rollouts but lives in already-merged infrastructure (#39977);
+ # introducing it can be a cross-entity follow-up.
+ message = _("Dataset could not be restored.")
Review Comment:
**Suggestion:** Add a class docstring describing the restore failure case
instead of relying only on inline comments. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python class and it has no class docstring. The rule
explicitly requires new classes to be documented inline, so this is a real
violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c9c7199fa233465aba886a1cd61cfa49&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=c9c7199fa233465aba886a1cd61cfa49&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/commands/dataset/exceptions.py
**Line:** 173:180
**Comment:**
*Custom Rule: Add a class docstring describing the restore failure case
instead of relying only on inline comments.
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=71be2e155862e7a2ba2a79740ecc9c4a5a944b975ee2e9b3bc83ba81e1974866&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=71be2e155862e7a2ba2a79740ecc9c4a5a944b975ee2e9b3bc83ba81e1974866&reaction=dislike'>👎</a>
##########
superset/commands/dataset/exceptions.py:
##########
@@ -170,6 +170,26 @@ class DatasetUpdateFailedError(UpdateFailedError):
message = _("Dataset could not be updated.")
+class DatasetRestoreFailedError(UpdateFailedError):
+ # Restore semantically clears ``deleted_at``; it is an UPDATE, not a new
+ # row. ``UpdateFailedError`` is the nearest typed middle-tier base in the
+ # codebase. A dedicated ``RestoreFailedError`` in
+ # ``superset/commands/exceptions.py`` would be more precise across the
+ # entity rollouts but lives in already-merged infrastructure (#39977);
+ # introducing it can be a cross-entity follow-up.
+ message = _("Dataset could not be restored.")
+
+
+class DatasetLogicalDuplicateError(CommandException):
+ status = 422
+ message = _(
+ "Dataset cannot be restored because another active dataset already "
+ "references the same physical table (same database, catalog, schema, "
+ "and table name). Delete the duplicate or rename the table before "
+ "restoring."
+ )
Review Comment:
**Suggestion:** Add a class docstring that explains when this
logical-duplicate exception is raised. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is also a newly added Python class and it lacks a docstring. Inline
comments or a class message do not satisfy the stated docstring requirement, so
the suggestion is valid.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e54d54acd8e44e968a6addc692135450&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=e54d54acd8e44e968a6addc692135450&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/commands/dataset/exceptions.py
**Line:** 183:190
**Comment:**
*Custom Rule: Add a class docstring that explains when this
logical-duplicate exception is raised.
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=bdb10e80c7dc442d60e4ce54a2d5b5ac4ea56debb1d3f8927fcec27d79515ae6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=bdb10e80c7dc442d60e4ce54a2d5b5ac4ea56debb1d3f8927fcec27d79515ae6&reaction=dislike'>👎</a>
##########
superset/commands/dataset/restore.py:
##########
@@ -0,0 +1,60 @@
+# 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.
+"""Command to restore a soft-deleted dataset."""
+
+from superset.commands.dataset.exceptions import (
+ DatasetForbiddenError,
+ DatasetLogicalDuplicateError,
+ DatasetNotFoundError,
+ DatasetRestoreFailedError,
+)
+from superset.commands.restore import BaseRestoreCommand
+from superset.connectors.sqla.models import SqlaTable
+from superset.daos.dataset import DatasetDAO
+
+
+class RestoreDatasetCommand(BaseRestoreCommand[SqlaTable]):
+ """Restore a soft-deleted dataset by clearing its ``deleted_at`` field.
+
+ Most behaviour is inherited from ``BaseRestoreCommand``. The override
+ here adds a logical-duplicate check: DB-level enforcement of
+ ``SqlaTable`` logical uniqueness is inconsistent (the model-level
+ 4-column ``UniqueConstraint`` is metadata-only — no migration creates
+ it — and the legacy 3-column ``_customer_location_uc`` has no catalog
+ leg and is NULL-leaky), so another active dataset may have claimed the
+ same physical identity while this one was soft-deleted. The
+ application-level check refuses the restore with a clean 422 rather
+ than relying on the DB, which would either reject with an opaque
+ IntegrityError or — where no constraint applies — silently allow two
+ active datasets pointing at the same physical table.
+ """
+
+ dao = DatasetDAO
+ not_found_exc = DatasetNotFoundError
+ forbidden_exc = DatasetForbiddenError
+ restore_failed_exc = DatasetRestoreFailedError
+
+ def validate(self) -> SqlaTable: # type: ignore[override]
Review Comment:
**Suggestion:** Add an inline docstring to this newly added method to
describe its validation behavior and failure condition before executing the
method body. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python method and it does not include a docstring. The
custom rule requires newly introduced functions and classes to be documented
inline, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=81ed2e4a9eac4bf99147ca967d702fae&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=81ed2e4a9eac4bf99147ca967d702fae&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/commands/dataset/restore.py
**Line:** 51:51
**Comment:**
*Custom Rule: Add an inline docstring to this newly added method to
describe its validation behavior and failure condition before executing the
method body.
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=d558048b71d8fe892a5e9546f2b9c3212c12a15dff936f1e02c56c7e90544f79&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=d558048b71d8fe892a5e9546f2b9c3212c12a15dff936f1e02c56c7e90544f79&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
Review Comment:
**Suggestion:** Add an inline docstring to the newly introduced helper
function so every new function in this file is documented. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The helper function `raise_security` is newly introduced in this file and
does not have a docstring. This matches the custom rule requiring newly added
Python functions and classes to include docstrings.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f4e69947faca47d6bbd2efeaa4d03f28&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=f4e69947faca47d6bbd2efeaa4d03f28&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:** 8:9
**Comment:**
*Custom Rule: Add an inline docstring to the newly introduced helper
function so every new function in this file is documented.
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=26b3110364e49168062a169014ec5ee31e32d4a131c28951f18ab34729ada59b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=26b3110364e49168062a169014ec5ee31e32d4a131c28951f18ab34729ada59b&reaction=dislike'>👎</a>
##########
superset/datasets/filters.py:
##########
@@ -51,3 +57,40 @@ def apply(self, query: Query, value: bool) -> Query:
)
)
return query
+
+
+class DatasetDeletedStateFilter( # pylint: disable=too-few-public-methods
+ BaseDeletedStateFilter
+):
+ """Rison filter for the GET list that exposes soft-deleted datasets.
+
+ Soft-deleted rows are additionally scoped to the **restore audience**: only
+ the dataset's owners (or admins) may enumerate them. This mirrors
+ ``RestoreDatasetCommand``'s ``raise_for_ownership`` check, so a read-access
+ non-owner (who can see the dataset via ``datasource_access``) cannot list
+ soft-deleted datasets they could never restore. Live rows are unaffected —
+ they keep their normal ``DatasourceFilter`` visibility. The ownership
+ scoping is part of the cross-entity deleted-state contract: only the
+ restore audience may enumerate soft-deleted rows (kept consistent with the
+ deleted-state filters in the dashboard and chart soft-delete rollouts).
+ """
+
+ arg_name = "dataset_deleted_state"
+ model = SqlaTable
+
+ def apply(self, query: Query, value: Any) -> Query:
Review Comment:
**Suggestion:** Add an inline docstring to the newly added `apply` method so
the method behavior and intent are documented per the rule for new functions.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The newly added `DatasetDeletedStateFilter.apply` method has no docstring,
and the custom rule requires newly added Python functions and classes to be
documented inline. This is a real rule violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8c15c488ba5a4c1ebfe90ab6cc7b2861&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=8c15c488ba5a4c1ebfe90ab6cc7b2861&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/datasets/filters.py
**Line:** 81:81
**Comment:**
*Custom Rule: Add an inline docstring to the newly added `apply` method
so the method behavior and intent are documented per the rule for new functions.
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=dd8ea86ef60ecfb1f839a5d1246bfe42d7b6abb16b1c3e65b2b949737636a9e5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=dd8ea86ef60ecfb1f839a5d1246bfe42d7b6abb16b1c3e65b2b949737636a9e5&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]
