john-bodley commented on code in PR #24808:
URL: https://github.com/apache/superset/pull/24808#discussion_r1276543183
##########
superset/security/manager.py:
##########
@@ -2063,6 +2064,27 @@ def can_access_based_on_dashboard(datasource:
"BaseDatasource") -> bool:
)
exists = db.session.query(query.exists()).scalar()
+
+ # check for datasets that are only used by filters
+ if not exists:
+ dashboards_json = (
+ db.session.query(Dashboard.json_metadata)
+ .filter(Dashboard.id.in_(dashboard_ids))
+ .all()
+ )
+ for json_ in dashboards_json:
+ try:
+ json_metadata = json.loads(json_.json_metadata)
+ for filter_ in
json_metadata.get("native_filter_configuration", []):
+ filter_dataset_ids = [
+ target.get("datasetId")
+ for target in filter_.get("targets", [])
+ ]
+ if datasource.id in filter_dataset_ids:
+ exists = True
+ except ValueError:
+ pass
+
return exists
Review Comment:
Probably cleaner on line #2066 to have:
```python
if db.session.query(query.exists()).scalar():
return True
```
and then on line #2088 have
```python
return False
```
##########
superset/security/manager.py:
##########
@@ -2063,6 +2064,27 @@ def can_access_based_on_dashboard(datasource:
"BaseDatasource") -> bool:
)
exists = db.session.query(query.exists()).scalar()
+
+ # check for datasets that are only used by filters
Review Comment:
I think this logic makes sense. Note in
https://github.com/apache/superset/pull/24789 this method is slated for
removal, but the addition of the integration test will ensure that the logic
will be preserved.
##########
superset/security/manager.py:
##########
@@ -2063,6 +2064,27 @@ def can_access_based_on_dashboard(datasource:
"BaseDatasource") -> bool:
)
exists = db.session.query(query.exists()).scalar()
+
+ # check for datasets that are only used by filters
+ if not exists:
+ dashboards_json = (
+ db.session.query(Dashboard.json_metadata)
+ .filter(Dashboard.id.in_(dashboard_ids))
+ .all()
+ )
+ for json_ in dashboards_json:
+ try:
+ json_metadata = json.loads(json_.json_metadata)
+ for filter_ in
json_metadata.get("native_filter_configuration", []):
+ filter_dataset_ids = [
+ target.get("datasetId")
+ for target in filter_.get("targets", [])
+ ]
+ if datasource.id in filter_dataset_ids:
+ exists = True
Review Comment:
```suggestion
return True
```
We should short circuit when possible.
--
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]
