codeant-ai-for-open-source[bot] commented on code in PR #42472:
URL: https://github.com/apache/superset/pull/42472#discussion_r3672044626
##########
scripts/change_detector.py:
##########
@@ -88,9 +120,14 @@ def fetch_files_github_api(url: str): # type: ignore
# re-raise once the retry budget is exhausted.
status = getattr(err, "code", None)
is_transient = (
- status is None or status >= 500 or status in
RETRYABLE_STATUS_CODES
+ status is None
+ or status >= 500
+ or status in RETRYABLE_STATUS_CODES
+ or (status == 403 and _is_rate_limited(err)) # type:
ignore[arg-type]
)
if not is_transient or attempt == MAX_RETRIES:
+ if detail := _api_error_detail(err):
+ print(f"GitHub API error {status}: {detail}")
Review Comment:
**Suggestion:** Printing the raw fallback response body to CI logs can
expose secrets or sensitive proxy/authentication diagnostics when GitHub
returns a non-JSON error page. Restrict the logged detail to a bounded,
sanitized API message instead of emitting the entire body. [security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ CI logs may retain sensitive proxy or upstream diagnostics.
- ⚠️ Error responses are exposed to users with build-log access.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. A CI workflow invokes `main()` in `scripts/change_detector.py:181`, which
reaches
`fetch_changed_files_pr()` at `scripts/change_detector.py:140` or
`fetch_changed_files_push()` at `scripts/change_detector.py:150`; both call
`fetch_files_github_api()` at `scripts/change_detector.py:104`.
2. Make GitHub or an intervening proxy return a terminal non-JSON HTTP error
response,
such as an HTML proxy page, while `urlopen()` raises `HTTPError` at
`scripts/change_detector.py:116`.
3. On the final retry or a non-transient 4xx response, line `129` calls
`_api_error_detail()`; its fallback at `scripts/change_detector.py:99-101`
returns the
entire response body when it is not JSON or lacks a `message`.
4. Line `130` prints that unbounded body into the CI log, exposing any
credentials,
internal URLs, proxy diagnostics, or other sensitive data embedded in the
upstream error
page.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2572b1698e0d47019c6613aa433c50e0&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=2572b1698e0d47019c6613aa433c50e0&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:** scripts/change_detector.py
**Line:** 129:130
**Comment:**
*Security: Printing the raw fallback response body to CI logs can
expose secrets or sensitive proxy/authentication diagnostics when GitHub
returns a non-JSON error page. Restrict the logged detail to a bounded,
sanitized API message instead of emitting the entire 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%2F42472&comment_hash=8a40c623abd8bbb25b44a21149e793bbfc1a6c4516f32ba5cda33c4b14a67307&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=8a40c623abd8bbb25b44a21149e793bbfc1a6c4516f32ba5cda33c4b14a67307&reaction=dislike'>👎</a>
##########
scripts/change_detector.py:
##########
@@ -88,9 +120,14 @@ def fetch_files_github_api(url: str): # type: ignore
# re-raise once the retry budget is exhausted.
status = getattr(err, "code", None)
is_transient = (
- status is None or status >= 500 or status in
RETRYABLE_STATUS_CODES
+ status is None
+ or status >= 500
+ or status in RETRYABLE_STATUS_CODES
+ or (status == 403 and _is_rate_limited(err)) # type:
ignore[arg-type]
Review Comment:
**Suggestion:** Secondary GitHub rate-limit responses can be HTTP 403 while
`x-ratelimit-remaining` is still positive and `Retry-After` is absent. Those
responses do not satisfy `_is_rate_limited`, so the change detector immediately
fails instead of retrying the transient throttling response, defeating the
stated 403 rate-limit handling. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Change detection can fail while GitHub throttles transiently.
- ❌ CI matrix gating may stop before affected jobs are selected.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. A CI workflow calls `fetch_changed_files_pr()` or
`fetch_changed_files_push()` at
`scripts/change_detector.py:140-160`, placing GitHub API requests on the
CI-matrix gating
path.
2. Have GitHub return a secondary-rate-limit response with HTTP 403 while
the primary
quota remains available and without a `Retry-After` header; this is distinct
from the
primary limit represented by `x-ratelimit-remaining: 0`.
3. `fetch_files_github_api()` receives the `HTTPError` at
`scripts/change_detector.py:116`
and evaluates the transient predicate at lines `121-127`.
4. `_is_rate_limited()` at `scripts/change_detector.py:75-86` returns
`False` because
neither header condition is present, so `is_transient` is false and line
`131` immediately
re-raises instead of applying the retry/backoff loop. The existing test at
`tests/unit_tests/scripts/change_detector_test.py:133-146` covers only an
exhausted
primary quota, not this secondary-limit response.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7920148b286c44abba4cb946ef4db440&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=7920148b286c44abba4cb946ef4db440&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:** scripts/change_detector.py
**Line:** 126:126
**Comment:**
*Api Mismatch: Secondary GitHub rate-limit responses can be HTTP 403
while `x-ratelimit-remaining` is still positive and `Retry-After` is absent.
Those responses do not satisfy `_is_rate_limited`, so the change detector
immediately fails instead of retrying the transient throttling response,
defeating the stated 403 rate-limit handling.
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%2F42472&comment_hash=8e4ba232b0acef7bbc02bf8f6170b71db1dd96cb3f9ec5f4730bece3c9816028&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=8e4ba232b0acef7bbc02bf8f6170b71db1dd96cb3f9ec5f4730bece3c9816028&reaction=dislike'>👎</a>
##########
superset/daos/dashboard.py:
##########
@@ -429,11 +429,22 @@ def copy_dashboard(
raise DashboardForbiddenError()
dash = Dashboard()
+ # The copied dashboard and every chart cloned below share one creator,
+ # so both lookups are resolved here rather than inside the loop, where
+ # they would cost two extra queries for each chart in the dashboard.
+ creator_editors: list[Any] = []
+ creator_viewers: list[Any] = []
if g.user:
- from superset.subjects.utils import get_user_subject
+ from superset.subjects.utils import (
+ get_default_viewers_for_new_asset,
+ get_user_subject,
+ )
user_subject = get_user_subject(g.user.id)
- dash.editors = [user_subject] if user_subject else []
+ creator_editors = [user_subject] if user_subject else []
+ creator_viewers = get_default_viewers_for_new_asset(g.user.id)
+ dash.editors = creator_editors
+ dash.viewers = creator_viewers
Review Comment:
**Suggestion:** Assigning creator-group viewers only to the copied dashboard
leaves reused charts unchanged when `duplicate_slices` is false. If those
charts already have a non-empty viewer list that excludes the creator's groups,
group members can pass the dashboard access check but fail the separate chart
access check and cannot load the dashboard's charts. Either copy the charts or
ensure the copied dashboard's chart access is handled consistently. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Group members may open copied dashboards but cannot load charts.
- ⚠️ Copy behavior differs based on `duplicate_slices`.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use the dashboard copy command at
`superset/commands/dashboard/copy.py:35-56` with
`duplicate_slices` false; this reaches `DashboardDAO.copy_dashboard()` at
`superset/daos/dashboard.py:425`.
2. With `ASSIGN_CREATOR_GROUPS_AS_VIEWERS` enabled, copy a dashboard while
logged in as a
user belonging to a group, and provide an original dashboard whose charts
retain non-empty
viewer ACLs that do not include that group.
3. Lines `446-447` assign the creator's group subjects to the new dashboard,
while the
`else` branch at `superset/daos/dashboard.py:483-484` reuses
`original_dash.slices`
without adding those subjects to the charts.
4. When the group member loads chart data,
`security_manager.raise_for_access()` reaches
the chart branch at `superset/security/manager.py:4255-4265`; because
`chart.viewers` is
non-empty, access depends on `self.is_viewer(chart)` and does not use the
datasource
fallback. The user can therefore pass the copied dashboard check at
`superset/security/manager.py:4239-4249` but receive a chart-access denial.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9b49935b9c5643e18770e9976d927124&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=9b49935b9c5643e18770e9976d927124&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/dashboard.py
**Line:** 446:447
**Comment:**
*Security: Assigning creator-group viewers only to the copied dashboard
leaves reused charts unchanged when `duplicate_slices` is false. If those
charts already have a non-empty viewer list that excludes the creator's groups,
group members can pass the dashboard access check but fail the separate chart
access check and cannot load the dashboard's charts. Either copy the charts or
ensure the copied dashboard's chart access is handled consistently.
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%2F42472&comment_hash=7a54fa933d38490aff0b7e10a8a5e54547b0e9f26467f82a9af3b968905144ec&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=7a54fa933d38490aff0b7e10a8a5e54547b0e9f26467f82a9af3b968905144ec&reaction=dislike'>👎</a>
##########
superset/views/base_api.py:
##########
@@ -501,8 +501,15 @@ def _add_extra_ids_to_result(
values = [row["value"] for row in result]
ids = [id_ for id_ in ids if id_ not in values]
pk_col = datamodel.get_pk()
- # Fetch requested values from ids
- extra_rows =
db.session.query(datamodel.obj).filter(pk_col.in_(ids)).all()
+ # Fetch requested values from ids, applying the same scoping as the
+ # unforced query so ``include_ids`` cannot resolve rows the
+ # related-field filters deliberately hide.
+ query = db.session.query(datamodel.obj).filter(pk_col.in_(ids))
+ if base_filters :=
self.base_related_field_filters.get(column_name):
+ query = datamodel.apply_filters(
+ query,
datamodel.get_filters().add_filter_list(base_filters)
+ )
Review Comment:
**Suggestion:** `add_filter_list` mutates the `Filters` instance and returns
`None`; passing that return value to `datamodel.apply_filters` causes forced-ID
related queries with configured base filters to fail at runtime or skip the
intended scoping, depending on the callee's handling of `None`. Build the
filters instance first, call `add_filter_list` on it, and then pass that
instance to `apply_filters`. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Forced-ID related lookups can fail at runtime.
- ❌ Hidden principals may bypass related-field scoping.
- ⚠️ Dashboard and asset picker requests are affected.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the shared `related()` endpoint in
`superset/views/base_api.py:624-686` with
`include_ids` for a resource and related column whose API defines
`base_related_field_filters`; the endpoint invokes
`_add_extra_ids_to_result()` at
`superset/views/base_api.py:684-686`.
2. For example, dashboard-related APIs configure base related filters in
`superset/superset/dashboards/api.py:474-476`, so the `viewers` or
`created_by` related
lookup can enter this branch when forced IDs are requested.
3. `_add_extra_ids_to_result()` constructs the forced-ID query at
`superset/views/base_api.py:507-508`, then calls
`datamodel.get_filters().add_filter_list(base_filters)` at
`superset/views/base_api.py:509-511`.
4. `add_filter_list()` mutates the `Filters` object, as shown by the
repository's normal
usage at `superset/views/base_api.py:53-54` and
`superset/views/base_api.py:65-66`, where
the object is created first and then mutated. Its return value is therefore
`None`; the
current code passes `None` into `datamodel.apply_filters()`, causing the
forced-ID request
to fail or preventing the configured related-field filter from being
applied. Create the
filters object first, call `add_filter_list(base_filters)`, and pass that
object to
`apply_filters()`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e3fcf8fbba2d449aa094184eb34c3e31&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=e3fcf8fbba2d449aa094184eb34c3e31&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/base_api.py
**Line:** 508:511
**Comment:**
*Api Mismatch: `add_filter_list` mutates the `Filters` instance and
returns `None`; passing that return value to `datamodel.apply_filters` causes
forced-ID related queries with configured base filters to fail at runtime or
skip the intended scoping, depending on the callee's handling of `None`. Build
the filters instance first, call `add_filter_list` on it, and then pass that
instance to `apply_filters`.
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%2F42472&comment_hash=59d4bc90baa1f53261bcc94dae9b2f1c2964a58d09b49cc505897d6a14b8b9e4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=59d4bc90baa1f53261bcc94dae9b2f1c2964a58d09b49cc505897d6a14b8b9e4&reaction=dislike'>👎</a>
##########
superset/subjects/utils.py:
##########
@@ -79,6 +80,117 @@ def get_user_subject_ids_subquery(user_id: int) ->
CompoundSelect:
return union_all(user_subj, role_subj, group_subj, group_role_subj)
+def get_user_group_subject_ids_subquery(user_id: int) -> Select:
+ """Return a Select of GROUP-type Subject IDs for a user's groups.
+
+ Unlike :func:`get_user_subject_ids_subquery` this covers group membership
+ only — it excludes the user's own subject and any roles.
+ """
+ from flask_appbuilder.security.sqla.models import assoc_user_group
+
+ return (
+ select(Subject.id)
+ .join(assoc_user_group, Subject.group_id ==
assoc_user_group.c.group_id)
+ .where(
+ assoc_user_group.c.user_id == user_id,
+ Subject.type == SubjectType.GROUP,
+ )
+ )
+
+
+def get_user_group_subjects(user_id: int) -> list[Subject]:
+ """Return the GROUP-type Subjects for every group a user belongs to.
+
+ Any group whose Subject row is missing is backfilled on demand (see
+ :func:`get_or_create_group_subject`) rather than skipped: a single viewer
+ disables an asset's datasource-access fallback, so a partial viewer set
+ would silently lock out the members of an unsynced group.
+ """
+ from flask_appbuilder.security.sqla.models import assoc_user_group
+
+ subjects = (
+ db.session.query(Subject)
+ .filter(Subject.id.in_(get_user_group_subject_ids_subquery(user_id)))
+ .all()
+ )
+ member_group_ids = {
+ row[0]
+ for row in db.session.execute(
+ select(assoc_user_group.c.group_id).where(
+ assoc_user_group.c.user_id == user_id
+ )
+ )
+ }
+ for group_id in member_group_ids - {subject.group_id for subject in
subjects}:
+ if subject := get_or_create_group_subject(group_id):
+ subjects.append(subject)
Review Comment:
**Suggestion:** Calling `begin_nested()` flushes all pending objects in the
session before establishing the SAVEPOINT. During imports or other multi-object
operations, resolving a missing group subject can therefore flush unrelated
pending assets and trigger their integrity errors before the savepoint can
isolate the subject backfill, potentially leaving the surrounding transaction
failed. Avoid performing this backfill through the caller's active session or
establish isolation before pending work is present. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Asset creation or imports can fail from unrelated pending objects.
- ⚠️ Group-subject backfill cannot isolate its database failure safely.
- ⚠️ Bulk dashboard and chart imports share this session behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable `ASSIGN_CREATOR_GROUPS_AS_VIEWERS` and create or import an asset
through callers
such as `superset/models/helpers.py:647-671` (`reset_ownership`) or
`superset/commands/dashboard/importers/v1/utils.py:450-471`
(`import_dashboard`).
2. Use a creator belonging to a group whose Subject row is missing, causing
`superset/subjects/utils.py:124-126` in `get_user_group_subjects()` to call
`get_or_create_group_subject()`.
3. `get_or_create_group_subject()` at `superset/subjects/utils.py:317-334`
enters
`db.session.begin_nested()` at line 330; SQLAlchemy flushes all pending
session objects
before establishing that SAVEPOINT.
4. If another pending asset, relationship, or import object violates an
integrity
constraint, that flush occurs before the SAVEPOINT can isolate the subject
backfill, so
the unrelated integrity failure propagates through the creator-group
defaulting path and
can abort the surrounding create/import transaction.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9c25700311e74204a868ac8e14812930&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=9c25700311e74204a868ac8e14812930&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/subjects/utils.py
**Line:** 124:126
**Comment:**
*Logic Error: Calling `begin_nested()` flushes all pending objects in
the session before establishing the SAVEPOINT. During imports or other
multi-object operations, resolving a missing group subject can therefore flush
unrelated pending assets and trigger their integrity errors before the
savepoint can isolate the subject backfill, potentially leaving the surrounding
transaction failed. Avoid performing this backfill through the caller's active
session or establish isolation before pending work is present.
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%2F42472&comment_hash=fbc7c08688ee468ceed1d8a0885d71337987c97d0a297be4e6bcb376a142b446&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=fbc7c08688ee468ceed1d8a0885d71337987c97d0a297be4e6bcb376a142b446&reaction=dislike'>👎</a>
##########
superset/subjects/utils.py:
##########
@@ -176,6 +288,76 @@ def get_or_create_role_subject(role_id: int) -> Subject |
None:
return get_role_subject(role_id)
+def get_group_subject(group_id: int) -> Subject | None:
+ """Get the GROUP-type Subject for a given group ID."""
+ return (
+ db.session.query(Subject)
+ .filter_by(
+ group_id=group_id,
+ type=SubjectType.GROUP,
+ )
+ .first()
+ )
+
+
+def get_or_create_group_subject(group_id: int) -> Subject | None:
+ """Get or create the GROUP-type Subject for a given group ID.
+
+ Mirrors ``get_or_create_role_subject``: a group that exists but has no
+ Subject row yet (e.g. created before the sync hooks were installed and not
+ yet backfilled) is synced on demand rather than skipped. Returns ``None``
+ only when no such group exists.
+
+ The insert runs inside a SAVEPOINT. ``Subject.group_id`` is unique, so a
+ concurrent request backfilling the same group would otherwise fail this
+ flush with an ``IntegrityError`` and poison the surrounding transaction. On
+ that conflict the savepoint rolls back and the row the other request wrote
+ is reloaded instead.
+ """
+ if subject := get_group_subject(group_id):
+ return subject
+
+ from sqlalchemy.exc import IntegrityError
+
+ from superset import security_manager
+ from superset.subjects.sync import sync_group_subject
+
+ group = db.session.get(security_manager.group_model, group_id)
+ if not group:
+ return None
+
+ try:
+ with db.session.begin_nested():
+ sync_group_subject(group)
+ except IntegrityError:
+ pass
Review Comment:
**Suggestion:** The handler swallows every `IntegrityError`, not only the
expected concurrent unique-key conflict. Any other subject-creation failure,
such as invalid group-derived data, is converted into a missing subject;
`get_user_group_subjects` then silently returns a partial viewer list, which
can disable datasource fallback while excluding members of the failed group.
Restrict handling to the specific duplicate-row conflict and propagate other
integrity failures. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ⚠️ New dashboards may omit one creator group’s viewers.
- ⚠️ Imported charts and dashboards can receive incomplete access.
- ❌ Partial viewers can alter datasource-access fallback behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable `ASSIGN_CREATOR_GROUPS_AS_VIEWERS` and invoke
`get_default_viewers_for_new_asset()` through
`superset/models/helpers.py:647-671` or the
dashboard importer at
`superset/commands/dashboard/importers/v1/utils.py:450-471`.
2. Ensure the creator belongs to multiple groups and one group has no
Subject row, causing
`get_user_group_subjects()` at `superset/subjects/utils.py:124-126` to invoke
`get_or_create_group_subject()`.
3. At `superset/subjects/utils.py:329-332`, make `sync_group_subject()` fail
with any
`IntegrityError`; the function can raise while creating the non-null
`Subject.label` or
`Subject.type` fields defined at `superset/subjects/models.py:49-54`, or
while inserting
the unique `group_id` defined at `superset/subjects/models.py:67-72`.
4. The unconditional handler at `superset/subjects/utils.py:332-333`
suppresses the error,
then line 334 returns no Subject; the caller retains only the other groups,
producing a
partial viewer list instead of failing or retrying the backfill.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1085c1cf90f048919ccd149b45b061a0&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=1085c1cf90f048919ccd149b45b061a0&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/subjects/utils.py
**Line:** 329:333
**Comment:**
*Possible Bug: The handler swallows every `IntegrityError`, not only
the expected concurrent unique-key conflict. Any other subject-creation
failure, such as invalid group-derived data, is converted into a missing
subject; `get_user_group_subjects` then silently returns a partial viewer list,
which can disable datasource fallback while excluding members of the failed
group. Restrict handling to the specific duplicate-row conflict and propagate
other integrity failures.
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%2F42472&comment_hash=984e9d54fd35e0ef978f3929c984affd036d48e949b876e80964c5bca16f9c42&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42472&comment_hash=984e9d54fd35e0ef978f3929c984affd036d48e949b876e80964c5bca16f9c42&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]
