You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/03/28 10:39:18 UTC
[GitHub] [airflow] jscheffl opened a new pull request #15057: Origin/bugfix/issue 15023
jscheffl opened a new pull request #15057:
URL: https://github.com/apache/airflow/pull/15057
With this PR new DAG triggers are validated that the submitted conf (=dag_run_conf) is a dictionary. Previously submissions were possible and the logic just validated if the parameter is a valid JSON. Nevertheless currently some business logic fails in Airflow if the con value is not a dictionary object.
Explicitly I added the validation NOT to the data model as instances with migrated legacy data (in Airflow 1.x non dictionary cases were supported) are not failing in loading data via ORM.
The PR mainly adds validation for Legacy Experimental API and WWW UI.
It adds pytest for Legacy Experimental API, "new" API and WWW UI.
Update Pylint and flynt
closes: #15023
related: #15023
--
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
[GitHub] [airflow] github-actions[bot] commented on pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#issuecomment-840550208
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.
--
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
[GitHub] [airflow] ashb commented on a change in pull request #15057: Origin/bugfix/issue #15023 / Add validation for dag_run conf to be a dict
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#discussion_r631666487
##########
File path: tests/providers/apache/hive/hooks/test_hive.py
##########
@@ -229,7 +229,7 @@ def test_load_file_create_table(self, mock_run_cli):
filepath = "/path/to/input/file"
table = "output_table"
field_dict = OrderedDict([("name", "string"), ("gender", "string")])
- fields = ",\n ".join(['`{k}` {v}'.format(k=k.strip('`'), v=v) for k, v in field_dict.items()])
+ fields = ",\n ".join([f"`{k.strip('`')}` {v}" for k, v in field_dict.items()])
Review comment:
```suggestion
fields = ",\n ".join(['`{k}` {v}'.format(k=k.strip('`'), v=v) for k, v in field_dict.items()])
```
--
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
[GitHub] [airflow] jscheffl commented on pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
jscheffl commented on pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#issuecomment-840510732
@ashb uups, something went bad in the rebase, will need one further attempt to integrate on rebase...
--
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
[GitHub] [airflow] github-actions[bot] commented on pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#issuecomment-840560885
[The Workflow run](https://github.com/apache/airflow/actions/runs/839003161) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
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
[GitHub] [airflow] ashb commented on a change in pull request #15057: Origin/bugfix/issue #15023 / Add validation for dag_run conf to be a dict
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#discussion_r631666896
##########
File path: airflow/providers/apache/hive/hooks/hive.py
##########
@@ -426,7 +426,7 @@ def load_file(
if create or recreate:
if field_dict is None:
raise ValueError("Must provide a field dict when creating a table")
- fields = ",\n ".join(['`{k}` {v}'.format(k=k.strip('`'), v=v) for k, v in field_dict.items()])
+ fields = ",\n ".join(['`{k}` {v}'.format(k=k.strip('`'), v=v) for k, v in field_dict.items()])
Review comment:
```suggestion
fields = ",\n ".join(['`{k}` {v}'.format(k=k.strip('`'), v=v) for k, v in field_dict.items()])
```
--
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
[GitHub] [airflow] github-actions[bot] commented on pull request #15057: Origin/bugfix/issue #15023 / Add validation for dag_run conf to be a dict
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#issuecomment-840180468
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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
[GitHub] [airflow] github-actions[bot] commented on pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#issuecomment-840607433
[The Workflow run](https://github.com/apache/airflow/actions/runs/839203576) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
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
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#issuecomment-865942844
Awesome work, congrats on your first merged pull request!
--
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
[GitHub] [airflow] ashb commented on a change in pull request #15057: Origin/bugfix/issue #15023 / Add validation for dag_run conf to be a dict
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#discussion_r631665767
##########
File path: airflow/providers/apache/hive/hooks/hive.py
##########
@@ -426,7 +426,7 @@ def load_file(
if create or recreate:
if field_dict is None:
raise ValueError("Must provide a field dict when creating a table")
- fields = ",\n ".join(['`{k}` {v}'.format(k=k.strip('`'), v=v) for k, v in field_dict.items()])
+ fields = ",\n ".join([f"`{k.strip('`')}` {v}" for k, v in field_dict.items()])
Review comment:
This change looks unrelated -- is it?
##########
File path: airflow/providers/apache/hive/hooks/hive.py
##########
@@ -426,7 +426,7 @@ def load_file(
if create or recreate:
if field_dict is None:
raise ValueError("Must provide a field dict when creating a table")
- fields = ",\n ".join(['`{k}` {v}'.format(k=k.strip('`'), v=v) for k, v in field_dict.items()])
+ fields = ",\n ".join([f"`{k.strip('`')}` {v}" for k, v in field_dict.items()])
Review comment:
```suggestion
fields = ",\n ".join(['`{k}` {v}'.format(k=k.strip('`'), v=v) for k, v in field_dict.items()])
```
--
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
[GitHub] [airflow] ashb commented on pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#issuecomment-840565109
```
tests/www/views/test_views_trigger_dag.py:87:30: F821 undefined name 'DR'
tests/www/views/test_views_trigger_dag.py:87:41: F821 undefined name 'DR'
```
--
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
[GitHub] [airflow] jscheffl commented on pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
jscheffl commented on pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#issuecomment-866320943
@ashb Thanks for finalizing the merge! Looking forward for the next PR - I already have an idea what to contribute next... and hope I'll find some weekend time soon.
--
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
[GitHub] [airflow] github-actions[bot] commented on pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#issuecomment-840561427
[The Workflow run](https://github.com/apache/airflow/actions/runs/839005213) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
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
[GitHub] [airflow] ashb merged pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #15057:
URL: https://github.com/apache/airflow/pull/15057
--
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
[GitHub] [airflow] ashb commented on pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#issuecomment-840493814
@jscheffl Could you rebase this change please? (We split tests_views.py in to multiple files).
--
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
[GitHub] [airflow] jscheffl commented on pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
jscheffl commented on pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#issuecomment-840518665
Ready for review (again)
--
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
[GitHub] [airflow] ashb commented on a change in pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#discussion_r631814053
##########
File path: tests/www/views/test_views_trigger_dag.py
##########
@@ -78,6 +78,16 @@ def test_trigger_dag_conf_malformed(admin_client):
assert run is None
+def test_trigger_dag_conf_not_dict(self):
+ test_dag_id = "example_bash_operator"
+
+ response = self.client.post(f'trigger?dag_id={test_dag_id}', data={'conf': 'string and not a dict'})
+ self.check_content_in_response('must be a dict', response)
+
+ run = self.session.query(DR).filter(DR.dag_id == test_dag_id).first()
Review comment:
```suggestion
run = self.session.query(DagRun).filter(DagRun.dag_id == test_dag_id).first()
```
--
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
[GitHub] [airflow] ashb commented on a change in pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#discussion_r631740413
##########
File path: airflow/providers/apache/hive/hooks/hive.py
##########
@@ -426,7 +426,7 @@ def load_file(
if create or recreate:
if field_dict is None:
raise ValueError("Must provide a field dict when creating a table")
- fields = ",\n ".join(['`{k}` {v}'.format(k=k.strip('`'), v=v) for k, v in field_dict.items()])
+ fields = ",\n ".join([f"`{k.strip('`')}` {v}" for k, v in field_dict.items()])
Review comment:
It _should_ be ignored if you don't otherwise change the file.
--
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
[GitHub] [airflow] github-actions[bot] commented on pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#issuecomment-841153641
[The Workflow run](https://github.com/apache/airflow/actions/runs/841775706) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
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
[GitHub] [airflow] jscheffl commented on a change in pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
jscheffl commented on a change in pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#discussion_r631738950
##########
File path: airflow/providers/apache/hive/hooks/hive.py
##########
@@ -426,7 +426,7 @@ def load_file(
if create or recreate:
if field_dict is None:
raise ValueError("Must provide a field dict when creating a table")
- fields = ",\n ".join(['`{k}` {v}'.format(k=k.strip('`'), v=v) for k, v in field_dict.items()])
+ fields = ",\n ".join([f"`{k.strip('`')}` {v}" for k, v in field_dict.items()])
Review comment:
Hi @ashb Yes, the changes in hive.py were unrelated but the test scripts failed on this preventing me to open the PR. I needed to fix there... don't know why :-(
--
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
[GitHub] [airflow] ashb commented on a change in pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#discussion_r631795643
##########
File path: airflow/providers/apache/hive/hooks/hive.py
##########
@@ -429,7 +429,7 @@ def load_file(
if create or recreate:
if field_dict is None:
raise ValueError("Must provide a field dict when creating a table")
- fields = ",\n ".join([f"`{k.strip('`')}` {v}" for k, v in field_dict.items()])
+ fields = ",\n ".join(['`{k}` {v}'.format(k=k.strip('`'), v=v) for k, v in field_dict.items()])
Review comment:
```suggestion
fields = ",\n ".join([f"`{k.strip('`')}` {v}" for k, v in field_dict.items()])
```
Should be able to do this
--
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
[GitHub] [airflow] ashb commented on a change in pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#discussion_r631795901
##########
File path: tests/providers/apache/hive/hooks/test_hive.py
##########
@@ -229,7 +229,7 @@ def test_load_file_create_table(self, mock_run_cli):
filepath = "/path/to/input/file"
table = "output_table"
field_dict = OrderedDict([("name", "string"), ("gender", "string")])
- fields = ",\n ".join([f"`{k.strip('`')}` {v}" for k, v in field_dict.items()])
+ fields = ",\n ".join(['`{k}` {v}'.format(k=k.strip('`'), v=v) for k, v in field_dict.items()])
Review comment:
```suggestion
fields = ",\n ".join([f"`{k.strip('`')}` {v}" for k, v in field_dict.items()])
```
--
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
[GitHub] [airflow] ashb commented on a change in pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#discussion_r631848642
##########
File path: tests/www/views/test_views_trigger_dag.py
##########
@@ -78,6 +78,16 @@ def test_trigger_dag_conf_malformed(admin_client):
assert run is None
+def test_trigger_dag_conf_not_dict(self):
+ test_dag_id = "example_bash_operator"
+
+ response = self.client.post(f'trigger?dag_id={test_dag_id}', data={'conf': 'string and not a dict'})
+ self.check_content_in_response('must be a dict', response, status_code=400)
+
+ run = self.session.query(DagRun).filter(DagRun.dag_id == test_dag_id).first()
Review comment:
```suggestion
def test_trigger_dag_conf_not_dict():
test_dag_id = "example_bash_operator"
response = admin_client.post(f'trigger?dag_id={test_dag_id}', data={'conf': 'string and not a dict'})
check_content_in_response('must be a dict', response, status_code=400)
with create_session() as session:
run = session.query(DagRun).filter(DagRun.dag_id == test_dag_id).first()
```
--
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
[GitHub] [airflow] ashb commented on a change in pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#discussion_r631796895
##########
File path: tests/www/views/test_views_trigger_dag.py
##########
@@ -78,6 +78,16 @@ def test_trigger_dag_conf_malformed(admin_client):
assert run is None
+def test_trigger_dag_conf_not_dict(self):
+ test_dag_id = "example_bash_operator"
+
+ response = self.client.post(f'trigger?dag_id={test_dag_id}', data={'conf': 'string and not a dict'})
+ self.check_content_in_response('must be a dict', response)
Review comment:
```suggestion
self.check_content_in_response('must be a dict', response, status_code=400)
```
--
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
[GitHub] [airflow] ashb commented on a change in pull request #15057: Origin/bugfix/issue #15023 / Add validation for dag_run conf to be a dict
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#discussion_r631666972
##########
File path: tests/providers/apache/hive/hooks/test_hive.py
##########
@@ -229,7 +229,7 @@ def test_load_file_create_table(self, mock_run_cli):
filepath = "/path/to/input/file"
table = "output_table"
field_dict = OrderedDict([("name", "string"), ("gender", "string")])
- fields = ",\n ".join(['`{k}` {v}'.format(k=k.strip('`'), v=v) for k, v in field_dict.items()])
+ fields = ",\n ".join(['`{k}` {v}'.format(k=k.strip('`'), v=v) for k, v in field_dict.items()])
Review comment:
```suggestion
fields = ",\n ".join(['`{k}` {v}'.format(k=k.strip('`'), v=v) for k, v in field_dict.items()])
```
--
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
[GitHub] [airflow] github-actions[bot] commented on pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#issuecomment-841153737
[The Workflow run](https://github.com/apache/airflow/actions/runs/841787079) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
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
[GitHub] [airflow] ashb commented on pull request #15057: Ensure that dag_run conf is a dict
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #15057:
URL: https://github.com/apache/airflow/pull/15057#issuecomment-866672649
@jscheffl This PR will be in 2.1.1 too -- I merged it just before the freeze for release prep
--
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