You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "tien238lnd (via GitHub)" <gi...@apache.org> on 2026/10/10 18:10:16 UTC
[PR] fix(datasets): apply Editor, Modified by and Certified filters on the combined datasource list [superset]
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(datasets): apply Editor, Modified by and Certified filters on the combined datasource list [superset]
Posted by "tien238lnd (via GitHub)" <gi...@apache.org>.
tien238lnd commented on code in PR #45175:
URL: https://github.com/apache/superset/pull/45175#discussion_r4267221789
##########
superset/commands/datasource/list.py:
##########
@@ -298,63 +362,79 @@ def _apply_sql_null_filter(
return type_filter, sql_filter
@staticmethod
- def _parse_filters(
- filters: list[dict[str, Any]],
- ) -> tuple[
- str, str | None, bool | None, str | None, int | None, str | None, str | None
- ]:
+ def _parse_legacy_filter(
+ col: str, opr: Any, value: Any, current: dict[str, Any]
+ ) -> dict[str, Any]:
+ """Parse the columns the combined list supported from the start.
+
+ These stay lenient: an operator that is not the expected one is ignored
Review Comment:
Done in 38ee316: every column the combined list filters on now rejects an operator it does not apply and a value it cannot read with the same 400 as `/api/v1/dataset/` (`table_name` with `ct`, `schema` with `eq`, `sql` with `dataset_is_null_or_empty`, `database` with `rel_o_m`, `source_type` and `semantic_layer_uuid` with `eq`). The tests from #43829 that pinned the lenient behavior now assert the 400, and the `UPDATING.md` note lists the operators. The Datasets page only sends the expected operator for each column, and the integration tests only send `eq` for `source_type` and `schema`.
_๐ค Addressed by [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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(datasets): apply Editor, Modified by and Certified filters on the combined datasource list [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45175:
URL: https://github.com/apache/superset/pull/45175#discussion_r4238698595
##########
superset/commands/datasource/list.py:
##########
@@ -121,31 +174,34 @@ def _resolve_connection_source_type(
if database_id is not None:
return "database"
elif semantic_layer_uuid is not None:
- # A semantic-layer connection selects only that layer's
- # (schema-less) views, so a dataset-only schema filter matches
+ # A semantic-layer connection selects only that layer's views,
+ # so a dataset-only filter (schema, editors, certified) matches
# nothing: the honest result is empty. Unlike an explicit
# Source="Semantic layer" selection (handled in
# _resolve_source_type), the user never picked a source type
# here, so the "explicit selection wins" rule does not apply.
- if schema_filter is not None:
+ if dataset_only:
Review Comment:
**Suggestion:** With a semantic-layer UUID and a SQL toggle, this check misses the dataset-only filter; the connection selects views and silently drops the SQL filter.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Incorrect condition logic`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e49c1df010cc4c929d9fecaae4913ad6&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=e49c1df010cc4c929d9fecaae4913ad6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/datasource/list.py
**Line:** 183:183
**Comment:**
*Incorrect Condition Logic: With a semantic-layer UUID and a SQL toggle, this check misses the dataset-only filter; the connection selects views and silently drops the SQL filter.
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%2F45175&comment_hash=86e937616a4ee6eaf36b831bf891760334824c4e223e7dcd6d297c88e5d5fe6e&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45175&comment_hash=86e937616a4ee6eaf36b831bf891760334824c4e223e7dcd6d297c88e5d5fe6e&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(datasets): apply Editor, Modified by and Certified filters on the combined datasource list [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #45175:
URL: https://github.com/apache/superset/pull/45175#issuecomment-6100793569
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45175?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `46.80851%` with `50 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 65.29%. Comparing base ([`b9e8188`](https://app.codecov.io/gh/apache/superset/commit/b9e8188ad606f61317403c3be5ef9450dbe0e381?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`709cd43`](https://app.codecov.io/gh/apache/superset/commit/709cd43fe062c81a1f139ed9be50199dc3ea3973?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/45175?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/commands/datasource/list.py](https://app.codecov.io/gh/apache/superset/pull/45175?src=pr&el=tree&filepath=superset%2Fcommands%2Fdatasource%2Flist.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvZGF0YXNvdXJjZS9saXN0LnB5) | 48.68% | [32 Missing and 7 partials :warning: ](https://app.codecov.io/gh/apache/superset/pull/45175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/daos/datasource.py](https://app.codecov.io/gh/apache/superset/pull/45175?src=pr&el=tree&filepath=superset%2Fdaos%2Fdatasource.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGFvcy9kYXRhc291cmNlLnB5) | 20.00% | [4 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/superset/pull/45175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/datasets/filters.py](https://app.codecov.io/gh/apache/superset/pull/45175?src=pr&el=tree&filepath=superset%2Fdatasets%2Ffilters.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGF0YXNldHMvZmlsdGVycy5weQ==) | 62.50% | [1 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/superset/pull/45175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (b9e8188) and HEAD (709cd43). Click for more details.
>
> <details><summary>HEAD has 4 uploads less than BASE</summary>
>
>| Flag | BASE (b9e8188) | HEAD (709cd43) |
>|------|------|------|
>|python|7|3|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #45175 +/- ##
===========================================
- Coverage 82.85% 65.29% -17.56%
===========================================
Files 3017 3017
Lines 192923 192978 +55
Branches 44976 44988 +12
===========================================
- Hits 159841 126011 -33830
- Misses 30042 64164 +34122
+ Partials 3040 2803 -237
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/45175/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage ฮ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/45175/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `35.30% <32.97%> (+<0.01%)` | :arrow_up: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/45175/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/45175/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/45175/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.14% <32.97%> (+<0.01%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/45175/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.57% <46.80%> (-32.93%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/45175/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.45% <46.80%> (-0.01%)` | :arrow_down: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/45175/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/45175?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(datasets): apply Editor, Modified by and Certified filters on the combined datasource list [superset]
Posted by "tien238lnd (via GitHub)" <gi...@apache.org>.
tien238lnd commented on code in PR #45175:
URL: https://github.com/apache/superset/pull/45175#discussion_r4240176414
##########
superset/commands/datasource/list.py:
##########
@@ -121,31 +174,34 @@ def _resolve_connection_source_type(
if database_id is not None:
return "database"
elif semantic_layer_uuid is not None:
- # A semantic-layer connection selects only that layer's
- # (schema-less) views, so a dataset-only schema filter matches
+ # A semantic-layer connection selects only that layer's views,
+ # so a dataset-only filter (schema, editors, certified) matches
# nothing: the honest result is empty. Unlike an explicit
# Source="Semantic layer" selection (handled in
# _resolve_source_type), the user never picked a source type
# here, so the "explicit selection wins" rule does not apply.
- if schema_filter is not None:
+ if dataset_only:
Review Comment:
Fixed in ea063b8: the connection stage now also treats the physical/virtual (`sql`) toggle as dataset-only, so a semantic-layer connection plus the toggle resolves to an empty result instead of listing every view of the layer. Covered by `test_run_semantic_layer_connection_with_sql_filter_returns_empty`.
_๐ค Addressed by [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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(datasets): apply Editor, Modified by and Certified filters on the combined datasource list [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #45175:
URL: https://github.com/apache/superset/pull/45175#discussion_r4256176115
##########
superset/commands/datasource/list.py:
##########
@@ -298,63 +362,79 @@ def _apply_sql_null_filter(
return type_filter, sql_filter
@staticmethod
- def _parse_filters(
- filters: list[dict[str, Any]],
- ) -> tuple[
- str, str | None, bool | None, str | None, int | None, str | None, str | None
- ]:
+ def _parse_legacy_filter(
+ col: str, opr: Any, value: Any, current: dict[str, Any]
+ ) -> dict[str, Any]:
+ """Parse the columns the combined list supported from the start.
+
+ These stay lenient: an operator that is not the expected one is ignored
Review Comment:
This is the part I'd tighten. `schema` with any operator other than `eq`, or `table_name` with `eq`, still comes back as an unfiltered list that looks filtered, which is exactly the failure this PR removes for the new columns.
I'd make them strict too. If the #43829 tests pin the lenient behaviour a follow-up is fine, but it deserves an issue so it doesn't get lost.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(datasets): apply Editor, Modified by and Certified filters on the combined datasource list [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #45175:
URL: https://github.com/apache/superset/pull/45175#issuecomment-6100690477
## CodeAnt PR Risk: Medium Risk
- The PR needs attention before merging.
- A semantic-layer UUID combined with the SQL filter selects semantic views and silently drops the dataset-only filter.
**Assessed commit:** `709cd43fe062`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(datasets): apply Editor, Modified by and Certified filters on the combined datasource list [superset]
Posted by "tien238lnd (via GitHub)" <gi...@apache.org>.
tien238lnd commented on PR #45175:
URL: https://github.com/apache/superset/pull/45175#issuecomment-6196305106
Addressed @mikebridge's review in 0ca1e7e: an explicit `Source = Semantic layer` combined with Editor, or with `Certified = Yes`, now resolves to empty (the resolver checks it before returning the explicit source). `Certified = No` and Modified by still list views, and the Schema exception from #43829 is unchanged; regression tests cover each combination at the resolver and `run()` level. The description is updated: question 2 now reflects that the legacy columns were made strict in 38ee316. The OpenAPI drift check was master being red on #45071; master is green again and the branch has it merged.
_๐ค Addressed by [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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org