You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "potiuk (via GitHub)" <gi...@apache.org> on 2023/02/06 23:10:14 UTC
[GitHub] [airflow] potiuk opened a new pull request, #29395: Fix validation of date-time field in API and Parameter schemas
potiuk opened a new pull request, #29395:
URL: https://github.com/apache/airflow/pull/29395
The open-api-schema-validator 0.4.3 made rfc3999 validation of the date-time fields mandatory and this revealed problems in our test URLs - the '+' was not url-encoded and it was replaced with space - thus the dates passed were not valid rfc3999 date-time specifications.
This however revealed one more problem. The rfc3339-validator package is automatically installed by the open-api-schema-validator, but when installed, it also adds validation to date-time fields validated by the Params of ours - for example naive date-time parameters are not supported any more (but they were in the past).
This might introduce breaking changes for users who use non-valid date-time parameters - however we should consider that as a bugfix, and accidental support, because the date-time schema should expect rfc3999-formatted date time.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1419987758
also > 4 on jsonschema should be added to this one.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1421487545
Static check fails (but irrelevant - I will fix it separately) and two flaky tests. Merging.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1421089549
Yep change `YYYY-MM-DD HH:mm:ssZ` to `YYYY-MM-DDTHH:mm:ssZ` make it pick as RFC3339 datetime
Just interesting if change in this place is it possible that in other places something break 🤔
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1419931607
I think this is the only way to proceed even though it introduces potential backwards-compatibility for those for whom the validation accidentally worked before:
Example snipppet of code showing how `date-time` fields will be validated after installing the validator:
```
>>> from airflow.utils.timezone import datetime
>>> from rfc3339_validator import validate_rfc3339
>>> dtt = datetime(2020, 1, 1)
>>> print(dtt)
2020-01-01 00:00:00+00:00
>>> validate_rfc3339(str(dtt))
False
>>> validate_rfc3339('2020-01-01T00:00:00+00:00')
True
>>> validate_rfc3339('2020-01-01T00:00:00')
False
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1420009395
> We could combine or could make it as follow up. I do not have strong opinion for that and may be need more time for additional fixes in it. Potentially we could also drop some code which do not required anymore as follow-up, like this:
I think better to combine it - it will make it more cherry-pickable. I think this one is a candidate to 2.5.2 because of this 'side-effect' behaviour for jsonschema.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1420984842
We might want to fix it as well
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1421337361
Shall we go Yolo and Add the T :) ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1420013266
The tests seems to be green BTW
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1420702344
Yeah. I am with @Taragolis here.
> I think we use transformation by this function, unfortunately it happen after string validation.
Previously it didn't failed because jsonschema doesn't validate datetime if rfc3339-validator or strict-rfc3339 (jsonschema<4) not installed.
I think we need to bite the bullet now indeed. With Params it's a different story because we have full control over definition and validation and we even had an invalid example so far :( (so users might complain on it).
But our Python/Go Clients for example would work correctly (I believe? - this is something we could possibly check with @pierrejeambrun ?) and I think for the APIs the standard compliance. The OpanAPI standard is very clear https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md#data-types:
> string | date-time | As defined by date-time - RFC3339
There are no options here. "date-time" means RFC 3339.
So what this change brings is a compliance with standards. Yes. It might cause some troubles, but this should be an easy fix for any "hand-crafted" client (And a good reason to use our Python client as well).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1421498494
Thanks for the checks Jarek, LGTM !
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1421238509
> Quickly reading through the python client code tells me that there is some `iso 8601` involved for deserialization.
For deserialization it is fine, `rfc3339` it is a valid `iso8601` format.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1420349101
@potiuk I rebased on your changes and make small changes:
> Just check is it valid iso8601, in case if rfc3339 validation failed, but do not change actual values and only warn users
We could use this commit 51594ba05e4ca0c9eac5696d540abf2095416bdc if everything alright
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1420692067
Applied your change, rebased made you co-author and updated docs/commit message @Taragolis
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1420607406
Yep I will merge + rebate shortly
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1420738758
And in additional, if we select data from data picker than it send wit space separator instead of `T`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #29395:
URL: https://github.com/apache/airflow/pull/29395#discussion_r1098031946
##########
newsfragments/29395.significant.rst:
##########
@@ -0,0 +1,7 @@
+The date-time fields passed as API parameters or Params should be RFC3999-compliant.
+
+In case of API calls, it was possible that "+" passed as part of the date-time fields were not URL-encoded, and
+such date-time fields could pass validation. Such date-time parameters should now be URL-encoded (as ``%2B``).
Review Comment:
OMG! I've broke my head when I tried to understand why on Earth `tests/api_connexion/endpoints/test_dag_run_endpoint.py::TestGetDagRunsPaginationFilters::test_date_filters_gte_and_lte` failed what the component changed `2020-06-18T18:00:00+00:00` to `2020-06-18T18:00:00 00:00`
##########
newsfragments/29395.significant.rst:
##########
@@ -0,0 +1,7 @@
+The date-time fields passed as API parameters or Params should be RFC3999-compliant.
Review Comment:
```suggestion
The date-time fields passed as API parameters or Params should be RFC3339-compliant.
```
##########
newsfragments/29395.significant.rst:
##########
@@ -0,0 +1,7 @@
+The date-time fields passed as API parameters or Params should be RFC3999-compliant.
+
+In case of API calls, it was possible that "+" passed as part of the date-time fields were not URL-encoded, and
+such date-time fields could pass validation. Such date-time parameters should now be URL-encoded (as ``%2B``).
+
+In case of parameters, it was possible that ' ' was used instead of ``T`` separating date from time and no
+timezone was specified, which was invalid specification (not compliant with RFC3999).
Review Comment:
```suggestion
timezone was specified, which was invalid specification (not compliant with RFC3339).
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1420002526
> Yes. I just realized (sorry I was tired back then) that you already attempted to fix it.
I guess I will spend couple of days before realise that the problem with URI encoding 🤦
I've already checked `connexion`, `werkzeug`, `marshmallow` 🤣
> Yes. I think we could combine those two PRs - and once mine gets green (I just pushed a fix) - we can add the try_fix_not_rfc3339_dt.
We could combine or could make it as follow up. I do not have strong opinion for that and may be need more time for additional fixes in it.
Potentially we could also drop some code which do not required anymore as follow-up, like this:
https://github.com/apache/airflow/blob/2e1635a9c1d6faaa9d5cd5cfd1a364091cd62bde/airflow/api_connexion/parameters.py#L38-L53
>also > 4 on jsonschema should be added to this one (from your PR).
Yep! This behaviour of `jsonschema` always confuse me - base on dependencies which install in environment behaviour of validation for date-time (and other to honest) could be different.
I checked by `pipdeptree` and seems like we do not use it directly only pre-commit hooks and `moto` use it. Anyway sooner or later we would have to install `rfc3339-validator` as a dependency.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1421358199
I can't find any side effects with other date pickers, except strange behaviour with changing timezone UTC (default) -> Local -> UTC but I found that it not related to this changes.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1421188545
@Taragolis for the front end, I know we also have a date picker in the grid that we might want to check as well. https://github.com/apache/airflow/blob/main/airflow/www/static/js/dag/nav/FilterBar.tsx#L65
For the clients, this is something that we need to check indeed. Quickly reading through the python client code tells me that there is some `iso 8601` involved for deserialization. I would have to test that to be sure that this would work as intended.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #29395:
URL: https://github.com/apache/airflow/pull/29395#discussion_r1098031946
##########
newsfragments/29395.significant.rst:
##########
@@ -0,0 +1,7 @@
+The date-time fields passed as API parameters or Params should be RFC3999-compliant.
+
+In case of API calls, it was possible that "+" passed as part of the date-time fields were not URL-encoded, and
+such date-time fields could pass validation. Such date-time parameters should now be URL-encoded (as ``%2B``).
Review Comment:
OMG! I've broke my head when I tried to understand why on Earth `tests/api_connexion/endpoints/test_dag_run_endpoint.py::TestGetDagRunsPaginationFilters::test_date_filters_gte_and_lte` failed, and which component/package changed `2020-06-18T18:00:00+00:00` to `2020-06-18T18:00:00 00:00`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1421062557
I guess we need to change this value, let me check
https://github.com/apache/airflow/blob/0d2555b318d0eb4ed5f2d410eccf20e26ad004ad/airflow/www/static/js/main.js#L255
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1421405239
Also tested the client API:
```python
import datetime
import airflow_client.client
from pprint import pprint
from airflow_client.client.api import dag_run_api
#
# In case of the basic authentication below. Make sure:
# - Airflow is configured with the basic_auth as backend:
# auth_backend = airflow.api.auth.backend.basic_auth
# - Make sure that the client has been generated with securitySchema Basic.
# Configure HTTP basic authorization: Basic
configuration = airflow_client.client.Configuration(
host="http://localhost:8080/api/v1",
username='admin',
password='admin'
)
# Enter a context with an instance of the API client
with airflow_client.client.ApiClient(configuration) as api_client:
# Create an instance of the API class
api_instance = dag_run_api.DAGRunApi(api_client)
try:
# Get current configuration
api_response = api_instance.get_dag_runs(dag_id='example_params_ui_tutorial',
start_date_lte=datetime.datetime.now(
tz=datetime.timezone.utc))
pprint(api_response)
except airflow_client.client.ApiException as e:
print("Exception when calling ConfigApi->get_config: %s\n" % e)
```
Results in no warnings and properly formatted params:
When we remove timezone it correctly fails:
And we get this as response:
So it looks like everything works as expected. Even if the last one (no timezone failure) might be not the same as before, this is a **proper** behaviour - due to ambiguity of no-timezone query
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #29395:
URL: https://github.com/apache/airflow/pull/29395
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1419961594
> it also adds validation to date-time fields validated by the Params of ours - for example naive date-time parameters are not supported any more (but they were in the past).
I guess if we fix URI tests by this PR, we could use backward compatible version by attempt to fix Params: https://github.com/apache/airflow/pull/29374
I've also found weird thing that data-time from UI DagRun send as "2020-01-02 03:04:05+00:00" param instead on "2020-01-02T03:04:05+00:00"
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1419985655
> I guess if we fix URI tests by this PR, we could use backward compatible version by attempt to fix Params: https://github.com/apache/airflow/pull/29374
Yes. I just realized (sorry I was tired back then) that you already attempted to fix it. Yes. I think we could combine those two PRs - and once mine gets green (I just pushed a fix) - we can add the try_fix_not_rfc3339_dt. I feel little uneasy with Params earlier supporting the non-rfc3339 compliant date param.
You can add a fixup to my PR :) @Taragolis then (and correct the newsfragment).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1420022737
> I think better to combine it - it will make it more cherry-pickable. I think this one is a candidate to 2.5.2 because of this 'side-effect' behaviour for jsonschema/parameters
I will check my branch with changes from this branch In the morning or afternoon.
One thing what I think we should decide, we have two options with Param date-time:
1. As it implemented right now in https://github.com/apache/airflow/pull/29374, convert values iso8601 -> rfc3339 and naive -> awaire
2. Just check is it valid iso8601, in case if rfc3339 validation failed, but do not change actual values and only warn users
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1420684511
> Nice.
I wonder if it's 'possible' to handle the change in the endpoint itself using something like https://github.com/apache/airflow/blob/main/airflow/api_connexion/parameters.py#L38 and avoid breaking change? The issue seems to affect only dates in the query string
I think we use transformation by this function, unfortunately it happen after string validation.
Previously it didn't failed because `jsonschema` doesn't validate datetime if `rfc3339-validator` or `strict-rfc3339` (jsonschema<4) not installed.
As another workaround we could overwrite default datetime checker in `jsonschema` however as result every user who validate json in Airflow Context would use our checker.
Potentially there is win-win condition but I can't find it 😞
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1419941135
cc: @msumit @mik-laj @ephraimbuddy - since you were mostly involved with API/Params definition. I think your input on making date-time mandatory validated by RFC3999 validator would be needed.
The https://github.com/p1c2u/openapi-schema-validator made rfc3999 mandatory, so if we want to keep up-to-date version of the validator, this is the best way to follow.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1421376616
I checked the behviour of the grid datepicker @pierrejeambrun and it works fine without warnings.
Even if the date is "isoFormatWithoutTZ", it produces a valid RFC3339 timestamp:
This is a valid rfc3339.txt:
```python
from rfc3339_validator import validate_rfc3339
validate_rfc3339('2023-02-11T21:00:00Z')
True
```
Because it is used like this:
```
(localDate: string) => moment(localDate).utc().format(),
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29395: Fix validation of date-time field in API and Parameter schemas
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29395:
URL: https://github.com/apache/airflow/pull/29395#issuecomment-1421408346
I pushed the change with datepicker format change. I think it should be ready to go .
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org