[GitHub] [airflow] ashb commented on a change in pull request #14895: Add REST API query sort and order to some endpoints

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

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



##########
File path: tests/api_connexion/endpoints/test_dag_endpoint.py
##########
@@ -468,7 +520,7 @@ def test_should_respond_200_with_granular_dag_access(self):
     def test_should_respond_200_and_handle_pagination(self, url, expected_dag_ids):
         self._create_dag_models(10)
 
-        response = self.client.get(url, environ_overrides={'REMOTE_USER': "test"})
+        response = self.client.get(url + "&sort=asc", environ_overrides={'REMOTE_USER': "test"})

Review comment:
       This test shouldn't really need changing - if its failing without this change then you should update the endpoint code to have this as the default.

##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -142,8 +155,15 @@ def _fetch_dag_runs(
     )
     # Count items
     total_entries = query.count()
+    # sort
+    if sort == 'asc':
+        query = query.order_by(asc(order_by))

Review comment:
       This would break if `sort` was passed by `order_by` wasn't (as order_by has a default of None.)

##########
File path: airflow/api_connexion/endpoints/dag_endpoint.py
##########
@@ -59,10 +60,19 @@ 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, sort, offset=0, order_by='dag_id'):
     """Get all DAGs."""
+    columns = [i.name for i in DagModel.__table__.columns]  # pylint: disable=no-member
+    if order_by not in columns:

Review comment:
       I think this will work
   
   ```suggestion
       if order_by not in (i.name for i in DagModel.__table__.columns):  # pylint: disable=no-member
   ```

##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -376,6 +376,8 @@ paths:
       parameters:
         - $ref: '#/components/parameters/PageLimit'
         - $ref: '#/components/parameters/PageOffset'
+        - $ref: '#/components/parameters/SortBy'
+        - $ref: '#/components/parameters/OrderBy'

Review comment:
       I'm still a fan of making this a _single_ field, rather than two as we can then easily extend this to sort by multiple columns.
   
   For example.
   
   Current: 
   - `endpoint?sort=asc&order_by=execution_date`
   - `endpoint?sort=desc&order_by=execution_date`
   
   Proposed:
   - `endpoint?order_by=execution_date`
   - `endpoint?order_by=-execution_date`
   
   This then allows future to easily support (one or both of thse)
   
   - `endpoint?order_by=dag_id,-execution_date` (`ORDER BY dag_id, execution_date DESC`)
   - `endpoint?order_by=dag_id&order_by=-execution_date` (Same, multiple params of the same name is valid and produces a list of values in Python.)
   
   _At the very least_ if this is kept as two parameters, than `sort_direction` and `sort_column` names are clearer -- sort and order_by names are effectively synonyms so we should use clearer names.




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