mikebridge commented on PR #43829: URL: https://github.com/apache/superset/pull/43829#issuecomment-5542572009
## Review — Schema filter on the combined datasource list Reviewed at head `f0eb781a8f` with two independent multi-lens passes (six Claude lenses + a blind six-lens Codex run), reconciled with fresh diff verification. **The core fix is sound and better-tested than most PRs this size** — the bound predicate composes after `get_dataset_access_filters` so it can only narrow (we verified the security claim rather than assuming it), count and page rows share the filtered subquery, and the unique-suffix exact-set integration assertion plus the views-only-user guard drew explicit praise from multiple lenses. **Suggest before merge** (all small, in functions this PR already edits): 1. **Schema × semantic-layer composition** (all 12 lens-runs converged): `_resolve_source_type` returns `semantic_layer` — explicitly, or implicitly via the `semantic_layer_uuid` narrowing at `list.py:114-120` — before consulting `schema_filter`, so every visible semantic view is returned under an active Schema chip. That contradicts the honest-AND rule the PR itself applies three lines later for `type_filter="semantic_view"` + schema. Suggest `"empty"` for every semantic-layer-only source with an active schema filter, both routes, with tests (the pinned explicit-precedence test would flip); the implicit route is currently untested. 2. **The mirror inside the views-only branch**: `sql_filter` is equally dataset-only but falls through to `return "semantic_layer"` (`list.py:216`) — a views-only user with Type=Physical gets the full semantic-view list with the filter dropped. One condition: `if schema_filter is not None or sql_filter is not None: return "empty"`. 3. **Hygiene nits**: move the deferred `from superset.models.core import Database` in `tests/integration_tests/datasource/api_tests.py:122` to module top (the file already top-imports heavier superset modules); add `-> None` to the new test; tick `Changes UI` in the PR description. **Follow-up ticket, not this PR**: the combined endpoint also silently drops **Editors, Certified, and Modified-by** — filters the Datasets page sends today and the canonical `/api/v1/dataset/` honors. Pre-existing, same wrong-results class; worth one ticket to honor them or reject unknown filter columns with a 400 so the next gap is loud. Non-blocking polish noted in the full report (7-tuple → `ParsedFilters` dataclass at the next growth ring, `str(value)` type guard, comment scoping): available on request. *Reviewed with Claude Code (multi-lens + blind Codex cross-check + reconciliation), on Mike's behalf.* -- 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]
