bito-code-review[bot] commented on code in PR #38807:
URL: https://github.com/apache/superset/pull/38807#discussion_r3609801715
##########
superset-frontend/src/pages/DatasetList/index.tsx:
##########
@@ -57,6 +57,8 @@ import {
DatasetTypeLabel,
Loading,
List,
+ RlsBadge,
+ type RlsFilterSummary,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test coverage for RLS badge</b></div>
<div id="fix">
The `DatasetList.testHelpers.tsx` `DatasetFixture` interface (line 60-81)
lacks the `rls_filters` field, meaning no tests validate the new RLS badge
functionality. Add test fixtures with `rls_filters` and assertions checking the
badge renders when filters are present and renders nothing when filters are
absent.
</div>
</div>
<small><i>Code Review Run #e75529</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/daos/dataset.py:
##########
@@ -666,6 +668,346 @@ def get_filterable_columns_and_operators(cls) ->
Dict[str, List[str]]:
filterable.update(DATASET_CUSTOM_FIELDS)
return filterable
+ @staticmethod
+ def get_rls_filters_for_datasets(
+ dataset_ids: list[int],
+ ) -> dict[int, list[dict[str, Any]]]:
+ """
+ Return a mapping of dataset_id -> list of RLS filter summaries
+ for the given dataset IDs. Only returns datasets that have at least
+ one RLS filter attached. For virtual datasets, also includes RLS
+ filters from physical tables referenced in the dataset's SQL.
+ """
+
+ if not dataset_ids:
+ return {}
+
+ # Get direct RLS filters for all requested datasets
+ rows = (
+ db.session.query(
+ RLSFilterTables.c.table_id,
+ RowLevelSecurityFilter.id,
+ RowLevelSecurityFilter.name,
+ RowLevelSecurityFilter.filter_type,
+ RowLevelSecurityFilter.group_key,
+ )
+ .join(
+ RowLevelSecurityFilter,
+ RLSFilterTables.c.rls_filter_id == RowLevelSecurityFilter.id,
+ )
+ .filter(RLSFilterTables.c.table_id.in_(dataset_ids))
+ .all()
+ )
+
+ result: dict[int, list[dict[str, Any]]] = {}
+ for table_id, rls_id, name, filter_type, group_key in rows:
+ result.setdefault(table_id, []).append(
+ {
+ "id": rls_id,
+ "name": name,
+ "filter_type": filter_type,
+ "group_key": group_key,
+ }
+ )
+
+ # For virtual datasets, also check underlying physical tables
+ virtual_datasets = (
+ db.session.query(
+ SqlaTable.id, SqlaTable.sql, SqlaTable.schema,
SqlaTable.database_id
+ )
+ .filter(SqlaTable.id.in_(dataset_ids), SqlaTable.sql.isnot(None))
# type: ignore[attr-defined,unused-ignore]
+ .all()
+ )
+
+ if virtual_datasets:
+ inherited = DatasetDAO._get_inherited_rls_for_virtual_datasets(
+ virtual_datasets
+ )
+ for ds_id, filters in inherited.items():
+ existing_ids = {f["id"] for f in result.get(ds_id, [])}
+ for f in filters:
+ if f["id"] not in existing_ids:
+ existing_ids.add(f["id"])
+ result.setdefault(ds_id, []).append(f)
+
+ return result
+
+ @staticmethod
+ def _parse_tables_from_virtual_datasets(
+ virtual_datasets: list[tuple[int, str, str | None, int]],
+ db_engines: dict[int, str] | None = None,
+ ) -> tuple[dict[int, set[Table]], dict[int, int]]:
+ """
+ Parse SQL from virtual datasets and return:
+ - ds_to_tables: mapping of dataset_id -> set of referenced Table
objects
+ (with schema/catalog preserved from the SQL, unqualified refs
resolved
+ to the virtual dataset's own schema)
+ - ds_db_map: mapping of dataset_id -> database_id
+ """
+ if db_engines is None:
+ db_engines = {}
+ ds_to_tables: dict[int, set[Table]] = {}
+ ds_db_map: dict[int, int] = {}
+ for ds_id, sql, default_schema, database_id in virtual_datasets:
+ ds_db_map[ds_id] = database_id
+ engine = db_engines.get(database_id, "")
+ try:
+ parsed = SQLScript(sql, engine=engine)
+ table_refs: set[Table] = set()
+ for statement in parsed.statements:
+ for table_ref in statement.tables:
+ # Qualify unqualified references with the virtual
dataset's
+ # own schema so we match the correct physical dataset.
+
table_refs.add(table_ref.qualify(schema=default_schema))
+ if table_refs:
+ ds_to_tables[ds_id] = table_refs
+ except Exception: # noqa: BLE001
+ logger.warning(
+ "Failed to parse SQL for virtual dataset %d", ds_id,
exc_info=True
+ )
+ return ds_to_tables, ds_db_map
+
+ @staticmethod
+ def _fetch_physical_rls_map(
+ all_tables: set[Table],
+ db_ids: set[int],
+ ) -> tuple[dict[tuple[str, str | None, int], int], dict[int,
list[dict[str, Any]]]]:
+ """
+ Look up physical datasets matching the given Table objects and
database IDs,
+ then fetch their RLS filters.
+
+ Returns:
+ - physical_map: (table_name, schema, database_id) -> physical dataset
id
+ - phys_rls: physical dataset id -> list of RLS filter summaries
+ """
+
+ all_table_names = {t.table for t in all_tables}
+ physical_tables = (
+ db.session.query(
+ SqlaTable.id,
+ SqlaTable.table_name,
+ SqlaTable.schema,
+ SqlaTable.database_id,
+ )
+ .filter(
+ SqlaTable.table_name.in_(all_table_names),
+ SqlaTable.database_id.in_(db_ids),
+ SqlaTable.sql.is_(None),
+ )
+ .all()
+ )
+
+ physical_map: dict[tuple[str, str | None, int], int] = {}
+ physical_ids: set[int] = set()
+ for phys_id, table_name, schema, db_id in physical_tables:
+ physical_map[(table_name, schema, db_id)] = phys_id
+ physical_ids.add(phys_id)
+
+ if not physical_ids:
+ return physical_map, {}
+
+ rls_rows = (
+ db.session.query(
+ RLSFilterTables.c.table_id,
+ RowLevelSecurityFilter.id,
+ RowLevelSecurityFilter.name,
+ RowLevelSecurityFilter.filter_type,
+ RowLevelSecurityFilter.group_key,
+ )
+ .join(
+ RowLevelSecurityFilter,
+ RLSFilterTables.c.rls_filter_id == RowLevelSecurityFilter.id,
+ )
+ .filter(RLSFilterTables.c.table_id.in_(physical_ids))
+ .all()
+ )
+
+ phys_rls: dict[int, list[dict[str, Any]]] = {}
+ for table_id, rls_id, name, filter_type, group_key in rls_rows:
+ phys_rls.setdefault(table_id, []).append(
+ {
+ "id": rls_id,
+ "name": name,
+ "filter_type": filter_type,
+ "group_key": group_key,
+ }
+ )
+ return physical_map, phys_rls
+
+ @staticmethod
+ def _get_inherited_rls_for_virtual_datasets(
+ virtual_datasets: list[tuple[int, str, str | None, int]],
+ ) -> dict[int, list[dict[str, Any]]]:
+ """
+ For virtual datasets, parse their SQL to find referenced physical
+ tables and return any RLS filters attached to those tables.
+
+ Each tuple is (dataset_id, sql, schema, database_id).
+ """
+ # Batch-fetch database engines for accurate SQL parsing
+ unique_db_ids = {row[3] for row in virtual_datasets}
+ db_engines: dict[int, str] = {}
+ if unique_db_ids:
+ db_objs = (
+ db.session.query(Database)
+ .filter(Database.id.in_(unique_db_ids)) # type:
ignore[attr-defined,unused-ignore]
+ .all()
+ )
+ for database_obj in db_objs:
+ try:
+ db_engines[database_obj.id] = database_obj.backend
+ except Exception: # noqa: BLE001
+ db_engines[database_obj.id] = ""
+
+ ds_to_tables, ds_db_map =
DatasetDAO._parse_tables_from_virtual_datasets(
+ virtual_datasets, db_engines=db_engines
+ )
+
+ if not ds_to_tables:
+ return {}
+
+ all_table_refs: set[Table] = set()
+ db_ids: set[int] = set()
+ for ds_id, table_refs in ds_to_tables.items():
+ all_table_refs.update(table_refs)
+ db_ids.add(ds_db_map[ds_id])
+
+ physical_map, phys_rls = DatasetDAO._fetch_physical_rls_map(
+ all_table_refs, db_ids
+ )
+
+ result: dict[int, list[dict[str, Any]]] = {}
+ for ds_id, table_refs in ds_to_tables.items():
+ database_id = ds_db_map[ds_id]
+ for table_ref in table_refs:
+ phys_id = physical_map.get(
+ (table_ref.table, table_ref.schema, database_id)
+ )
+ if phys_id and phys_id in phys_rls:
+ result.setdefault(ds_id, []).extend(phys_rls[phys_id])
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-233: Missing Duplication Deduplication</b></div>
<div id="fix">
`_get_inherited_rls_for_virtual_datasets` uses `extend()` at line 887, which
appends all filters from a physical table without checking for duplicates. When
the same RLS filter is attached to multiple physical tables referenced in a
virtual dataset's SQL, the filter will appear multiple times in the result. The
tests at lines 228 and 260 explicitly assert deduplication should occur. Fix:
use a deduplication loop similar to `get_rls_filters_for_dataset` lines
984-1001. (See also: [CWE-233](https://cwe.mitre.org/data/definitions/233.html))
</div>
</div>
<small><i>Code Review Run #e75529</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/dao/dataset_rls_test.py:
##########
@@ -0,0 +1,509 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from sqlalchemy.orm.session import Session
+
+from superset.daos.dataset import DatasetDAO
+
+
+def _setup_tables(session: Session) -> None:
+ """Create metadata tables for tests that need them."""
+ from superset.connectors.sqla.models import SqlaTable
+
+ SqlaTable.metadata.create_all(session.get_bind())
+
+
+def test_get_rls_filters_for_datasets_empty(session: Session) -> None:
+ result = DatasetDAO.get_rls_filters_for_datasets([])
+ assert result == {}
+
+
+def test_get_rls_filters_for_dataset_empty(session: Session) -> None:
+ """get_rls_filters_for_dataset returns [] for a nonexistent dataset."""
+ _setup_tables(session)
+ result = DatasetDAO.get_rls_filters_for_dataset(999999)
+ assert result == []
+
+
+def test_get_rls_filters_for_datasets(session: Session) -> None:
+ from superset import db
+ from superset.connectors.sqla.models import (
+ RowLevelSecurityFilter,
+ SqlaTable,
+ )
+ from superset.models.core import Database
+
+ _setup_tables(session)
+
+ database = Database(
+ database_name="my_db",
+ sqlalchemy_uri="sqlite://",
+ )
+ dataset1 = SqlaTable(
+ table_name="table1",
+ schema="main",
+ database=database,
+ )
+ dataset2 = SqlaTable(
+ table_name="table2",
+ schema="main",
+ database=database,
+ )
+ rls_filter = RowLevelSecurityFilter(
+ name="test_filter",
+ filter_type="Regular",
+ group_key="dept",
+ clause="dept = 'Finance'",
+ tables=[dataset1],
+ )
+ db.session.add_all([database, dataset1, dataset2, rls_filter])
+ db.session.flush()
+
+ result = DatasetDAO.get_rls_filters_for_datasets([dataset1.id,
dataset2.id])
+ assert dataset1.id in result
+ assert dataset2.id not in result
+ assert len(result[dataset1.id]) == 1
+ assert result[dataset1.id][0]["name"] == "test_filter"
+ assert result[dataset1.id][0]["filter_type"] == "Regular"
+ assert result[dataset1.id][0]["group_key"] == "dept"
+
+
+def test_get_rls_filters_for_dataset_includes_roles(session: Session) -> None:
+ """Detail endpoint should include roles with id and name."""
+ from superset import db
+ from superset.connectors.sqla.models import RowLevelSecurityFilter,
SqlaTable
+ from superset.models.core import Database
+
+ _setup_tables(session)
+
+ database = Database(database_name="my_db", sqlalchemy_uri="sqlite://")
+ dataset = SqlaTable(table_name="t1", schema="main", database=database)
+ rls_filter = RowLevelSecurityFilter(
+ name="detail_filter",
+ filter_type="Base",
+ group_key=None,
+ clause="1 = 0",
+ tables=[dataset],
+ )
+ db.session.add_all([database, dataset, rls_filter])
+ db.session.flush()
+
+ result = DatasetDAO.get_rls_filters_for_dataset(dataset.id)
+ assert len(result) == 1
+ assert result[0]["name"] == "detail_filter"
+ assert result[0]["filter_type"] == "Base"
+ assert result[0]["clause"] == "1 = 0"
+ assert result[0]["roles"] == []
+
+
+def test_get_rls_filters_for_dataset_no_filters(session: Session) -> None:
+ from superset import db
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.models.core import Database
+
+ _setup_tables(session)
+
+ database = Database(database_name="my_db", sqlalchemy_uri="sqlite://")
+ dataset = SqlaTable(table_name="table1", schema="main", database=database)
+ db.session.add_all([database, dataset])
+ db.session.flush()
+
+ result = DatasetDAO.get_rls_filters_for_dataset(dataset.id)
+ assert result == []
+
+
+def test_get_rls_filters_inherited_from_virtual_dataset_list(
+ session: Session,
+) -> None:
+ """Virtual datasets should inherit RLS filters from physical tables in
their SQL."""
+ from superset import db
+ from superset.connectors.sqla.models import RowLevelSecurityFilter,
SqlaTable
+ from superset.models.core import Database
+
+ _setup_tables(session)
+
+ database = Database(database_name="my_db", sqlalchemy_uri="sqlite://")
+ physical_table = SqlaTable(table_name="orders", schema="main",
database=database)
+ virtual_dataset = SqlaTable(
+ table_name="my_virtual",
+ schema="main",
+ database=database,
+ sql="SELECT * FROM orders WHERE status = 'active'",
+ )
+ rls_filter = RowLevelSecurityFilter(
+ name="orders_filter",
+ filter_type="Regular",
+ group_key=None,
+ clause="region = 'EU'",
+ tables=[physical_table],
+ )
+ db.session.add_all([database, physical_table, virtual_dataset, rls_filter])
+ db.session.flush()
+
+ result = DatasetDAO.get_rls_filters_for_datasets([virtual_dataset.id])
+ assert virtual_dataset.id in result
+ assert len(result[virtual_dataset.id]) == 1
+ assert result[virtual_dataset.id][0]["name"] == "orders_filter"
+
+
+def test_get_rls_filters_inherited_from_virtual_dataset_detail(
+ session: Session,
+) -> None:
+ """Detail view for virtual dataset should show inherited filters with
flag."""
+ from superset import db
+ from superset.connectors.sqla.models import RowLevelSecurityFilter,
SqlaTable
+ from superset.models.core import Database
+
+ _setup_tables(session)
+
+ database = Database(database_name="my_db", sqlalchemy_uri="sqlite://")
+ physical_table = SqlaTable(table_name="customers", schema="main",
database=database)
+ virtual_dataset = SqlaTable(
+ table_name="my_report",
+ schema="main",
+ database=database,
+ sql="SELECT * FROM customers",
+ )
+ rls_filter = RowLevelSecurityFilter(
+ name="customer_filter",
+ filter_type="Base",
+ group_key=None,
+ clause="1 = 0",
+ tables=[physical_table],
+ )
+ db.session.add_all([database, physical_table, virtual_dataset, rls_filter])
+ db.session.flush()
+
+ result = DatasetDAO.get_rls_filters_for_dataset(virtual_dataset.id)
+ assert len(result) == 1
+ assert result[0]["name"] == "customer_filter"
+ assert result[0]["inherited"] is True
+ assert result[0]["clause"] == "1 = 0"
+
+
+def test_dedup_inherited_filters_list(session: Session) -> None:
+ """Same RLS filter inherited via two tables in SQL should appear only
once."""
+ from superset import db
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Remove duplicate test setup code</b></div>
<div id="fix">
Found syntactic duplication (27 lines) in dataset_rls_test.py at lines
200-226 and 234-260. Recommend extracting common setup code into a shared
fixture function to eliminate duplication and improve maintainability.
</div>
</div>
<small><i>Code Review Run #e75529</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
