Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ScheduleInterval schema in OpenAPI specs should have "nullable: true" otherwise generated OpenAPI client will throw an error in case of nullable "schedule_interval" #22358

Closed
2 tasks done
omarsmak opened this issue Mar 18, 2022 · 6 comments · Fixed by #24253
Labels
area:core contributors-workshop Issues that are good for first-time contributor's workshop good first issue kind:bug This is a clearly a bug

Comments

@omarsmak
Copy link
Member

Apache Airflow version

2.2.4 (latest released)

What happened

Currently we have this schema definition in the OpenAPI specs:

    ScheduleInterval:
      description: |
        Schedule interval. Defines how often DAG runs, this object gets added to your latest task instance's
        execution_date to figure out the next schedule.
      readOnly: true
      oneOf:
        - $ref: '#/components/schemas/TimeDelta'
        - $ref: '#/components/schemas/RelativeDelta'
        - $ref: '#/components/schemas/CronExpression'
      discriminator:
        propertyName: __type

The issue with above is, when using an OpenAPI generator for Java for example (I think is same for other languages as well), it will treat ScheduleInterval as non-nullable property, although what is returned under /dags/{dag_id} or /dags/{dag_id}/details in case of a None schedule_interval is null for schedule_interval.

What you think should happen instead

We should have nullable: true in ScheduleInterval schema which will allow schedule_interval to be parsed as null.

How to reproduce

No response

Operating System

Linux

Versions of Apache Airflow Providers

No response

Deployment

Official Apache Airflow Helm Chart

Deployment details

No response

Anything else

If the maintainers think is a valid bug, I will be more than happy to submit a PR :)

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@omarsmak omarsmak added area:core kind:bug This is a clearly a bug labels Mar 18, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Mar 18, 2022

Thanks for opening your first issue here! Be sure to follow the issue template!

@uranusjr
Copy link
Member

Good catch. Pull request would be very welcomed!

@kianelbo
Copy link
Contributor

kianelbo commented Apr 3, 2022

Does adding allow_none=True argument to schedule_interval field suffice?
If so, may I open the PR?

@omarsmak
Copy link
Member Author

omarsmak commented Apr 3, 2022

I think it doesn't really help, the issue will be on the generated client side.
Feel free to open a PR as I haven't get the time yet

@eladkal eladkal added the contributors-workshop Issues that are good for first-time contributor's workshop label May 27, 2022
@Bowrna
Copy link
Contributor

Bowrna commented Jun 3, 2022

Currently when I hit the below API
http://127.0.0.1:28080/api/v1/dags/example_trigger_target_dag/details,

I get the response:

{
  "catchup": false,
  "concurrency": 16,
  "dag_id": "example_trigger_target_dag",
  "dag_run_timeout": null,
  "default_view": "grid",
  "description": null,
  "doc_md": null,
  "end_date": null,
  "file_token": ".eJxT0s8vKNFPzCxKy8kvh9OpFYm5BTmp8SmJ6cVwTklRZnp6alF8SWJRemoJSE6voFIJADOyGRw.vE-k8zZfRBEacXoSycU5gDkSXDk",
  "fileloc": "/opt/airflow/airflow/example_dags/example_trigger_target_dag.py",
  "is_active": true,
  "is_paused": false,
  "is_paused_upon_creation": null,
  "is_subdag": false,
  "last_parsed": "2022-06-03T11:07:15.950865+00:00",
  "max_active_runs": 16,
  "max_active_tasks": 16,
  "orientation": "LR",
  "owners": [
    "airflow"
  ],
  "params": {

  },
  "pickle_id": null,
  "render_template_as_native_obj": false,
  "schedule_interval": null,
  "start_date": "2021-01-01T00:00:00+00:00",
  "tags": [
    {
      "name": "example"
    }
  ],
  "timezone": "Timezone('UTC')"
}

How does setting the ScheduleInterval param to nullable:true will be useful?
Edit: @uranusjr @omarsmak Could you help me in this?

@uranusjr
Copy link
Member

uranusjr commented Jun 6, 2022

From what I understand, the problem is not in the API response, but the generated API clients. The response you have above is currently not consumable by an API client. Adding nullable: true would allow the clients to handle "schedule_interval": null correctly. The generated API clients are not a part of the main Airflow repository, but they do rely on the openapi/v1.yml file in here, so we need to fix it for them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core contributors-workshop Issues that are good for first-time contributor's workshop good first issue kind:bug This is a clearly a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants