JSON as response for 404 not found #12305
Conversation
…y could be handled by Flask and therefore we need to handle it ourselves.
|
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
|
@Zedmor can you fix the CI? It is very important to us because it is the first criterion on the basis of which we review PR. |
airflow/www/extensions/init_views.py
Outdated
| @@ -134,12 +134,29 @@ def init_error_handlers(app: Flask): | |||
|
|
|||
There was a problem hiding this comment.
There was a problem hiding this comment.
We override handler only for API, therefore in case we will build it w/o API default handler (with circles) should still be applied. 500 is handled properly by blueprint handler (with JSONify) btw, only 404 and 405 are not.
| # unless raised from a view func so actual 404 errors, | ||
| # i.e. "no route for it" defined, need to be handled | ||
| # here on the application level | ||
| return common_error_handler(ex) |
There was a problem hiding this comment.
Is common_error_handler support this exception type?
There was a problem hiding this comment.
Yes, see
problem is that flask do not delegate that to common error handler from blueprint (connexion)
|
Awesome! Thank you for taking care of it so quickly. Can you also add automated tests to prevent regression? |
|
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
|
Can issue #12325 be resolved through this PR? You'll just need to update and add the error in openapi specification.I can see 405 handled in your code, but there's no corresponding link in openapi |
Linting Add link to a doc
Yes, it handles it. I put it to link map but it handle it anyway since link map is just a way to generate link to a documentation (we can add it later). I added the test to check for 405 for issue #12325 |
|
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
|
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
|
Nice! Please also add the Method Not Allowed description here: https://github.com/apache/airflow/blob/master/airflow/api_connexion/openapi/v1.yaml, You'll see an example |
|
Nitpicking here. Something like this would be a nice addition: airflow/airflow/api_connexion/openapi/v1.yaml Line 193 in 648dd7c |
|
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
In case of existing issue, reference it using one of the following:
closes: #12131
JSON is not returned for API requests with incorrect endpoint
Reason of 12131 is happening is that blueprint level handlers are not possible for 404 and 405. See references:
https://stackoverflow.com/questions/58728366/python-flask-error-handling-with-blueprints
pallets/flask#1935
and therefore 404 exception handling spill out to application level handler (circles).
I am looking forward to better implementation but this works.