pierrejeambrun commented on code in PR #32887:
URL: https://github.com/apache/airflow/pull/32887#discussion_r1281241900
##########
airflow/api_connexion/openapi/v1.yaml:
##########
@@ -4659,7 +4642,9 @@ components:
*Changed in version 2.4.0*: 'sensing' state has been removed.
*Changed in version 2.4.2*: 'restarting' is added as a possible
value
type: string
+ nullable: true
Review Comment:
Indeed, we allow "none" for convenience but I do not find this really
consistent not allow 'null' where the TaskInstanceState does. (And a few other
endpoint do as well).
It is not easy to work around this in the OpenAPI spec only. (anyOf, allOf,
NullRef only etc...). Those solutions didn't work when I tried them, taking
into account the constraint that it needs to be compatible both for `connexion`
and `open-api-generator`. We can always duplicate the schema, have one nullable
and the original not nullable, but this is not maintainable.
I see 2 solutions moving forward:
1. I have updated the code to allow `null` as a valid value for
`ListTaskInstanceForm`. This makes sense to me as we have consistency across
endpoints and allowing null alongside 'none' doesn't hurt IMHO.
2. Remove the originally ignored 'nullable: true' in the spec, and handle
this later without modifying the actual `State` schema. This would keep a bug
in the spec, as other ref using `TaskState` expect a nullable field, but the
prop is simply ignored. (all but the one in the `ListTaskInstanceForm`)
Also this is an issue for the python client generation, creates bugs and
also requires some manual patching at release time.
I think this is a small adjustment that will make our life easier, even if
allowing both `'none'` and `null` in the form might be a bit overkill.
Let me know if you see a better way to handle this.
Example of query (tested locally):
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
