Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix incorrect default on readonly property in our API #32510

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jul 11, 2023

The new version of openapi-spec-validator (0.6.0) started to detect a bug in our API specification - readonly values cannot have defaults (by definition).

This PR fixes the problem and also bumps the openapi-spec-validator to >= 0.6.0 to make sure it is used everywhere.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
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.

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:dev-tools area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Jul 11, 2023
@potiuk potiuk changed the title Fix incorrect default on readonly propertu in our API Fix incorrect default on readonly property in our API Jul 11, 2023
The new version of openapi-spec-validator (0.6.0) started to
detect a bug in our API specification - readonly values cannot
have defaults (by definition).

This PR fixes the problem and also bumps the openapi-spec-validator
to >= 0.6.0 to make sure it is used everywhere.
@potiuk potiuk force-pushed the fix-openapi-default-on-readonly-property branch from 6643c8d to bcacf52 Compare July 11, 2023 06:49
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

I don’t know what the issue this finds, but if the validator says so, so it is.

@potiuk
Copy link
Member Author

potiuk commented Jul 11, 2023

I don’t know what the issue this finds, but if the validator says so, so it is.

It makes no sense to have default on readOnly property.

@potiuk
Copy link
Member Author

potiuk commented Jul 11, 2023

default -> what value should be set automatically when we call an API when we miss to set it. This property is only returned by the API, so it should always be present and needs to be set (so there will never be case that we will have to assume some defaults).

@potiuk
Copy link
Member Author

potiuk commented Jul 11, 2023

https://swagger.io/docs/specification/data-models/data-types/#readonly-writeonly

TIL:

You can use the readOnly and writeOnly keywords to mark specific properties as read-only or write-only. 
This is useful, for example, when GET returns more properties than used in POST – you can use the same
schema in both GET and POST and mark the extra properties as readOnly. 
readOnly properties are included in responses but not in requests,
and writeOnly properties may be sent in requests but not in responses.

@potiuk
Copy link
Member Author

potiuk commented Jul 11, 2023

Static checks passed, API tests passed. merging.

@potiuk potiuk merged commit d6a4f85 into apache:main Jul 11, 2023
@potiuk potiuk deleted the fix-openapi-default-on-readonly-property branch July 11, 2023 07:06
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.0 milestone Aug 2, 2023
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Aug 2, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area:API Airflow's REST/HTTP API area:dev-tools 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.

5 participants