add readonly endpoints for dagruns by ephraimbuddy · Pull Request #9153 · apache/airflow

ephraimbuddy · 2020-06-05T09:53:47Z

Closes: #8129
Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Unit tests coverage for changes (not needed for documentation changes)
Target Github ISSUE in description if exists
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

mik-laj · 2020-06-08T23:09:46Z

Suggested change

# This endpoint allows specifying ~ as the dag_id to retrieve DAG Runs for all DAGs.

if dag_id == '~':

dag_run = query.all()

return dagrun_collection_schema.dump(DAGRunCollection(

dag_runs=dag_run,

total_entries=len(dag_run))

)

query = query.filter(DagRun.dag_id == dag_id)

if dag_id != '~':

query = query.filter(DagRun.dag_id == dag_id)

We still want to have filters and paginations. ~ is a wildcard. It allows any value, so we should give up filtering in this field.

mik-laj · 2020-06-08T23:12:16Z

Suggested change

if start_date_gte and start_date_lte:

query = query.filter(DagRun.start_date <= start_date_lte,

DagRun.start_date >= start_date_gte)

elif start_date_gte and not start_date_lte:

query = query.filter(DagRun.start_date >= start_date_gte)

elif start_date_lte and not start_date_gte:

query = query.filter(DagRun.start_date <= start_date_lte)

if start_date_gte:

query = query.filter(DagRun.start_date >= start_date_gte)

if start_date_lte:

query = query.filter(DagRun.start_date <= start_date_lte)

Is this causing any problems?

mik-laj · 2020-06-08T23:12:41Z

mik-laj · 2020-06-08T23:20:15Z

Why do we have a string here? The specification define object here.

mik-laj · 2020-06-09T00:37:29Z

Suggested change

self.assertEqual(response.json.get('total_entries'), 2)

self.assertEqual(response.json['total_entries'], 2)

mik-laj · 2020-06-09T00:46:01Z

I would prefer to use a fixed date in tests. This allows us to use text strings in tests. Now it is very difficult for me to check if the data format is correct.

mik-laj · 2020-06-09T00:50:40Z

What do you think about webargs library? Would it be helpful to you?

I found that this can solve it for us without any validation code from our side:

app.add_api("openapi.yaml", strict_validation=True)

https://connexion.readthedocs.io/en/latest/request.html#parameter-validation

I will be using webargs. Connexion no longer validate date-format because of license of one of its library.
spec-first/connexion#476

I'm glad you did research. We can change it in a separate change. Now it works, so it's great. In next changes, we will be able to make a refactor and remove repetitive code fragments.

mik-laj · 2020-06-09T01:03:31Z

It should. be defined as a custom fields. This allows you to eliminate some hacks from the code.

class DagStateField(fields.String): def __init__(self, **metadata): super().__init__(**metadata) self.validators = ( [validate.OneOf(State.dag_states)] + list(self.validators) )

mik-laj · 2020-06-10T08:53:31Z

Suggested change

session.add_all(dagruns)

session.add_all(dagruns)

dagruns[0].dag_id.= "TEST_DAG_ID_2"

Can you check here if two different DAGs can be fetched?

Check if this is ok a809933

~ means that you can fetch any DAG ID. Your assertion should check if you have two DAGs e.g. DAG A and DAG B, you can fetch them. For now, you only have one DAG, which does not allow you to check the behavior of this view.

Check this c41c771

mik-laj · 2020-06-10T10:05:50Z

What do you think to assert the start/execution dates here? Identifiers poorly describe these objectshere.

Check if this is ok 5545d36

mik-laj

We are near the end.

mik-laj · 2020-06-11T21:11:59Z

Suggested change

["TEST_START_EXEC_DAY_10"],

["TEST_START_EXEC_DAY_10", "TEST_START_EXEC_DAY_11"],

We have an inclusive filter, so it should probably be included as well. However, this may change in the future.

Related issue:
#9237

This seems like a problem with <= operator in sqlalchemy. It returns only the less than values.
I have also tried separating the operator and using the or_ operator but it's still not inclusive
I am wondering if there is another way I could do it?

I have a day off today, so I have limited options to check it out, but it can help you.
https://github.com/apache/airflow/blob/master/TESTING.rst#tracking-sql-statements
If something can be done at the SQL level, then SQL can do it too.

Thanks for the link, it helped.
What was happening was that on the view, dates that have this format '2020-06-11T18:00:00+00:00' are changed to '2020-06-11T18:00:00 00:00'. That is, the + are replaced with empty space. So when it is parsed, only the date part were parsed. I then replaced +00:00 with Z and the problem was solved.

The fix 7a72935

This is the command I used to investigate:
pytest --trace-sql=num,sql,parameters --capture=no tests/api_connexion/endpoints/test_dag_run_endpoint.py -k test_date_filters_gte_and_lte_1_api_v1_dags_TEST_DAG_ID_dagRuns_start_date_lte_2020_06_11T18_00_00_00_00

I set pdb debuger at this point in dag_run_endpoint:

if start_date_lte: import pdb; pdb.set_trace() query = query.filter(DagRun.start_date <= timezone.parse(start_date_lte))

Check image below:

mik-laj · 2020-06-14T23:40:40Z

I would like to review this change once more and look at the filter by dates, but today I see that there is a merge conflict. if you have moments could you do a rebase?

turbaszek · 2020-06-16T14:56:54Z

Suggested change

mik-laj

One test is sad. We need to fetch sorted items from the database.

Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>

boring-cyborg Bot added area:API Airflow's REST/HTTP API area:webserver Webserver related Issues labels Jun 5, 2020

ephraimbuddy marked this pull request as draft June 5, 2020 09:54

mik-laj reviewed Jun 8, 2020

View reviewed changes

Comment thread tests/api_connexion/endpoints/test_dag_run_endpoint.py Outdated

mik-laj reviewed Jun 8, 2020

View reviewed changes

Comment thread airflow/api_connexion/openapi/v1.yaml Outdated

mik-laj Jun 8, 2020

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

turbaszek reacted with thumbs up emoji

mik-laj reviewed Jun 8, 2020

View reviewed changes

mik-laj reviewed Jun 9, 2020

View reviewed changes

Comment thread tests/api_connexion/endpoints/test_dag_run_endpoint.py Outdated

mik-laj reviewed Jun 9, 2020

View reviewed changes

mik-laj reviewed Jun 10, 2020

View reviewed changes

ephraimbuddy commented Jun 10, 2020

View reviewed changes

Comment thread airflow/api_connexion/endpoints/dag_run_endpoint.py Outdated

ephraimbuddy commented Jun 10, 2020

View reviewed changes

Comment thread airflow/api_connexion/endpoints/dag_run_endpoint.py Outdated

ephraimbuddy changed the title ~~[WIP] add readonly endpoints for dagruns~~ add readonly endpoints for dagruns Jun 11, 2020

ephraimbuddy marked this pull request as ready for review June 11, 2020 15:47

mik-laj reviewed Jun 11, 2020

View reviewed changes

turbaszek approved these changes Jun 16, 2020

View reviewed changes

Comment thread airflow/api_connexion/endpoints/dag_run_endpoint.py Outdated

turbaszek Jun 16, 2020

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

mik-laj approved these changes Jun 16, 2020

View reviewed changes

mik-laj requested changes Jun 16, 2020

View reviewed changes

ephraimbuddy added 5 commits June 18, 2020 21:48

add readonly endpoint for dagruns

e5a23f3

add dagruns collection endpoint

ababab4

add pagination and tilde character tests

8a8fb9d

add end date as nullable and improve baseschema

da0c636

add tests for filter parameter

c0d7354

ephraimbuddy and others added 22 commits June 18, 2020 21:48

add getdagrunsbatch endpoint, schemas and test

d5b3fb2

apply review suggestion

18b1a1e

Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>

remove dagrun post endpoint

0dcc32c

add tests and validate datetime string in query

cf6eb4b

work on total entries

0bc9367

add review

a1c71ba

apply review suggestions

ec24218

Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>

apply review suggestions including parameterized tests

813b544

Small code style changes

f2f4033

test if two different dags can be fetched

07e3e27

make identifiers describe the objects in date tests

acbafb0

improve test for wild card ~

cb75aed

remove utils.py

2b309a6

refactor now to default_time

8d8c2b7

apply review suggestions

01cd617

change +00:00 to Z to conform to spec

3765974

add docstring for DagState

3157ae1

add custom datetime parser

9563257

add test for util

371b3d8

apply review suggestions

e9df3ae

improve query

d723ca4

fetch sorted items from db

cdaf3a6

mik-laj approved these changes Jun 19, 2020

View reviewed changes

mik-laj merged commit b6f4837 into apache:master Jun 19, 2020

-    #  This endpoint allows specifying ~ as the dag_id to retrieve DAG Runs for all DAGs.
-    if dag_id == '~':
-        dag_run = query.all()
-        return dagrun_collection_schema.dump(DAGRunCollection(
-            dag_runs=dag_run,
-            total_entries=len(dag_run))
-        )
-    query = query.filter(DagRun.dag_id == dag_id)
+    if dag_id != '~':
+        query = query.filter(DagRun.dag_id == dag_id)

-    if start_date_gte and start_date_lte:
-        query = query.filter(DagRun.start_date <= start_date_lte,
-                             DagRun.start_date >= start_date_gte)
-    elif start_date_gte and not start_date_lte:
-        query = query.filter(DagRun.start_date >= start_date_gte)
-    elif start_date_lte and not start_date_gte:
-        query = query.filter(DagRun.start_date <= start_date_lte)
+    if start_date_gte:
+        query = query.filter(DagRun.start_date >= start_date_gte)
+    if start_date_lte:
+        query = query.filter(DagRun.start_date <= start_date_lte)

	self.assertEqual(response.json.get('total_entries'), 2)
	self.assertEqual(response.json['total_entries'], 2)

	session.add_all(dagruns)
	session.add_all(dagruns)
	dagruns[0].dag_id.= "TEST_DAG_ID_2"

	["TEST_START_EXEC_DAY_10"],
	["TEST_START_EXEC_DAY_10", "TEST_START_EXEC_DAY_11"],

Uh oh!

Conversation

ephraimbuddy commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ephraimbuddy Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mik-laj Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mik-laj Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mik-laj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mik-laj Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ephraimbuddy Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ephraimbuddy Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mik-laj commented Jun 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mik-laj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

ephraimbuddy commented Jun 5, 2020 •

edited

Loading

ephraimbuddy Jun 9, 2020 •

edited

Loading

mik-laj Jun 10, 2020 •

edited

Loading

mik-laj Jun 10, 2020 •

edited

Loading

mik-laj Jun 11, 2020 •

edited

Loading

ephraimbuddy Jun 11, 2020 •

edited

Loading

ephraimbuddy Jun 12, 2020 •

edited

Loading