EnxDev opened a new pull request, #43988:
URL: https://github.com/apache/superset/pull/43988
### SUMMARY
Creating a chart from a SQL Lab query currently returns a 500 error:
```text
File "superset/models/slice.py", line 504, in set_related_perm
target.catalog_perm = ds.catalog_perm
AttributeError: 'Query' object has no attribute 'catalog_perm'
```
`set_related_perm` is a `before_insert`/`before_update` listener that copies
`perm`, `catalog_perm`, and `schema_perm` from the datasource to the chart.
This allows the access filter in `superset/utils/filters.py` to scope charts
without performing a join.
The listener assumed that every datasource model had all three permission
attributes, but that is not the case:
| `datasource_type` | Model | `perm` | `catalog_perm` | `schema_perm` |
|---|---|---|---|---|
| `table` | `SqlaTable` | ✅ | ✅ | ✅ |
| `semantic_view` | `SemanticView` | ✅ | ✅ | ✅ |
| `query` | `Query` | ✅ | ❌ | ✅ |
| `saved_query` | `SavedQuery` | ❌ | ❌ | ❌ |
| `dataset` | Unmapped | — | — | — |
| `view` | Unmapped | — | — | — |
`catalog_perm` is only defined by `SqlaTable` and `SemanticView`, so
accessing it on a SQL Lab `Query` raises an `AttributeError`. The unconditional
assignment was introduced in #29840; before that change, query-backed charts
simply left the column set to its default `NULL` value.
While investigating this issue, I found two related failures in the same
function. The chart API accepts these datasource types, but the listener does
not handle them:
- `saved_query` raises an `AttributeError` when reading `perm`.
- `dataset` and `view` raise a `KeyError` because they are not registered in
`DatasourceDAO.sources`.
These failures all come from the same assumption, so this PR handles them
together.
**Change:** Read each permission attribute with a default value of `None`.
If the datasource type is not registered, skip the update and log a warning
instead of raising an exception.
**Design decisions:**
- **Use `NULL` instead of deriving a permission value.** This restores the
behavior from before #29840 and remains fail-closed. The access filter uses
conditions such as `<perm>.in_(...)`, and `NULL IN (...)` does not evaluate to
true. Leaving a permission unset therefore cannot make a chart visible through
that permission. Access can still match through predicates such as
`Database.id.in_(...)` or `perm`.
- **Do not derive `catalog_perm` for `Query`.** `Query` has `catalog` and
`database` columns, so deriving the value would be possible. However, doing so
could widen access, which is outside the scope of this 500-error fix. It would
also be inconsistent with `Query.schema_perm`, which currently returns the
legacy `f"{db}.{schema}"` format instead of using
`security_manager.get_schema_perm(...)`. Resolving that format mismatch is a
separate issue.
The original ticket suggested that `ds` was an unexecuted SQLAlchemy `Query`
object or that the problem was related to session management. That is not the
cause: `ds` resolves to the expected datasource model instance. The `Query`
name in the error refers to `superset.models.sql_lab.Query`.
This listener previously had no direct test coverage. This PR adds
parametrized coverage for all six accepted datasource types.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only change with no UI impact.
### TESTING INSTRUCTIONS
Automated:
```bash
pytest tests/unit_tests/models/slice_test.py
```
When run against `master`, the new cases for `query`, `saved_query`,
`dataset`, and `view` fail with the production `AttributeError` or `KeyError`.
The `table` and `semantic_view` cases pass both before and after this change,
protecting the existing behavior from regressions.
Manual:
1. In SQL Lab, run a query such as `SELECT * FROM birth_names LIMIT 10`.
2. From the results pane, click **Create Chart** to explore the query
results.
3. Save the chart.
4. Confirm that the chart saves successfully. Before this change, the
request returns a 500 with `AttributeError: 'Query' object has no attribute
'catalog_perm'`.
5. Confirm that the normal dataset flow still works: create and save a chart
from a regular dataset, then verify that `perm`, `catalog_perm`, and
`schema_perm` are populated on the corresponding `slices` row.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow the approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback, and is backward-compatible
- [ ] Database migration upgrade and downgrade have been tested
- [ ] Runtime estimates and expected downtime are documented
- [ ] 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]
