tien238lnd opened a new pull request, #45175:
URL: https://github.com/apache/superset/pull/45175
### SUMMARY
The Datasets list loads its rows from `GET /api/v1/datasource/`.
`_parse_filters` of that endpoint only knew `source_type`, `table_name`, `sql`,
`database`, `semantic_layer_uuid` and (since #43829) `schema`; any other filter
was dropped without an error, so the **Editor**, **Modified by** and
**Certified** filters returned the whole list while the chip stayed lit.
Details and the history of the endpoint are in #45174.
What this changes:
- `_parse_filters` returns a typed `_Filters` dataclass (replacing the
growing positional tuple) and accepts three more filters, with the same column
and operator names the page sends to `/api/v1/dataset/`:
- `editors` / `rel_m_m`, compared with `Subject.id` (the value the Editor
dropdown sends). The predicate is `table.id IN (SELECT table_id FROM
sqlatable_editors WHERE subject_id = :id)`, so a dataset with several editors
is returned once and user, role and group editors all match.
- `changed_by` / `rel_o_m`, applied to datasets and to semantic views
(both are audited).
- `id` / `dataset_is_certified`, using a new `dataset_certified_clause()`
that `DatasetCertifiedFilter` now shares, so both endpoints agree on what
"certified" means.
- A column the endpoint cannot filter on, and a wrong operator or value on
the three new columns, answers `400` with the same messages as
`/api/v1/dataset/`, instead of being ignored.
- Semantic views have no schema, no editors and are never certified, so
under AND semantics those filters exclude them (`_Filters.dataset_only`, which
feeds the existing source-type resolution). `certified = false` keeps semantic
views, because they are not certified.
This overlaps with #41580, which restores the same filters. It could not be
applied as is: it matches `owners` where the page sends `editors` since #38831,
and it compares `Subject.user_id`, so the Editor filter would still not match
(and role/group editors never would). The typed `_Filters` object here follows
the one introduced in that PR, which is also why this should not conflict with
it in spirit; if #41580 is revived it would need the two fixes above.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Backend only. On a deployment with about 300 datasets, `GET
/api/v1/datasource/?q=(filters:!((col:editors,opr:rel_m_m,value:<id>)),page:0,page_size:25)`
returned `count: 305` before and `count: 35` after, the same set as
`/api/v1/dataset/`.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/commands/datasource/list_test.py
tests/unit_tests/dao/datasource_test.py
```
The new DAO tests run against real rows: a dataset with several editors of
different subject types, `changed_by` on datasets and on semantic views, and
certified / warning / plain `extra`.
Manual: on the Datasets page pick an Editor, a Modified by user and a
Schema, and check that the list narrows. Send `col:owners` to
`/api/v1/datasource/` and expect `400`.
### ADDITIONAL INFORMATION
- [x] Has associated issue: part of #45174
- [ ] Required feature flags:
- [ ] 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
### Questions for reviewers
cc @betodealmeida (combined endpoint) and @mikebridge (reviewed #43829 and
#43887):
1. Is `400` for an unknown filter column acceptable for
`/api/v1/datasource/`? It matches `/api/v1/dataset/` and is what makes a future
frontend/backend mismatch visible, but it is a behavior change for API callers
(noted in `UPDATING.md`).
2. The older columns (`table_name`, `sql`, `schema`) still ignore a wrong
operator, because the tests added in #43829 pin that. Should they be made
strict too, in this PR or a follow-up?
3. Is the semantic-view reading right: dataset-only filters exclude semantic
views, `changed_by` applies to both, `certified = false` keeps them?
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
