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

Fix Rest API update user output #29409

Merged
merged 3 commits into from
Feb 20, 2023
Merged

Conversation

vincbeck
Copy link
Contributor

@vincbeck vincbeck commented Feb 7, 2023

The Rest API update user returns the wrong schema. It currently return the role schema instead of the user's. I found out this bug while reading the documentation. You can see here the response of update user API is Role's model

@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 Feb 7, 2023
@ephraimbuddy
Copy link
Contributor

Can you describe the issue and update the title too

@vincbeck vincbeck changed the title Fix patch user API Fix Rest API update user Feb 8, 2023
@vincbeck vincbeck changed the title Fix Rest API update user Fix Rest API update user output Feb 8, 2023
@vincbeck
Copy link
Contributor Author

vincbeck commented Feb 8, 2023

Yeah sorry, I confess I had been lazy on this one :|

@@ -2288,7 +2288,7 @@ paths:
content:
application/json:
schema:
$ref: '#/components/schemas/Role'
$ref: '#/components/schemas/User'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$ref: '#/components/schemas/User'
$ref: '#/components/schemas/UserCollectionItem'

UserCollectionItem is more appropriate here. You also need to add test to protect accidental changing in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks for the suggestion. Regarding the tests, it seems there are already some tests here but regardless of the schema referenced in v1.yaml, it always return the same schema of data (the correct one). Any idea why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any idea @ephraimbuddy?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's strange to me that the response is not enforced in the spec. I had thought that the spec provides input and output validation but this proves otherwise

@potiuk
Copy link
Member

potiuk commented Feb 20, 2023

@ephraimbuddy ?

@potiuk potiuk merged commit 20206e9 into apache:main Feb 20, 2023
@pierrejeambrun pierrejeambrun added this to the Airflow 2.5.2 milestone Feb 27, 2023
@pierrejeambrun pierrejeambrun added the type:bug-fix Changelog: Bug Fixes label Feb 27, 2023
@vincbeck vincbeck deleted the vincbeck/patch_user branch March 1, 2023 19:57
pierrejeambrun pushed a commit that referenced this pull request Mar 7, 2023
* Fix patch user API

* Use UserCollectionItem

(cherry picked from commit 20206e9)
pierrejeambrun pushed a commit that referenced this pull request Mar 8, 2023
* Fix patch user API

* Use UserCollectionItem

(cherry picked from commit 20206e9)
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.

4 participants