[GitHub] [airflow] ashb commented on a change in pull request #14306: Adding `only_active` parameter to /dags endpoint

[GitBox <gi...@apache.org>]

ashb commented on a change in pull request #14306:
URL: https://github.com/apache/airflow/pull/14306#discussion_r579283454



##########
File path: airflow/api_connexion/endpoints/dag_endpoint.py
##########
@@ -59,11 +59,21 @@ def get_dag_details(dag_id):
 
 @security.requires_access([(permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG)])
 @format_parameters({'limit': check_limit})
-def get_dags(limit, offset=0):
+def get_dags(limit, offset=0, only_active=False):

Review comment:
       This should be True by default.

##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -377,6 +377,12 @@ paths:
       parameters:
         - $ref: '#/components/parameters/PageLimit'
         - $ref: '#/components/parameters/PageOffset'
+        - name: only_active
+          in: query
+          schema:
+            type: boolean
+          required: false
+          description: Only return DAGs which were seen on the last DagBag load.

Review comment:
       This isn't quite true -- we'll need to reword it. Something like "DAGs which are currently seen by the schedulers"

##########
File path: airflow/models/dag.py
##########
@@ -804,6 +804,12 @@ def get_is_paused(self, session=None) -> Optional[None]:
         qry = session.query(DagModel).filter(DagModel.dag_id == self.dag_id)
         return qry.value(DagModel.is_paused)
 
+    @provide_session
+    def get_is_active(self, session=None) -> Optional[None]:
+        """Returns a boolean indicating whether this DAG is active"""
+        qry = session.query(DagModel).filter(DagModel.dag_id == self.dag_id)
+        return qry.value(DagModel.is_active)
+

Review comment:
       Please don't "split" get_is_paused and is_paused (move this up or down one function.)

##########
File path: airflow/api_connexion/endpoints/dag_endpoint.py
##########
@@ -59,11 +59,21 @@ def get_dag_details(dag_id):
 
 @security.requires_access([(permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG)])
 @format_parameters({'limit': check_limit})
-def get_dags(limit, offset=0):
+def get_dags(limit, offset=0, only_active=False):
     """Get all DAGs."""
     readable_dags = current_app.appbuilder.sm.get_readable_dags(g.user)
-    dags = readable_dags.order_by(DagModel.dag_id).offset(offset).limit(limit).all()
-    total_entries = readable_dags.count()
+    if only_active:
+        dags = (
+            readable_dags.filter(DagModel.is_active)
+            .order_by(DagModel.dag_id)
+            .offset(offset)
+            .limit(limit)
+            .all()
+        )
+        total_entries = readable_dags.filter(DagModel.is_active).count()
+    else:
+        dags = readable_dags.order_by(DagModel.dag_id).offset(offset).limit(limit).all()
+        total_entries = readable_dags.count()

Review comment:
       Easier way:
   
   ```suggestion
       dags = readable_dags.order_by(DagModel.dag_id).offset(offset).limit(limit).all()
       if only_active:
           dags = dags.filter(DagModel.is_active)
       total_entries = readable_dags.count()
   ```

##########
File path: tests/api_connexion/endpoints/test_dag_endpoint.py
##########
@@ -274,6 +279,7 @@ def test_should_response_200_for_null_start_date(self):
             "fileloc": __file__,
             "file_token": FILE_TOKEN,
             "is_paused": None,
+            "is_active": None,

Review comment:
       is_active should never be None .




----------------------------------------------------------------
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.

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