sadpandajoe commented on code in PR #35662:
URL: https://github.com/apache/superset/pull/35662#discussion_r4170300651
##########
superset/daos/dataset.py:
##########
@@ -676,24 +677,45 @@ def get_table_by_name(database_id: int, table_name: str)
-> SqlaTable | None:
@staticmethod
def get_table_by_catalog_schema_and_name(
- database_id: int,
- schema: str | None,
table_name: str,
- catalog: str | None = None,
+ database_id: int | str | _Unset = _UNSET,
+ schema: str | _Unset | None = _UNSET,
+ catalog: str | _Unset | None = _UNSET,
+ skip_base_filter: bool = False,
) -> SqlaTable | None:
- # Filter by the full ``(database_id, catalog, schema, table_name)``
- # uniqueness key so callers can disambiguate datasets that share a
- # ``table_name`` across schemas or catalogs (#30377).
- return (
- db.session.query(SqlaTable)
- .filter_by(
- database_id=database_id,
- catalog=catalog,
- schema=schema,
- table_name=table_name,
+ # Filter by ``table_name`` and any additional identification attributes
+ # provided (``database_id``, ``catalog``, ``schema``). The full
+ # ``(database_id, catalog, schema, table_name)`` uniqueness key can be
used
+ # to disambiguate datasets sharing the same ``table_name`` (#30377),
while
+ # partial criteria may match multiple datasets (#35662).
+ query = db.session.query(SqlaTable).filter(SqlaTable.table_name ==
table_name)
+
+ if not skip_base_filter:
+ query = DatasetDAO._apply_base_filter(query)
+
+ if database_id is not _UNSET:
+ if isinstance(database_id, int):
+ query = query.filter(SqlaTable.database_id == database_id)
+ else:
+ query = query.join(Database).filter(
Review Comment:
For a user without all-datasource access, `DatasourceFilter` already joins
`Database`, so `dataset("facts", database_id="examples")` adds a second
unaliased `dbs` join and fails with an ambiguous-column/table error even when
the dataset is granted. Could this qualification avoid the second join, with a
regression test using a restricted user and a database name?
##########
superset/jinja_context.py:
##########
@@ -1281,22 +1291,87 @@ def get_template_processor(
def dataset_macro(
- dataset_id: int,
+ dataset_id: int | str,
include_metrics: bool = False,
columns: list[str] | None = None,
+ from_dttm: datetime | None = None,
Review Comment:
The new `from_dttm`/`to_dttm` arguments are accepted but discarded: the
query object still sets both to `None`, so a caller supplying a reporting
interval gets SQL generated without those bounds. Should these be forwarded
into the underlying dataset context, or removed until that behavior is
supported?
##########
tests/unit_tests/jinja_context_test.py:
##########
@@ -1292,10 +1293,7 @@ def test_dataset_macro(mocker: MockerFixture) -> None:
)
DatasetDAO = mocker.patch("superset.daos.dataset.DatasetDAO") # noqa: N806
DatasetDAO.find_by_id.return_value = dataset
- mocker.patch(
-
"superset.connectors.sqla.models.security_manager.get_guest_rls_filters",
- return_value=[],
- )
+ DatasetDAO.get_table_by_catalog_schema_and_name.return_value = dataset
Review Comment:
Mocking the whole DAO means removing the new access filter would still leave
these tests green, allowing the ungranted virtual-dataset exposure to return
unnoticed. Could an integration test call the name macro as Gamma plus SQL Lab
without broad grants, assert an ungranted tableless virtual dataset raises
`DatasetNotFoundError` before SQL generation, and verify a granted same-name
dataset resolves?
--
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]
