Add extra links endpoint #9475

mik-laj · 2020-06-22T14:26:54Z

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.

ephraimbuddy · 2020-06-22T17:50:12Z

tests/api_connexion/endpoints/test_extra_link_endpoint.py

Suggested change

execution_date=quote(dttm.isoformat()),

execution_date=quote_plus(dttm.isoformat()),

I am thinking that quote_plus will be better here?

2020-01-01T00:00:00+00:00
It doesn't make a difference here because we don't have spaces.

OmairK · 2020-06-22T17:55:52Z

airflow/api_connexion/endpoints/extra_link_endpoint.py

+    dagbag: DagBag = current_app.dag_bag
+    dag: DAG = dagbag.get_dag(dag_id)
+    if not dag:
+        raise NotFound("DAG not found")


Suggested change

raise NotFound("DAG not found")

raise NotFound(detail=f"DAG with id: '{dag_id}' not found")

The errors are a bit more descriptive this way, what do you think?

I think we should change the title too. Otherwise, it will have a default value - "Object not found". After that it's a good idea.

OmairK · 2020-06-22T18:12:18Z

tests/api_connexion/endpoints/test_extra_link_endpoint.py

+    @parameterized.expand(
+        [
+            (
+                "/api/v1/dags/INVALID/dagRuns/TEST_DAG_RUN_ID/taskInstances/TEST_SINGLE_QUERY/links",
+                'DAG not found'
+            ),
+            (
+                "/api/v1/dags/TEST_DAG_ID/dagRuns/INVALID/taskInstances/TEST_SINGLE_QUERY/links",
+                "DAG Run not found"
+            ),
+            (
+                "/api/v1/dags/TEST_DAG_ID/dagRuns/TEST_DAG_RUN_ID/taskInstances/INVALID/links",
+                "Task not found"
+            ),
+        ]
+    )
+    def test_should_response_404_on_invalid_task_id(self, url, expected_title):
+        response = self.client.get(url)


Suggested change

@parameterized.expand(

[

(

"/api/v1/dags/INVALID/dagRuns/TEST_DAG_RUN_ID/taskInstances/TEST_SINGLE_QUERY/links",

'DAG not found'

),

(

"/api/v1/dags/TEST_DAG_ID/dagRuns/INVALID/taskInstances/TEST_SINGLE_QUERY/links",

"DAG Run not found"

),

(

"/api/v1/dags/TEST_DAG_ID/dagRuns/TEST_DAG_RUN_ID/taskInstances/INVALID/links",

"Task not found"

),

]

)

def test_should_response_404_on_invalid_task_id(self, url, expected_title):

response = self.client.get(url)

@parameterized.expand(

[

(

"invalid dag_id",

"/api/v1/dags/INVALID/dagRuns/TEST_DAG_RUN_ID/taskInstances/TEST_SINGLE_QUERY/links",

'DAG not found'

),

(

"invalid run_id",

"/api/v1/dags/TEST_DAG_ID/dagRuns/INVALID/taskInstances/TEST_SINGLE_QUERY/links",

"DAG Run not found"

),

(

"invalid task_id",

"/api/v1/dags/TEST_DAG_ID/dagRuns/TEST_DAG_RUN_ID/taskInstances/INVALID/links",

"Task not found"

),

]

)

def test_should_response_404_on(self, name, url, expected_title):

del name

response = self.client.get(url)

These tests are dealing with multiple invalid id(s), this is IMO more descriptive, what do you think ?

In many places, we don't use names. We already have one name that allows you to identify a specific test, then the number allows you to specify a specific variant. Many names will limit the number of tests. What I love in parameterized is the ability to generate many cases in a simple way. It is possible that some cases may overlap, but this may change in the future.

ephraimbuddy · 2020-06-22T18:22:55Z

airflow/api_connexion/endpoints/extra_link_endpoint.py

 # TODO(mik-laj): We have to implement it.
 #     Do you want to help? Please look at: https://github.com/apache/airflow/issues/8140


Suggested change

# TODO(mik-laj): We have to implement it.

# Do you want to help? Please look at: https://github.com/apache/airflow/issues/8140

I removed this note.

OmairK · 2020-06-22T18:24:49Z

tests/api_connexion/endpoints/test_extra_link_endpoint.py

+            "/api/v1/dags/TEST_DAG_ID/dagRuns/TEST_DAG_RUN_ID/taskInstances/TEST_SINGLE_QUERY/links"
+        )
+
+        self.assertEqual(200, response.status_code, response.data)


Suggested change

self.assertEqual(200, response.status_code, response.data)

self.assertEqual(200, response.status_code)

I have never used it but I guess response.data returns the response.json in bytes literal?

3 argument is just a message for the developer. It can be in the form of bytes if it is safer and more readable.

ephraimbuddy · 2020-06-22T18:39:29Z

airflow/api_connexion/endpoints/extra_link_endpoint.py

+        raise NotFound("Task not found")
+
+    execution_date = (
+        session.query(DR.execution_date).filter(DR.dag_id == dag_id).filter(DR.run_id == dag_run_id).scalar()


Suggested change

session.query(DR.execution_date).filter(DR.dag_id == dag_id).filter(DR.run_id == dag_run_id).scalar()

session.query(DR.execution_date).filter(DR.dag_id == dag_id, DR.run_id == dag_run_id).scalar()

I'm wrong about using one. scalar() or first() is better

ephraimbuddy · 2020-06-22T18:40:47Z

tests/api_connexion/endpoints/test_extra_link_endpoint.py

+
+    @provide_session
+    def setUp(self, session) -> None:
+        self.now = datetime(2020, 1, 1)


Suggested change

self.now = datetime(2020, 1, 1)

self.default_time = datetime(2020, 1, 1)

I suggest we use default_time instead of now

Fixed. Thanks.

ephraimbuddy · 2020-06-22T18:48:28Z

tests/api_connexion/endpoints/test_extra_link_endpoint.py


-import pytest
+from parameterized import parameterized
+from test_utils.mock_plugins import mock_plugin_manager


Suggested change

from test_utils.mock_plugins import mock_plugin_manager

from tests.test_utils.mock_plugins import mock_plugin_manager

Fixed. Thanks.

ephraimbuddy · 2020-06-22T18:59:28Z

tests/api_connexion/endpoints/test_extra_link_endpoint.py

+            BigQueryExecuteQueryOperator(task_id="TEST_SINGLE_QUERY", sql="SELECT 1")
+            BigQueryExecuteQueryOperator(task_id="TEST_MULTIPLE_QUERY", sql=["SELECT 1", "SELECT 2"])


Suggested change

BigQueryExecuteQueryOperator(task_id="TEST_SINGLE_QUERY", sql="SELECT 1")

BigQueryExecuteQueryOperator(task_id="TEST_MULTIPLE_QUERY", sql=["SELECT 1", "SELECT 2"])

BigQueryInsertJobOperator(task_id="TEST_SINGLE_QUERY", sql="SELECT 1")

BigQueryInsertJobOperator(task_id="TEST_MULTIPLE_QUERY", sql=["SELECT 1", "SELECT 2"])

This operator BigQueryExecuteQueryOperator is deprecated

BigQueryInsertJobOperator doesn't have extra links.

mik-laj · 2020-06-23T23:05:33Z

@turbaszek @michalslowikowski00 Can you look at it?

turbaszek · 2020-06-24T08:49:31Z

airflow/api_connexion/endpoints/extra_link_endpoint.py

+    dagbag: DagBag = current_app.dag_bag
+    dag: DAG = dagbag.get_dag(dag_id)
+    if not dag:
+        raise NotFound("DAG not found", detail=f'DAG with ID = "{dag_id}" not found')


Should we create DagNotFound and TaskNotFound exceptions to unify this between endpoints?

In the future, this is possible when we focus on improving exceptions to add additional information (type field).

airflow/api_connexion/endpoints/extra_link_endpoint.py

Add extra links endpoint

cc6e773

boring-cyborg bot added the area:API Airflow's REST/HTTP API label Jun 22, 2020

mik-laj changed the title ~~Add extra links endpoint~~ [WIP] Add extra links endpoint Jun 22, 2020

mik-laj marked this pull request as draft June 22, 2020 14:27

mik-laj marked this pull request as ready for review June 22, 2020 17:39

mik-laj changed the title ~~[WIP] Add extra links endpoint~~ Add extra links endpoint Jun 22, 2020

ephraimbuddy reviewed Jun 22, 2020

View reviewed changes

fixup! Add extra links endpoint

6e556bd

mik-laj force-pushed the extra-link-endpoint branch from f2cef7f to 6e556bd Compare June 22, 2020 17:52

OmairK reviewed Jun 22, 2020

View reviewed changes

ephraimbuddy reviewed Jun 22, 2020

View reviewed changes

OmairK reviewed Jun 22, 2020

View reviewed changes

ephraimbuddy reviewed Jun 22, 2020

View reviewed changes

fixup! fixup! Add extra links endpoint

8a3eae9

mik-laj requested a review from turbaszek June 23, 2020 23:05

turbaszek reviewed Jun 24, 2020

View reviewed changes

airflow/api_connexion/endpoints/extra_link_endpoint.py Show resolved Hide resolved

turbaszek reviewed Jun 24, 2020

View reviewed changes

airflow/api_connexion/endpoints/extra_link_endpoint.py Show resolved Hide resolved

turbaszek approved these changes Jun 24, 2020

View reviewed changes

fixup! fixup! fixup! Add extra links endpoint

6209980

mik-laj merged commit 2b61912 into apache:master Jun 24, 2020

mik-laj deleted the extra-link-endpoint branch June 24, 2020 10:26

	execution_date=quote(dttm.isoformat()),
	execution_date=quote_plus(dttm.isoformat()),

	raise NotFound("DAG not found")
	raise NotFound(detail=f"DAG with id: '{dag_id}' not found")

		# TODO(mik-laj): We have to implement it.
		# Do you want to help? Please look at: https://github.com/apache/airflow/issues/8140

	self.assertEqual(200, response.status_code, response.data)
	self.assertEqual(200, response.status_code)

	session.query(DR.execution_date).filter(DR.dag_id == dag_id).filter(DR.run_id == dag_run_id).scalar()
	session.query(DR.execution_date).filter(DR.dag_id == dag_id, DR.run_id == dag_run_id).scalar()

	self.now = datetime(2020, 1, 1)
	self.default_time = datetime(2020, 1, 1)

	from test_utils.mock_plugins import mock_plugin_manager
	from tests.test_utils.mock_plugins import mock_plugin_manager

		BigQueryExecuteQueryOperator(task_id="TEST_SINGLE_QUERY", sql="SELECT 1")
		BigQueryExecuteQueryOperator(task_id="TEST_MULTIPLE_QUERY", sql=["SELECT 1", "SELECT 2"])

Add extra links endpoint #9475

Add extra links endpoint #9475

Uh oh!

Conversation

mik-laj commented Jun 22, 2020

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!

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!

ephraimbuddy Jun 22, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ephraimbuddy Jun 22, 2020 •

edited

Loading

mik-laj commented Jun 23, 2020 •

edited

Loading