Skip to content

Fix incorrect default on readonly property in our API#32510

Merged
potiuk merged 1 commit into
apache:mainfrom
potiuk:fix-openapi-default-on-readonly-property
Jul 11, 2023
Merged

Fix incorrect default on readonly property in our API#32510
potiuk merged 1 commit into
apache:mainfrom
potiuk:fix-openapi-default-on-readonly-property

Conversation

@potiuk

@potiuk potiuk commented Jul 11, 2023

Copy link
Copy Markdown
Member

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

@uranusjr uranusjr left a comment

Copy link
Copy Markdown
Member

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

potiuk commented Jul 11, 2023

Copy link
Copy Markdown
Member Author

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

potiuk commented Jul 11, 2023

Copy link
Copy Markdown
Member Author

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

potiuk commented Jul 11, 2023

Copy link
Copy Markdown
Member Author

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

potiuk commented Jul 11, 2023

Copy link
Copy Markdown
Member Author

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
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: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