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

Raise 404 from Variable PATCH API if variable is not found #33885

Merged
merged 9 commits into from
Aug 30, 2023

Conversation

abhishekbhakat
Copy link
Contributor

@abhishekbhakat abhishekbhakat commented Aug 29, 2023

Raise NotFound exception if variable search via session returns empty.
A possible fix for Issue.
closes: #33871

@boring-cyborg boring-cyborg bot added the area:API Airflow's REST/HTTP API label Aug 29, 2023
@potiuk
Copy link
Member

potiuk commented Aug 29, 2023

Is it possible to add a test for that one? Should be easy following other tests in https://github.com/apache/airflow/tree/main/tests/api_connexion/endpoints. - and at the same time you will be able to see if removing your new code will trigger the error (thus giving ultimate certainty it's the solution) ?

@potiuk potiuk added this to the Airflow 2.7.1 milestone Aug 29, 2023
@abhishekbhakat abhishekbhakat marked this pull request as draft August 29, 2023 14:44
@abhishekbhakat abhishekbhakat marked this pull request as ready for review August 29, 2023 15:07
@abhishekbhakat
Copy link
Contributor Author

abhishekbhakat commented Aug 29, 2023

@potiuk, I added the test. And tried with removing the code, below is the result with test failing.

FAILED tests/api_connexion/endpoints/test_variable_endpoint.py::TestPatchVariable::test_should_reject_invalid_update - AttributeError: 'NoneType' object has no attribute 'key'

Also didn't wanted to leave the detail key in response as None. So added the code while raising exception.

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.

Tested locally to confirm the issue and fix does resolve it.

@uranusjr uranusjr changed the title Raise variable not found if session returns empty Raise 404 from Variable PATCH API if variable is not found Aug 30, 2023
@ephraimbuddy ephraimbuddy merged commit 701c3b8 into apache:main Aug 30, 2023
42 checks passed
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Sep 1, 2023
ephraimbuddy pushed a commit that referenced this pull request Sep 1, 2023
* Raise variable not found if session returns empty

* Added detail to the exception for json reponse

* tests for patch api when variable doesn't exist

* Dropped fstring

* Unify varialbe not found message

---------

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
(cherry picked from commit 701c3b8)
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 type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Airflow API for PATCH a variable key which doesn't exist, sends Internal Server Error
4 participants