geido opened a new pull request, #44392:
URL: https://github.com/apache/superset/pull/44392
### SUMMARY
Deleting a database connection opens a confirmation modal that enumerates
the objects the delete will affect. That preview counted charts, dashboards and
SQL Lab tabs — but not **datasets**, because `GET
/api/v1/database/<id>/related_objects/` never returned them.
That is the one dependency class the delete actually refuses on.
`DeleteDatabaseCommand.validate()` raises
`DatabaseDeleteDatasetsExistFailedError` (HTTP 422) while any `SqlaTable` row
references the database, so for a connection whose only dependents are datasets
the modal read:
> The database **qa_warehouse** is linked to 0 charts that appear on 0
dashboards and users have 0 SQL Lab tabs using this database open. **Are you
sure you want to continue?** Deleting the database will break those objects.
…which asserts nothing will break, and gates a confirmation that cannot
succeed. The user types `DELETE`, confirms, and gets `There was an issue
deleting qa_warehouse: Cannot delete a database that has datasets attached` —
with the same misleading modal still on screen.
This adds datasets to the preview and rewords the confirmation when they are
present:
- `DatabaseDAO.get_related_objects` returns the `SqlaTable` rows referencing
the database. It queries them under `skip_visibility_filter`, exactly as
`DeleteDatabaseCommand.validate()` does — `Database.tables` hides soft-deleted
datasets, which would otherwise let the preview report zero dependents for a
database the delete still refuses.
- `related_objects` gains a `datasets` block (`count`, `result[{id,
table_name, schema}]`) and `DatabaseRelatedObjectsResponse` documents it.
- The modal names the datasets under **Affected Datasets** and states the
connection cannot be deleted until they are removed, instead of asking the user
to confirm.
**Design decision worth reviewing:** dataset *names* are access-filtered
with `security_manager.can_access_datasource`, the same way chart and dashboard
names already are. The *count* is deliberately left unfiltered. This route only
requires `can_read` on `Database`, and `DatabaseFilter` admits a caller holding
`datasource_access` on a single dataset in that database — returning every name
would let them enumerate datasets they hold no permission on. The count has to
stay unfiltered because it is what explains the delete being blocked, and a
bare number discloses far less than a name and schema. Both halves are pinned
by tests.
The frontend tolerates a backend that predates the `datasets` block
(`json.datasets ?? { count: 0, result: [] }`), so a version-skewed deployment
degrades to the previous copy rather than throwing.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Verified in a running instance against a connection (`qa_warehouse`) whose
only dependents are two datasets — no charts, no dashboards, no SQL Lab tabs.
Exact modal text, captured from the app in both states:
**Before** — the preview reports nothing attached, and invites the delete:
> **Delete Data connection?**
> The data connection **qa_warehouse** is linked to 0 charts that appear on
0 dashboards and users have 0 SQL Lab tabs using this database open. Are you
sure you want to continue? Deleting the database will break those objects.
Typing `DELETE` and confirming then fails, because the backend refuses it:
> There was an issue deleting qa_warehouse: Cannot delete a database that
has datasets attached
**After** — the preview names what is actually blocking the delete:
> **Delete Data connection?**
> The data connection **qa_warehouse** cannot be deleted because 2 datasets
are still attached to it. Delete or move those datasets first.
>
> **Affected Datasets**
> • birth_names
> • cleaned_sales_data
A connection with no datasets is byte-identical to before — the ordinary
"Are you sure you want to continue?" confirmation is untouched.
**Scope note for reviewers:** this changes the copy and names the datasets;
it does not disable the `Type "DELETE" to confirm` affordance in the blocked
state, so a user can still submit a request the backend will refuse. Worth
deciding whether the primary action should also be disabled when
`datasets.count >= 1`.
### TESTING INSTRUCTIONS
Automated:
```bash
# backend
pytest tests/unit_tests/databases/dao/related_objects_test.py
pytest tests/unit_tests/databases/api_test.py -k related_objects
# frontend
cd superset-frontend && npx jest src/pages/DatabaseList
```
Manual:
1. Add a database connection.
2. Create a dataset on it (Datasets → **+ Dataset**), and no charts.
3. Go to **Settings → Database Connections**, hover the connection, click
the trash icon.
4. The modal names the dataset under **Affected Datasets** and says the
connection cannot be deleted until it is removed. Before this change it
reported `0 charts / 0 dashboards / 0 SQL Lab tabs` and offered to continue.
5. Remove the dataset and reopen the modal — the ordinary "Are you sure you
want to continue?" confirmation is unchanged.
To check the access-filtering: as a user holding `datasource_access` on only
one of several datasets in the connection, the modal counts them all but names
only the accessible one.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
