codeant-ai-for-open-source[bot] commented on code in PR #41580:
URL: https://github.com/apache/superset/pull/41580#discussion_r3498736381
##########
superset/commands/datasource/list.py:
##########
@@ -193,53 +199,59 @@ def _resolve_source_type(
@staticmethod
def _parse_filters(
filters: list[dict[str, Any]],
- ) -> tuple[str, str | None, bool | None, str | None, int | None, str |
None]:
+ ) -> _Filters:
"""
Translate raw rison filter dicts into typed query parameters.
- Returns:
- source_type: "all" | "database" | "semantic_layer"
+ Returns a ``_Filters`` dataclass with the following fields:
+
+ source_type: ``"all"`` | ``"database"`` |
``"semantic_layer"``
name_filter: substring to match against name/table_name
- sql_filter: True → physical only, False → virtual only,
None → both
- type_filter: "semantic_view" when caller wants only
- semantic views
+ sql_filter: ``True`` → physical only, ``False`` → virtual
only, ``None`` → both
+ type_filter: ``"semantic_view"`` when caller wants only
semantic views
database_id: filter datasets to a specific database ID
semantic_layer_uuid: filter semantic views to a specific semantic
layer UUID
+ schema_filter: filter datasets by schema name
+ owners_filter: filter datasets by owner user IDs
+ changed_by_filter: filter datasets by last-modified user ID
+ certified_filter: ``True`` → certified only, ``False`` →
uncertified only, ``None`` → both
"""
- source_type = "all"
- name_filter: str | None = None
- sql_filter: bool | None = None
- type_filter: str | None = None
- database_id: int | None = None
- semantic_layer_uuid: str | None = None
+ result = _Filters()
for f in filters:
col = f.get("col")
opr = f.get("opr")
value = f.get("value")
if col == "source_type":
- source_type = value or "all"
+ result.source_type = value or "all"
elif col == "table_name" and f.get("opr") == "ct":
- name_filter = value
+ result.name_filter = value
elif col == "sql":
if opr == "dataset_is_null_or_empty" and value ==
"semantic_view":
- type_filter = "semantic_view"
+ result.type_filter = "semantic_view"
elif opr == "dataset_is_null_or_empty" and isinstance(value,
bool):
- sql_filter = value
+ result.sql_filter = value
elif col == "database" and value is not None:
try:
- database_id = int(value)
+ result.database_id = int(value)
except (TypeError, ValueError):
pass
elif col == "semantic_layer_uuid" and value is not None:
- semantic_layer_uuid = str(value)
-
- return (
- source_type,
- name_filter,
- sql_filter,
- type_filter,
- database_id,
- semantic_layer_uuid,
- )
+ result.semantic_layer_uuid = str(value)
+ elif col == "schema" and opr == "eq":
+ result.schema_filter = value
+ elif col == "owners" and opr == "rel_m_m":
+ try:
+ result.owners_filter = [int(value)]
+ except (TypeError, ValueError):
+ pass
+ elif col == "changed_by" and opr == "rel_o_m":
+ try:
+ result.changed_by_filter = int(value)
+ except (TypeError, ValueError):
+ pass
+ elif col == "id" and opr == "dataset_is_certified":
+ result.certified_filter = bool(value)
Review Comment:
**Suggestion:** Coercing the certified filter with `bool(value)` is unsafe
because non-empty strings like `"false"` become `True`, which inverts filter
behavior and returns certified datasets instead of uncertified ones. Preserve
the tri-state contract by only accepting real booleans (or explicitly parsing
known string forms) before assigning this field. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Certified filter may return opposite dataset set.
- ⚠️ Tri-state certified filtering contract is broken.
- ⚠️ External callers using string values get misfiltered results.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The combined datasource list endpoint `combined_list` in
`superset/datasource/api.py:2-11` exposes `GET /api/v1/datasource/` and
accepts rison
filters using the same custom `dataset_is_certified` operator that the
dataset API uses
(`superset/datasets/filters.py:38-42` and `superset/datasets/api.py:15-18`).
2. A client issues `GET /api/v1/datasource/?q=<rison>` with a certified
filter whose
`value` is not a native boolean, for example `{"filters": [{"col": "id",
"opr":
"dataset_is_certified", "value": "false"}]}`, which is a realistic payload
when the value
is serialized as a string from the UI or an external integration.
3. `combined_list` constructs `GetCombinedDatasourceListCommand` and calls
`.run()` at
`superset/datasource/api.py:41-46`; `_parse_filters` is invoked at
`superset/commands/datasource/list.py:199-256`. When it reaches the
certified branch at
lines 254-255 (`elif col == "id" and opr == "dataset_is_certified":`), the
code assigns
`result.certified_filter = bool(value)`. For a non-empty string such as
`"false"` or
`"true"`, `bool(value)` evaluates to `True`, so user intent `value="false"`
(meaning
uncertified datasets) is misinterpreted as `certified_filter=True`.
4. `_build_combined_query` at `superset/commands/datasource/list.py:119-138`
forwards this
mis-coerced `certified_filter=True` into `DatasourceDAO.build_dataset_query`
at
`superset/daos/datasource.py:95-105`. The certified filtering logic at
`superset/daos/datasource.py:150-160` then applies the `"certification"`
ilike condition
(`SqlaTable.extra.isnot(None) &
SqlaTable.extra.ilike('%"certification":%')`), returning
only certified datasets even though the caller requested uncertified ones;
similarly, any
other truthy non-boolean value is treated as `True`, and falsey non-None
values are
treated as `False`, collapsing the intended tri-state contract
(True/False/None) into a
two-state interpretation and inverting filters when string values are used.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=835cea5a7ab24529ae58578bceb189d3&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=835cea5a7ab24529ae58578bceb189d3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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:** 254:255
**Comment:**
*Incorrect Condition Logic: Coercing the certified filter with
`bool(value)` is unsafe because non-empty strings like `"false"` become `True`,
which inverts filter behavior and returns certified datasets instead of
uncertified ones. Preserve the tri-state contract by only accepting real
booleans (or explicitly parsing known string forms) before assigning this field.
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%2F41580&comment_hash=0d3b4ce207819884107ca7a4af3cb0670ba2bd97f58583611daecf473a1ce845&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41580&comment_hash=0d3b4ce207819884107ca7a4af3cb0670ba2bd97f58583611daecf473a1ce845&reaction=dislike'>👎</a>
##########
superset/commands/datasource/list.py:
##########
@@ -193,53 +199,59 @@ def _resolve_source_type(
@staticmethod
def _parse_filters(
filters: list[dict[str, Any]],
- ) -> tuple[str, str | None, bool | None, str | None, int | None, str |
None]:
+ ) -> _Filters:
"""
Translate raw rison filter dicts into typed query parameters.
- Returns:
- source_type: "all" | "database" | "semantic_layer"
+ Returns a ``_Filters`` dataclass with the following fields:
+
+ source_type: ``"all"`` | ``"database"`` |
``"semantic_layer"``
name_filter: substring to match against name/table_name
- sql_filter: True → physical only, False → virtual only,
None → both
- type_filter: "semantic_view" when caller wants only
- semantic views
+ sql_filter: ``True`` → physical only, ``False`` → virtual
only, ``None`` → both
+ type_filter: ``"semantic_view"`` when caller wants only
semantic views
database_id: filter datasets to a specific database ID
semantic_layer_uuid: filter semantic views to a specific semantic
layer UUID
+ schema_filter: filter datasets by schema name
+ owners_filter: filter datasets by owner user IDs
+ changed_by_filter: filter datasets by last-modified user ID
+ certified_filter: ``True`` → certified only, ``False`` →
uncertified only, ``None`` → both
"""
- source_type = "all"
- name_filter: str | None = None
- sql_filter: bool | None = None
- type_filter: str | None = None
- database_id: int | None = None
- semantic_layer_uuid: str | None = None
+ result = _Filters()
for f in filters:
col = f.get("col")
opr = f.get("opr")
value = f.get("value")
if col == "source_type":
- source_type = value or "all"
+ result.source_type = value or "all"
elif col == "table_name" and f.get("opr") == "ct":
- name_filter = value
+ result.name_filter = value
elif col == "sql":
if opr == "dataset_is_null_or_empty" and value ==
"semantic_view":
- type_filter = "semantic_view"
+ result.type_filter = "semantic_view"
elif opr == "dataset_is_null_or_empty" and isinstance(value,
bool):
- sql_filter = value
+ result.sql_filter = value
elif col == "database" and value is not None:
try:
- database_id = int(value)
+ result.database_id = int(value)
except (TypeError, ValueError):
pass
elif col == "semantic_layer_uuid" and value is not None:
- semantic_layer_uuid = str(value)
-
- return (
- source_type,
- name_filter,
- sql_filter,
- type_filter,
- database_id,
- semantic_layer_uuid,
- )
+ result.semantic_layer_uuid = str(value)
+ elif col == "schema" and opr == "eq":
+ result.schema_filter = value
+ elif col == "owners" and opr == "rel_m_m":
+ try:
+ result.owners_filter = [int(value)]
+ except (TypeError, ValueError):
+ pass
Review Comment:
**Suggestion:** The `rel_m_m` owners filter currently assumes a scalar value
and silently drops the filter when the API sends the standard multi-value
payload (e.g. a list of owner IDs). This causes owner filtering to be ignored
for valid requests. Parse both scalar and list inputs into a list of ints so
many-to-many filtering works reliably. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Combined datasource list ignores multi-owner filters.
- ⚠️ Users see datasets beyond selected owners.
- ⚠️ Owner-based auditing of datasets becomes unreliable.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The combined datasource list endpoint `combined_list` in
`superset/datasource/api.py:2-11` is exposed as `GET /api/v1/datasource/`
with
rison-parsed query arguments via `@rison(get_list_schema)` at
`superset/datasource/api.py:6`.
2. A client (or the Superset UI) calls `GET /api/v1/datasource/?q=<rison>`
with a filters
payload containing an owners many-to-many filter, e.g. `{"filters": [{"col":
"owners",
"opr": "rel_m_m", "value": [admin.id, gamma.id]}]}`, matching the
established pattern in
`tests/integration_tests/dashboards/api_tests.py:861` for `rel_m_m` owner
filters.
3. `combined_list` constructs `GetCombinedDatasourceListCommand` and calls
`.run()` at
`superset/datasource/api.py:41-46`, which in turn calls
`_parse_filters(self._args.get("filters", []))` at
`superset/commands/datasource/list.py:71-79` to translate the raw rison
filters.
4. Inside `_parse_filters` at
`superset/commands/datasource/list.py:221-256`, when it
reaches the owners branch at lines 244-248 (`elif col == "owners" and opr ==
"rel_m_m":`),
`value` is a list of user IDs (e.g. `[admin.id, gamma.id]`), so `int(value)`
raises a
`TypeError`. The exception is caught, the `pass` at line 248 executes, and
`result.owners_filter` remains `None` instead of being populated with the
list of owner
IDs.
5. `_build_combined_query` at `superset/commands/datasource/list.py:119-138`
passes
`filters.owners_filter` (still `None`) into
`DatasourceDAO.build_dataset_query` at
`superset/daos/datasource.py:95-105`. Because `owners_filter is not None`
check at
`superset/daos/datasource.py:141-145` fails, the join on
`sqla_models.sqlatable_user` and
the `user_id.in_(owners_filter)` predicate are never applied, so the
resulting combined
list ignores the owners many-to-many filter and returns datasets owned by
any user.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=413fab80e8894160993ea12afea99f3e&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=413fab80e8894160993ea12afea99f3e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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:** 244:248
**Comment:**
*Api Mismatch: The `rel_m_m` owners filter currently assumes a scalar
value and silently drops the filter when the API sends the standard multi-value
payload (e.g. a list of owner IDs). This causes owner filtering to be ignored
for valid requests. Parse both scalar and list inputs into a list of ints so
many-to-many filtering works reliably.
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%2F41580&comment_hash=81424dcf36bed3cc68a5357f2e55ac635a85bd51a00e7a8a0b49126e8fdc5ef0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41580&comment_hash=81424dcf36bed3cc68a5357f2e55ac635a85bd51a00e7a8a0b49126e8fdc5ef0&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: [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]
