codeant-ai-for-open-source[bot] commented on code in PR #41550:
URL: https://github.com/apache/superset/pull/41550#discussion_r3690973964
##########
tests/integration_tests/datasets/soft_delete_tests.py:
##########
@@ -603,3 +613,173 @@ def
test_create_blocked_by_soft_deleted_logical_duplicate(self) -> None:
)
row.restore()
db.session.commit()
+
+
+class TestDatasetArchiveListing(SupersetTestCase):
Review Comment:
**Suggestion:** The archive-listing and restore authorization tests in this
class do not enable `SOFT_DELETE`, unlike the surrounding dataset soft-delete
tests. With the default flag disabled, these tests do not verify the
feature-enabled archive contract and the restore request can pass its assertion
merely because the endpoint returns the flag-gated 404. Apply
`@with_feature_flags(SOFT_DELETE=True)` to the relevant tests or the class.
[possible bug]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Dataset archive tests omit enabled-mode coverage.
- โ ๏ธ Visibility-filter regressions can remain undetected.
- โ ๏ธ Restore authorization lacks rollout-mode validation.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=91031fb86d0c472fa6cac5d5537337a0&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=91031fb86d0c472fa6cac5d5537337a0&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/soft_delete_tests.py
**Line:** 618:618
**Comment:**
*Possible Bug: The archive-listing and restore authorization tests in
this class do not enable `SOFT_DELETE`, unlike the surrounding dataset
soft-delete tests. With the default flag disabled, these tests do not verify
the feature-enabled archive contract and the restore request can pass its
assertion merely because the endpoint returns the flag-gated 404. Apply
`@with_feature_flags(SOFT_DELETE=True)` to the relevant tests or the class.
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%2F41550&comment_hash=e8df94ebbd5cde225910321ee8d00cec4c0f468c80550735cd32e94a44473b6f&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41550&comment_hash=e8df94ebbd5cde225910321ee8d00cec4c0f468c80550735cd32e94a44473b6f&reaction=dislike'>๐</a>
##########
tests/integration_tests/dashboards/soft_delete_tests.py:
##########
@@ -730,3 +745,154 @@ def test_restore_via_import_with_slug_rename(self) ->
None:
finally:
_hard_delete_dashboard(original_id)
_hard_delete_dashboard(claimant_id)
+
+
+class TestDashboardArchiveListing(SupersetTestCase):
Review Comment:
**Suggestion:** The archive-listing tests in this class do not enable
`SOFT_DELETE`, unlike the existing soft-delete tests in the module. On the
default configuration the flag is false, so these tests exercise a manually
populated `deleted_at` row through an un-gated filter and accept a 404 for
restore, allowing archive listing and authorization regressions to pass without
testing the feature-enabled behavior. Apply
`@with_feature_flags(SOFT_DELETE=True)` to the relevant tests or the class.
[possible bug]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Dashboard archive tests omit enabled-mode coverage.
- โ ๏ธ Visibility-listener regressions can pass unnoticed.
- โ ๏ธ Restore authorization is not tested in rollout mode.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bcd3d73f2e43410da6a01931fc970e4b&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=bcd3d73f2e43410da6a01931fc970e4b&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/dashboards/soft_delete_tests.py
**Line:** 750:750
**Comment:**
*Possible Bug: The archive-listing tests in this class do not enable
`SOFT_DELETE`, unlike the existing soft-delete tests in the module. On the
default configuration the flag is false, so these tests exercise a manually
populated `deleted_at` row through an un-gated filter and accept a 404 for
restore, allowing archive listing and authorization regressions to pass without
testing the feature-enabled behavior. Apply
`@with_feature_flags(SOFT_DELETE=True)` to the relevant tests or the class.
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%2F41550&comment_hash=fe64e5b5beb433c51c2bb34e40653bf5b3d8e5a58082eaffd8222a649f530c4f&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41550&comment_hash=fe64e5b5beb433c51c2bb34e40653bf5b3d8e5a58082eaffd8222a649f530c4f&reaction=dislike'>๐</a>
##########
superset/commands/deletion_retention/purge_cascade.py:
##########
@@ -239,6 +273,28 @@ def cascade_hard_delete(
entity_uuid=uuid,
blocked_reason=str(ex),
)
+ except IntegrityError as ex:
+ # Not a policy decision: a restrictive FK the cascade did not handle.
+ # Two audiences, two messages. The curated reason goes to the caller
+ # (and from there into a user toast), because raw driver text carries
+ # the failing SQL and bind parameters. The constraint detail goes to
+ # the log at WARNING, because an entity permanently unpurgeable via an
+ # unknown FK is a cascade-coverage bug someone has to be able to
+ # diagnose -- reported at INFO as a policy block, it read as intended
+ # behaviour.
+ logger.warning(
+ "deletion_retention: %s id=%s purge failed on a restrictive "
+ "foreign key the cascade does not handle: %s",
+ entity_type,
+ entity_id,
+ ex,
+ )
+ return CascadeResult(
+ purged=False,
+ entity_type=entity_type,
+ entity_uuid=uuid,
+ blocked_reason="blocked by database references",
+ )
Review Comment:
**Suggestion:** The `IntegrityError` handler discards the original database
error and returns the generic text `blocked by database references`. The purge
contract requires the original error text in `blocked_reason`, particularly so
external or plugin-table foreign-key blockers can be identified by the caller;
preserve `str(ex)` in the result while keeping the raw detail out of
user-facing logs only if that is handled separately. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major โ ๏ธ</summary>
```mdx
- โ ๏ธ REST purge responses lose constraint-specific diagnostics.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8dd1d17daeb1486c8aca775ee9091a01&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=8dd1d17daeb1486c8aca775ee9091a01&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/deletion_retention/purge_cascade.py
**Line:** 292:297
**Comment:**
*Api Mismatch: The `IntegrityError` handler discards the original
database error and returns the generic text `blocked by database references`.
The purge contract requires the original error text in `blocked_reason`,
particularly so external or plugin-table foreign-key blockers can be identified
by the caller; preserve `str(ex)` in the result while keeping the raw detail
out of user-facing logs only if that is handled separately.
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%2F41550&comment_hash=470ea9f837d048d6ef1a4ae036a0e6966416fc2b611490650bfb9d3e32edecb4&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41550&comment_hash=470ea9f837d048d6ef1a4ae036a0e6966416fc2b611490650bfb9d3e32edecb4&reaction=dislike'>๐</a>
##########
superset/views/filters.py:
##########
@@ -292,6 +314,37 @@ def _mark_response_for_deleted_at_augmentation() -> None:
setattr(g, AUGMENT_RESPONSE_WITH_DELETED_AT, True)
+class BaseDeletedRecencyFilter(BaseFilter): # pylint:
disable=too-few-public-methods
+ """Keep rows archived within the last *value* days, by the server's clock.
+
+ The archive UI's time-range presets used to send an absolute cutoff
+ computed client-side in UTC. ``deleted_at`` is stamped with the server's
+ naive-local ``datetime.now()``, so on any non-UTC deployment those
+ cutoffs were shifted by the server offset -- and because the cutoff was
+ frozen when the page mounted, a long-lived tab drifted further. Taking a
+ day count and resolving it here, on the clock that stamped the column,
+ removes both failure modes and lets the client keep stable, shareable
+ filter values.
+
+ Subclasses set ``arg_name`` (e.g. ``"chart_deleted_recency"``).
+ """
+
+ name = lazy_gettext("Archived within")
+
+ def apply(self, query: Query, value: Any) -> Query:
+ try:
+ days = int(value)
+ except (TypeError, ValueError):
+ # Filter values arrive from the URL; refusing loudly would turn a
+ # mangled query string into a 500. An unfiltered list is the same
+ # answer every other malformed FAB filter value produces.
+ return query
+ if days <= 0:
+ return query
+ cutoff = datetime.now() - timedelta(days=days)
+ return query.filter(self.model.deleted_at > cutoff)
Review Comment:
**Suggestion:** The recency filter only adds a `deleted_at` predicate but
never opts the model into the soft-delete visibility bypass. Consequently, a
request using this registered filter without the separate
`*_deleted_state:only` filter is still subject to the global `deleted_at IS
NULL` listener and cannot return archived rows, even though this filter's
contract is to select rows archived within the requested window. Make the
recency filter establish archived visibility and apply the same
restore-audience scoping, or reject/use it only as part of the deleted-state
filter. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major โ ๏ธ</summary>
```mdx
- โ Direct API recency queries return no archived rows.
- โ ๏ธ Archive filtering depends on an undocumented second filter.
- โ ๏ธ Custom archive clients can silently display empty results.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4482793dd59347d6b356e1e15363b71b&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=4482793dd59347d6b356e1e15363b71b&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/views/filters.py
**Line:** 345:345
**Comment:**
*Api Mismatch: The recency filter only adds a `deleted_at` predicate
but never opts the model into the soft-delete visibility bypass. Consequently,
a request using this registered filter without the separate
`*_deleted_state:only` filter is still subject to the global `deleted_at IS
NULL` listener and cannot return archived rows, even though this filter's
contract is to select rows archived within the requested window. Make the
recency filter establish archived visibility and apply the same
restore-audience scoping, or reject/use it only as part of the deleted-state
filter.
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%2F41550&comment_hash=3f65f6c1365ce3c9b66228b57218aeff741c577f5f43406159afdd79baeed3e2&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41550&comment_hash=3f65f6c1365ce3c9b66228b57218aeff741c577f5f43406159afdd79baeed3e2&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]
