potiuk commented on code in PR #34317:
URL: https://github.com/apache/airflow/pull/34317#discussion_r1355894015


##########
airflow/api_connexion/endpoints/task_instance_endpoint.py:
##########
@@ -61,13 +61,8 @@
 T = TypeVar("T")
 
 
-@security.requires_access(
-    [
-        (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
-        (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG_RUN),
-        (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
-    ],
-)
+@security.requires_access_dag("GET", DagAccessEntity.RUN)
+@security.requires_access_dag("GET", DagAccessEntity.TASK_INSTANCE)

Review Comment:
   Finally got to review it after the break (and this and next week will have a 
lot of focus on those).
   
   I think this really calls for small refactoring: 
   
   ```python
   def requires_access_dag(
       method: ResourceMethod, access_entities: tuple[DAGAccessEntity] | 
DagAccessEntity | None = None
   ```
   
   and then:
   
   ```python
   @security.requires_access_dag("GET", (DagAccessEntity.RUN, 
DagAccessEntity.TASK_INSTANCE))
   ```
   
   and convert `access_entity` to `access_entities: tuple[DAGDetails]` (we can 
convert single dag detail to one-element tuple).
   
   Not sure if now or as a follow up to this PR, but I am quite sure this will 
be much better for performance point of view (one callback instead of two and 
it will be pretty much impossible to optimize it if we have two decorators. 
   
   And since it will change the callback, it's likely better to do it it now.
   



-- 
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: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to