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

OpenAPI Spec fix nullable alongside $ref #32887

Merged

Conversation

pierrejeambrun
Copy link
Member

@pierrejeambrun pierrejeambrun commented Jul 27, 2023

Fix wrong usage of nullable prop next to a ref. TLDR $ref shouldn’t have sibling. (unless OpenAPI 3.1 that allows description and summary only I believe).
This induce wrong api documentation and most importantly bugs in our python clients such as:

This allows to generate the expected api doc and the correct client.
I Tried that locally by regenerating the client and making the problematic call, such as:

# The one that cancelled 2.6.2rc1 client.
dag_api_instance.get_tasks(DAG_ID)

It is working with this spec.

After this we can:

  • Update the client release process to remove the cherry picking part. (manual fix)
  • Close Update task_instance.py airflow-client-python#89
  • Add a pre-commit hook to prevent siblings on $ref elements. (check why the current spec validator wrongfully allows it)

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Jul 27, 2023
Copy link
Contributor

@ferruzzi ferruzzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my first time looking at OpenAPI, but the changes all appear logical to me. Call this a non-binding +1 :P

@pierrejeambrun
Copy link
Member Author

More work is required, this is breaking some tests. Converting to draft.

uranusjr
uranusjr previously approved these changes Jul 28, 2023
@uranusjr uranusjr dismissed their stale review July 28, 2023 06:00

Hmm the errors seem more serious than I thought

@uranusjr uranusjr marked this pull request as draft July 28, 2023 06:00
@pierrejeambrun pierrejeambrun marked this pull request as ready for review July 31, 2023 23:03
@pierrejeambrun
Copy link
Member Author

pierrejeambrun commented Jul 31, 2023

CI should be green now. The hard part is that both connexion package and the openapi-generator need to understand the spec. This limits us to not use certain syntax and I went for an easy, straightforward solution.

@pierrejeambrun pierrejeambrun added this to the Airflow 2.7.0 milestone Aug 2, 2023
@pierrejeambrun pierrejeambrun added the type:bug-fix Changelog: Bug Fixes label Aug 2, 2023
@pierrejeambrun pierrejeambrun merged commit 3141d4b into apache:main Aug 2, 2023
42 checks passed
@pierrejeambrun pierrejeambrun deleted the fix-nullable-ref-open-api-spec branch August 2, 2023 18:54
ephraimbuddy pushed a commit that referenced this pull request Aug 3, 2023
* OpenAPI Spec fix nullable alongside $ref

* Fix CI

* Update following code review

* Add deprecation warning for 'none' state.

(cherry picked from commit 3141d4b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants